-
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 model applicant #69
Branch model applicant #69
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.
Overall good quality of work. Just a few changes to be made before it can be merged
@@ -16,4 +16,5 @@ | |||
public static final Prefix PREFIX_INDEX = new Prefix("i/"); | |||
public static final Prefix PREFIX_LEAVE_START = new Prefix("ls/"); | |||
public static final Prefix PREFIX_LEAVE_END = new Prefix("le/"); | |||
public static final Prefix PREFIX_INTERVIEW_DATE = new Prefix("id/"); |
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.
maybe instead of coming up with a myriad of prefixes when we make a new command like ls, le, id, we can standardize a date input to always just use PREFIX_DATE which is d: or d/ or something along these lines?
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.
Sure!, we can discuss in upcoming meeting to standardise this.
src/main/java/com/eva/model/person/applicant/ApplicationStatus.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #69 +/- ##
============================================
- Coverage 49.56% 48.59% -0.98%
- Complexity 448 454 +6
============================================
Files 101 105 +4
Lines 2090 2175 +85
Branches 228 240 +12
============================================
+ Hits 1036 1057 +21
- Misses 1015 1079 +64
Partials 39 39
Continue to review full report at Codecov.
|
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.
minor optional change. LGTM
@@ -24,7 +22,7 @@ public ApplicationStatus(String status) { | |||
requireNonNull(status); | |||
status = status.strip().toLowerCase(); | |||
checkArgument(isValidApplicationStatus(status), MESSAGE_CONSTRAINTS); | |||
this.status = status; | |||
this.value = PossibleApplicationStatus.getStatus(status); |
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 a better way to process statuses from strings would be to use the valueOf
function of enums. i.e
PossibleApplicationStatus.valueOf(status.toUpperCase()); //throws Illegal argument exception if its not a valid enum
not really sure if it can be applied here as you would have to try catch either here or whatever is calling the constructor.
public static PossibleApplicationStatus getStatus(String status) { | ||
if (status.equals(RECEIVED.status)) { | ||
return RECEIVED; | ||
} else if (status.equals(PROCESSING.status)) { | ||
return PROCESSING; | ||
} else if (status.equals(ACCEPTED.status)) { | ||
return ACCEPTED; | ||
} else if (status.equals(REJECTED.status)) { | ||
return REJECTED; | ||
} else { | ||
throw new IllegalArgumentException(); | ||
} | ||
} |
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.
with relation to my previous comment, this can be removed i think but im not sure how the constructors and the enums tagged to strings work.
lgtm |
No description provided.