Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#188] Adopt a uniform toString() format #189

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/main/java/seedu/address/AppParameters.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import javafx.application.Application;
import seedu.address.commons.core.LogsCenter;
import seedu.address.commons.util.FileUtil;
import seedu.address.commons.util.ToStringBuilder;

/**
* Represents the parsed command-line parameters given to the application.
Expand Down Expand Up @@ -61,4 +62,11 @@ public boolean equals(Object other) {
public int hashCode() {
return configPath.hashCode();
}

@Override
public String toString() {
return new ToStringBuilder(this)
.add("configPath", configPath)
.toString();
}
}
10 changes: 6 additions & 4 deletions src/main/java/seedu/address/commons/core/Config.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import java.util.Objects;
import java.util.logging.Level;

import seedu.address.commons.util.ToStringBuilder;

/**
* Config values used by the app
*/
Expand Down Expand Up @@ -54,10 +56,10 @@ public int hashCode() {

@Override
public String toString() {
StringBuilder sb = new StringBuilder();
sb.append("Current log level : " + logLevel);
sb.append("\nPreference file Location : " + userPrefsFilePath);
return sb.toString();
return new ToStringBuilder(this)
.add("logLevel", logLevel)
.add("userPrefsFilePath", userPrefsFilePath)
.toString();
}

}
12 changes: 7 additions & 5 deletions src/main/java/seedu/address/commons/core/GuiSettings.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import java.io.Serializable;
import java.util.Objects;

import seedu.address.commons.util.ToStringBuilder;

/**
* A Serializable class that contains the GUI settings.
* Guarantees: immutable.
Expand Down Expand Up @@ -70,10 +72,10 @@ public int hashCode() {

@Override
public String toString() {
StringBuilder sb = new StringBuilder();
sb.append("Width : " + windowWidth + "\n");
sb.append("Height : " + windowHeight + "\n");
sb.append("Position : " + windowCoordinates);
return sb.toString();
return new ToStringBuilder(this)
.add("windowWidth", windowWidth)
.add("windowHeight", windowHeight)
.add("windowCoordinates", windowCoordinates)
.toString();
}
}
19 changes: 19 additions & 0 deletions src/main/java/seedu/address/commons/core/Messages.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package seedu.address.commons.core;

import seedu.address.model.person.Person;

/**
* Container for user visible messages.
*/
Expand All @@ -10,4 +12,21 @@ public class Messages {
public static final String MESSAGE_INVALID_PERSON_DISPLAYED_INDEX = "The person index provided is invalid";
public static final String MESSAGE_PERSONS_LISTED_OVERVIEW = "%1$d persons listed!";

/**
* Formats the {@code person} for display to the user.
*/
public static String format(Person person) {
final StringBuilder builder = new StringBuilder();
builder.append(person.getName())
.append("; Phone: ")
.append(person.getPhone())
.append("; Email: ")
.append(person.getEmail())
.append("; Address: ")
.append(person.getAddress())
.append("; Tags: ");
person.getTags().forEach(builder::append);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.

  2. 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.

  3. 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.

Copy link
Contributor Author

@Eclipse-Dominator Eclipse-Dominator Jun 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 1, isn't the code already using ; as the attribute separator?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Just realized that this method is specifically if for showring Person object details on the UI. So, the above doesn't apply.

return builder.toString();
}

}
7 changes: 7 additions & 0 deletions src/main/java/seedu/address/commons/core/index/Index.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package seedu.address.commons.core.index;

import seedu.address.commons.util.ToStringBuilder;

