[AY1920S1-CS2113-T13-1] ArchDuke#45
[AY1920S1-CS2113-T13-1] ArchDuke#45Lucria wants to merge 1261 commits intonusCS2113-AY1920S1:masterfrom
Conversation
…14-2#45) * joeys developer guide (some problems)
akshayeshenoi
left a comment
akshayeshenoi
left a comment
There was a problem hiding this comment.
Great effort!
Few things to keep in mind:
- Always try to separate concerns. The
CLIViewshould not be aware of the project repository, for example. - Enforce strict abstraction levels. Does
ProjectInputControllerandConsoleInputControllersit at the same level? - Lower tiers in a software system must not be aware of higher tiers. Try to establish clear data flow patterns in your code.
You have thorough test cases things look good on the UI front. Keep up the good work there!
| DukeLogger.logInfo(ProjectInputController.class, "Managing:" | ||
| + projectToManage.getDescription() + ",input:'" | ||
| + projectCommand + "'"); | ||
| if (projectCommand.length() == 4 && ("exit").equals(projectCommand.substring(0, 4))) { |
There was a problem hiding this comment.
Any particular reason why we're checking for command length?
src/main/java/views/CLIView.java
Outdated
| * Method called when users wishes to view all Projects that are currently created or stored. | ||
| * @param allProjects List of Projects returned to View model by the Controller from the Repository | ||
| */ | ||
| public void viewAllProjects(ArrayList<Project> allProjects) { |
There was a problem hiding this comment.
Do you think this class is overloaded with functionality here? Business logic (such as these functions) should be placed in a more appropriate package. Think of a way to separate these concerns.
This is class is not only dealing with I/O, but also core business logic.
| case "bye": | ||
| consoleView.end(); | ||
| break; | ||
| case "list": |
There was a problem hiding this comment.
Such back and forth calling is not the most elegant way approach this problem. Consider having dedicated controllers for business logic related calls.
| public ConsoleInputController(CLIView view) { | ||
| this.consoleView = view; | ||
| loadProjectsData(); | ||
| this.projectInputController = new ProjectInputController(this.consoleView, this.projectRepository); |
There was a problem hiding this comment.
Think of better ways to pass data up or down. ProjectInputController should not be aware of CLIView. They are separate concerns and should be clearly divided.
In fact, if ConsoleInputController and ProjectInputController are implementing the same interface, they should be at the same level of abstraction.
| import java.util.Scanner; | ||
|
|
||
| public class ProjectInputController implements IController { | ||
| private Scanner manageProjectInput; |
There was a problem hiding this comment.
Consider handling all input at one layer (say CLIView), rather than delegating that responsibility to the controller. Think of a generic 'parser' that carries out high level parsing, and hands over the options/subcommand stuff over to the controller to deal with.
There was a problem hiding this comment.
Hmm, in that case, will this be a more elegant approach?
View -> Parser (Helper module) -> Controllers
With this flow, Parser will take in a Data model of String and return (perhaps) two things to the Controller: the command called and the subcommand/options
The controller relied on the very first word read in to detect the relevant command called by the user. We initially believed that the controller will do some input cleaning before calling the relevant classes for each command.
Moreover, when modifying things related to a Project, we thought of using a separate Controller to handle commands relevant at that 'instance' of program runtime. If we handled all inputs at one layer, it will result in a cluttered Controller so we decided to abstract it to two Controllers. Is this approach bad?
Sorry for the trouble! @akshayeshenoi
There was a problem hiding this comment.
Hmm, in that case, will this be a more elegant approach?
View -> Parser (Helper module) -> Controllers
With this flow, Parser will take in a Data model of String and return (perhaps) two things to the Controller: the command called and the subcommand/options
Yes, this sounds much cleaner. It's okay to split up your controllers based on similar functionalities. The controllers can be free to do what they want with the 'parsed' object.
| IMember newMember = memberFactory.create(memberDetails); | ||
| if (newMember.getName() != null) { | ||
| projectToManage.addMember((Member) newMember); | ||
| consoleView.addMember(projectToManage, newMember.getDetails()); |
There was a problem hiding this comment.
The lower level tiers must not be aware of higher level tiers (remember n-tier architecture?). The view should 'ask' for data and get a 'response' in return.
Try to enforce that pattern.
[CS2113]-sinteary-RefactorAssignmentViewHelperAndDocumentation
[CS2113-T13-1]-Dkenobi-AddReminderCategory
[CS2113-T13-1]-Dkenobi-AddedTestCaseForReminderCategory
[CS2113]-sinteary-UpdateUGDG
� Conflicts: � docs/UserGuide.adoc
# Conflicts: # docs/DeveloperGuide.adoc # docs/images/sequenceDiagram/ViewReminders_Sequence.png
[CS2113-T13-1]-iamabhishek98-UpdatedHelpCommand
[CS2113-T13-1]-Lucria-UpdatedUserGuide
[CS2113-T13-1]-seanlimhx-UpdateDG
[CS2113-T13-1]-seanlimhx-UpdateDG
[CS2113-T13-1]-iamabhishek98-UpdatedUGScreenshots
[CS2113-T13-1]-iamabhishek98-UGFixTypo
Lucria-SmallFix
* test * test * test * test * test * test * test * image * image * image * image * update * update * update * update * update * update * refactor Schedule to TimeFrame * Add implements Comparable<TimeFrame> * 092919 ui fix (nusCS2113-AY1920S1#2) * udate * udate * refactor Schedule to TimeFrame * update * update * update change in schedule command * fix merge conflict and bug * minor code change
7 participants
@sinteary
@iamabhishek98
@Dkenobi
@seanlimhx