Add Order-in-Advance feature #202
Conversation
docs/UserGuide.md
Outdated
| e.g. for a command with format `add-i n/NAME q/QUANTITY`, <br> | ||
| replace `NAME` and `QUANTITY` with your input: `add-i n/Chicken q/10`. | ||
|
|
||
| * In the format of commands, square brackets are to indicate optional fields. The square brackets are not necessary.<br> |
There was a problem hiding this comment.
| * In the format of commands, square brackets are to indicate optional fields. The square brackets are not necessary.<br> | |
| * In the format of commands, square brackets are to indicate optional fields. The fields in the square brackets are not mandatory to input.<br> |
There was a problem hiding this comment.
Actually i meant to say that the square brackets themselves are not necessary bc that was what Aileen feedbacked to us
There was a problem hiding this comment.
Iirc that was because Aileen had a misunderstanding between the command format and the actual command. Saying square brackets are not necessary is wrong because if you put them in the actual command then it's wrong
| /** {@code Comparator} that returns a ItemComparator */ | ||
| Comparator<Delivery> DELIVERY_COMPARATOR = new DeliveryEndTimeComparator(); | ||
|
|
There was a problem hiding this comment.
This doesn't feel like it should belong in the interface, but in the implementation
| long min = Long.parseLong(minutes); | ||
| LocalDateTime endTime = LocalDateTime.now().plusMinutes(min); | ||
|
|
||
| return new Time(minutes, endTime.format(DateTimeFormatter.ofPattern("dd MMMM yyyy HH:mm:ss"))); |
There was a problem hiding this comment.
It's a little weird how many times you need to format it. Perhaps have the constructor take in a LocalDateTime instead of a String
There was a problem hiding this comment.
In one of my earlier commits I made that change, for the constructor to take in a LocalDateTime but that implementation had its own problems, such as every single class that needed to use Time had to import LocalDateTime as well as ensure proper formatting of its inputs before passing into the constructor (or else it would throw a DateTimeParseException).
Hence i reverted it to this implementation where Time takes in a String and all parsing is done within the class
| public Time(String minutes, String endTime) { | ||
| requireNonNull(minutes, endTime); | ||
|
|
||
| LocalDateTime time = LocalDateTime.parse(endTime, DateTimeFormatter.ofPattern("dd MMMM yyyy HH:mm:ss")); |
There was a problem hiding this comment.
You should make the pattern string a constant since it's being used multiple times
There was a problem hiding this comment.
replaced with string constant in commit:
dfe617d
| /* | ||
| try { | ||
| LocalDateTime.parse(endTime, DateTimeFormatter.ofPattern("dd MMMM yyyy HH:mm:ss")); | ||
| } catch (DateTimeParseException e) { | ||
| throw new IllegalValueException(Time.MESSAGE_CONSTRAINTS); | ||
| }*/ |
There was a problem hiding this comment.
For the other parts of the code, this section here is to check if the JSON info is valid (ie. is the data file corrupt). However, this does not work for this implementation of Time, as we are not storing user input, but we are storing the endTime. Hence we cannot pass the endTime field through the same isValidQuantity/isValidName/etc. method, as that regex is used to test user input not to test this.
I removed this because I am handling the parsing inside the Time class
There was a problem hiding this comment.
Would it be better to just delete it
There was a problem hiding this comment.
ok true, fixed in latest commit
| endTime = delivery.getEndTime(); | ||
| initialize(); |
There was a problem hiding this comment.
perhaps a better name instead of initialise to make it clear what it's initialising. (Maybe initialiseDeliveryCountdown)
There was a problem hiding this comment.
changed to initializeDeliveryCountdown() in commit:
e156dfc
|
Could I also request two more things
|
| /* No such thing as duplicate delivery | ||
| @Test | ||
| public void execute_duplicateDelivery_throwsCommandException() { | ||
| Delivery delivery = model.getDeliveryBook().getDeliveryList().get(0); | ||
| assertCommandFailure(new DeliveryAddCommand(delivery), model, DeliveryAddCommand.MESSAGE_DUPLICATE_DELIVERY); | ||
| } | ||
| }*/ |
There was a problem hiding this comment.
almost LGTM, can you just remove this test case
Issues resolved:
#128
#129
#198