Peer Code Review 1 - Michael Nuenninghoff

[MNuenninghoff]

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 think that the problem statement does a really good job of articulating the problem and how your program will help solve the problem, in language that is highly understandable - I would remove the line about supporting the project (until there is a meaningful link to post there) - In the project technologies/techniques there are some TBDs and question marks, I would just make sure to update those as they are resolved/clarified
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	- your screen documentation was really well done! I was impressed with the quality of the screen design, and also very impressed with how you translated the screen design into reality! - Looking at the screen designs, I am not sure how the report screens are found. Is there a link that is missing? I am also wondering if there is a way to filter/edit the reports before printing or saving - a minor note, but I noticed that there are two ScreenDesign folders, I think the empty folder could be deleted
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	- the database is simple and easy to understand, while still representing everything on the screens and the project flow - One suggestion that I have is removing passwords from the user table, but I think that this will be covered with AWS - I wonder if there would be any utility to adding a organization or location level to the database. Like if a food pantry had multiple locations maybe you would want to see donations by location. I think that is more of a future feature though rather than anything immediately relevant
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).	- I saw that you have edited the .gitignore file to filter out the properties and other database files, as well as the log files. That is something that I definitely need to do in my own project! - I think that the DonationDAO and UsersDAO can probably be combined into a generic DAO - In your unit tests, the creation of a DAO in each test could be moved to the setup method, if you make an instance variable of the DAO.

Overall I really struggled to find suggestions for improvement, so far the donation log project looks awesome and I am impressed by all of the work you have already done!

[pawaitemadisoncollege]

Hi @MNuenninghoff Thank you for your thorough review of @dwellons project so far. It's clear you put in some good time and thought here. You did a nice job observing and identifying what is great and any opportunities for improvement, and provided that information in clear and concise way.

With regard to storing passwords on the DB, you are correct, Cognito will handle this for us (I realize you both probably already confirmed that with the week 7 exercise and I am late to the game, but also didn't want to leave that point "unaddressed").

I did a quick search for the word "print" and noticed a couple files where logging should be implemented instead.