[T13-2]ForumBook #74
[T13-2]ForumBook #74
Conversation
|
Comments for v1.2 AboutUs:
README:
DeveloperGuide:
|
There was a problem hiding this comment.
Overall great work and generally clean code. Good job!
However, Model component has been cut out from the solution AFAICS. Correct me if I'm wrong. All the work is done in Storage instead, via the UnitOfWork class, which itself is a rather neat design IMO 👍
But in AB4 all the Add, Delete, etc. had been done with ModelManager, which then does the work of linking to the GUI to display stuff, and updating the storage, etc. (don't forget undo/redo)
In fact, I see underlyingStorage being used in Storage component. This was in the Model component!
Storage in AB4 only takes in data from Model and converts it into the storage format (XML), which is exactly what FileStorage.java in your project does. So the rest of the stuff above this layer can and probably should be moved back to Model.
All the best!
|
|
||
| /** | ||
| * | ||
| */ |
There was a problem hiding this comment.
A lot of these empty header comments (add just to satisfy checkstyle right 😆), don't forget to fill them in later on!
| * | ||
| */ | ||
| public interface IEvent { | ||
| } |
There was a problem hiding this comment.
What might this be for? If it's to specify an event interface you could just extend from Java's existing Event
| @@ -27,12 +28,12 @@ | |||
| persons = new UniquePersonList(); | |||
| } | |||
|
|
|||
| public AddressBook() {} | |||
| public ForumBook() {} | |||
There was a problem hiding this comment.
Actually after looking at the code, you guys completely skipped this layer of abstraction right? Data from commands is written directly to storage via the UnitOfWork interface.
ModelManager etc. are not being used anymore in your project right now AFAICS, but that shouldn't be the case. Have a look at how Model component wraps around the storage.
There was a problem hiding this comment.
Hi, for this, May I discuss with you about this? I've taken a brief look at ModelManager, this class defines what we can do with the data.
Eg:
- We can add a
Personto storage - We can update a
Personto storage - We can check if a
Personexists - We CANNOT directly check if a
Personwith a specific attribute exists
This is what I mean by defining the behaviour.
This is a good design in AddressBook case since there is only one entity to manage, Person.
However, in our case, we have more entities, Thread, Comment, User. To better manage the data, UnitOfWork is implemented to manage them, it does not directly manage the data, instead, there is another layer of abstraction EntityRepository, these repositories interface defines how we can interact with data.
Eg:
- We can add
Announcement - We can add
User - We can get latest
Announcement - We CANNOT directly get
Userby its password
This is preciously what model manager does. Yes, we are not currently using ModelManager, because I don't see it fit. If I modify ModelManager to wrap UnitOfWork, it will end up more than 20 methods managing Thread, Comment, User etc. entities. This is obviously not a good design (single responsibility?).
TLDR:
EntityRepository is somehow similar to ModelManager, UnitOfWork is an additional layer of abstraction which takes care of data consistency(commit, rollback) and manages EntityRepository.
I'm not sure if I got this correctly, do let me know what I can improve on this. Thanks a lot for your effort.
refactor code, add author info
add facade for Context
updated test path for coverage
updated CURD
updated all the admin commands to use UnitOfWork instead of wraping t…
added UpdateCommentCommandTest
2) Developer Guide updated 3) User Guide Updated 4) User delete command done
Update test
merged with master
updated sample data folder
merged with master
updated DeveloperGuide and Ui
1) Added the PPP for E0191729
fix tests
updated updateCommentCommand
updated tests
No description provided.