/**
* Represents a zero-based or one-based index.
*
Expand Down Expand Up @@ -51,4 +53,9 @@ public boolean equals(Object other) {
|| (other instanceof Index // instanceof handles nulls
&& zeroBasedIndex == ((Index) other).zeroBasedIndex); // state check
}

@Override
public String toString() {
return new ToStringBuilder(this).add("zeroBasedIndex", zeroBasedIndex).toString();
}
}
53 changes: 53 additions & 0 deletions src/main/java/seedu/address/commons/util/ToStringBuilder.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package seedu.address.commons.util;

/**
* Builds a string representation of an object that is suitable as the return value of {@link Object#toString()}.
*/
public class ToStringBuilder {
private static final String OBJECT_PREFIX = "{";
private static final String OBJECT_SUFFIX = "}";
private static final String FIELD_SEPARATOR = ", ";
private static final String FIELD_NAME_VALUE_SEPARATOR = "=";

private final StringBuilder stringBuilder = new StringBuilder();
private boolean hasField;

/**
* Constructs a {@code ToStringBuilder} whose formatted output will be prefixed with {@code objectName}.
*/
public ToStringBuilder(String objectName) {
stringBuilder.append(objectName).append(OBJECT_PREFIX);
}

/**
* Constructs a {@code ToStringBuilder} whose formatted output will be prefixed with the
* canonical class name of {@code object}.
*/
public ToStringBuilder(Object object) {
this(object.getClass().getCanonicalName());
}

/**
* Adds a field name/value pair to the output string.
*
* @param fieldName The name of the field.
* @param fieldValue The value of the field.
* @return A reference to this {@code ToStringBuilder} object, allowing method calls to be chained.
*/
public ToStringBuilder add(String fieldName, Object fieldValue) {
if (hasField) {
stringBuilder.append(FIELD_SEPARATOR);
}
stringBuilder.append(fieldName).append(FIELD_NAME_VALUE_SEPARATOR).append(fieldValue);
hasField = true;
return this;
}

/**
* Returns the built formatted string representation.
*/
@Override
public String toString() {
return stringBuilder.toString() + OBJECT_SUFFIX;
}
}
11 changes: 10 additions & 1 deletion src/main/java/seedu/address/logic/commands/AddCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
import static seedu.address.logic.parser.CliSyntax.PREFIX_PHONE;
import static seedu.address.logic.parser.CliSyntax.PREFIX_TAG;

import seedu.address.commons.core.Messages;
import seedu.address.commons.util.ToStringBuilder;
import seedu.address.logic.commands.exceptions.CommandException;
import seedu.address.model.Model;
import seedu.address.model.person.Person;
Expand Down Expand Up @@ -55,7 +57,7 @@ public CommandResult execute(Model model) throws CommandException {
}

model.addPerson(toAdd);
return new CommandResult(String.format(MESSAGE_SUCCESS, toAdd));
return new CommandResult(String.format(MESSAGE_SUCCESS, Messages.format(toAdd)));
}

@Override
Expand All @@ -64,4 +66,11 @@ public boolean equals(Object other) {
|| (other instanceof AddCommand // instanceof handles nulls
&& toAdd.equals(((AddCommand) other).toAdd));
}

@Override
public String toString() {
return new ToStringBuilder(this)
.add("toAdd", toAdd)
.toString();
}
}
11 changes: 11 additions & 0 deletions src/main/java/seedu/address/logic/commands/CommandResult.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

import java.util.Objects;

import seedu.address.commons.util.ToStringBuilder;

/**
* Represents the result of a command execution.
*/
Expand Down Expand Up @@ -68,4 +70,13 @@ public int hashCode() {
return Objects.hash(feedbackToUser, showHelp, exit);
}

@Override
public String toString() {
return new ToStringBuilder(this)
.add("feedbackToUser", feedbackToUser)
.add("showHelp", showHelp)
.add("exit", exit)
.toString();
}

}
10 changes: 9 additions & 1 deletion src/main/java/seedu/address/logic/commands/DeleteCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import seedu.address.commons.core.Messages;
import seedu.address.commons.core.index.Index;
import seedu.address.commons.util.ToStringBuilder;
import seedu.address.logic.commands.exceptions.CommandException;
import seedu.address.model.Model;
import seedu.address.model.person.Person;
Expand Down Expand Up @@ -41,7 +42,7 @@ public CommandResult execute(Model model) throws CommandException {

Person personToDelete = lastShownList.get(targetIndex.getZeroBased());
model.deletePerson(personToDelete);
return new CommandResult(String.format(MESSAGE_DELETE_PERSON_SUCCESS, personToDelete));
return new CommandResult(String.format(MESSAGE_DELETE_PERSON_SUCCESS, Messages.format(personToDelete)));
}

