-
Notifications
You must be signed in to change notification settings - Fork 4
TA Feedback Project Part 4
aewilson-ua edited this page Mar 26, 2015
·
1 revision
- Converting your Claim ids from UUID to String is weird. UUIDs are generally stored as integers so they can be very easily hashed/compared etc. Plus it is more semantic to use UUID instances to store UUIDs than String instances. (See also comment in OOD section).
- code style seems consistent
- ExceptionHandler makes code nice
- Semantic exceptions. This is very nice!
- code is readable and generally makes sense
- Classes/Interfaces/Enums generally split out where it makes sense
- Activities seem to mostly contain view code
- some repeated code (eg. index checking everywhere). This will give you good opportunities to refactor in part 5
- date.setYear(date.getYear() - 1900); ?? This is really weird and should at least have a comment
- adapters are views, not controllers
- Why is Tagmanager.manager static??
- commented out code in SaveItem. Just put a comment with a link to your references
- some places manage claims by index instead of ID (eg ExpenseManagerFragment)
- 94 warnings in Eclipse!! Many of them can be fixed automatically by Eclipse, and some of them are actually important. Try this: Right-click src/ -> source -> Organize Imports. Doint this on both your app and your tests removed almost 50% of your warnings
Very good, just a few things were a little broken.
A bit all over the place. Some classes have very few comments, while others go a bit overboard (eg. Claim.java). The rubric asks for the comments to be understandable by a 3rd party, and the lack of detail in some places hinders this.
-
you don't need parrot documentation. EG:
Claim.getStartDate() This will return the start Date for the claim
-
Indentation in Claimant comment makes it look like poetry.
-
Documentation was not properly compiled.. I'm not sure what happened there
- good comments on activities/fragments
- most classes have a comment describing them
- some classes have lots of method documentation
- Some comments that I think were meant to be javadoc were not properly exported. Don't forget to use /** instead of /* eg. LoadItem.java
- no code examples
- comments are just not very detailed, which makes it hard to understand the system just by reading the docs
- interesting methods such as ClaimList.setTagMan should be documented as well. How does ClaimList use the TagMananager
- same for Claimant.getPermittableClaims
- Nice comments on each TestCase
- Failing tests are all for unimplemented features, and known to be failing, obviously marked as such. This is great
- nice coverage of model and some controller stuff too!
- UML not too cluttered
- UML demonstrates good aggregation/composition understanding and includes multiplicities
- UML seems to cover your app well. (No Fragments or execptions, but that's OK)
- MVC is decent
- Some design patterns used and obvious (eg. Singleton, Facade (sort of .._))
- models should not talk directly to controllers. For instance, ClaimList.filter should be a method in a controller, not a model
- String claimID :( Consider using something a little more related to what it does, either a UUID class or a ClaimID class or something like that.
- I would not expect ExpenseItem to keep track of what currencies and categories are allowed. Use an Enum for the categories, and use the Currency.CurrencyEnum for the currencies! You should be using the Currency class in ExpenseItem.java instead of a String and a double.
- In UML: Listener is just floating off in space, but it is actually aggregated by ClaimList
- Some of your Singleton things are more like models than they are controllers
Very good. Nice overview on wiki
- ExceptionHandler.throwExceptionIfEmpty :)
- Used "Adressing Feedback" tag on inssue tracker
- Primitive obsession partially addressed
- Improved UML quite a bit
- Code became much cleaner