-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add lesson class and fields #68
Add lesson class and fields #68
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 good :) Just a few minor nits.
// valid name | ||
assertTrue(LessonName.isValidLessonName("Lecture")); // alphabets only | ||
assertTrue(LessonName.isValidLessonName("Tutorial")); // numbers only | ||
assertTrue(LessonName.isValidLessonName("Laboratory")); // alphanumeric characters | ||
assertTrue(LessonName.isValidLessonName("Recitation")); // with capital letters | ||
assertTrue(LessonName.isValidLessonName("Sectional Teachings")); // long 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.
The test cases look good! Minor nitpick, the comments next to the valid test cases do not match what is being tested. Also, if the lesson names do not accept numerals in the name, you may want to consider changing the validation regex to only accept alphabets :)
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.
I removed these tests because Assignment already tests Name. :) Thanks!
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.
okie
assertFalse(Time.isValidDeadline("1200 1999-10-10")); // invalid time | ||
|
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 test case :)
import static seedu.address.commons.util.AppUtil.checkArgument; | ||
|
||
/** | ||
* Represents an assignment's module code in the address book. |
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 comment can be changed to just 'module code'? Since it is applicable to both lessons and assignments
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.
Amended. Changed to Task instead. :)
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! Just one small feedback
public class ModuleCode { | ||
|
||
public static final String MESSAGE_CONSTRAINTS = | ||
"Module codes should begin with 2 or 3 alphabets, have 4 numbers and end with an alphabet/number.\n" |
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.
I think module code might not have to end with an alphabet or number all the time right haha. maybe can phrase it '... have 4 numbers and may end with an alphabet.'
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.
Amended!
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! Minor nitpicks on the naming but nothing that can't be easily fixed in v1.2b :) LGTM
public Lesson(Name name, Deadline deadline, ModuleCode moduleCode) { | ||
super(name, deadline, moduleCode); | ||
requireAllNonNull(name, deadline, moduleCode); | ||
} | ||
|
||
public Deadline getDeadline() { | ||
return super.getTime(); | ||
} | ||
|
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.
Naming the deadline field time may be more appropriate for the Lesson class as it doesn't really make sense for a Lesson to have a deadline.
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.
Amended!
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 :)
No description provided.