Feedback from CS2103/T and CS2113/T AY1819S1 students

[pyokagan]

To: CS2103/T and CS2113/T AY1819S1 students

Congratulations on completing your project! Now that you have spent a few weeks working with AddressBook-Level4, we would like to know your thoughts on the code base.

Specifically,

• Were there any issues with the code that made it difficult for you to implement your features?
  • I found it hard to write UI tests because...
• Were there any design decisions that you found puzzling?
  • e.g. "Why was Person suddenly made immutable in Level4? I want ReadOnlyPerson back!"
  • e.g. "Why is there Config when there is already UserPrefs?"
• Did you encounter any bugs with the code provided to you?
• Do you have any enhancement ideas?
  • e.g. "The parser should tell us about all arguments that were unsuccessfully parsed, not only the first one."
  • (Note: it is very unlikely we will be adding any new features)
• Others, if applicable.

Not quite applicable:

• Feedback on lectures/tutorials/pace etc. of CS2103/CS2113 (we are not the teaching team)
  • Give your feedback via the NUS Student Feedback Exercise instead.

This is a chance for you to get your burning questions answered, and to contribute towards improving later iterations of the AddressBook-Level4 code.

Note: we are not part of the CS2103/CS2113 teaching team, and so any feedback given by you will have no effect on your grade (positively or negatively). As such, we would appreciate your most candid feedback 😄

Alternatively, you may contact us or chat privately with us on Gitter.

(PS: We are always in need of help! Do consider contributing!)

[0WN463]

Inb4 Java 10 no longer available publicly.

I don't know, I feel the difference in love put into AB3 compared to AB4. So maybe updating it would be great. (Especially the documentation / usage of tools that are present in 4 but not in 3). I guess most of ya expected us to use AB4 instead of 3, but the issue was this cohort did not have prior Java knowledge so it was hard to grasp how AB4 work within the time span.

Were there any design decisions that you found puzzling?

Why the usage of prefixes to delimit arguments (other than making it easier to parse). Feels a bit unnatural. Cause I feel that if ya gonna use prefixes, then also prefix the index commands (delete i/1), cause I feel its a tiny bit of conflicting design ideas.

Did you encounter any bugs with the code provided to you?

Void commands (list) and index commands (view/delete) can accept trailing arguments (ie list garbage), which is considered a bug wrt CS2113T.

[tenvinc]

Did you encounter any bugs with the code provided to you?
There might be unhandled exceptions in AB4 which might cause the app to crash. In particular, for each field defined, there is a corresponding checkArgument function to double check whether the argument passed in is valid. However, this function returns IllegalArgumentException which floats up to the top but is not changed into any other exceptions that are handled.
One instance of this bug is in the storage methods. Storage for field checks the validity of the value even before the constructor is called. However, if for some reason the checks pass but the constructor failed, the app may not run as this exception is not handled.

[souless94]

Were there any issues with the code that made it difficult for you to implement your features?
I found it Hard to:

1. change the app to work without internet. This is due to the browser panel using webview. And the tests for the browser panel causing errors if i use app without internet.
2. change browser panel html file to fxml file with ease.
3. change person easily without touching a lot of the files.
4. disconnect undo/redo function from the new commands which may/should not have undo and redo capabilities.
5. hard to connect to the browser panel if i want to show something other than person properties.
6. change to cover entries for users , like for private entries eg passwords.

Did you encounter any bugs with the code provided to you? ( i am not sure if it was intentional):

1. I Got confused with inputs with 2 same prefixes causing the last entry to be entered only, (didnt fix it) Just accepted it.
2. Changing html file of browser panel cause the reload page in browser panel not to work (didnt fix it). Accepted it.
3. No error thrown for find command if undo was used after find command. (didnt fix it).
4. no hash checking for AB4 xml file. (didnt fix it).

[nianfei97]

General issues:

1) There are two different formats for the overriding of the equals() method in different classes, namely:

