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

feature 7 course creation + course enrolment + tasks to tasklist use cases #34

Closed
wants to merge 25 commits into from

Conversation

jltng
Copy link
Contributor

@jltng jltng commented Nov 19, 2022

EDIT: New branch "feature 7 merge conflicts" is the duplicate branch

There is a lot of things going on even more things that need to be fixed!

For task creation:

  • Currently have a 'task name' and 'task id' on screen (task id should be unique and not up to instructor to create, will change it so that the id is the localdatetime created or something else that is unique)
  • Then, would need to take those 2 arguments and create a new Task with that.
  • Then, would need to take task id and at it to the tasks arraylist argument of the Course entity in CourseMap
  1. CourseEnrolmentInteractor
    Need to still add student id to Course object’s list of students in CourseMap, save and update, and then send success view to presenter

  2. tasks_to_tasklist_use_case
    Needs to be implemented, one of the TAs said that since it's returning a simple data structure (only a succcess view message) there is a particular way to do it .... I shall ask them during the week to figure it out

  3. test cases:
    to be finished

@jltng jltng linked an issue Nov 19, 2022 that may be closed by this pull request
4 tasks
@jltng jltng self-assigned this Nov 19, 2022
@jltng jltng added the enhancement New feature or request label Nov 19, 2022
Copy link
Contributor

@yyutiw yyutiw left a comment

Choose a reason for hiding this comment

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

Coding style: Class, package, and variable names all follow proper naming conventions and clearly describe their purpose (thank you for changing my entities package to lowercase).

Packaging: Good organization of code into entities/use cases/screens

Clean arch: Implementation of use cases follow clean architecture and SOLID principles very well

Javadoc: Good documentation of code, each step is mostly labeled. However, documentation is missing for most methods/constructors.

Approving because there are no major issues present

}

/**
* Change getters to public?
Copy link
Contributor

Choose a reason for hiding this comment

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

yes?

yyutiw
yyutiw previously approved these changes Nov 20, 2022
…ompleted, and documentation added (minus data persistence and tests)
@jltng jltng requested a review from yyutiw November 21, 2022 02:28
TiareMar
TiareMar previously approved these changes Nov 21, 2022
Copy link
Contributor

@TiareMar TiareMar left a comment

Choose a reason for hiding this comment

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

No major problems in code, although testing and java documentation still need to be completed.

Good packaging organization and naming conventions followed well.

As far as I can see, no big SOLID or Clean Architecture violations.

Approving in the interest of getting our program working first, and then fixing all the smaller issues later.

yyutiw
yyutiw previously approved these changes Nov 21, 2022
CC-3636
CC-3636 previously approved these changes Nov 21, 2022
Copy link
Contributor

@CC-3636 CC-3636 left a comment

Choose a reason for hiding this comment

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

overall comments:

  • good adherence to CA framework and SOLID principles
  • good documentation
  • good luck

@jltng jltng dismissed stale reviews from CC-3636, yyutiw, and TiareMar via df33dec November 21, 2022 18:15
@jltng
Copy link
Contributor Author

jltng commented Nov 21, 2022

request closed because created new branch and merged use case files

@jltng jltng closed this Nov 21, 2022
@jltng jltng added the duplicate This issue or pull request already exists label Nov 28, 2022
@jltng jltng added this to the Use Case Core Functionalities milestone Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants