Peer Review 2 (Erick Reyes)

[erickr19]

@tmirkes @pawaitemadisoncollege

Hello Tim! Our call going over our projects was definitely informative! My previous peer code review was asynchronous, but I prefer having them over some kind of call now! I feel like I got a good grasp on your project's functionality and, hopefully, this peer review benefits your project.

Overall, your project functions as it should when we called. I did try to test your project through your Elastic Beanstalk link and it seemed to work up until I tried to make a user account. I'm not sure if you were aware of this, but I thought I'd let you know!

To summarize my notes in the table below:

Consider removing your .idea and logs directory from your repo using the .gitignore file. These directories aren't absolutely needed for another developer to recreate your project. I highly suggest you remove your .ebextensions directory, privatekey.pem, public.crt, csr.pem, and cognito.properties. Having these exposed on your repo is a security risk, but adding them to your .gitignore file should resolve this.

This note isn't covered in the table but you could add JSTL to your JSPs in head.jsp. It would reduce the amount of time you would have to hardcode it on line 1 for your JSPs. I do remember you using the JSTL fn prefix as well. You might be able to put that in the head.jsp as well, but it would be redundant to do so for your JSPs that don't need it. That is a call for you to make :).

Make sure you add Javadoc comments to your entity classes' setter and getter methods. Also, document your controller classes as well as a comment for all your classes that describes your class's purpose.

You might want to analyze your project using QAPlug or your preferred analysis tool as well. I've run an analysis on my project as well and I find it very helpful to catch issues that I definitely overlooked.

I know you're still trying to catch up from your previous iterations of this project so maybe this is near the end of your task list, but make sure you add your project design files on this repo as well!

Can't wait to see your presentation!

Design/Code Review 2

Project: Trek Roulette

Developer: Tim Mirkes

Reviewer: Erick Reyes

Areas for Improvement	Criteria	Items Met or Exceeded
Make sure to include previous repo objectives and project specs	Project effectively utililizes the technologies and technques specified in the project objectives	Meets the specs stated in previous repos such as Hibernate, Maven, Web Services, Cognito, MySQL, Logging, and Testing
Planned MVP Functionality	Met own MVP
Logging framework used, i.e., no System.out.println() or printStacktrace() statements.	Met
Hibernate used for all data access.	Met
Authentication implemented.	Met
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
Deployed to AWS for public access.	Met
Unsure. However, project doesn't have much for text-based input.	Implements best practices (for example, data validation)
Synthesis of multiple concepts in unfamiliar situations requiring research beyond the scope of the class	Met. He had to research more into how to interface with the API he's working with than what our class covered.
Experiments individually, exhibits independence and drive, shows originality in the solution.	Met. When on call with Tim, he expressed how he derived the idea for a random number generator from old first person shooter games. I thought this was very cool!
Unsure as project specs haven't been updated.	Implemented technologies or techniques not covered in course materials.
Analyzed with QAPlug. Most issues are in reliability and usability but they aren't critical for the project's functionality. Mostly coding standard issues.	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.
Consider adding .idea, logs, .ebextensions, privatekey.pem, public.crt, csr.pem, cognito.properties to your .gitignore file. Just to remove your own development needs as well as sensitive information that could be a security risk.	Well-structured project
Descriptive naming of packages, classes, methods, variables	Met.
Classes appropriately-sized (no monster classes)	Met. No monster classes.
CPD (copy paste detection, meaning are the same lines of code repeated?	Met. No lines repeated.
Are there candidates for super/subclass relationships, abstract classes, interfaces are any values hard-coded that should be in a properties file?	Met. None seen. Already implemeted them where needed.
Proper exception handling	Met.
Proper error reporting to the user - custom error pages?	Met
Ensure documenting entity classes' setters and getters. Document class purpose. Also make sure you're documenting controller classes and their methods.	Code documentation
There could be some cases of this. Such as FetchEpisodeId.	Is there code in the servlet doGet/doPost that should be refactored into testable classes or methods?
Evaluate the JSPs for templating, data validation, overall look and feel.	Met. JSPs use templating and are formatted nicely.
JSPs use JSTL and EL, no java code	Met
Missing tests.	Unit tests are truly a unit test rather than a high level functional test
Missing tests.	Test data is appropriately cleaned up or handled
Missing tests.	There is full coverage of methods that perform business logic or utility functions
Missing tests.	Redundant code is eliminated by using set up and tear down methods, i.e., @Before, @After
Other comments/notes?	Project is looking good. Our call was helpful for figuring out how your project functions. It was more meaningful having a call than an asynchronous peer review.
Demonstrates initiative and thoughtful planning to leverage available resources (time, equipment, external expertise) to meet milestones and project objectives.	Met.
Evidence of significant revision and incorporation of feedback.	Met.
Project complexity	Met.
Additional Comments	Can't wait to see your final project and presentation! :)

[pawaitemadisoncollege]

Hi @erickr19 Thanks for sharing your thoughtful review. You did a nice job identifying some areas for improvement that will help @tmirkes make final improvements. As you continue practicing giving feedback, consider including a reason for each "rating". Sometimes it helps to think about it this way, "Met" because ... and then add a brief statement about how or why. In some cases you did this well: "Met. No lines repeated.", for example.