jbowkett/alice-bob-charlie — pull request #1 by jbowkett (2014-09-23 08:19:05+00:00)
Title: Review comments

Summary of Changes:

* Shell (class and package): Not a domain name—too technical. [ rolled into console class ]
* Shell: Too many collaborators. [ pushed execution code into command classes ]
* Shell/Console: Why Console class doesn’t read from console as well? [ rolled into console, originally, didn't want console to have state ]
* Shell.submit(): Misnomer. JavaDoc starts with “Parse…” I would call this method “execute(shellCommand)” instead and would delete the JavaDoc. [ change borne out by command refactoring ]
* Shell.submit(): It highlights a design problem. If you need to use “instanceof” and casting when you have an interface (Command in this case), the interface is not good enough—it’s not abstracting as it should.
Shell.submit(): Violates OCP. If you add a new Command, this method needs to be changed. It shouldn’t be the case if Command interface was better defined. [ code rolled into individual command classes ]
* Shell: The class violates OCP and SRP. It has the behaviour of all commands and would need to be changed if a new command is added or a command is removed. [ code rolled into individual command classes ]
* CommandParser: It’s not a parser. It’s a factory class (DDD) with a factory method (GoF pattern). [ renamed ]
* CommandParser.submit(): Bad name. We normally would call a “parse” method on a parser and not submit. [ refactored, was originally conscious of being present in the "land of nouns" (Steve Yegge) ]
* CommandParser: This should be the only (factory) class that knows about different commands. [ done ]
* Command: Badly implemented. It should be implemented as the Command pattern (GoF Pattern) with a single method execute(). There are a few ways you could implement it: a) Pass the shellCommand and Repository into the constructor and have an execute() method with no parameters; b) Pass the Repository in the constructor and pass the shellCommand as a parameter to the execute(shellCommand) method. The command would know if the shellCommand belongs to it, would parse it, and invoke the respective method on the repository. [ implemented as `execute(console)` ]
* Command implementations (Follow, Post, View, Wall): Should not be dummy data structures. It should have the respective behaviour (currently on Shell class). [ done ]
* Repositories: No need for 3 repositories. [ Kept a data layer as an aggregate with different implementations of DAOs to keep to a single responsibility for each ]
* “View” command: Slightly deviation from the spec, that says “Read”. [ Renamed ]
* OrderedPosts: Is this a domain class? [ Used by the domain classes, so kept in the domain package for now ]
* Subscriptions: This word does not exist in the domain language. [ Made an internal class for the following repo instead ]. Maybe the User could have a list of users he or she follows? That way, you could get all the posts from the user and followed users, add them to the same list, and sort them. [ kept the separation, as I disagree that the user class has any business knowing about who they follow ]
* Timeline: Public methods size() and anyMatch() just exist for testing purpose. That is a smell. It tells us that the design is not quite right or that we should be looking at other side effects in order to test the desired behaviour. [ Taken out and tested in a different fashion, however, it's not *necessarily* a smell ]
* Wall: Probably unnecessary if the repository could return a list posts, in the reverse chronological order for a given user (containing the list of followed users). [ kept as first class concept ]
* dal package: Did you mean “dao” (Data Access Object)? You should not mix DAO and Repositories. Although similar things, they represent different concepts. DAO is a Core J2EE Pattern and Repository is a DDD building block, which treat entities/aggregates as a collection. Subtle but different. In a DAO you would have “create”, “read”, “delete”, etc. In a Repository you would have “add”, “find”, “remove”. [ refactored as the mixed modelling was incorrect, however method names still do not feel very natural ]
* TimeFormatTest: It’s in the wrong repository. Should be aligned with the package where TimeFormat is (shell). [ refactored, overlooked by IntelliJ originally ]
* StepDefinitionsTest: I’m not sure why we need this(?) [ Kept in as it was checking something I wasn't sure of within the step definitions to enable better grammar in the step declarations ]