pawaitemadisoncollege
commented
1 year ago Thanks @tmirkes for your thoughtful review of @bryanmk's indie project.
I have a couple things to add:
- [ ] it looks like there are two instances of the PropertiesLoader class - I don't think this should be necessary.
- [ ] there are a few sneaky print statements - those in the PropertiesLoader may need to be replaced with a throws, as we may not be able to add the Logger into an interface (throw the error to the class implementing the interface to handle). The print statements in the Database class should be replaced with logging.
bryanmk
Design/Code Review 1
Project: Questie
Developer: Matt Bryan
Reviewer: Tim Mirkes
2. Is professional and free of typos, slang, etc.
3. Fully explains the problem and the solution
4. Is understandable by the average person
- It might be useful to provide some details about the benefits of the site for a user new to WoW and the community around it.
- It could be a good detail to include links back to the official WoW resources, if only to avoid any weirdness with the IP usage if you choose to incorporate logos or other official art.
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
- Much of the documentation is still in its initial form, so it might be worth taking a break to flesh out the documentation a bit more with the explanation that is currently residing in mental map form.
- I know that the API entities have been throwing a wrench in the project, but I would definitely suggest coming back to document the entities and their purpose/place in the flow as it becomes clear. Get ahead of that end-of-project "now what does everything do" documentation rush.
2. There is at least one 1-to-many relationship.
3. The model represents good database design
- A fair amount of project data is coming from APIs; I might suggest as those are added to the project and begin to fit into the workflow, adding documentation to the existing database to describe how the API data fits into the tables might help in future maintenance.
- It might be worth considering a single table collecting all of the entities of a given class (quest, item, etc.) into a database representation. While this might be a bit of extra overhead, again, in the interest of future-proofing, having the data set centralized might not be a bad idea.
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.
- With so much of the project being reliant on the API and its derived entities, it will be a good opportunity to look for replicated code and potentially create some generic entities to manage the common attributes.
- Once the API entities have been implemented a bit more thoroughly, don't miss the opportunity to use unit tests to validate their functionality and interaction with your other hand-coded structures, including your AWS database.