In Logic, e.g. AddComamnd:

    public boolean equals(Object other) {
        return other == this // short circuit if same object
                || (other instanceof AddCommand // instanceof handles nulls
                && toAdd.equals(((AddCommand) other).toAdd));
    }

In Model, e.g. Person:

    public boolean equals(Object other) {
        if (other == this) {
            return true;
        }

        if (!(other instanceof Person)) {
            return false;
        }

        Person otherPerson = (Person) other;
        return otherPerson.getName().equals(getName())
                && otherPerson.getPhone().equals(getPhone())
                && otherPerson.getEmail().equals(getEmail())
                && otherPerson.getAddress().equals(getAddress())
                && otherPerson.getTags().equals(getTags());
    }

Personally, I find the latter to be easier to read (and modify should the need arise), but it would have been better if it was just standardised across the entire project.

Also, a related point: it is usually recommended to override both equals() and hashCode() at the same time, but this is not done in most classes. Perhaps this could be looked into?

2) Defensive programming against null

There appears to be two different ways to prevent null values from being passed, namely requireNonNull(arg) and assert arg != null. I am sure that there are pros and cons of each decision (asserts can be disabled etc.), but it appears that this can be something that can be standardised.

3) Awkward style for switch-case

It's unconventional, IntelliJ doesn't like it when we try to indent switch and case to the same indentation level, and at times this seems to serve no purpose other than:
a) To drill home the point "follow the style guide or else", as taught in CS2113/CS2103;
b) To annoy us endlessly when IntelliJ tries to "correct" our indentation;
c) To annoy us again when our PRs fail the Travis build because of checkstyle issues

4) Unnecessarily stringent checkstyle rules

Particularly annoying are:
a) "No trailing whitespaces, even on blank lines" - IntelliJ inserts them automatically when pressing Enter while indented, so extra effort is wasted deleting said spaces
b) "No newline at EOF" - Lost count of how many rebuilds had to occur because of this "error"

5) focus_helpWindowNotFocused_focused test is always skipped due to "an illegal reflective access operation"

Not sure whether this counts as a bug, but if this test is always skipped and never runs, it seems to defeat its purpose. I have been ignoring it all this while and it never did cause any issues, but just thought it was strange. Never had the time to look into it either.

Overall, I feel that most of my "issues" are nitpicks and aren't major issues. The learning curve was definitely steep at the beginning, but working with AB4 has really taught me a lot, so big props to the developers and maintainers!

(Nov 13, 23:15) EDIT: Fixed some typos

[yamgent]

Thanks for your feedback @nianfei97. Am writing this post because there's something I would like to highlight for one of your feedback:

Particularly annoying are:
a) "No trailing whitespaces, even on blank lines" - IntelliJ inserts them automatically when pressing Enter while indented, so extra effort is wasted deleting said spaces
b) "No newline at EOF" - Lost count of how many rebuilds had to occur because of this "error"

Actually, IntelliJ has some settings that you can tweak, which automatically helps you to clean up the trailing whitespaces on save, and also add EOF if necessary.

They can be found in File -> Settings..., then Editor -> General:

Other than the fact that the corrections can be automated (which is why se-edu developers rarely have the checkstyle problems that you listed 😉), having these stringent style checks ensure that the diff shown by GitHub does not show changes to these unnecessary trailing whitespaces (which pollutes the diff and makes it harder to read).

Thanks for your feedback again, I hope you enjoyed the module!

[damithc]

Thanks for the detailed feedback @nianfei97 I would like to comment on the below as it was a style adopted before the current se-edu team joined the project.

3) Awkward style for switch-case

It's unconventional, IntelliJ doesn't like it when we try to indent switch and case to the same indentation level, and at times this seems to serve no purpose other than:
a) To drill home the point "follow the style guide or else", as thought in CS2113/CS2103;
b) To annoy us endlessly when IntelliJ tries to "correct" our indentation;
c) To annoy us again when our PRs fail the Travis build because of checkstyle issues

