Doriane Olewicki: implementation of the two challenges #12
Conversation
There was a problem hiding this comment.
Hi @olewicki,
I have a few comments regarding your submission; these are general comments on the results and the quick glance I performed on your code:
-
The results are off, in terms of files,
diffstatfor example reports 1284 files over the 3 diff files we provide with the challenge, your challenge accounts for 921 files modified -
In terms of indentation, 2 spaces isn't really my preference, though it is a personal choice on which I don't really have a say; however, I do prefer when code is evenly indented, and in files such as
DiffResult.java, tabs and spaces are mixed, leading to weird indentation depending on the tab width (on Github for example, tabs are 8 spaces) -
The time results are expressed using
seconds, where they should bemilliseconds -
Some function calls have negative number of references, why is that?
-
In terms of organization, there is very few factorization, which coupled with the 2 spaces for indentation makes it hard for me to understand the code; nesting is quite deep also
-
I think handling errors systematically with a try/catch + print is inefficient when it comes to software reliability
-
Kudos for the
Makefile
DiffResult.java
Outdated
| int regions = 0; | ||
| //How many line were added total | ||
| int lineAdded = 0; | ||
| //How many line were deleted totla |
There was a problem hiding this comment.
I just noticed this typo was actually already in the DiffResult.go file. I corrected it in my file.
Main.java
Outdated
| StartTime = System.currentTimeMillis(); | ||
| System.out.println(computeAST()); | ||
| EndTime = System.currentTimeMillis(); | ||
| System.out.println(String.format("%s took %d s", "compute AST", EndTime - StartTime)); |
There was a problem hiding this comment.
I feel this was meant to be factorized; why the litteral string as format argument otherwise?
There was a problem hiding this comment.
I modified my code to have a factorization at that point, as well as in the rest of the code.
|
Regarding the number of files, I corrected it and found 1248 files. The error was in my regular expression were I forgot to grab the files without extensions. For the indentation, I forgot to verify it was uniform over the files. I am sorry for that mistake. The millisecond error was corrected. For the number of calls, I counted initially the number of added calls minus the number of removed calls, to have the net increase. I am not sure what the count should be so this time I put 3 versions of count, the first one counting each call (this one is the one with which I generated the results in out.txt) and two other versions are in comments and explained in the code it-self. Regarding the factorization, I didn't put factorization at first because I thought it was readable enough. But looking back at the code I see how wrong I was and I factorized it. Now the functions are methods of the DiffResult and AstResult classes. Some instances where added to those classes to ease the use of the methods and limit the number of parameters. I added the command "e.printStackTrace()" to the catches, which are in fact better practice than just printing the error. |
Good afternoon,
Here is my implementation of the two challenges. I am a student of Bram Adams, I know you were in contact with him.
Thank you in advance for the attention you will give to my implementation. There is some comments at the beginning of the README.md about my code.
Best regards,
Doriane Olewicki