-
Notifications
You must be signed in to change notification settings - Fork 109
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
base: master
Are you sure you want to change the base?
[T10][F11-C1] #201
Conversation
There was a problem hiding this 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 @@author
tags 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 | ||
*/ |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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 { | ||
|
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; | ||
} |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
2c0f07e
to
ed011d9
Compare
Added menu items and key combinations for each view
CalendarItemFilter Part 1
CalendarItemFilter Part 2
Fix parseIsTaskEvent
Collated Codes
delete incorrect format collated codes
Sorry instead of delete the file, I wipe out the content only
Delete the file instead
Duplicated collated files
Fix UC21 - UC24 alignment
Update links for code written
Sorry Update wrongly. Just realise
Update links for code written
Ready for Review