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

[W5][T12-1]Tejas Bhuwania #173

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Tejas2805
Copy link

added a new function which counts total number of people in AddressBook
updated parser test file and parser
updated other classes to maintain with the two new functions added
updated Message Display to show the two new functions added

added a new function which counts total number of people in AddressBook
updated parser test file and parser
updated other classes to maintain with the two new functions added
updated Message Display to show the two new functions added
@Tejas2805
Copy link
Author

@sreycodes can you help me review?

@ZhangCX10032
Copy link

The new command is very handy. It is much easier to get the size of the address book now. Maybe it is better if there are more small commits instead of a single big one.

@sreycodes
Copy link

Really useful PR once the address book starts becoming big. Great work!

import seedu.addressbook.data.person.Email;
import seedu.addressbook.data.person.Phone;
import seedu.addressbook.data.person.Name;
import seedu.addressbook.data.person.Address;

Choose a reason for hiding this comment

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

There should be a line break after the imports


Shows a list of the top ten persons, along with their non-private details, in the address book.
Format: 'topTen'

Choose a reason for hiding this comment

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

Did you forget to add total ?

int size = allPersons.size();
if (size < 10) {
return new CommandResult(getMessageForPersonListShownSummary(allPersons), allPersons);
} else {

Choose a reason for hiding this comment

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

Perhaps you don't need else here since you already have a guard clause

+ ": Displays the total number of people in the Address Book.\n"
+ "Example: " + COMMAND_WORD;

public static final String MESSAGE_SUCCESS = " Contacts found ";

Choose a reason for hiding this comment

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

There is a trailing white space in this string

/**
*Lists top 10 people in the addressBook
*/

Choose a reason for hiding this comment

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

Is this line break unintended?

/**
* Gets total count of contants
*/

Choose a reason for hiding this comment

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

Unintended line break here as well

import seedu.addressbook.commands.ViewAllCommand;
import seedu.addressbook.commands.ViewCommand;
import seedu.addressbook.data.exception.IllegalValueException;

import seedu.addressbook.commands.*;

Choose a reason for hiding this comment

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

Some IDEs automatically change imports into wildcard like this, which is against our coding standards. Please configure your IDE so that it stops doing this :)

|| ===================================================
|| Enter command: || [Command entered:topTen]
||
|| 0 persons listed!

Choose a reason for hiding this comment

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

I don't get it, why 0 instead of 1?

@@ -15,6 +15,7 @@
import seedu.addressbook.commands.AddCommand;
import seedu.addressbook.commands.ClearCommand;
import seedu.addressbook.commands.Command;
import seedu.addressbook.commands.TopTenCommand;

Choose a reason for hiding this comment

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

Hmm I don't think this import should be here as imports are sorted alphabetically

public void parse_topTenCommand_parsedCorrectly() {
final String input = "topTen";
parseAndAssertCommandType(input, TopTenCommand.class);
}

Choose a reason for hiding this comment

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

Did you forget to test TotalCountCommand ? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants