Project Peer Review by Jubble
Review by JUBBLE
General
-
G1: How creative is the project? (
0
: I've seen this on Youtube,1
: not creative2
: average,3
: creative,4
: very creative,5
: wow!)- Answer: 5
- Justification: Very original! I think there did an original reinterpretation of the classic "tasks scheduling app", which has the potential to make the organization of study sessions less stressing (particularly important during exam sessions) for who studies often with friends. Features a timer for focus mode!
-
G2: How difficult was it to implement the project? (
0
: no challenge,1
: very easy,2
: easy,3
: fair,4
: complex,5
: insane)- Answer: 5
- Justification: It seems it they put a lot of effort in it, adding Spring framework and a working website.
-
G3: List 3 positive aspects of the project. Motivate your selection.
- 1. Tech stack: The team did the tough decision to base the project on the spring framework, that helped them to make a good use of the MVC pattern. My impression as newcomer to Spring is, that they did a good use of inversion of control and of the design principles of this framework. To add the cherry on top (after having reflected on what my project lacked) I would have paired this framework with Lombook to generate getter/setters to have an even higher code quality.
- 2. CSV import/export: It is possible to upload Tasks and export them just using CSV import/export.
- 3. Friend and TaskTeams: it is very interesting that is possible to share tasks with friends.
-
G4: List 3 negative aspects of the project. Motivate your selection.
- 1. Unintuitive Run: it is not straightforward how to run the project, overall because it is needed to upload the database. But I know those processes are always hard to automate without containers (eg. Docker).
- 2. GUI inconsistency : the GUI is good but sometimes it completely changes in some website areas. It seems that the user is entering in a total different website.
- 3. Use of version control: All the commit messages didn't explained the modification and often many unrelated files were committed at once, leaving the history very dirty. Especially the first commit is very large, but I believe is due the fact they generated the project boilerplate with Spring boot (it was not specified in the messaeg). Also some code was commented: WePlanFileRepositoryTests.java has a commented test, better leave those to the version control.
Programming techniques
-
PT1: Evaluate how well 5 programming techniques we learned throughout the course were adopted in the project. Assign a mark from
0
to5
to each technique, where0
means completely incorrect usage and5
means perfect usage.- 1. <Custom Exceptions>
- Answer: 4
-
Justification: They created many exception types (extending RuntimeException in order to handle unwanted events in the app). But we feel that two things are not completely clear in the design of this module:
- why the exceptions had many different constructors?
- why AlreadySentRequestException extends a Throwable? Wasn't it better to extend another RuntimeException for consinstency? Anyway, all the exceptions were also well documented.
-
2. <Test Hooks>
- Answer: 4
- Justification: Test Hooks (BeforeEach and AfterEach) are properly used, but we would have isolated tests that depended by data in those hooks from self contained test, using different classes or maybe nested classes to make them more readable and mantainable.
-
3. <Interfaces>
- Answer: 4
- Justification: they are simple and containing frequently used queries. Sometimes their purpose is not much clear.
-
4. <Serialization (CSV)>
- Answer: 5
- Justification: It works as supposed, but maybe we would have used a different library with less boilerplate.
-
5. <Exception Handling>
- Answer: 2
- Justification: Exceptions are handled OK but their message errors are not clear the most of the times, and also not tested. Moreover one time a bad exception handled (happened probably due to a bad way in creating a task) just blocked me in every page, the only solution was to reset the database.
Git repository
-
GR 1: How appropriate is the
.gitignore
file of the project? (0
: missing,1
: very bad,2
: bad3
: average,4
: good,5
: very good)- Answer: 2
- Justification: There was a jar file in the .mvn folder that they ignored, but probably remained cached in the repo. In general, all .jar and .class are not ignored.
-
GR 2: How appropriate is the
README.md
file of the project? (0
: missing,1
: very bad,2
: bad3
: average,4
: good,5
: very good)- Answer: 4
- Justification: It is clear, concise and explanatory but I would have recommend more details about the general structure of the project, by describing each package, also to help people not familiar with Spring projects. Furthermore, more details on uploading the database to run the project would have been useful.
Maven
- M1: Can you compile the project via Maven? (
0
: no,1
: yes, but...,2
: yes)- Answer: 2
- Justification: yes we can
- M2: Can you clean the project via Maven? (
0
: no,1
: yes, but...,2
: yes)- Answer: 2
- Justification: yes we can
- 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: 1
- Justification: yes but there is no direct execution command. It is possible to build it and launch it though.
- M4: Are the project's dependencies appropriately configured in
pom.xml
? (0
: no,1
: some,2
: yes)- Answer: 2
- Justification: yes
- M5: Are all Maven plugins appropriately configured in
pom.xml
? (0
: no,1
: some,2
: yes)- Answer: 2
- Justification: yes
- M6: Does the project adopt Maven's standard directory layout? (
0
: no,1
: yes, but...,2
: yes)- Answer: 1
- Justification: yes but there is also .mvn/wrapper as a main subdirectory while maven apache guides says that there can only be two subdirectories at the very top of the mvn project that are src and target (which is obviusly ignored). Moreover in the tests they did not follow the exact folder structure of the source.
Testing
-
T1: Are all tests passing? Run
mvn test
. (0
: no,1
: yes, but...,2
: yes)- Answer: 1
-
Justification: yes but the tests fail when the server is not running because the app is searching for a
database connection. In our opinion:
- This had to be specified in the readme.
- The database had to be mocked, Spring provides annotations for this.
-
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
- Justification: No explicit tests on task services and on controllers.
-
T3: Do the tests actually verify the expected behavior 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
- Justification: Yes they check most important things about entities of the website
-
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 test are understandable)- Answer: 4
- Justification: They cover most important parts of the code, however the purpose of some test was hard to guess. I would have suggested the usage of @DisplayName where they could have provided a more detailed explanation.
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: 4
- Justification: In some portions the Javadocs are very clear! In some others, like explanation of entities it is poor and superficial.
-
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
- Justification: Running "mvn javadoc:javadoc" gives warnings on missing docs on classes and methods. However, the covered components were written because often abstracted the description of code to describe the method/class behavior in relation to the logic of the application.
-
D3: Can you generate documentation files for this project? Run
mvn javadoc:javadoc
. (0
: no,1
: yes, but...,2
: yes)- Answer: 2
- Justification: yes
-
D4: How adequate are the non-javadoc comments written throughout the code? (
0
: mostly inadequate,1
: average,2
: mostly adequate,3
: awesome)- Answer: 0
- Justification: The only non-javadoc comments is commented code. These cases are few and probably only due to distraction, but better avoid this bad pattern.
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 as an example of code style guidelines. (
0
: no,1
: yes, but...,2
: yes)- Answer: 1
- Justification: yes but there were important style violations: some methods were written in PascalCase, which should be only for classes (eg. CreateSimpleTask(), CreateTeamsTask(), CreateScheduledTask()). There were also methods in snake_case (eg. register_login). Use checkstyle next time.
- 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: 1
- First Justification: Tasks For example in TaskController.java. CreateSimpleTask(), CreateTeamsTask() and CreateScheduledTask() contain repeated code that could have been wrapped in a submethod called by all of them. I think also that Tasks had too many parameters and could have been simplified those using a builder. Also here taskMap should have been more restrictive (of type <String, Task> instead of <String, Object>. I find the package structure to be logical organized and there weren't unnecessary nested folders. I noticed too many magic constants along the app to define user areas and task names. This makes the code hard to mantain, so please refactor using enums and finals. Many methods were null unsafe, this could have been avoided elegantly using Spring's Null-Safe annotations.
- Second Justification: In GlobalExceptionHandler there are many repetitions. They could have used a private method taking advantage of the fact that every exception is child of RuntimeException (so they could have put runtime exception as parameter of this private function), by doing for example:
private ModelAndView exceptionHandler (RuntimeException ex) {
ModelAndView mv = new ModelAndView("error");
mv.addObject("Exception",ex.getMessage());
return mv;
}
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: 2
-
Justification: Mostly yes, also for someone who never used the Spring framework.
-
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: 2
- Justification: The method getUsers() in TaskService was too long. Its length could have been reduced: by creating a method named like Stream filterTask(Stream taskType) of this fragment:
List<SimpleTask> simpleTasks = taskRepository.findAll() .stream() .filter(task -> task.getTaskType().equals("Simple Task"))
The tests take too much time to run because need to interact with the database, as far as I know, Spring should provide methods/annotations to do this.
- 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: 2
- Justification: It never crashed and the exceptions seemed to be caught very well. But once, I have inserted a task that the website did not like (saying error) that completely blocked me in any page. I have also tried to create a new account but without any result. I had to reset the database to exit by this situation.