-
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
Branch fixes delete training 2 #93
Branch fixes delete training 2 #93
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.
LGTM.
return studentsList.stream() | ||
.filter(student -> student.getId().equals(id)) | ||
.findFirst() | ||
.get(); |
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.
if studentsList empty, is it safe that .get() will not throw a null pointer error?
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.
It's assumed here that there will always be the Id existing in the list, is there a scenario that this wont happen?
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.
makes sense, but might be good to include an assertion here in case of code changes in future
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 thanks, shall do that. We see how this find by id goes, best is we abstract it to a util method
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.
LGTM.
IMPORTANT: Search student by Id
Given how tricky it is to maintain immutable student objects in an implicitly mutable manner (we modify a lot of stuff) and especially how the "same" student is being contained in both student list and INDIVIDUAL training, going forward, we should point to students using Id values instead of equality checks.
A True Story Example - Major bug
ts-add 1 1
ts-add 2 1
delete-training 1
a. Loop through all students within T1 training list of students
b. Remove T1's date time from that student's training schedule
c. Update the
Model
usingmodel.setStudent(oldStudent, newStudent)
There will be a
StudentNotFoundException
for 4c because:.equals()
will be false ==> Student not foundT1 stores the older Alex object and will not match any other newer Alex object IF we use
.equals()
.Hence, the next best thing we can use is
Id
which was created to be unique and immutable.The bug above has been fixed.