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

Collated for review #224

Merged
merged 8 commits into from
Nov 10, 2017

Conversation

tshradheya
Copy link
Member

No description provided.

@tshradheya
Copy link
Member Author

@okkhoy Would be great if you could review the code quality for /collated/main/tshradheya.md

Made a clean(almost) PR for that.

Thanks a lot :)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 90.879% when pulling f13122d on tshradheya:tshradheyaCollate into 70d39b7 on CS2103AUG2017-W14-B1:master.

Copy link

@okkhoy okkhoy left a comment

Choose a reason for hiding this comment

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

in general good code I would say!

import seedu.address.commons.events.BaseEvent;

/**
* Event to trigger reading and storing of image
Copy link

Choose a reason for hiding this comment

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

better comment would be Triggers event ...

return isRead;
}

public void setRead(boolean b) {
Copy link

Choose a reason for hiding this comment

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

b ? seriously?

import seedu.address.model.person.ReadOnlyPerson;

/**
* Event raised on 'select' command's successful execution
Copy link

Choose a reason for hiding this comment

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

may fix all header comments to follow the convention we specify

import seedu.address.model.person.exceptions.PersonNotFoundException;

/**
* Selects a person identified using it's last displayed index from the address book.
Copy link

Choose a reason for hiding this comment

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

selects or shows details?

|| (other instanceof ViewTagCommand // instanceof handles nulls
&& this.predicate.equals(((ViewTagCommand) other).predicate)); // state check
}

Copy link

Choose a reason for hiding this comment

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

errr... why so many blank lines at the end of file?

try {
index = ParserUtil.parseIndex(splitArgs[0]);
} catch (IllegalValueException ive) {
throw new ParseException(String.format(MESSAGE_INVALID_COMMAND_FORMAT, DisplayPictureCommand.MESSAGE_USAGE));
Copy link

Choose a reason for hiding this comment

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

can split this line

@coveralls
Copy link

Coverage Status

Coverage remained the same at 90.879% when pulling bbe5fc0 on tshradheya:tshradheyaCollate into 70d39b7 on CS2103AUG2017-W14-B1:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 90.879% when pulling bbe5fc0 on tshradheya:tshradheyaCollate into 70d39b7 on CS2103AUG2017-W14-B1:master.

@tshradheya
Copy link
Member Author

Code quality improved. Merging

@tshradheya tshradheya merged commit f4feddf into CS2103AUG2017-W14-B1:master Nov 10, 2017
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.

None yet

3 participants