jhaefner97 / FishingGuide

Repo for Enterprise Java Spring 2024
0 stars 0 forks source link

Peer Review #5

Open Carlos-Leon98 opened 7 months ago

Carlos-Leon98 commented 7 months ago

Good morning @jhaefner97 and @pawaitemadisoncollege

I also owe you and apologies, due work I have been overloaded with a huge amount of task and barely had time to do other activities or task, but here I provide the peer review.

`# Design/Code Review 1

Project: Fishing Guide

Developer: Josh Haefner

Reviewer: Carlos Leon Rivas

Item Considerations Comments/Suggestions
Reviewer comments and suggestions go here. Each item should have at least one "kudos" and two suggestions for improvement
Problem Statement 1. Accurately describes project purpose
2. Is professional and free of typos, slang, etc.
3. Fully explains the problem and the solution
4. Is understandable by the average person
After Josh explained to me the problem statement of his individual project, it was clear to me from the beginning that the focus on improving the experience of all fishing fans, based on previous bad experiences with different applications, made him decide to create this application. Also providing a solution using a fish scoring program along with a user friendly interface.
Design Documentation 1. Navigation/flow through the application is logical and easy to use.
2. The order in which values are displayed are logical and easy to understand/use
3. The order in which the form fields entered are logical and easy to understand/use
4. All data discussed/documented (problem statement, flow, db design, etc.) is represented on the screens
As Josh mentioned previously, he had already created a similar program and uses that program as a base to create the logic for this application. At the time of the code review, Josh's main focus was more on the UI of the application, and then integrating the application logic. As a person external to the project I got the interface easy to use for a user who has an idea about the project as well as a user who has no idea about the project. The fishing score result is easy to understand. At the time I saw the application, the only input value was the location (OR the simulated input) which is easy for the user.
Data model/Database 1. Everything on the screens and problem statement/flow is represented in the model
2. There is at least one 1-to-many relationship.
3. The model represents good database design
We talked mostly about API integration at the time but we couldn't discuss much about databases. At one point in the code review, he explained to me that one of his tables has a one-to-many relationship, however, we did not discuss much about the design of the database.
Code 1. Proper Maven project structure is used
2. a .gitignore file for IntelliJ Java projects has been implemented
3. There is not any redundant or copy/paste code in the JSPs or classes
4. Classes are appropriately-sized (no monster classes)
Property files are used appropriately: no hard-coded values
5. Logging statements are used rather than System.out.println and printStackTrace.
6. There are appropriate unit tests/code coverage. 7. No sensitive information is visible in the repository (secrets, passwords, etc).
The structure of the project follows the Maven model as explained in the different activities of this class, including the .gitignore, and no redundant code within the JSPs. At the time, there was only one class, which had the setters and getters configured properly. I was not able to confirm if I had already implemented the loggers instead of System.out.println and printStackTrace, since the main focus was the UI, which was what I had made the most progress on. And at the time there was no discussion about any unit test which is already ready to test. `
pawaitemadisoncollege commented 6 months ago

Hi @Carlos-Leon98, thank you for sharing a bit about your thoughts from seeing (what I think was) a demo and discussion of the application. The expectation for this review is that you also take a careful look at all documents and code in Josh's repository - this generally happens after the code review meeting. From your comments, I am unsure if the happened or not. For example, in order to identify whether println statements are used, you can do a search in GitHub as show below. Screenshot 2024-03-13 at 2 42 53 PM.

Your review would be improved by including some suggestions for improvement: "Each item should have at least one "kudos" and two suggestions for improvement".

Carlos-Leon98 commented 6 months ago

Good morning @pawaitemadisoncollege,

During the code review call we had I got the chance to see problem statement and the goal Josh is trying to reach within his project, however, after the call, when I wanted to check his code more deeply, the code wasn't pushed at that point, so I couldn't take a look on all documents and code at the moment after we did the code review.

I took time to review the repository after the second commit and this is the new feedback I have.

`

Design/Code Review 1

Project: Fishing Guide

Developer: Josh Haefner

Reviewer: Carlos Leon Rivas

Item Considerations Comments/Suggestions
Reviewer comments and suggestions go here. Each item should have at least one "kudos" and two suggestions for improvement
Problem Statement 1. Accurately describes project purpose
2. Is professional and free of typos, slang, etc.
3. Fully explains the problem and the solution
4. Is understandable by the average person
I keep my Kudo about how easy is for the average person to understand the problem statement and the solution that Josh project tries to offer, as a suggestion I would include more information about the tech it is used for this application, and not a huge manul instruction of how to use the app, but at least a high level information of how the user will use the app.
Design Documentation 1. Navigation/flow through the application is logical and easy to use.
2. The order in which values are displayed are logical and easy to understand/use
3. The order in which the form fields entered are logical and easy to understand/use
4. All data discussed/documented (problem statement, flow, db design, etc.) is represented on the screens
The strucute of how the project is built is easy to understand, even for people that has no ideo about the project can easely go to the project structure and understand where each class can be located. One suggestion would be to complete the User Stories in case they are not complete, If you go to the UserStories.md, people can see that there are some empty points or at least points that aren't easy to understand for everyone. Related with ProjectPlan.md, it is easy to understand what the plan for developing the project is, I would only recommend to add due dates.
Data model/Database 1. Everything on the screens and problem statement/flow is represented in the model
2. There is at least one 1-to-many relationship.
3. The model represents good database design
Within the demo we had to show our projects, he showed me a form where the user should enter the information to run the application logic, I am not seeing that form in the repository so I would subject to add it. As I mention in my previous review, we didn't discuss much about database, I recall he mention a one-to-many relationship, but once I reviewed the code in the GitHub repository, there is nothing that suggest me that there is any one-to-many relationship within the project
Code 1. Proper Maven project structure is used
2. a .gitignore file for IntelliJ Java projects has been implemented
3. There is not any redundant or copy/paste code in the JSPs or classes
4. Classes are appropriately-sized (no monster classes)
Property files are used appropriately: no hard-coded values
5. Logging statements are used rather than System.out.println and printStackTrace.
6. There are appropriate unit tests/code coverage. 7. No sensitive information is visible in the repository (secrets, passwords, etc).
Keep my Kudos on the maven structure, and the way all classes and JPSs are short avoiding redundancy. I saw that there is the class Database.java where should connect the with the database, but based on what I saw in the repository, it is calling the Database.properties which is not in the repo, I check the .gitignore and it is not set in there so I would recomment to even add the .properties file or add it to the .gitignore, same happens with test code, the test code is not in the repository so I cannot provide my feedback on it. I would recomment to try doing micro commits, that way the progress on the project is visible for everyone.

`

I own you an apologies for the poor review I provided. I hope this one reachs the expectation or the requirements.

Thank you, Carlos Leon Rivas.

pawaitemadisoncollege commented 6 months ago

Hi @Carlos-Leon98 Thank you for taking some additional time to improve your feedback - I hope this will help @jhaefner97 in his work, and also help you think about items to include when providing feedback in the future. Remember there is more opportunity to practice giving feedback on the the professional development presentations in the student repository.