[AY1920S1-CS2113T-W13-1] Coach Manager#87
[AY1920S1-CS2113T-W13-1] Coach Manager#87Sfloydzy wants to merge 790 commits intonusCS2113-AY1920S1:masterfrom
Conversation
flxffy
left a comment
flxffy
left a comment
There was a problem hiding this comment.
A lot more refactoring needs to be done.
Things to note:
- There are a few variables that should be constants but are not.
Controlleris well done.- Look up default constructors. Don't leave empty constructors that don't do anything.
- Good use of interfaces in
Details. - Code that have been commented out - those can be deleted.
- Conditional paths - in some cases, the flow could be better. Choose the happy path when it makes sense.
- Read the docs!! Data structures offered in Java are pretty robust and already offer a lot of what you are trying to implement.
- Why do you have tests from Duke, and dummy tests?
| /** | ||
| * Constructor for CategoryList. | ||
| * @param subMenuDescription The description for the sub menu. | ||
| * @param menu The name of the menu | ||
| */ | ||
| public CategoryList(final String subMenuDescription, final String menu) { | ||
| super(subMenuDescription, menu); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why is the constructor at the end of the class?
src/main/java/menu/MyMenu.java
Outdated
| // public void trainingScheduleHeading() { | ||
| // System.out.flush(); | ||
| // System.out.println("TRAINING SCHEDULE: \n"); | ||
| // } | ||
| // public void manageStudentsHeading() { | ||
| // System.out.flush(); | ||
| // System.out.println("MANAGE STUDENTS: \n"); | ||
| // } | ||
| // public void trainingProgramHeading() { | ||
| // System.out.flush(); | ||
| // System.out.println("TRAINING PROGRAM: \n"); | ||
| // } |
There was a problem hiding this comment.
Don't leave commented out code. Just go ahead and delete unused code. Now that we are using a revision control system, we can recover any deleted code later.
src/main/java/duke/module/Goal.java
Outdated
| if (!hasGoal) { | ||
| return "There is no goal of the day"; | ||
| } else { | ||
| return message; | ||
| } |
There was a problem hiding this comment.
This conditional path could flow better for readability. Consider setting the happy path first.
| for (Date d : lessons.keySet()) { | ||
| if (d.equals(today)) { | ||
| if (!lessons.get(d).isEmpty()) { | ||
| hasLesson = true; | ||
| for (String str : lessons.get(d)) { | ||
| message += str + "\n"; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic is convoluted and uses unnecessary nesting. Look through the Map documentation and you'll find a better way to do this!
| /** | ||
| * Getter function to get the current date. | ||
| * @return A string containing the current date. | ||
| */ | ||
| public final Date getTodayDate() { | ||
| System.out.println("Today's date " + todayDate); | ||
| System.out.println("Saved date: " + endDate); | ||
| return todayDate; | ||
| } |
There was a problem hiding this comment.
Violating single responsibility principle. Think about why this is so
| /** | ||
| * Represents the list for the current number of plans saved. | ||
| */ | ||
| private ArrayList<String> toc = new ArrayList<>(); |
There was a problem hiding this comment.
What is toc? Use a more descriptive variable name.
| * @throws FileNotFoundException if file is not found. | ||
| */ | ||
| public MyPlan() throws FileNotFoundException { | ||
| filePath = ".\\src\\main\\java\\duke\\data\\plan.txt"; |
There was a problem hiding this comment.
Windows file path? Are you sure this works on all platforms? Refer to the project constraints, your product should be cross-platform compatible.
| /** | ||
| * Create a plan of specified intensity. | ||
| * @param intensity intensity of plan to be created. | ||
| */ | ||
| public void createPlan(final String intensity) { | ||
| clearPlan(); | ||
| if (Intensity.contains(intensity)) { | ||
| System.out.println("Creating plan of " + intensity + " intensity."); | ||
| System.out.println("Please input activity to add in format of " | ||
| + "[activity] [number of sets] [number of reps]."); | ||
| while (true) { | ||
| Scanner sc = new Scanner(System.in); | ||
| if (sc.hasNextLine()) { | ||
| String input = sc.nextLine(); |
There was a problem hiding this comment.
Should MyPlan be scanning user input? This is violating one of the design principles we went through last week.
| /** | ||
| * status: whether Item is complete or not. | ||
| */ | ||
| private Boolean status; |
There was a problem hiding this comment.
Your variable name isn't descriptive nor indicative of what it is meant for. Refer to the coding style for a more appropriate name.
| public String getDate() { | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
Why is the getter returning an empty string?
Creating ParserGoal
Add in Schedule inside ParserCommand
Add a few command classes
MyPlan: Added functionalities
Merge Updates
Fix error
Checkstyle for ParserCommand
Merge update
Added testings and edited progress list for students
MyPlan: Functionalities added(Still in progress)
Merge ppp updates and tests
ManageStudents code edits
MyPlan fixed functionalities
Final Changes to gradle file
Final edit/bug fixing
Final changes
Txt file directory for JAR fixed
Final bug edit
5 participants
@danisheddie @eujingsen @NotTheRealEdmund