jvjohnson1 / TheFortress

A secure coding demonstration that is intended to be attacked.
0 stars 0 forks source link

Peer Code Review 1 feedback #4

Open yocarrotyo opened 3 years ago

yocarrotyo commented 3 years ago

Design/Code Review 1

Project: The Fortress

Developer: Julie Johnson

Reviewer: Caroline Hughes

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
Typeo in "comments striped from files..." - should be "stripped"
The assertion that the main purpose is a demonstration of actual security techniques (as opposed to a game or learning tool) is helpful. I am curious if anyone who gains the rank of leutenant would be able to see how you did this, for learning purposes.
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 text over image is a bit hard to read, I suggest a text box that has a semi-transparent fill color.
Consider making "the game" possible to win, or at least define what it means to win even if you hope it's impossible? I think it would be interesting to define the manner in which the captain can transfer his role to someone else without making the fortress vulnerable.
It could be interesting to change the visual as someone moves up in the ranks.
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
I think the rmove soldier process flow is really cool! If you find yourself struggling to communiate hit works, ow make a visio/flow chart.
As Simon mentioned, it's a good idea to make a database diagram at some point.
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 coverages.
Class structure seems straightforward--I was reading some of your slack threads about not doing an abstract class. You're already aware that the remove soldier process is one that will require lots of unit tests. Many opportunities for bugs. To get started coding, I highly recommend repeating the week 4 and 5 exercise using your project. It made me feel better to have some classes written without worrying about the entire database.