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: 4
- Justification: The idea is not too creative by itself but we have never seen something similar done with java before.
- G2: How difficult was it to implement the project? (
0
: no challenge,1
: very easy,2
: easy,3
: fair,4
: complex,5
: insane)- Answer: 4
-
Justification: Judging from the size, we think VisualCodeEditor required the creators an important amount of energy and effort. The projects have many modules and never seem to end. As downside, we think the project suffered from the limited amount of time the creators disposed, since there are many undocumented parts, multiple code repetition and the use of deprecated dependencies. In our impression, it seems they rushed on delivery a big load of features, challenge that was almost successfully accomplished looking at the delivered product, but that in our opinion negatively influenced the code quality. We feel sceptical about the originality of the project: the firsts commits count 3000+ one committed insertions, so it should be later clarified:
- if it was all written by the authors, how did they managed to write working code with a marginal use of Git.
- if there were external sources, to provide the reference.
- G3: List 3 positive aspects of the project. Motivate your selection.
- 1. Graphical Features This project is much complex but it is possible to see (using it) that they put an incredible attention to the graphical features. From that aspect everything is nice.
- 2. Zip Export The visual code editor allows you to export the program you have built as a zip, which is awesome.
- 3. Full Of blocks This code editor contains many many blocks which allow to create good programs.
- G4: List 3 negative aspects of the project. Motivate your selection.
- 1. Documentation We feel this project needs more high level documentation, if the authors want to create a user friendly block programming application for Java. During the beginning of the review, we struggled understanding why was the project was named visual code editor until we saw it running.
- 2. Too Big and maybe too Ambitious This project is very big and it kept much time to be understood and probably we did not even understand everything. Probably it has been a too ambitious idea for the time we had (considering also having other exams and projects). In our opinion had they shipped less features, there would be less bugs, a cleaner code and a better architecture.
- 3. Does Not Run Unfortunately it seems that if you try to build a program in the editor with the blocks provided by this project, this does not run because "the jar is corrupted". So we did not manage to use it for its main purpose.
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. <Lambdas>
- Answer: 3
- Justification: They are overused in the project and in fact sometimes too big. Their use is in some sense justified but they could have been splitted creating methods. In some cases there are big lambdas inside other big lambdas, making them almost impossible to be read.
-
2. <Abstract Classes>
- Answer: 4
- Justification: When they are used, they seem helpful, but they could have been used more, abstracting more things in common by the classes which extends them. For example Node Types share VNode as an abstract class, but this could have abstracted more things and/or sub abstract classes could have been created to simplify some of these actual nodes (therefore creating VCreatorNodes, VLoopNodes, etc as abstract).
-
3. <Singleton>
- Answer: 4
- Justification: Its application is not obvious at the beginning, we found it and understood it because we did research on singleton pattern implementation but its presence is not straightforward. This is also because it is not documented, and is good practice to make present the use of a design pattern in a comment. Moreover they forgot to put the constructor of the class which uses Singleton private, meaning that that class could be instantiated again somewhere, ruining one of the features that Singleton offers. Nevertheless, its use seems right and, but this point, its application seems correct.
-
4. <Regular Expression>
- Answer: 3
- Justification: They work, but are very hard to understand since are not documented and magic constants.
-
5. <Serialization using GSon>
- Answer: 3
- Justification: it is not bad used but its potential is not exploited to the fullest. For example all the node types override toJSON() adding to a general JSON object particular properties. They could just have used the general method of GSon that is toJson to the abstract class which automatically should transform the instance to a json object. Instead they used a deprecated method of the library, which is unsafe.
-
1. <Lambdas>
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: 4
- Justification: it ignores Java file but .idea is tracked. It was supposed to be ignored but is present in the repo because they ignored if after having committed it. Or probably they should have added ".idea/" to ignore it.
- 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: 3
- Justification: it is a bit poor. It should have contained more explanation, also because also the documentation is poor and the project is very big. So they should have at least explained more things on the structure in order to allow to understand the code better.
Maven
- M1: Can you compile the project via Maven? (
0
: no,1
: yes, but...,2
: yes)- Answer: 2
- Justification: yes
- M2: Can you clean the project via Maven? (
0
: no,1
: yes, but...,2
: yes)- Answer: 2
- Justification: yes
- 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: 0
- Justification: No, neither with the jar, nor with a specific maven command. We used the IDE to run it.
- M4: Are the project's dependencies appropriately configured in
pom.xml
? (0
: no,1
: some,2
: yes)- Answer: 1
- Justification: the configuration presents the broken directory layout, for example.model.Runner or the definition of a external resource, which could have been avoided with standard layout.
- M5: Are all Maven plugins appropriately configured in
pom.xml
? (0
: no,1
: some,2
: yes)- Answer: 1
- Justification: The javadoc gives errors when you try to generate.
- M6: Does the project adopt Maven's standard directory layout? (
0
: no,1
: yes, but...,2
: yes)- Answer: 1
- Justification: yes but outside , there are doc/ folder and .idea/ folder which is not correct because there should be only src/ and target/ (which is correctly ignored). Moreover resources/ is inside main/java/ folder while it should be only in main/. Some test classes do not have Test name in their name. Also would have been better to have set up the com name to have a package root.
Testing
- T1: Are all tests passing? Run
mvn test
. (0
: no,1
: yes, but...,2
: yes)- Answer: 1
- Justification: yes, but with mvn test it only runs 3 tests while there should be 18. We do not know why.
- 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: 2
- Justification: the coverage is poor, not full. They have testes some important things for sure but other many classes, overall easily testable (because they offer static services), are not tested. For Example: Persistence class and all the Handler classes.
- 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, most do
- 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: 3
- Justification: they are not always clear and also because the code they refer to is not clear too. It is not easy therefore and they did not even use @DisplayName to make them more understandable.
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: 2
- Justification: documentation is very poor, there are only few methods documented. The project is really big and hard to understand, it should have required more javadoc.
- 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: 4
- Justification: the few javadocs are very useful the most of the times.
- D3: Can you generate documentation files for this project? Run
mvn javadoc:javadoc
. (0
: no,1
: yes, but...,2
: yes)- Answer: 1
- Justification: yes but there are errors
- D4: How adequate are the non-javadoc comments written throughout the code? (
0
: mostly inadequate,1
: average,2
: mostly adequate,3
: awesome)- Answer: 2
- Justification: there are some non-javadoc comments inside the code but they are useful and adequate in understanding what is happening in the code
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: In general the classes are properly formatted. However, it was forgot to set a horizontal line number limit: often the line length goes too way beyond the 100 characters, reaching infinite. Test classes are not named starting with the name of the class they are testing. Commit messages were describing the actions done, however we feel they could have used more commit messages, instead of inserting all the changes in one commit comment, which can make progress restore hard. Moreover some classes are written as follows "Handler_Assign_Operation" which does not follow the standard they used in the other parts of the code.
-
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: 0
- Justification: There were classes with too much duplication, for instance JsonIterator.java has the fragment:
if (node.has("next"))
startIterationOnNode(node.get("next").getAsString());
That is repeated at every condition. Also would have been better to use a switch case here. Moreover simple simplifications of the code could have been made, which are also suggested by the IDE. For example:
if (!((line = bufferedReader.readLine()) != null))
can become
if ((line = bufferedReader.readLine()) == null)
which makes it easier to read. Other repetitions comes up from a not completely correct use of Singleton pattern. Some methods in that class do this:
public static void addEnvVariableStatic(String key, Object value){
if(envInstance == null){
EditorEnv.getInstance();
}
envInstance.addEnvVariable(key, value);
}
which is wrong, because this getInstance() already checks if the instance is null, so that it can be instantiated. So this method could have been simplified as following:
public static void addEnvVariableStatic(String key, Object value){
getInstance().addEnvVariable(key, value);
}
-
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: 1
-
Justification: Not so easy due the fact that seems that the code was written without using a linter (indeed intellij shows many suggestions). We feel the majority of problems in the code, were caused by not using a linter during the development. Not only intellij has one, but there are also pluggable versions. This would have prevented:
- reduntant variables (eg. Handler_Variable.java);
- double negations in conditionals;
- unnecessary type injections in templates (eg. List list = new ArrayList();)
- the use of switch cases instead of long if-else;
- warings about possible code duplication.
-
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: 1
- Justification: As we said, there is much repetition, bad use of some code which we already specified. So yes we believe that these problems obviously cause inefficiency.
-
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
- Justification: No, but in the source code all the exceptions were muted without specifying.