Peer Code Review 2

[dwellons]

Design/Code Review 2

Project: Fitness App

Developer: Oscar Johnson

Reviewer: Darin Wellons

Feedback Tool Used: QAPlug Plugin

@OscarJohnson6 , @pawaitemadisoncollege

Areas for Improvement / Notes	Criteria	Items Met or Exceeded
	Project effectively utililizes the technologies and technques specified in the project objectives
Asterisks used in UserStories	Planned MVP Functionality	Met
	Logging framework used, i.e., no System.out.println() or printStacktrace() statements.	Met
	Hibernate used for all data access.	Met
	Authentication implemented and adds value to the application.	Met
apiNinja - exercises	Consumes at least one web service or public api using Java.	Met
	Application is database-driven using full CRUD.	Met
	Database includes multiple one to many relationships.	Exceeded
I see in your Checkpoint 3 that it is complete and you were able to remove your app from AWS	Deployed to AWS for public access.	Met
I'm not sure if I see anywhere the users input is checked before being entered into the database, but I could be missing it	Implements best practices (for example, data validation)
	Synthesis of multiple concepts in unfamiliar situations requiring research beyond the scope of the class
	Experiments individually, exhibits independence and drive, shows originality in the solution.	Met
Week 9 Issue, examples are given of resources that were researched and used.	Implemented technologies or techniques not covered in course materials.	Met
	Code quality - Evaluate code quality for the following and identify specific areas for improvement (class, method or line number). Be sure to list which code quality plugins/tools you used to assist with this analysis
	Single-purpose methods	Met
Your project looks well organized and none of the classes look like they are doing more than one thing.	Well-structured project	Met
The Java/fit/app package name was a little confusing	Descriptive naming of packages, classes, methods, variables	Met
	Classes appropriately-sized (no monster classes)	Met
In HealthCalculations.java, lines 206-211 and 222-227 have the same javadoc	CPD (copy paste detection, meaning are the same lines of code repeated?
	Are there candidates for super/subclass relationships, abstract classes, interfaces	Met
I dont see any hard coded properties, but I can see the certificate and key in the .ebextensions directory.	Are any values hard-coded that should be in a properties file?	Met
	Proper exception handling	Met
error.jsp	Proper error reporting to the user - custom error pages?	Met
	Code documentation
	Is there code in the servlet doGet/doPost that should be refactored into testable classes or methods?	Met
During our meeting you had mentioned needing to go through and fine tune your CSS	Evaluate the JSPs for templating, data validation, overall look and feel.
	JSPs use JSTL and EL, no java code	Met
You demonstrated when we met that you have almost 100% coverage.	Unit tests are truly a unit test rather than a high level functional test	Met
April 23rd commit message suggests that a class used for Development purposes was removed.	Test data is appropriately cleaned up or handled	Met
	There is full coverage of methods that perform business logic or utility functions	Met
Multiple @BeforeEach tags in different classes for setUp methods.	Redundant code is eliminated by using set up and tear down methods, i.e., @Before, @After	Met
The abbreviations in your Javadoc are confusing for me, for example bfp, bmr and tdee in the javadoc in HealthCalculationsTest.java. It might not be relevant if they are all referenced the same way.	Other comments/notes?
You have a complex project and have planned and executed effectively, to meet your goals and the class objectives.	Demonstrates initiative and thoughtful planning to leverage available resources (time, equipment, external expertise) to meet milestones and project objectives.	Met
Looking at Checkpoint 2 and the recommendations made and implemented. For example the GenereicDao	Evidence of significant revision and incorporation of feedback.	Met
I am still trying to remember also but using more branches	What does github history and insights demonstrate about the work on this project?	I can see that you have worked on the project consistently since you've started. You have descriptive commit messages that describe the work you've done and it would be easy to go back through if needed.
	Project complexity
	Consider the user display exercise to be a simple project; how does this project compare?	I think your project is much more complex than our user display exercise. You have multiple tables and types of information that you are working with, and then using an API to return exercice ideas to users based on their input. I think its a great project.
	What aspects of the project add complexity?	Generating a suggested exercise to the user based on criteria that is going to be different for each user. Organizing the application to recieve that information and to return information to the user in an organized and meaningful way.
	Additional Comments	I was impressed by the complexity of your database, the overall build quality and amount of thought you've put into your project. Its clear that you have put a lot of work into creating this and implementing feedback you've been given.

QAPlugScanResults CodeReview2.pdf

[pawaitemadisoncollege]

Hi @dwellons! Thanks for your review of @OscarJohnson6 's project. The level of detail you provided in some cases, such as using line number or specific issue references is really helpful in providing clear, actionable (when necessary) feedback. It shows you took time and care in reviewing those parts!

I appreciated that you attached the QA Plug results for thoroughness. It saved the dev and me time as we don't have to run the report ourselves. Some of the items in the QA Plug results provide some easy areas for best practices improvement. Helping the dev identify these would be one more way to provide valuable feedback. For example, unused imports and using array list rather than list seem like a couple easy wins.

Last thing! With regard to error reporting - this could be improved even further by adding that custom page to the web.xml to handle situations that might not be caught within the code itself. For example specifying the error page for 404 and 500 errors would be a nice touch. Sometimes those generic 500 error pages can expose things about our backend that we don't necessarily want users to see. More about how to do this in the indie project channel in slack, if you didn't see that yet.