Peer Review #2

[JBostroem96]

@pawaitemadisoncollege @chstudebaker

Design/Code Review 2

Project: herobase

Developer: Cole Studebaker

Reviewer: Jimmy Bostroem

Areas for Improvement	Criteria	Items Met or Exceeded
Project effectively utililizes the technologies and technques specified in the project objectives
Planned MVP Functionality
Logging framework used, i.e., no System.out.println() or printStacktrace() statements.	No system.out.prints or printstacktraces. Some <% for the taglib.
Hibernate used for all data access.	As far as I can tell, Hibernate is used for all data access.
Authentication implemented and adds value to the application.	Authentication is used; a user can sign into the application. Since restricting access can be done differently depending on the application, I'm not sure how this program has it implemented. I saw a "only_users" JSP that's apparently restricted to users only, so it seems to be implemented as well.
Consumes at least one web service or public api using Java.	API is implemented.
Application is database-driven using full CRUD.	Application is using CRUD for the heroes, mostly visible in the controllers of the application.
Database includes multiple one to many relationships.	Yes. Going by the ERD there seems to be three of them.
If I recall correctly, I've seen his application deployed; however, it appears he doesn't have it linked in the student repos.	Deployed to AWS for public access.
Implements best practices (for example, data validation)	For data validation, HTML required is being used extensively. I'm not sure if there is any other form of data validation (not seeing any JavaScript for it, among other things).
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.
Implemented technologies or techniques not covered in course materials.	His application can upload images; I believe that counts as originality within the scope of the class.
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	His methods seem to be single-purpose.
I don't find the project easy to navigate. Perhaps it's just a terminology thing, but some of the directories aren't as self-explanatory as I'd like.	Well-structured project	I think the methods (namely the controllers) are well-structured with a fitting naming scheme; nearly all of them start with a verb, correlating to http verbs.
Descriptive naming of packages, classes, methods, variables	By the looks of it, variables, methods, and more, are appropriately named
Classes appropriately-sized (no monster classes)	In terms of Monster classes, I'm not too sure. It seems to be mostly free of them. Some of the controllers have three methods doing things; maybe that can be refactored (I'm not sure if that even counts as being monster classes, just pointing it out)?
CPD (copy paste detection, meaning are the same lines of code repeated?	CPD seems to be handled well
Are there candidates for super/subclass relationships, abstract classes, interfaces	The application implements the propertiesloader. I don't think there is anything else.	are any values hard-coded that should be in a properties file?
Proper exception handling	Exception Handling can be a broad thing. Perhaps it can be improved. For example, perhaps add some when interacting with the database/tables in various ways (such as CRUD). What if these processes fail (failures with constraints, truncated data, etc.)? Maybe in this case it doesn't matter, just pointing it out.
Proper error reporting to the user - custom error pages?	There are error pages as shown in the web.xml
Code documentation
Is there code in the servlet doGet/doPost that should be refactored into testable classes or methods?	Good question. I'm not sure. Some of them seem to be doing quite a lot.
Evaluate the JSPs for templating, data validation, overall look and feel.	While I'm unable to view the application visually, looking at the JSPs I like the identation, making it easier to look through it.
JSPs use JSTL and EL, no java code	They don't use any java code. There is the <% for the taglib.
Unit tests are truly a unit test rather than a high level functional test	Unit testing is good. It can be expanded upon by testing more. For example, some of them only test one variable. If extensive testing is to be done, maybe overriding the equals and hash methods can become ideal (to remove hard-coded stuff with less code).
Test data is appropriately cleaned up or handled	The clean.db has a lot of data in it. I'm not sure if that amount can hinder the flexibility of the application. But if it's all good it's all good.
There is full coverage of methods that perform business logic or utility functions	Does this refer to the coverage when unit testing? I didn't see this.
Redundant code is eliminated by using set up and tear down methods, i.e., @Before, @After	The unit testing is properly using BeforeEach
Other comments/notes?	When looking at the code, I like that he has added documentation beyond just the classes/methods. I think that being able to document more advanced logic can really help you out in the long run, especially when returning to a project weeks, months, or even years later.
Demonstrates initiative and thoughtful planning to leverage available resources (time, equipment, external expertise) to meet milestones and project objectives.	The project seems to be going well
Evidence of significant revision and incorporation of feedback.
What does github history and insights demonstrate about the work on this project?	The commits are very descriptive and possess the proper verbs. I think they can be improved by being more concise and shorter, and trying to stick to a single change rather than numerous ones.
Project complexity
Consider the user display exercise to be a simple project; how does this project compare?	This application seems more advanced. For one, it uses the full CRUD. Secondly, it can upload images (something I think we haven't covered in class), and the scope is larger overall, resulting in a more complete and overarching application.
What aspects of the project add complexity?	Image uploader, authentication, error pages, CRUD, and more.
Additional Comments

I think this project is looking good; it seems to cover the necessary ingredients for a successful CRUD-oriented application. I'm looking forward to seeing it in its final state soon.

[pawaitemadisoncollege]

Thanks @JBostroem96 for your review of @chstudebaker's project. The level of detail you provide shows that you put some good time into reviewing the code, and have a good eye for many of the details related to java web dev.

I had a question about this comment "While I'm unable to view the application visually..." Was there a reason you weren't able to access the deployed application or have it demoed to you in some way? I think being able to comment on the usability of the application would be beneficial.

I was also curious about "Does this refer to the coverage when unit testing? I didn't see this." Yes, the idea is that any tricky logic is unit tested. For example, if I had a bunch of calculations happening in a servlet's doGet, it would be best to put that code block in a utility method and unit test it (servlet methods like doGet, doPost, can be hard to test, so moving it into a reusable, testable class is ideal).