Code Review 2 from Sam

[ssoperMadisonCollege]

Design/Code Review 2

Project: snagLog

Developer: Aaron Briggs

Reviewer: Sam Soper

Item	Criteria	Rating	Comments/Suggestions
Use the following scale: 1= It needs more work 2 = Somewhat 3 = Yes it is great	Reviewer comments and suggestions go here. Each item should have at least one "kudos" and two suggestions for improvement
Application Functionality	1. At least 25% of the application's target functionality has been built and is working. 2. In comments/suggestions field list and evaluate the functionality that has been implemented.	3	Aaron has most of the scaffolding for this project in place. All entities are created. He has implemented a generic dao and subsequent tests. He has working JSPs for user creation, password reset, login, logout, and admin accessibility with tomcat authorization implemented. Probably the next step is to look at integrating an API into the project.
Unit Tests	1. Code coverage stats meets or exceeds the developer's code coverage goal. 2. Unit tests are meaningful - they test the business logic and data access aspects of the application (simple methods like getters and setters are not tested) 3. In comments/suggestions field list the code coverage goal and actual current coverage percentage. 4. @Before/@BeforeClass/@After/@AfterClass used to reduce redundant code, like creation of entities. 5. A test database is used for unit testing and data is cleaned up/reset before or after the tests, so that the tests are re-runnable without developer intervention/cleanup.	2	Aaron can run his tests with high code coverage, easily meeting his goal for the entities he is currently testing. One suggestion would be to add unit test classes for some of the other classes in your edu.matc.controller and entity packages to broaden test coverage. I might be a bit ignorant of the way the generic dao testing works though, so this advice must be taken with a grain of salt.
JSPs	1. Templating is used to eliminate rundandant code(for example, header.jsp, footer.jsp, etc.) 2. CSS or a CSS framework has been implemented to give the site a professional look and feel 3. There is not any embedded java code in the JSPs (EL and JSTL is used appropriately).	3	Much of Aaron's de-duplication of JSP code takes place in the page_sections directory (within the webapp directory). Uses EL and JSTL well. I like the clear directory hierarchy for easy navigation. One suggestion would be to keep doing what you're doing, this looks great.
Code quality	1. Methods are single-purpose 2. Classes appropriately-sized classes (no monster classes). 3. Methods do not contain many branches or loops that could be simplified or broken up into smaller methods. 4. The same lines of code are not repeated at all 5. Uses interfaces/super/subclass relationships rather than repeating functionality in multiple classes? For example, using a generic/abstract dao. 6. Code does not contain any hard-coded values that should be in a properties file 7. Appropriate logging levels are used. Info, debug, error, etc. 8. There are not any System.out.println() or printStackTrace() statements in the code 9. Package, class, method and variable names are descriptive and meaningful. 10. Include in the comments section what technique you used to evaluate the items above. Using a code quality plug-in is highly recommended as a starting point for code quality reviews.	3	Aaron's code reads like a book, methods are concise and tend toward doing one thing at a time. Variables and methods and classes are well named. He uses a generic dao to reduce repetitive code. Makes appropriate use of logging. I would highly suggest using the code quality plugin as suggested to dial things in even more than they already are if time allows.
Web Service/API integration	1. Web service/api has been identified in the project problem statement/list of technologies. 2. Web Service integration has been built. 3. Web Service integration has been unit tested.	1	Aaron will be using google maps API to plot snag locations in the application. It has yet to be implemented but he knows where he's going with it.
Independent research topic	1. Topic has been identified in the project problem statement/list of technologies. 2. Topic research is complete and documented. 3. Topic has been implemented in the project	1	It looks like Aaron would like to integrate a weather service API into the project as an independent research topic, but it is not documented yet. One suggestion would be to document your independent research topic in your README, currently it resides in your project plan.
Key takeaways	List at least 3 things you learned while reviewing this project that will help you in your own project.	3	1) Generic dao seems to eliminate a lot of redundant code that I have in my project currently, I would like to implement this as well. 2) I like how Aaron has laid out headers, footers, sidebars and navigation sections in his JSPs and would like to do something similar. 4) Aaron has branched his project in git to have a development branch. I think this would be benficial for me to implement as well so I can have a cleaner commit history on my master branch. Great work Aaron!

[ssoperMadisonCollege]

Great job Aaron, very curious to see how your project turns out!