Hi all, this is a code review to help you improve your codebase.
To be clear, this review will not affect your grade by itself, but at the end of the class part of your grade will be on the code, so this review should help you improve the code sooner rather than later.
Overall, the code is fine, but sometimes your intent is hard to follow.
Here are the main areas in which you can improve:
Package hierarchy. Your current hierarchy does not seem well-defined, there are a few packages that group some classes together, then a "fragment" package that contains some of the UI but not all of it (not even all of the fragments). Reorganizing your code into well-defined packages should help you clean up the app design by thinking about which modules need to depend on which other modules. One way to ensure packages have a meaning is to create a package-info.java file for each package, filling in the Javadoc; this will force you to write down what packages are about and thus to think about it.
Documentation. Some of your classes are well-documented, but many are not, which makes it hard to tell exactly what they are used for. We don't need you to write the description of every method argument, especially since that usually results in boring descriptions, but at least a description of each class.
Redundant layouts? What are the "_dev" layouts for? Are the activity layouts for which there is also a fragment layout still used?
Hardcoded layout values. You did a good job at ensuring there are no hardcoded strings so you could translate the app to French; you should do the same with other layout values such as margins. You may need a few resources for basic margins, but you should ideally find ways to layout elements that do not require specific margins or sizes.
User encapsulation. Your Account class exposes whether it's a Google account, and even provides a getAccount() method to get the Google account. It appears your code relies on the fact that you use Google accounts in the real app and non-Google accounts in tests. This means your tests aren't testing the exact same path as your real code, and your real code is aware of the tests, which is not very clean. Do you really need this? If you do, you could be clearer and literally have a "are we in tests" property, which would make the code easier to read.
Commented-out code. There is quite a lot of commented-out code in your repo, most if it being one or two lines here and there, but a few bigger offenders as well (e.g. DataReceiverMock in tests). If you think you'll need some code again in the future, you can either use git tag to keep track of which commit still contains a piece of code, or just paste the code into some kind of shared documentation like the repository's Wiki, and then delete the commented-out code from the codebase.
Here are a few more minor remarks that may help:
AccountFactory seems poorly named, it does not implement a Factory pattern, it looks more like some kind of adapter.
Your Callback interface looks a lot like the Java Consumer one, perhaps you can use it?
CachingDataSender is an odd interface; it has a set of 3 "alert" default methods that seem unrelated to the rest of the interface, could they be static methods instead?
There are a lot of TODOs; it's good to keep track of what to do, but are they all up to date? For some of them it may be easier to open GitHub issues.
AccountFragment does a catch (NullPointerException e), which is concerning, since this exception typically indicates a bug in your code... if the bug is in the Android SDK instead, add a comment to explain this.
Hi all, this is a code review to help you improve your codebase. To be clear, this review will not affect your grade by itself, but at the end of the class part of your grade will be on the code, so this review should help you improve the code sooner rather than later.
Overall, the code is fine, but sometimes your intent is hard to follow.
Here are the main areas in which you can improve:
package-info.javafile for each package, filling in the Javadoc; this will force you to write down what packages are about and thus to think about it.Accountclass exposes whether it's a Google account, and even provides agetAccount()method to get the Google account. It appears your code relies on the fact that you use Google accounts in the real app and non-Google accounts in tests. This means your tests aren't testing the exact same path as your real code, and your real code is aware of the tests, which is not very clean. Do you really need this? If you do, you could be clearer and literally have a "are we in tests" property, which would make the code easier to read.git tagto keep track of which commit still contains a piece of code, or just paste the code into some kind of shared documentation like the repository's Wiki, and then delete the commented-out code from the codebase.Here are a few more minor remarks that may help:
AccountFactoryseems poorly named, it does not implement a Factory pattern, it looks more like some kind of adapter.Callbackinterface looks a lot like the JavaConsumerone, perhaps you can use it?CachingDataSenderis an odd interface; it has a set of 3 "alert" default methods that seem unrelated to the rest of the interface, could they be static methods instead?AccountFragmentdoes acatch (NullPointerException e), which is concerning, since this exception typically indicates a bug in your code... if the bug is in the Android SDK instead, add a comment to explain this.