Reviewing Code for a chat application for Android
I recently had a job to review a chat application. We agreed upon some time and the amount for the job. We signed an NDA ( Non Disclosure Agreement) and then i started with my review.
The chat application was developed with smack. They had also used Realm. I had never used Realm as database. I had to download the realm samples run it. I also tried to modify the samples available on the git-hub repository. They also wanted me to review RxJava code used in their application.
They also wanted review on the performance of the app with respect to memory and battery usage. I downloaded the code.
I looked into their code there was no unit tests written. While this was simple chat app with not a lot of features like whats app. It took me some time to understand the flow.
Few things i noted down
- I looked into app’s build.gradle file. I saw that they were no targeting the latest android version. They were not using the latest support libs. The chat application supported only English. So they could optimise by having resConfigs “en” to reduce .apk size and i din’t use Apk Analyser at this point of review, this is something i look forward to in the future.
- I looked into the manifest file to understand which activity was the launcher and what components have been delcared. The manifest file had lot of permissions and some of them were duplicates.
- I looked into the package structure next. They din’t have package by feature. So they had packages like activities, fragments, adapters and so on..
- I started to understand the flow and found that they have not followed any design pattern like MVP/MVVM.
- The app has individual chat between two people and also group chat.
Coming to the review process. Since they had specified they wanted me to review the usage of Realm and RxJava in their code, i concentrated on that part.
- They had a helper class which was a Singleton and methods which had a RealmInstance as a parameter and the method returned a RealmObject. They had not closed the RealmInstance. So i asked them close the RealmInstance before returning the RelamObject. I was wrong. If you close the RealmInstance and try to use the RealmObject after close you get a illegalstateexception — The RealmInstance is closed and is unusable. I learnt that RealmInstance is tied to the Lifecycle of the Activity. Also looking at Realm examples you can have a RelamInstance in activity onCreate and close that in onDestroy. I also looked at a blog written by Gabor Varadi and asked the devs to check the github repository and the use of RealmManager @ https://github.com/Zhuinden/realm-helpers.
- A lot of Realm Transaction happened on the ui thread with executeTransaction. I asked the devs to look into executeTransactionAsync
- While reading the Realm documentation there is @Index annotation which helps in faster reads. There is a slight increase in the Realm file but the reads are faster. So i suggested them to use @Index annotation for all fields that you want to query.
- They had a separate file where they did all operations using RxJava. They used RxJava2. I found that they never disposed the observables. They had used the observable types correctly. They also use Schedulers.newThread() instead of Schdeulers.io() in many places. I suggested them to dispose the observables in onDestory and also use Scheduelrs.io() as the threads are re-used from the thread pool and also since they were doing a i/o operation.
- They had use AsyncTask in many places. Like for downloading images/files in chat adapter on click they started a asynctask and also passed the view reference to AsyncTask. I suggested them to use WeakReference and also understand RecyclerView recycling mechanism. I asked them to use RxJava in all places. They were scaling the image but they were using two AsyncTask one for downloading and one for scaling which I think was unnecessary. https://android-developers.googleblog.com/2010/07/multithreading-for-performance.html
- They had a service and they had created an instance of Service class. They had also passed the activity context to the constructor of the service class. I told them to start the service from a component or using the binding mechanism. I asked them to look at https://stackoverflow.com/questions/41400060/android-implications-of-creating-an-instance-of-a-service-class. The other thing they did was start a service from asyctask doInbackground and wrapped that in a try catch block. The asynctask was in application class and then the service was started in doInbackground of asynctask. I asked them get rid of asynctask and start a service normally. If need be start a thread in service and do some operation required. They had a XmppConnection logic in service. Quoting docs“The
startService()method now throws an
IllegalStateExceptionif an app targeting Android 8.0 tries to use that method in a situation when it isn't permitted to create background services.” . The flag used in onStartCommand was START_STICKY meaning if services is destroyed the system can start the service again. But as noted you cannot start service when your app is in background. Had to remove the flag and use START_NOT_STICKY also start and stop service from activity besides suggesting JobScheduler.
- Some BroadCastReceiver’s were not declared in the manifest file and they had forgotten to unregister the broadcast on activity destroy.
- They also had activity context static and was referenced in another activity. I told them that context should live within the life-cycle of the activity or you risk memory leaks. They asked me if there are any tools for detecting memory leaks. I suggested them to use Leak-canary.
- Since they had not targeted the latest Android Version i asked them to check the android documentation regarding the behaviour changes in android O.
- They had a mute/unmute option for chat and based on a flag they used to change the drawable in chatroomadapter and update the realmdb. Since they had not done it right the app crashed and ended up Cannot modify managed objects outside of a write transaction. As the message stated they did the transaction outside of executeTransaction and i moved the code within and it worked fine.
- They had a scheme=”appname” in manifest file for activity tag and i din’t have a clue of what they were trying to achieve. I assumed they wanted deeplinking and directed them to the android documentation on deeplinking.
- Regarding security — they had a content provider and no exported = false in manifest file for the said component. I asked them add the same. Their realm db was not encrypted and i directed them to the sample on github repository which has a sample on how to encrypt messages stored in realm db. Also asked them to use https instead of http .
- Deleting messages in chat and the chat messages got re-arranged in adapter. ( i need to take a deeper look into how its implemented). Actually they used RealmRecyclerAdapter and they deleted the messages from realm db. ( Not sure if the media file is deleted from the gallery/folder also). Apart from that sorting message based on date/time to be implemented. After righting this review i looked into the sort method and implemented sort based on date field (long time).
- They had used findFirst and findAll methods. While going through the realm documentation I found that you can use findFirstAsync and findAllAsync to make your query asynchronous. I suggested the same.
- After inserting message to the database, the message is displayed in the RecyclerView, but the recyclerview does not scroll. Just a case of using scrollToPositionWithOffset method of LinearLayoutManager. But if you try scrolling in onCreate it won’t work. Look at https://stackoverflow.com/questions/26580723/how-to-scroll-to-the-bottom-of-a-recyclerview-scrolltoposition-doesnt-work
Apart from this i had a few other suggestion
- Use of MVP/MVVM architecture pattern.
- Write unit tests. Continuos Integration. They wanted a sample directed them to google code lab on Android Testing. Suggested Jenkins/Circle CI for CI tools.
- Use of cloud testing like firebase test lab.
- Use of Picasso/Glide as image loading libraries
- Use of ConstraintLayout. Pointed to google codelab and android documentation besides explaining them overdraw.
- Use of Android Profiler and pointed them to video talk on how to use the same besides checking battery usage with battery historian.
- Follow proper naming conventions and use of access modifiers correctly.
- It takes some time in learning RxJava and infact i am still learning and asked them to check the stackoverflow tag on RxJava and pointed them couple of blogs and samples on github.
Apart from that they had few queries on Push Notification — On some devices if you swipe your app from recent’s its stopped and thereafter you don’t receive push messages ( you have to open the app again). I asked them to check android documentation on White Listing there app and also enable auto-start from Settings->Permission->Enable autoStart and then check whether they get push messages. This generally happens on xiaomi phones and few other Chinese manufactured devices. I also pointed them to this thread on GitHub https://github.com/firebase/quickstart-android/issues/41 discussion on the same.
There are definitely bugs and some things i might have missed while reviewing the code. This review was just a brief overview what needs to fixed on urgent basis. There was lots of problems related to ui, locking screen orientation and use of non static inner classes. All these count and this is something i asked them to fix later. As some things needs more time and attention.
I also benefited from the review and learnt a bit about realm. Also thanks Gabor Varadi for his blogs on Realm which was a big help. Also thanks to https://stackoverflow.com/users/1843331/tim-castelijns ( Tim) who also helped by suggesting a few tips on Realm usage.