Peer Code Review 2

[OscarJohnson6]

Design/Code Review 2

Project:

Donation Log

Developer:

Darin Wellons

Reviewer:

Oscar Johnson

Areas for Improvement	Criteria	Items Met or Exceeded
	Project effectively utililizes the technologies and technques specified in the project objectives	Met, I don't see any problems in the technologies used.
	Planned MVP Functionality	Met, everything that was planned and add to functionality meets a minimum viable product and I don't see anything excessive in the user stories.
	Logging framework used, i.e., no System.out.println() or printStacktrace() statements.	Met, I dont see any System.out.println() or printStacktrace() but I can see that Log4J is used properly to log errors.
	Hibernate used for all data access.	Met, Both entity classes I looked at use hibernate annotations properly and successfully as was shown in our review.
	Authentication implemented and adds value to the application.	Met, In our review you mentioned needed to implement authentication but while writing my code review I saw that you had updated you repository and successfully added authentication.
	Consumes at least one web service or public api using Java.	Met, You mentioned and displayed that you are using a weather API successfully.
	Application is database-driven using full CRUD.	Met, In our code review you demonstrated doing each crud process for a donation log. Proving that it meets using full crud being able to read all the donation logs, delete a donation log, add a donation log, and edit a donation log.
	Database includes multiple one to many relationships.	Met, The database includes a one to a many relationship with one User have one or many donation logs.
	Deployed to AWS for public access.	Met, I can see that you have the .ebextensions and .plationform folders for deploying publicly to aws.
	Implements best practices (for example, data validation)	Exceeded, I saw that you were using regex to validate user inputs which I think exceeds what was expected and fits best practices.
	Synthesis of multiple concepts in unfamiliar situations requiring research beyond the scope of the class	Exceeded, In our review we talked about different concepts that required research, one that I remember you said you researched was limiting the amount of records queried.
	Experiments individually, exhibits independence and drive, shows originality in the solution.	Exceeded, I think your project is unique and interesting and I liked the way you combined the different aspects like the API links, calendar, and weather API response.
	Implemented technologies or techniques not covered in course materials.	Exceeded, Like I have mentioned an example of a technology used that wasn't covered in the course was limiting the amount queried back in the dao.
	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, All the methods make sense to me and their purpose is clear.
	Well-structured project	Met, The structure makes sense with a package/directory to divide up its own API, the weather API, cognito, and the donation log classes.
I noticed their classes ReadDelete and ReadUpdate for both Users and Donation classes, I wonder it could be combined into the Delete or Update classes.	Descriptive naming of packages, classes, methods, variables	Met, Aside from what I commented on everything make sense and was descriptive in naming.
	Classes appropriately-sized (no monster classes)	Met, Everything is appropriately sized.
I noticed in you dao class you had methods for the Donation and Users class. You could use the generic dao and pass each each class's type. Also theres a index and homepage jsp pages that I think are the same.	CPD (copy paste detection, meaning are the same lines of code repeated?	Met, Each method looks pretty different and I can't think of other ways to refactor besides what I pointed out in my other comment.
	Are there candidates for super/subclass relationships, abstract classes, interfaces	Met, You are using the PropertiesLoader interface when the application first loads.
	are any values hard-coded that should be in a properties file?	Met, You are using properties files for any hard coded values and I don't see any missing.
	Proper exception handling	Met, You are properly using Log4J for any exception messages and I don't see any miscall exceptions.
	Proper error reporting to the user - custom error pages?	Met, You have good error reporting but you could add a error page for a 404, I am missing the same.
I noticed a some doGet methods were missing comments. Also that both /* / and // and used for comment, I think javadoc prefers /* /	Code documentation	Met, Besides my other comment I think the documentation is good and properly describes what its for.
	Is there code in the servlet doGet/doPost that should be refactored into testable classes or methods?	Met, All of the doGet and doPosts seem fine to me.
	Evaluate the JSPs for templating, data validation, overall look and feel.	Met, You mentioned that you had a template you were using which you used effectively and looks nice.
	JSPs use JSTL and EL, no java code	Met, I didn't see any JSP's with Java code or the <?.
	Unit tests are truly a unit test rather than a high level functional test	Met, The coverage of the unit tests make sense to me.
	Test data is appropriately cleaned up or handled	Met, All the units tests make sense and clean up after each other.
	There is full coverage of methods that perform business logic or utility functions	Met, I can't remember if code coverage was ran during the review but I think you mentioned it works you just had to fix a part.
	Redundant code is eliminated by using set up and tear down methods, i.e., @Before, @After	Met, in the unit tests the @before is used to help set up each test.
	Other comments/notes?
	Demonstrates initiative and thoughtful planning to leverage available resources (time, equipment, external expertise) to meet milestones and project objectives.	Met, Your planning is clear and thoughtful of the course material it is also similar to what I did for my planning.
	Evidence of significant revision and incorporation of feedback.	Met, It seems you have been able to follow and implement feedback you have gotten from the code review and check points.
I noticed some commits messages had just update, you could add another part or two about what was being updated so you can easily reference them in future commits.	What does github history and insights demonstrate about the work on this project?	Met
	Project complexity
I think this project exceeds the user display project. Everything looks nice displaying to the user, it uses an api to give their user weather, and it effectively implements its own rest api to have another of getting all the donation logs.	Consider the user display exercise to be a simple project; how does this project compare?	Met
Having the weather API be able to get relevant weather information. Limiting the amount of results given back from the database. Verifying user inputs with regex.	What aspects of the project add complexity?	Met
I like a lot of the validation/verification that has been done to check the user inputs, so that no problems can happen.	Additional Comments

[pawaitemadisoncollege]

Hi @OscarJohnson6 - thanks for providing your review of @dwellons project. You provided some actionable details in the areas for improvement column. I noticed the feedback in the Met/Exceeded column all read "Met" - it would be helpful to provide some additional detail there to help the developer, and anyone viewing this, better understand how it was met. I realize that I too, sometimes do not provide enough detail in feedback - something I'm working on as well!

Did you happen to use any tools to help perform this analysis? A QA Plug report (or similar) like @dwellons included on your project would be helpful to have here.

[OscarJohnson6]

Hi, @pawaitemadisoncollege I updated the review. I didn't use any tools for the analysis I had trouble having it work with someone else's repository.