From what I remember, this style rule comes from Oracle's Java coding standard. It is certainly not the most common style -- I would imagine the rationale of the styel is to reduce indentation (the alternative style adds two levels of indentation to the code). Again, you can tweak Intellij to follow this style, or any other style you want. You are right about (a) but the other intended learning outcome is learning how to tweak IDE style settings.

[damithc]

1. I Got confused with inputs with 2 same prefixes causing the last entry to be entered only, (didnt fix it) Just accepted it.

@souless94 I'm commenting on the above just in case the current se-edu dev team is not aware of the rationale for this "feature", which is, it allows the user to correct certain types of mistakes in the earlier part of the command without having to use backspace. i.e., the user can simply type the incorrect field without breaking the flow of typing. It's kind of a hidden feature meant for advanced users.

[pyokagan]

@0WN463

Thanks, you raised a lot of real issues. :-)

Inb4 Java 10 no longer available publicly.

😛

Unfortunately moving AB-4 into a JDK-11+ future will require quite a bit of effort. If nobody steps up to do it soon we'll probably be going back to JDK-8.

I don't know, I feel the difference in love put into AB3 compared to AB4. So maybe updating it would be great. (Especially the documentation / usage of tools that are present in 4 but not in 3). I guess most of ya expected us to use AB4 instead of 3, but the issue was this cohort did not have prior Java knowledge so it was hard to grasp how AB4 work within the time span.

Yes, this is a problem too. Unfortunately keeping AB-3 in sync with AB-4 isn't a trivial affair either.

imo, in the near future (when we are forced to remove JAXB and hopefully EventBus is removed as well), AB-4 won't require much additional technical knowledge that is already required by AB-3, with the exception of GUI/system testing. I hope that CS2113 would try using AB-4 in the future.

Were there any design decisions that you found puzzling?

Why the usage of prefixes to delimit arguments (other than making it easier to parse). Feels a bit unnatural. Cause I feel that if ya gonna use prefixes, then also prefix the index commands (delete i/1), cause I feel its a tiny bit of conflicting design ideas.

Eh, I don't think its that bad to warrant a change. Its serviceable.

Did you encounter any bugs with the code provided to you?

Void commands (list) and index commands (view/delete) can accept trailing arguments (ie list garbage), which is considered a bug wrt CS2113T.

Yes, this can be fixed. Feel free to submit a PR.

[pyokagan]

@tenvinc

There might be unhandled exceptions in AB4 which might cause the app to crash. In particular, for each field defined, there is a corresponding checkArgument function to double check whether the argument passed in is valid. However, this function returns IllegalArgumentException which floats up to the top but is not changed into any other exceptions that are handled.

Yes, that is because its a programmer error. Something has gone very wrong if invalid data hits the model layer, so there is no point trying to accept it or gracefully recover from it.

One instance of this bug is in the storage methods. Storage for field checks the validity of the value even before the constructor is called. However, if for some reason the checks pass but the constructor failed, the app may not run as this exception is not handled.

The onus is on the storage layer to properly validate its data and fulfill all preconditions before passing it to the model layer. If for some reason the storage layer does not properly validate its data, then the storage layer's code should be fixed directly, rather than writing error handling code to work around it.

As an aside, the parsing layer has the same responsibility too -- it needs to validate user's input before passing it to the model for processing.

[pyokagan]

@nianfei97

2) Defensive programming against null

There appears to be two different ways to prevent null values from being passed, namely requireNonNull(arg) and assert arg != null. I am sure that there are pros and cons of each decision (asserts can be disabled etc.), but it appears that this can be something that can be standardised.

We followed https://docs.oracle.com/javase/7/docs/technotes/guides/language/assert.html

requireNonNull(...), checkArgument(...) is used for verifying preconditions in public methods. assert is used for verifying preconditions in private methods and verifying postconditions.

5) focus_helpWindowNotFocused_focused test is always skipped due to "an illegal reflective access operation"

