TA Feedback Project Part 4 - RostarSynergistics/ShinyExpenseTracker GitHub Wiki

Code base of prototype

misc.

• consider using a EnumMap<Currency, BigDecimal> for summing your expenses. You could cut down on the ExpenseTotalsAdapter code quite a bit!
• at some point your claims will probably get uuids (this is how elasticsearch handles identifying a document) so you can use those instead of passing indexes around, which is pretty fragile
• since a tag is uniquely identified by its string value, why not just use that instead of its index in the TagController?
• FileNotFoundException can be thrown when making a new file if you try to create it in a directory that doesn't exist (FilePersistenceStrategy.java)

good

• packages are nice
• only one warning in Eclipse
• persistence interface/implementation decoupling is nice
• comments on event handlers (eg "called when ... pressed")
• good comments, generally
• Log.wtf
• citations

what is holding you back from excellence

• magical numbers such as -1 for missing destination should be constants. eg NEW_DESTINATION
• refactoring opportunities in repeated code. Eg. DatePickerDialog creation, alert dialogs (these big chunks of android code really harm readability, too)
• a few style inconsistencies here and there. Mostly extra whitespace and weird braces on if/else stuff

Tangible Demo

misc.

• did you practice beforehand?

good

• your app seems to work well

what is holding you back from excellence

• a few mishaps here and there

Code Documentation

misc.

good

• exists
• you even documented some test stuff!
• examples
• covers lots of things

what is holding you back from excellence

• some javadoc is not properly exported (eg. AddExpenseClaimActivity)
• some missing javadoc (eg. IExpenseClaimListPersister does not have an interface-level comment).

Test Cases

misc.

• one test failure. Seems like that test is supposed to fail, but it fails a bit earlier than that..

good

• lots of tests
• mocks :)
• nice view tests

what is holding you back from excellence

• some tests are very big. It's less of a priority in tests, but I wonder if they could be refactored a bit to be smaller.
• you should assert that mocks are called as you would expect. This will allow you to make sure things are persisted properly

OOD

misc.

• that UML diagram is getting big!

good

• service locator pattern
• MVC

what is holding you back from excellence

• MVC could be a little cleaner, some activities have a bit too much controllery code for my taste
• in many activities the controller is only used to get a claim. After that point, your view handles the model changes itself which makes it sort of a controller.
• UML diagram has classes that are not in your code and will probably never be. Eg. ClaimDataExporter is obsolete considering the existence of IExpenseClaimListPersister, I would assume. If the persisters are not going to be used for ES, then they seem less useful
• UML class diagram has interface IModel instead of abstract base class
• Basically, UML looks like it was updated right before the due date, and you missed things. Your git commits seem to corroborate this.

Release Planning

Still very good. You figured this out early, and have continued to do a good job with it.

Addressing Feedback

misc.

• It is very hard to see what was done to address feedback.. From now on please make a note of it on the wiki

good

• goodbye FIXME
• fixed IModel/IView
• lots of testing
• added javadoc
• added binding data to UML