Skip to content
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 #49

Merged

Conversation

Nikhilalalalala
Copy link

Added necessary classes to represent the Applicants.

Copy link

@TheSpaceCuber TheSpaceCuber left a 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 comment. I think we can just stick to dates first? Or do you guys want to include timings as well.

public class ApplicationStatus {
public static final String MESSAGE_CONSTRAINTS =
"Application Status should only contain the words: processing, accepted, rejected. It should not be blank";
public static final String[] POSSIBLE_STATUSES = {"processing", "accepted", "rejected"};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we include pre-interview? I think processing means after the interview but no update yet right?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Royce. Maybe we can use an enum or separate class for this as well.

Copy link

@Ben-Hanan Ben-Hanan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor changes would be good to have but not necessary as we do not have a standardised way of doing things yet. Good job!

Comment on lines 17 to 18
protected Optional<InterviewDate> interviewDate;
protected ApplicationStatus applicationStatus;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment, but maybe it could be private instead of protected? Since we would just use getter methods to access these fields.

Comment on lines 41 to 52
public static boolean isValidInterviewDate(String date) {
try {
if (date.length() != 8) {
return false;
} else {
parseDate(date);
return true;
}
} catch (DateTimeException e) {
return false;
}
}
Copy link

@Ben-Hanan Ben-Hanan Oct 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can make use of the regex method used by the other classes in model? In this case the validation format would be "^(3[01]|[12][0-9]|0[1-9])/(1[0-2]|0[1-9])/[0-9]{4}$" for dd/mm/yyyy. I believe it would make our code seem more in line with the existing code from AB-3.

src/main/java/com/eva/model/applicant/Applicant.java Outdated Show resolved Hide resolved
public class ApplicationStatus {
public static final String MESSAGE_CONSTRAINTS =
"Application Status should only contain the words: processing, accepted, rejected. It should not be blank";
public static final String[] POSSIBLE_STATUSES = {"processing", "accepted", "rejected"};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Royce. Maybe we can use an enum or separate class for this as well.

* @param status
*/
public ApplicationStatus(String status) {
requireNonNull(status);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good use of existing functions.

Comment on lines 19 to 25
protected final Name name;
protected final Phone phone;
protected final Email email;

// Data fields
private final Address address;
private final Set<Tag> tags = new HashSet<>();
protected final Address address;
protected final Set<Tag> tags = new HashSet<>();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same goes for these. Don't think it should be protected.

@Nikhilalalalala
Copy link
Author

LGTM! Just one small comment. I think we can just stick to dates first? Or do you guys want to include timings as well.

Yeah, I think we can stick to dates first and build upon timings at a later stage.

@TheSpaceCuber
Copy link

LGTM

@Hou-Rui Hou-Rui merged commit 71acd40 into AY2021S1-CS2103T-W13-1:master Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants