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

Add Internship, Application and Item classes #98

Merged
merged 19 commits into from
Oct 2, 2020

Conversation

keanecjy
Copy link

@keanecjy keanecjy commented Sep 30, 2020

Include for data fields and Item list

@keanecjy keanecjy added this to the v1.2 milestone Sep 30, 2020
@keanecjy keanecjy self-assigned this Sep 30, 2020
Copy link

@shawn-nyk shawn-nyk left a comment

Choose a reason for hiding this comment

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

The code looks great, thanks for the great effort and for doing this up so swiftly! I've added some questions/suggestions, and apologies if I misunderstood anything. They mostly stem from some concerns / perhaps not fully understanding your implementation. As there may be quite a bit to discuss, perhaps we can go over them during a meeting!

src/main/java/seedu/address/model/internship/Status.java Outdated Show resolved Hide resolved
src/main/java/seedu/address/model/job/Job.java Outdated Show resolved Hide resolved
@ZoroarkDarkrai
Copy link

ZoroarkDarkrai commented Sep 30, 2020

I noticed that many of the javadocs used "person" and "address book", should we change it now? or leave it for later?

@orzymandias
Copy link

I noticed that many of the javadocs used "person" and "address book", should we change it now? or leave it for later?

Hmm. The dilemma now is that this is just a proof of concept and we may be abstracting common logic down the line and scrapping all these, so overwriting Javadocs now may redundant. The danger is that if the code is kept and the comments are missed in the future. What is the team's opinion on this? @ZoroarkDarkrai @keanecjy @seanjyjy @shawn-nyk

@seanjyjy
Copy link

If we are planning to create other classes in replacement of person and address book, I don't think we need to change the Javadoc yet?

@keanecjy
Copy link
Author

Yup I think that there isn't a need to concern about javadocs / comments as of now as they can be easily fixed at any point in time. @orzymandias I stopped editing the List portions as I noticed that there were too much code duplication with your pr. Do we plan to either extend an Item class or use generics for our list?

@orzymandias
Copy link

Yup I think that there isn't a need to concern about javadocs / comments as of now as they can be easily fixed at any point in time. @orzymandias I stopped editing the List portions as I noticed that there were too much code duplication with your pr. Do we plan to either extend an Item class or use generics for our list?

I agree. I think there is unnecessary duplication for collections, generics is a good idea for collections but one issue is with throwing custom exceptions for different types of items within the generic list. Should we just have DuplicateItemException then something like throw new DuplicateItemException(T.getDuplicateExceptionMsg())?

@keanecjy
Copy link
Author

keanecjy commented Oct 1, 2020

ecessary duplication for collections, generics is a good idea for collections but one issue is with throwing custom exceptions for different types of items within the generic list. Should we just have DuplicateItemException then something like throw new DuplicateItemException(T.getDuplicateExceptionMsg())?

I don't think this works because we are not able to access methods in java generics. I figured since we all are classifying our classes as Item, we can have an Item interface to contain all the methods similar methods like isSameItem and likewise do throw new DuplicateItemException(item.getItemName())?

@orzymandias
Copy link

ecessary duplication for collections, generics is a good idea for collections but one issue is with throwing custom exceptions for different types of items within the generic list. Should we just have DuplicateItemException then something like throw new DuplicateItemException(T.getDuplicateExceptionMsg())?

I don't think this works because we are not able to access methods in java generics. I figured since we all are classifying our classes as Item, we can have an Item interface to contain all the methods similar methods like isSameItem and likewise do throw new DuplicateItemException(item.getItemName())?

Sounds good.

@shawn-nyk
Copy link

Hmm. The dilemma now is that this is just a proof of concept and we may be abstracting common logic down the line and scrapping all these, so overwriting Javadocs now may redundant. The danger is that if the code is kept and the comments are missed in the future. What is the team's opinion on this? @ZoroarkDarkrai @keanecjy @seanjyjy @shawn-nyk

I agree that overwriting Javadocs now may be redundant, especially because our code can change over time up until the final version. At the same time, I also agree that we may miss them in the future, and doing a one-time run-through of all our code at the end may be very tedious and error-prone. As such, how about we do something like: every time we create new bits of code that require Javadocs, or we modify code that already has Javadocs attached to them, we place a standardized comment, such as TODO: Javadocs above the code we need to write/edit Javadocs for? That way we get the syntax highlighting that IntelliJ provides for the TODO keyword, and we can also search for this standardised comment throughout the code base to hone in on where we'll eventually need to write/edit Javadocs when the time comes. Any thoughts on this?

@seanjyjy
Copy link

seanjyjy commented Oct 1, 2020

Hmm. The dilemma now is that this is just a proof of concept and we may be abstracting common logic down the line and scrapping all these, so overwriting Javadocs now may redundant. The danger is that if the code is kept and the comments are missed in the future. What is the team's opinion on this? @ZoroarkDarkrai @keanecjy @seanjyjy @shawn-nyk

I agree that overwriting Javadocs now may be redundant, especially because our code can change over time up until the final version. At the same time, I also agree that we may miss them in the future, and doing a one-time run-through of all our code at the end may be very tedious and error-prone. As such, how about we do something like: every time we create new bits of code that require Javadocs, or we modify code that already has Javadocs attached to them, we place a standardized comment, such as TODO: Javadocs above the code we need to write/edit Javadocs for? That way we get the syntax highlighting that IntelliJ provides for the TODO keyword, and we can also search for this standardised comment throughout the code base to hone in on where we'll eventually need to write/edit Javadocs when the time comes. Any thoughts on this?

I think this is a great idea what do the rest think?

@keanecjy
Copy link
Author

keanecjy commented Oct 1, 2020

Sure, I think this is a solid idea.

@keanecjy keanecjy modified the milestones: v1.2, v1.2 mid Oct 1, 2020
This was linked to issues Oct 1, 2020
@keanecjy keanecjy changed the title Add Internship and job classes Add Internship, Application and Item classes Oct 2, 2020
@keanecjy keanecjy linked an issue Oct 2, 2020 that may be closed by this pull request
Copy link

@shawn-nyk shawn-nyk left a comment

Choose a reason for hiding this comment

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

Everything looks great! Perhaps just a few minor edits and I think we're good to merge!

src/main/java/seedu/address/model/item/Item.java Outdated Show resolved Hide resolved
Copy link

@shawn-nyk shawn-nyk left a comment

Choose a reason for hiding this comment

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

LGTM!

@shawn-nyk shawn-nyk merged commit e9c8011 into AY2021S1-CS2103T-T15-4:master Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Item parent class Create InternshipApplication class Create Internship class
5 participants