Not sure whether this counts as a bug, but if this test is always skipped and never runs, it seems to defeat its purpose.

The test is only skipped in headless mode (because Monocle isn't perfect).

The "illegal reflective access operation" warning is another separate issue, which isn't really an issue at present. (Basically Java saying that our code may or may not break in the future :-P )

[pyokagan]

Other than the fact that the corrections can be automated (which is why se-edu developers rarely have the checkstyle problems that you listed wink), having these stringent style checks ensure that the diff shown by GitHub does not show changes to these unnecessary trailing whitespaces (which pollutes the diff and makes it harder to read).

Yes, it's a good habit to be aware of every single thing that is committed to version control, even down to the single byte. :-P Keeping every change as small and minimal as possible can help in e.g. identifying the exact source of a regression.

The whitespace/EOF checks are an automated way of allowing us to ensure that no unnecessary whitespace/EOF changes occur in commits.

[tenvinc]

As an aside, the parsing layer has the same responsibility too -- it needs to validate user's input before passing it to the model for processing.

I see. So the throwing of exception is something like an assert to ensure that the input is correct but the actual checks should be done beforehand. Then, just for curiosity sake, why the design choice to throw an exception instead of using the assert keyword provided by Java?

[pyokagan]

@tenvinc

Then, just for curiosity sake, why the design choice to throw an exception instead of using the assert keyword provided by Java?

I mentioned the reason in #931 (comment)

requireNonNull(...), checkArgument(...) is used for verifying preconditions in public methods. assert is used for verifying preconditions in private methods and verifying postconditions.

[davidchoo12]

Were there any issues with the code that made it difficult for you to implement your features?

I find that all components of AB4 are dependent on Model. So when we do changes to the model, everything crumbles and needs to be refactored to accommodate the new model. As my team decided to morph the project, we had to change all the model classes and spend a week refactoring all other components to use the new model. But I guess this issue just applies when morphing the project, which I think many teams did.

Another difficulty is adding dependency to Logic component (perhaps also applies to other components). For my project, I had to add a new component for certain (but not all) command classes in Logic to call. Judging by how Model (existing dependency of Logic) is a parameter in every command class, I thought the only way to inject the new component is to add the new parameter in the Command parent class but that means all command classes would inherit the dependency. In the end, I just make the new component singleton out of laziness and time pressure.
My point is adding dependency to Logic the normal way (adding new parameter to every command classes) seems tedious. If I were to add many other dependencies, doesn't that make the commands constructor long.

a) "No trailing whitespaces, even on blank lines" - IntelliJ inserts them automatically when pressing Enter while indented, so extra effort is wasted deleting said spaces

IntelliJ can display the whitespaces as dots.


Notice the tiny white dots in place of every whitespace.

[pyokagan]

@hidingmode

For my project, I had to add a new component for certain (but not all) command classes in Logic to call. Judging by how Model (existing dependency of Logic) is a parameter in every command class, I thought the only way to inject the new component is to add the new parameter in the Command parent class but that means all command classes would inherit the dependency.

Well, yes, if the LogicManager will pass your new component to all commands, then it is possible that any command could access or modify the component.

The dependency is already there, adding it as a parameter to every command just makes it explicit.

In the end, I just make the new component singleton out of laziness and time pressure.

There's still a dependency, but now it is implicit ;-) Now the entire codebase could modify the component, and readers can't tell unless they grep the source code for all uses of the singleton.

It's a workable hack under time pressure, but for the AB-4 codebase, probably not :-)

My point is adding dependency to Logic the normal way (adding new parameter to every command classes) seems tedious. If I were to add many other dependencies, doesn't that make the commands constructor long.

Yes, the constructor (or method signature etc.) will be long, but at least its showing the full picture, which is much more important.

(It may also indicate a design problem, e.g. two components could be combined into one larger component, but that really depends on the situation)

By the way, would Refactor -> Change Signature in IntelliJ help?