@Override
Expand All @@ -50,4 +51,11 @@ public boolean equals(Object other) {
|| (other instanceof DeleteCommand // instanceof handles nulls
&& targetIndex.equals(((DeleteCommand) other).targetIndex)); // state check
}

@Override
public String toString() {
return new ToStringBuilder(this)
.add("targetIndex", targetIndex)
.toString();
}
}
22 changes: 21 additions & 1 deletion src/main/java/seedu/address/logic/commands/EditCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import seedu.address.commons.core.Messages;
import seedu.address.commons.core.index.Index;
import seedu.address.commons.util.CollectionUtil;
import seedu.address.commons.util.ToStringBuilder;
import seedu.address.logic.commands.exceptions.CommandException;
import seedu.address.model.Model;
import seedu.address.model.person.Address;
Expand Down Expand Up @@ -83,7 +84,7 @@ public CommandResult execute(Model model) throws CommandException {

model.setPerson(personToEdit, editedPerson);
model.updateFilteredPersonList(PREDICATE_SHOW_ALL_PERSONS);
return new CommandResult(String.format(MESSAGE_EDIT_PERSON_SUCCESS, editedPerson));
return new CommandResult(String.format(MESSAGE_EDIT_PERSON_SUCCESS, Messages.format(editedPerson)));
}

/**
Expand Down Expand Up @@ -120,6 +121,14 @@ public boolean equals(Object other) {
&& editPersonDescriptor.equals(e.editPersonDescriptor);
}

@Override
public String toString() {
return new ToStringBuilder(this)
.add("index", index)
Eclipse-Dominator marked this conversation as resolved.
Show resolved Hide resolved
.add("editPersonDescriptor", editPersonDescriptor)
.toString();
}

/**
* Stores the details to edit the person with. Each non-empty field value will replace the
* corresponding field value of the person.
Expand Down Expand Up @@ -222,5 +231,16 @@ && getEmail().equals(e.getEmail())
&& getAddress().equals(e.getAddress())
&& getTags().equals(e.getTags());
}

@Override
public String toString() {
return new ToStringBuilder(this)
.add("name", name)
.add("phone", phone)
.add("email", email)
.add("address", address)
.add("tags", tags)
.toString();
}
}
}
8 changes: 8 additions & 0 deletions src/main/java/seedu/address/logic/commands/FindCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static java.util.Objects.requireNonNull;

import seedu.address.commons.core.Messages;
import seedu.address.commons.util.ToStringBuilder;
import seedu.address.model.Model;
import seedu.address.model.person.NameContainsKeywordsPredicate;

Expand Down Expand Up @@ -39,4 +40,11 @@ public boolean equals(Object other) {
|| (other instanceof FindCommand // instanceof handles nulls
&& predicate.equals(((FindCommand) other).predicate)); // state check
}

@Override
public String toString() {
return new ToStringBuilder(this)
.add("predicate", predicate)
.toString();
}
}
1 change: 1 addition & 0 deletions src/main/java/seedu/address/logic/parser/Prefix.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ public String getPrefix() {
return prefix;
}

@Override
public String toString() {
return getPrefix();
}
Expand Down
6 changes: 4 additions & 2 deletions src/main/java/seedu/address/model/AddressBook.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.util.List;

import javafx.collections.ObservableList;
import seedu.address.commons.util.ToStringBuilder;
import seedu.address.model.person.Person;
import seedu.address.model.person.UniquePersonList;

Expand Down Expand Up @@ -97,8 +98,9 @@ public void removePerson(Person key) {

@Override
public String toString() {
return persons.asUnmodifiableObservableList().size() + " persons";
// TODO: refine later
return new ToStringBuilder(this)
.add("persons", persons)
.toString();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import java.util.function.Predicate;

import seedu.address.commons.util.StringUtil;
import seedu.address.commons.util.ToStringBuilder;

/**
* Tests that a {@code Person}'s {@code Name} matches any of the keywords given.
Expand All @@ -28,4 +29,8 @@ public boolean equals(Object other) {
&& keywords.equals(((NameContainsKeywordsPredicate) other).keywords)); // state check
}

@Override
public String toString() {
return new ToStringBuilder(this).add("keywords", keywords).toString();
}
}
Loading