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
Week4 #9
Week4 #9
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.
The algorithm looks good. Great job breaking down query methods. Some of the larger methods would benefit from breaking down as well as some of the methods are quite long. Also consider adding some comments :)
@@ -14,10 +14,120 @@ | |||
|
|||
package com.google.sps; | |||
|
|||
import java.util.Collection; | |||
import java.util.*; |
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.
Let's avoid folded imports
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.
Noted, I have changed it to
import java.util.HashSet;
import java.util.Iterator;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Collection;
|
||
/* | ||
public final class FindMeetingQuery { | ||
public Collection<TimeRange> query(Collection<Event> events, MeetingRequest request) { |
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 stray comments
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.
okay
walkthroughs/week-4-tdd/project/src/main/java/com/google/sps/FindMeetingQuery.java
Outdated
Show resolved
Hide resolved
public ArrayList<TimeRange> filterTimes(Collection<Event> events, HashSet<String> attendees) { | ||
ArrayList<TimeRange> times = new ArrayList<TimeRange>(); | ||
for (Event event: events) { | ||
HashSet<String> curr_attendees = new HashSet<String>(); |
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.
Use camelCase naming to keep it similar to rest of the variable names
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.
okay, changed curr_attendees
to currAttendees
} | ||
}*/ | ||
|
||
|
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.
extra newline
|
||
|
||
public final class FindMeetingQuery { | ||
public boolean hasMatch(HashSet<String> first, HashSet<String> second) { |
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.
first and second as argument names sound unclear as to what they hold
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.
Okay, what should I change it to? I saw on Event.java
file that they used a
and b
for this kind of compare function. Should I change it to a
and b
?
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 good :)
|
||
public final class FindMeetingQuery { | ||
/** | ||
Check if any of the string that appear in stringSet appear in otherSet |
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.
Move comment to same line as opening and closing comment of the comment
Complete requirement for week 4