[#188] Adopt a uniform toString() format
#189
Conversation
|
Click here to submit a new iteration when this PR is ready for review. See this repository's contribution guide for more information. |
Eclipse-Dominator
force-pushed
the
188-adopt-a-uniform-tostring-format
branch
from
12f04bc
to
5db6be4
Compare
Codecov Report
@@ Coverage Diff @@
## master #189 +/- ##
============================================
+ Coverage 72.06% 73.61% +1.55%
- Complexity 399 420 +21
============================================
Files 70 71 +1
Lines 1235 1285 +50
Branches 127 126 -1
============================================
+ Hits 890 946 +56
+ Misses 314 307 -7
- Partials 31 32 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Eclipse-Dominator
force-pushed
the
188-adopt-a-uniform-tostring-format
branch
2 times, most recently
from
caa9b3a
to
460f200
Compare
v1@Eclipse-Dominator submitted v1 for review.
Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/189/1/head:BRANCHNAMEwhere |
|
Oh wow, blast from the past :-) Thanks for keeping my work alive! |
|
btw if you made any improvements you should absolutely claim some authorship using the |
Eclipse-Dominator
force-pushed
the
188-adopt-a-uniform-tostring-format
branch
2 times, most recently
from
dd25dfe
to
3806b69
Compare
It's actually not a typo, Github, automatically replaced it as a user mention so it auto corrected it to OverRide. I will enclose them with code tag to avoid this. |
v2@Eclipse-Dominator submitted v2 for review.
(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/189/2/head:BRANCHNAMEwhere |
@pyokagan Yes, finally we got around to working on your remaining unmerged PRs at AB4. |
|
@se-edu/tech-team-level1 please review this PR if you are around these days ... |
There was a problem hiding this comment.
The changes look good generally.
Other than the aforementioned issue with the Index Class, perhaps the returned strings could have more line separators since for classes like EditCommand it is lengthy (eg. seedu.address.logic.commands.EditCommand{index=seedu.address.commons.core.index.Index@22d9bc14, editPersonDescriptor=seedu.address.logic.commands.EditCommand.EditPersonDescriptor{name=null, phone=null, email=null, address=null, tags=null}})
There was a problem hiding this comment.
Awesome job on the pull request!
Here are a few things that jumped out at me:
ToStringBuilder Use: Really liking your use of ToStringBuilder. It's helping us keep our toString methods clean and easy to read.
New Utility Methods: The new utility methods you added, like the format in the Messages class, are super handy. One thing to think about is maybe adding some separators between the attributes to make the output easier to read.
Tests: Your tests are nice and succinct and generally follow good practices. One thing to keep in mind is we could be more descriptive with our test method names and maybe try out some more edge cases.
Code Style: There's some small stuff, like adding spaces around equals signs in toString methods, which could make our code even more readable.
All in all, your PR makes a lot of positive changes. Have a look over the detailed comments for more specific feedback and ideas.
| .append(" Address: ") | ||
| .append(person.getAddress()) | ||
| .append(" Tags: "); | ||
| person.getTags().forEach(builder::append); |
There was a problem hiding this comment.
Example Output: John Doe Phone: 12345678 Email: [email protected] Address: 123 Street Tags: [friend][classmate]
The Messages class serves as a utility class for formatting and storing user-visible messages. The class is utilized to create a user-friendly, readable string that represents a Person object. However, there are several aspects where this method could be improved:
-
Lack of Attribute Separators: The current formatting of the Person attributes doesn't include clear separators. This may lead to confusion when reading the output, particularly if a Person's name is erroneously interpreted as part of their phone number (for example, "John Doe Phone" might be misinterpreted as "John DoePhone"). Introducing distinct separators between attributes will greatly improve readability. A suggestion would be to include something like a colon (":") or a hyphen ("-") between the attributes.
-
Improving Code Cohesion: As it stands, the Messages class and ToStringBuilder class perform similar tasks, i.e., they both format objects into strings. To improve the code's cohesion, it might be beneficial to merge the Messages class's functionality into the ToStringBuilder class. This can help to keep all the string formatting logic in one place, making it easier for developers to find and use these APIs.
-
Enhancing Readability with ToStringBuilder: Since ToStringBuilder is already used in your codebase, it can be a good choice for implementing the format method, as it's designed for creating readable string representations of objects. The ToStringBuilder could be used to build the string with the added benefit of clear separation of fields, making the output easier to read.
There was a problem hiding this comment.
I appreciate the suggestion, but I have plans in a future commit/pr to move the Messages class to within logic package. As it currently stands, all functionalities of Messages class is related to displaying messages directly to the user via the GUI and it is right now exclusively used by the logic package. Thus, I feel that it is more appropriate to move the Messages class directly into the logic package instead of keeping it in commons package.
The purpose of ToStringBuilder is mainly for devs to easily identify object string representations and their related data in logs and unit testings etc. Thus, it mainly serves as a way to enable easy debugging and testing. To enable this, user-facing messages will thus need to be migrated to use a different formatting method which was what was implemented here.
I will definitely experiment adding additional purpose/functionalities to toStringBuilder based on your suggestion in 3!
There was a problem hiding this comment.
For 1, isn't the code already using ; as the attribute separator?
There was a problem hiding this comment.
For 1, isn't the code already using
;as the attribute separator?
The string output from the code when tested is as follows:
"John Doe Phone: 12345678 Email: [email protected] Address: 123 Street Tags: [friend][classmate]"
This output is used for user interface display. The separators between different elements (name, phone, email, address, tags) are crucial for readability and user understanding.
Consider the following examples:
"John Doe Phone" or "Address: 123 Street Tags: [friend][classmate]"
Without proper separators, the information can blend together and become confusing for the user. Therefore, I feel it's important to ensure that the separators are clearly defined and consistently used throughout the output.
There was a problem hiding this comment.
hmm... toString() is usually used for internal purposes (such as logging and debugging), to produce a compact string representation of the object. While it can be used for UI purposes, it might not be ideal as UI display have different considerations (easier reading, nicer formatting etc.). Is AB3 using toString() method of Person class for UI display?
There was a problem hiding this comment.
My apologies, I forgot to post a new iteration via CIHR sorry for the confusion, I have manually added the separator ; in the format.
Regarding adding multiple use case to the toStringBuilder, while it is possible I feel that the general use case for it might not as widely use. As mentioned before toStringBuilder was originally meant as an easy way to print out object string representation while formatting is related to UI side details. So after some tweaking of code, I feel that there isn't enough usage to justify expanding toStringBuilder to allow customizing its display format.
Perhaps it would be better to reconsider this when there are more use cases in the future which require similar formatting for user side message.
There was a problem hiding this comment.
hmm...
toString()is usually used for internal purposes (such as logging and debugging), to produce a compact string representation of the object. While it can be used for UI purposes, it might not be ideal as UI display have different considerations (easier reading, nicer formatting etc.). Is AB3 usingtoString()method of Person class for UI display?
Just realized that this method is specifically if for showring Person object details on the UI. So, the above doesn't apply.
Eclipse-Dominator
force-pushed
the
188-adopt-a-uniform-tostring-format
branch
from
3806b69
to
2afbbca
Compare
|
@Eclipse-Dominator if you updated the code, don't forget to post a new iteration using CIHR |
Eclipse-Dominator
force-pushed
the
188-adopt-a-uniform-tostring-format
branch
from
2afbbca
to
6a5003f
Compare
v3@Eclipse-Dominator submitted v3 for review.
(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/189/3/head:BRANCHNAMEwhere |
src/test/java/seedu/address/model/person/UniquePersonListTest.java
Outdated
Show resolved
Hide resolved
Eclipse-Dominator
force-pushed
the
188-adopt-a-uniform-tostring-format
branch
from
6a5003f
to
09d4b82
Compare
v4@Eclipse-Dominator submitted v4 for review.
(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/189/4/head:BRANCHNAMEwhere |
There was a problem hiding this comment.
Other than the small nitpick pointed out regarding the NameContainsKeywordsPredicate class, I have 2 suggestions that could be considered in a future PR:
- The output of the ToString() method could be better formatted although for the purpose of internal debugging and testing, the current format should be sufficient. My main gripe with the current format is the line length that could be fixed with line separators being called with the current StringBuilder. However, as suggested by @SPWwj, a separate method for formatting would be a better strategy since it would be less hard coded.
- I believe that classes like ExitCommand and ListCommand do not have a ToString() method, which I believe is a good decision since there is not much value added from it. However, it does feel inconsistent since objects from these classes will be printed as hash codes instead.
Ultimately, for the purpose of internal debugging and testing, the current iteration LGTM.
The criteria used for determining which classes to implement the
standardized toString() methods are:
* Must already implement equals()
This is a strong signal that the classes are "value classes", that
is, an instance is indistinguishable from another instance as long
as their states are the same.
Furthermore, these classes would likely be used with
assertEquals(...) / assertNotEquals(...) in tests. When these
assertions fail, the toString() representation of the
expected/actual objects would be printed, and thus developers would
benefit from the uniform toString() format which allows them to
compare the state of the objects.
On the other hand, classes which do not implement equals() would
likely not be used with assertEquals(...) / assertNotEquals(...),
and thus would not benefit from the uniform toString() format.
* Does not already have its own suitable toString() representation
For example, the toString() representations of Name, Phone, Email,
Address, Tag are suitable enough for them, as they already convey
the object's state, and thus would not benefit from the uniform
toString() format. These classes are also only designed to contain
one piece of information (e.g. for the Name class, a name, and for
the Email class, an email), and thus would not benefit from the
standardized toString() format which is designed for multiple pieces
of information.
Co-authored-by: Zhaoqi <[email protected]>
Commands may sometimes print a user-friendly representation of a person.
For example, when adding a person, we get:
New person added: John Doe Phone: 12345 Email: [email protected] Address: Dough Ave Tags:
The user-friendly representation (`John Doe Phone: 12345 Email...`) is
formatted using Person#toString(). This is not very ideal because it
means that the model is dealing with user-interface-related concerns,
which it should not be doing because that is the responsibility of the
logic and ui layers. Furthermore, using the toString() method for this
task means that Person#toString() can't be used for more useful
endeavours such as providing information for debugging purposes.
As such, let's introduce Messages#format(Person) which would be a common
method for formmating a Person for display to the user, and teach
commands that need that functionality to use it.
Co-authored-by: Zhaoqi <[email protected]>
Given that UniquePersonList is meant to be like a "list", let's make its string representation the same as Lists, by returning the string representation of its internalList.
Eclipse-Dominator
force-pushed
the
188-adopt-a-uniform-tostring-format
branch
from
09d4b82
to
8c6b9f2
Compare
v5@Eclipse-Dominator submitted v5 for review.
(📚 Archive) (📈 Interdiff between v4 and v5) (📈 Range-Diff between v4 and v5) Checkout this PR version locallygit fetch https://github.com/se-edu/addressbook-level3.git refs/pr/189/5/head:BRANCHNAMEwhere |
@cjyothika Outputs of toString() methods are often combined with outputs of toString() methods of other objects (i.e., when an object is a part of a bigger object structure). Therefore, we cannot assume it will be printed on its own and hence we cannot accurately predict where the best places to put line breaks and how much indentation to use. That is why it is typical for toString to be single liners of format |
|
Thanks to authors (@pyokagan @Eclipse-Dominator) and the reviewers. |
Fixes #188
Adapts parts of solution from AB4#974(se-edu/addressbook-level4#974)
Part of the PR that includes renames, file changes and relocation of variables unrelated to
toString()have been left out in this PR.As they can part of another issue