-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix overlap lesson bug #259
Fix overlap lesson bug #259
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.
Looks awesome overall, would it be possible to add in a json serialisable test for overlapping lesson times just in case?
/** | ||
* Returns true if the class contains a {@code Lesson} that overlaps with the given {@code lesson} | ||
* after removing {@code toRemove} from the list of lessons. | ||
* | ||
* @throws NullPointerException if the given {@code lesson} is null. | ||
*/ | ||
public boolean hasOverlapLesson(Lesson lesson, Lesson toRemove) { | ||
return lessons.stream().filter(l -> !l.equals(toRemove)).anyMatch(lesson::isOverlapLesson); | ||
} | ||
|
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.
Nice, I like this method of resolving the check issue. @junlong4321 perhaps you can use this for the edit-student
issue as well.
@ypinhsuan Would it be possible to overload the method instead of taking null for the second argument when it is an add operation?
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.
yep sure I think overload is better
# Conflicts: # src/main/java/tutorspet/logic/util/ModuleClassUtil.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.
Awesome work! LGTM!
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.
Nice 👍 just a minor suggestion.
@@ -94,10 +95,19 @@ private boolean hasDuplicateLessons(List<Lesson> lessons, Lesson lessonToCheck) | |||
return lessons.stream().anyMatch(lessonToCheck::isSameLesson); | |||
} | |||
|
|||
/** | |||
* Checks if a {@code lesson} timing overlaps with another {@code Lesson} in the list. | |||
* Overlapping lessons are detected by calling the {@code isOverlapLesson} method in {@code Lesson}. |
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.
Perhaps it might be good to specify class here?
* Overlapping lessons are detected by calling the {@code isOverlapLesson} method in {@code Lesson}. | |
* Overlapping lessons are detected by calling the {@code isOverlapLesson} method in {@code Lesson} class. |
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.
Nice 👍 just a minor suggestion
# Conflicts: # src/test/java/tutorspet/storage/JsonSerializableTutorsPetTest.java
Fixes #226