Peer review from team DaRo
Review form
General
- G1: How creative is the project? (
0
: I've seen this on YouTube, 1
: not creative 2
: average, 3
: creative, 4
: very creative, 5
: wow!)
-
Answer: 3 creative
-
Justification: They implemented an existing game, which makes this less creative, but their implementation is well-thought-out. Additionally, the presentation and user interface is mostly pleasantly designed and creative.
- G2: How difficult was it to implement the project? (
0
: no challenge, 1
: very easy, 2
: easy, 3
: fair, 4
: complex, 5
: insane)
-
Answer: 3 fair
-
Justification: While the logic is trivially simple, the presentation, including animation and custom styling of components like the dialogue, certainly required more effort. As such, we recognize that the project is not as trivial as the simplicity of the base game might suggest.
- G3: List 3 positive aspects of the project. Motivate your selection.
-
1. The program has very good and pleasant UI design. We really like the animations and sound effects. Additionally, we appreciate the soundtrack.
-
2. The code is easy to understand. We were able to simply understand the code and follow its logic.
-
3. The code is well documented. Most methods, classes and fields have Javadoc comments that explain how and why they should be used.
- G4: List 3 negative aspects of the project. Motivate your selection.
-
1. The scope of the project, specifically the logic of the game, is very small for a team of three people. The project only contains 17 classes and many of them are relatively short. We understand that given the goal of the project there is not a lot that should be implemented, but maybe there could have been more functionality.
-
2. Many programming techniques that are mentioned in the
README.md
where not used correctly, or in a way that seams forced. We went out of our way to chose correctly applied techniques for the following evaluation. Overall, as a result of the simple nature of the application they were also used very rarely.
-
3. The separation of logic and UI could be improved. An example for this is the
Controler
class that handles game state and UI state, determining for example win conditions and setting icons.
Programming techniques
- PT1: Evaluate how well 5 programming techniques we learned throughout the course were adopted in the project. Assign a mark from
0
to 5
to each technique, where 0
means completely incorrect usage and 5
means perfect usage.
-
1. Collection
-
Answer: 5
-
Justification: Collections were only used three time in the whole application, and only using the
ArrayList
implementation. Two times they were used in the Deck
where they could have used a deque 😉 and once in Hand
.
-
2. Custom Exceptions
-
Answer: 2
-
Justification: While we accept that customs exceptions were indeed used, the custom exception includes no logic or state that is not present in the default
Excetion
class. Also, the exception is used only once, directly as the single statement inside the try/catch block handling it, making it completely useless.
-
3. Lambdas
-
Answer: 5
-
Justification: Lambdas were used correctly in the event listeners and sometimes for stream operations. In some places we would have preferred a simpler implementation, but this did not affect our evaluation.
-
4. File I/O
-
Answer: 5
-
Justification: File I/O is done correctly inside the
LogUtil
class to a minimal extent for logging. While the README.md
states that file I/O is used for loading images and sounds, this is achieved by using existing Classes that handle the file I/O and not directly.
-
5. Regexp
-
Answer: 5
-
Justification: Regexp is only used once inside
LandingPage
at line 233. We accept this as a correct usage of the technique, would however suggest that the pattern that was used is overly complex for such a simple task.
Git repository
- GR 1: How appropriate is the
.gitignore
file of the project? (0
: missing, 1
: very bad, 2
: bad 3
: average, 4
: good, 5
: very good)
-
Answer: 4 good
-
Justification: The
.gitignore
includes a lot of different files, mainly for IntelliJ. Unfortunately, they missed including some files like the .iml
files that should be included when using Maven. Additionally, they missed the log.csv
file that is generated by the program during execution. Some additional notes that did not impact our evaluation: Files generated by eclipse and the eclipse language server are not included in the .gitignore
.
- GR 2: How appropriate is the
README.md
file of the project? (0
: missing, 1
: very bad, 2
: bad 3
: average, 4
: good, 5
: very good)
-
Answer: 5 very good
-
Justification: The
README.md
includes all the information requested for the delivery of the project.
Maven
- M1: Can you compile the project via Maven? (
0
: no, 1
: yes, but..., 2
: yes)
-
Answer: 2 yes
-
Justification: Executing compilation work without errors.
- M2: Can you clean the project via Maven? (
0
: no, 1
: yes, but..., 2
: yes)
-
Answer: 1 yes, but...
-
Justification: Yes, but the
log.csv
file will not be removed. This might be intentional, but we felt that mvn clean
should also remove the logs.
- M3: Can you run the project via Maven? Check
README.md
on how to run the project. (0
: no, 1
: yes, but..., 2
: yes)
-
Answer: 2 yes
-
Justification: The project can be run using
mvn javafx:run
- M4: Are the project's dependencies appropriately configured in
pom.xml
? (0
: no, 1
: some, 2
: yes)
-
Answer: 2 yes
-
Justification: The dependencies seam to be appropriately configured.
- M5: Are all Maven plugins appropriately configured in
pom.xml
? (0
: no, 1
: some, 2
: yes)
-
Answer: 2 yes
-
Justification: The plugins seam to be appropriately configured.
- M6: Does the project adopt Maven's standard directory layout? (
0
: no, 1
: yes, but..., 2
: yes)
-
Answer: 2 yes
-
Justification: To us, it seems that the directory layout follows the standard.
Testing
- T1: Are all tests passing? Run
mvn test
. (0
: no, 1
: yes, but..., 2
: yes)
-
Answer: 2 yes
-
Justification: All test pass on Linux and Windows.
- T2: How well do the tests cover the code? (
0
: no tests, 1
: no important method, 2
: some important methods, 3
: most important methods, 4
: all important methods, 5
: 100% test coverage)
-
Answer: 3 most important methods
-
Justification: Given most of the code is in the UI, testing is only really reasonable for some classes and methods. That said, test coverage is only 14.8% of instructions (8.1% excluding test code). We might suggest adding tests for the
Deck
class and test the code inside the Controller
class by creating a better separation between UI and game logic. We note: That given the low amount of actual logic required for the game, test coverage will always be lower.
- T3: Do the tests actually verify the expected behaviour of the program? (
0
: no tests, 1
: useless tests, 2
: most don't, 3
: some do, some don't, 4
: most do, 5
: awesome tests)
-
Answer: 4 most do
-
Justification: While the test verify the correct behaviour of the tested classes and methods, we note that only classes that serve as an encapsulation of state that do not include any significant logic are tested. Furthermore, only the happy path and no edge cases or failures are tested, which is understandable given the simple nature of the tested classes.
- T4: How well can you understand what the tests are supposed to verify? (
0
: no tests, 1
: what is going on?, 2
: most are not understandable, 3
: some are understandable, some aren't, 4
: most are understandable, 5
: all tests are understandable)
-
Answer: 5 all tests are understandable
-
Justification: The test are very simple and easy to understand, also because they are very short and have good names.
Documentation
- D1: How understandable is the Javadoc written for classes, fields and methods of the program? (
0
: no documentation, 1
: very poor, 2
: poor, 3
: average, 4
: good, 5
: awesome)
-
Answer: 5 awesome
-
Justification: Everything is explained well, including method parameter and member variables.
- D2: How useful is the Javadoc written for classes, fields and methods of the program? (
0
: no documentation, 1
: irrelevant, 2
: little utility, 3
: average, 4
: useful, 5
: very useful)
-
Answer: 5 very useful
-
Justification: It is our understanding that the documentation is generally useful, and given that we have not used the code we can not comment further on the usefulness of the given information.
- D3: Can you generate documentation files for this project? Run
mvn javadoc:javadoc
. (0
: no, 1
: yes, but..., 2
: yes)
-
Answer: 2 yes
-
Justification: The documentation is generated without errors or warnings.
- D4: How adequate are the non-javadoc comments written throughout the code? (
0
: mostly inadequate, 1
: average, 2
: mostly adequate, 3
: awesome)
-
Answer: 3 awesome
-
Justification: Comments are mostly used to divide sections of a long method, but given the low complexity of the logic, other comments are mostly not necessary. We would have liked to see more comments in very lone methods like the constructor of the
Main
class.
Code quality
- Q1: Is the code style adopted throughout the project consistent? Consider how whitespace is represented (spaces or tabs), tab size, naming conventions for classes, methods and variables, indentation, braces usage, line width. See Google Java Style Guide for an example of code style guidelines. (
0
: no, 1
: yes, but..., 2
: yes)
-
Answer: 2 yes
-
Justification: The code style is consistent from what we have seen.
- Q2: How would you rate the project in terms of code duplication? (
0
: a lot of duplication, 1
: some duplication, 2
: barely any duplication, 3
: no code duplication / only justifiable duplication)
-
Answer: 3 no code duplication / only justifiable duplication
-
Justification: We did not find any significant code duplication, but we note that given the small scope of the project, there are not a lot of opportunities for duplication.
- Q3: How easy it is to understand how the program works by looking at the source code? (
0
: mostly hard to understand, 1
: some fragments are hard to follow, 2
: not hard, but not easy, 3
: easy to understand)
-
Answer: 3 easy to understand
-
Justification: The program is generally easy to understand. We would have liked to see a better separation between UI code and game logic, specifically in the
Controller
class. Also, long methods like the before mentioned constructor of the Main
class could have been split into multiple methods to improve understandability.
- Q4: Is any section of the program excessively inefficient? (
0
: mostly hard to understand, 1
: some fragments are hard to follow, 2
: not hard, but not easy, 3
: easy to understand)
-
Answer: 3 No excessive inefficacy
-
Justification: While we did not find any significantly inefficient code, we are not fully familiar with the codebase and can as such not comment any further.
- Q5: Does the program crash unexpectedly (e.g. by an uncaught exception)? (
0
: all the time, 1
: rarely, 2
: it happened once, 3
: never)
-
Answer: 3 never
-
Justification: During our testing, the program never crashed.