Create commons classes #69
Conversation
| @@ -0,0 +1,112 @@ | |||
| package seedu.address.model.person; | |||
There was a problem hiding this comment.
Hi, is there a reason why this package is within the person package?
| private final LocalDateTime endDateTime; | ||
| //Duration chosen over Period as Events are unlikely to exceed a day. | ||
| private final Duration duration; | ||
| private final Priority priority; |
There was a problem hiding this comment.
Is there a reason why you would choose to put the priority in the event and task separately? I foresee problems with setting the priority as they might have different priority. Perhaps we should put it in the item class?
| * @param duration A Duration of the event. Defaults to Duration.ZERO if null. | ||
| * @param priority A Priority of the event. Defaults to Priority.MEDIUM if null. | ||
| */ | ||
| public Event(LocalDateTime startDateTime, Duration duration, Priority priority) throws IllegalArgumentException { |
There was a problem hiding this comment.
Would it be better to overload the constructor than to allow null to be passed into duration and priority.
| if (this.task == null) { | ||
| return Optional.empty(); | ||
| } else { | ||
| return Optional.of(this.task); |
There was a problem hiding this comment.
Consider using Optional.ofNullable(this.task) to remove the need for the if-else loop.
| private Task task; | ||
| private Event event; | ||
| private Reminder reminder; | ||
|
|
||
| // Data fields | ||
| private ItemDescription itemDescription; |
There was a problem hiding this comment.
Should all of these fields be initialized to null even before the itemBuilder is run?
| if (newItem.getTask() == null && newItem.getEvent() == null && newItem.getReminder() == null) { | ||
| throw new IllegalArgumentException("Task, Event & Reminder cannot all be empty!"); | ||
| } |
There was a problem hiding this comment.
Is there a reason why you do this check here instead of at the constructor level?
| if (newItem.getItemDescription() == null) { | ||
| throw new IllegalArgumentException("Description must be provided!"); | ||
| } |
There was a problem hiding this comment.
Should we move this check to the constructor of item instead? Since in all cases, items cannot have a null description.
| /** | ||
| * Available priority levels for tasks and events. | ||
| */ | ||
| public enum Priority { |
There was a problem hiding this comment.
If I am not wrong, there is a need to implement comparable so that we are able to compare across the enumerations.
| * @param priority A Priority of the event. Defaults to Priority.MEDIUM if null. | ||
| * @param complete Denotes whether the task has been completed or not. Defaults to false if null. | ||
| */ | ||
| public Task(Priority priority, Boolean complete) { |
There was a problem hiding this comment.
Perhaps we should consider overloading the constructor again so that null will not be passed into the constructor.
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { |
There was a problem hiding this comment.
Is there a reason for this method? Is it a way to store in storage?
| } | ||
| } | ||
|
|
||
| public Priority getPriority() { |
There was a problem hiding this comment.
Perhaps you can create a setPriority() method also so that the user can edit the priority level?
Add Item, Event, Reminder & Task classes.