lonelyplanet/lpm-testapp-android — pull request #2 by sgowen (2014-04-22 14:11:22+00:00)
Title: Refactoring the Test App to better adhere to best practices

1. Android manifest is missing XML declaration
2. Android manifest is missing versionCode and versionName
3. Android manifest is missing the “uses-sdk” element (not needed for Gradle builds, but it is bad practice to assume that ALL developers are going to be using Android Studio).
4. Android manifest MainActivity redefines the label attribute. Unless the Activity is going to have a unique label, it should leave this option out and default to the application label.
5. Missing xml declaration in strings.xml
6. Missing xml declaration in styles.xml
7. Missing xml declaration in place_row.xml
8. The package name ends in “test”. Typically, this is reserved for unit tests (src/test/java/com/lonelyplanet/android/test)
9. Speaking of unit tests, there is no test harness set up for this application. This is generally bad practice both from a development and CI perspective. In test-driven development (TDD), we should always be thinking about how to refactor our application into testable units of code. This way, when we check in code, standard J-Unit tests can be ran quickly. On a nightly basis, we might configure instrumentation tests to run. These are a big longer to process since they require an actual running Android device or emulator.

~~ Moving on to the MainActivity class ~~

10. The class should be final. This sends a clear message to other developers that the creator of the MainActivity class was not explicitly planning for inheritance. This way, whenever someone goes to extend this class, they have to consciously remove this final keyword.
11. The String literals should be private static final. We use private because it is best practice to make our class members as restrictive as possible (rather than relying on implicit Java access modifiers). We use static because these String literals only need to be set one time as opposed to every time an instance of MainActivity is created. We use final because these values are not to be changed once they are set. That last part is very important, because these String literals are used to manipulate a database. These Strings should only be changed at pre-compile time (if/when the database needs to change) as opposed to run-time (which could result in logical errors while accessing the database).
12. The inner LoadPlaces class should be removed from the MainActivity, as it could be useful in other areas of the application. This also reduces clutter in the MainActivity class. I have moved this class into a new package named “asynctask” and have renamed it to “LoadPlacesAsyncTask”. It is generally a good idea to append the name of the class you are extending at the end of your subclass, so that users of the class can easily follow along. To use this new class, the MainActivity will need to pass in a callback that will be fired when the LoadPlacesAsyncTask completes in onPostExecute
13. The LoadPlacesAsyncTask calls Thread.sleep(5000) in doInBackground. This method of course is wrapped in a try/catch block. The bad thing here is that the catch block is empty. At the very minimum, you have 2 options when implementing catch blocks: log it or re-throw it. Doing neither is not an option.
14. The Place class has no place inside the MainActivity (no pun intended).
15. Since the callback from the LoadPlacesAsyncTask ultimately creates a subclass of ArrayAdapter that requires a context, we need to make sure that we cancel the running AsyncTask if the onPause method is called. Otherwise, we may potentially instantiate the PlacesAdapter with a now invalid context. We can always re-run the LoadPlaces task when the user returns by checking a simply boolean flag “arePlacesLoaded”.
16. In various places of the MainActivity class, no access modifier is specified. This is bad practice. As quoted from the Oracle Docs: “Use private unless you have a good reason not to.”
17. The Places table is referenced as a String literal instead of a private static final String member.
18. In various areas of the MainActivity class, I am seeing the prefix “m”, as in mContext. This is Java, not C++. Don’t use any form of Hungarian notation anywhere. But if you absolutely must use Hungarian Notation, then you should at least be consistent with its usage.
19. There was no reason for the PlacesAdapter to be included inside the MainActivity
20. All of the class members of the PlacesAdapter should be private final, since the class will not function without these members set
21. Removing the “List of places” comment. If you need a comment to know that “List places” is a “List of places”, then you probably do not need to be on the team. In general, code should be self-documenting. Before you ever write a comment in your code, ask yourself, “Is this comment an apology for bad code, or does it provide something meaningful?”. The best comments are those that express why things were done a certain way from a business perspective. Sadly, most developers write comments that are more or less pseudo code of the actual code. Obviously, this is not a maintainable practice.
22. Just as mentioned above, I have removed the “This method fills the list with the places” comment, since the method is named is fillList. I have also renamed the method to fillListWithPlaces.
23. I have also removed the “Database helper” comment, since the class itself is named DatabaseHelper.
24. I have moved the DatabaseHelper class to its own file to remove even more clutter from the MainActivity.
25. The Create Table Places query was a String literal. I have moved this to a private static final String member. I initialize this member in a static block beneath it using a StringBuilder. This method is encouraged, as it makes the creation of the String literal much easier to read (no more scrolling to the right of your IDE window).
26. I have also moved the "DROP TABLE IF EXISTS Places" query to a private static final String member
27. There were no unit tests in this application. After all of the refactoring I have done, I was able to create a “PlaceTest” class that runs assertions against the Place class. To run this test in Android Studio, open the terminal and navigate to the root folder of the project (“lpm-testapp-android”) and run ./gradlew unitTest --debug