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

[T10][F11-C1] #201

Open
wants to merge 1,035 commits into
base: master
Choose a base branch
from
Open

Conversation

ChaseTiong
Copy link

Ready for Review

Copy link

@bongbongbee bongbongbee left a comment

Choose a reason for hiding this comment

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

Make the judgement call to SLAP whenever possible. It makes the code much more readable.
The process methods found in Controllers are rather long. Consider SLAP-ing them further if you can.
Try to insert the @@authortags before any header comments if there's any so that header comments can be seen in the collated file.
Try to standardize the names of constant variables whenever possible.
Replace all magic numbers with constants
Make sure header comments start with uppercase letter.
Only found a few indentation problems in the test cases.

AboutUs.md was not updated. Refer to this sample

*phew
Keep it up guys 👍

# A0093907W
###### /java/seedu/todo/controllers/AddController.java
``` java
*/

Choose a reason for hiding this comment

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

missing header comments

}

@Override
public float inputConfidence(String input) {

Choose a reason for hiding this comment

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

missing header comments


@Override
public void process(String input) throws ParseException {

Choose a reason for hiding this comment

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

remove extra spaces here

private LocalDateTime parseNatural(String natural) throws InvalidNaturalDateException {
Parser parser = new Parser();
List<DateGroup> groups = parser.parse(natural);
Date date = null;

Choose a reason for hiding this comment

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

hmm can consider moving the first 2 lines into the try block. Usually codes in the try blocks are considered success cases

} catch (InvalidNaturalDateException e) {
renderDisambiguation(isTask, name, naturalFrom, naturalTo);
return;
}

Choose a reason for hiding this comment

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

SLAP this

LocalDateTime testDateTime = fromEpoch(currTimeMs + 1);
Date actualDate = DateUtil.toDate(testDateTime);

assertNotEquals(testDate.getTime(), actualDate.getTime());

Choose a reason for hiding this comment

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

indentation problem here

LocalDateTime testDateTime1 = fromEpoch(testEpoch1);
LocalDateTime testDateTime2 = fromEpoch(testEpoch2);

assertEquals(DateUtil.floorDate(testDateTime1), DateUtil.floorDate(testDateTime2));

Choose a reason for hiding this comment

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

indentation problem here


@Test
public void floorDate_nullTest() {
assertEquals(DateUtil.floorDate(null), null);

Choose a reason for hiding this comment

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

indentation problem here

}

private static LocalDateTime fromEpoch(long epoch) {
return LocalDateTime.ofInstant(Instant.ofEpochMilli(epoch), ZoneId.systemDefault());

Choose a reason for hiding this comment

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

indentation problem here

* TODO: Extract out method in AddController that can return task from command,
* and possibly remove the need to have taskToAdd.
*/
private void assertAddSuccess(String command, Task taskToAdd) {

Choose a reason for hiding this comment

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

good job in extracting out a util method here

louietyj and others added 30 commits November 7, 2016 23:03
Sorry instead of delete the file, I wipe out the content only
Sorry Update wrongly. Just realise
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants