redfoxanna / diamond-poems

Memorializing diamond poems
0 stars 0 forks source link

Code Review Ready #5

Closed jbjohnson2 closed 6 months ago

jbjohnson2 commented 6 months ago

Hi Anna. You really have a great start on your project and I enjoyed looking through it!

Design/Code Review 1

Project:Diamond Poems

Developer: Anna Kessler

Reviewer: Jennifer Cecil

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
Your problem statement does a good job describing your project, why you would want to create it and the solution you will implement.
You mention that you want to turn the hand-written poems into searchable text. Maybe you could elaborate on why it is important that they are searchable. If you decide to include a genre option, you could also talk about that being a way to search or browse the poems in your problem statement/solution.
I like that you included examples photos. It really helps with the understanding of why this is an important project.
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
You homepage design looks great. The flow is good and it looks easy to navigate. The flowchart you made is a nice representation of the flow of you application.
With regard to your edit wireframe, it might be nice to have the image that was uploaded show up in the area that says Edit OCR Text so that it is easy to edit the any mistakes. Also, you may want to consider if you want a user to have to login before adding a poem. If so, you may want to update the navbar to be different depending on if a user is logged in or not.
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
Your dataabase looks good and has a 1 to many relationship with users and poems. I am curious what the user_status field will be used for? Depending on the answer, you may want to add it to your login page along with a birthdate field.
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).
Your project structure looks good, as does your .gitignore file. You have started templating some redundant code in your jsps by separating out the head and the header. It looks like you added the genericDao and that you just need to implement it. I didn't see any system.out.printlns. I like how you have added TODOs for yourself in some of the classes. It might be useful to add basic TODOS every time you add a file even if you aren't ready to put any code in it yet.
pawaitemadisoncollege commented 6 months ago

Hi @jbjohnson2! Thanks for your thorough review of @redfoxanna 's project. You did a good job clearly outlining your observations and making thoughtful, actionable suggestions for improvement.

I did a quick search for "print" in this repo using Github's search feature and did find some that snuck in here: https://github.com/redfoxanna/diamond-poems/blob/1f56196dc7d4ce56cb318ac8fbf3ed006b40bbd9/src/main/java/com/redfoxanna/util/PropertiesLoader.java#L28