-
Notifications
You must be signed in to change notification settings - Fork 3
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
7 feature 4 forming user groups task creation #32
7 feature 4 forming user groups task creation #32
Conversation
…nd declinedteammates. Added get and set methods for leader. Changed setRecurring so that recurring could be changed as well as frequency. Added java doc comments to all methods.
…eceiver so that it is just one Student User instead of an ArrayList of Student Users. Modified constructor accordingly.
…task-creation' into 7-feature-4-forming-user-groups-task-creation
Also, does any one know why workflow is failing? |
Also not 100% whether my methods are correctly public vs protected so if any one has any strong opinions because of how their feature interacts with a method please let me know. |
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.
This is my bad because I keep noticing issues in Task, but just delete the 3 abstract methods and everything should match again.
Aside from that your implementation looks good to me!
private ArrayList<StudentUser> pendingTeammates; | ||
private ArrayList<StudentUser> declinedTeammates; | ||
private StudentUser leader; | ||
|
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.
For collaborative scheduling a deadline instance variable is needed as well. So like: private LocalDateTime deadline. This would also need a getter method and I think you have to add it to your constructor since the deadline can't have a default value.
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.
Added two new constructor and getter/setter methods for new attribute.
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.
Would you ever need a Collaborative "Test" or "Assignment"? I am thinking of making CollaborativeEvent a subclass of Event and CollaborativeAssignment a subclass of Assignment. Let me know what you think of that idea @yyutiw @alyson647
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.
@sshmuylovich I'm not too sure about the idea to be honest. I viewed CollaborativeTask as separate to Test, Assignment, Event, and that it was its own thing since in our CRC cards it seemed like that. So, I don't know but I think that might complicate things a bit if we decide to make other entities like CollaborativeEvent, etc. It would also impact the functionality of my use case since I heavily rely on CollaborativeTask.
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.
@sshmuylovich @alyson647 Yeah, I'd say collaborative tasks could be something like "work on assignment" or "study for test" anyways so splitting them up further would probably overcomplicate things
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 don't think I use any of these entities, but yeah as mentioned above, necessary getters and setters would have to be public. Other than that, packages should be entities instead of Entities. Looks good and love the documentation!
Note that I am showing an error for Collaborative Task at the moment because my version of Task still has edit, delete, and save as abstract classes that I no longer include in Collaborative Task as per Yuti's 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.
Looks good overall
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!
f88b795
|
||
import java.util.ArrayList; | ||
|
||
public class internalUserList { |
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 believe @CC-3636 has this file as well as UserWhisperer removed (she no longer uses these entities anymore). Double check with her though
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.
correct, I removed both UserWhisperer and internalUserList because I no longer use them
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 deleted UserWhisperer -- the create method is now a part of UserFactory. I also got rid of internalUserList because we are using an external User Database.
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 great!
…move methods to one set method.
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.
thanks for making the changes I requested :)
This should be all entities for my feature basically complete. Will be pushing use cases and then UI soon.