Separated Repository related objects from bigger PR #24
Conversation
elliVM
left a comment
elliVM
left a comment
There was a problem hiding this comment.
Please take my comments and suggestions with a grain of salt since I do not have prior knowledge of this project, and I couldn't dive deep in the review process.
To me there seems to be some mixing of serialization and domain concerns, leading to some coupling and unclear responsibilities. This part of the code base would benefit from refactoring towards a clear split of these concern areas.
Testing was on a good level.
Summary
- Some unclear naming, especially when an object is a snapshot instead of a real entity
- Serialization is mixed with Storage work
- copy semantics are unclear
- Domain invariants are not enforced, e.g. empty IDs, empty titles, stub ctors as real objects
- equals includes mutable collections, can cause some problems
- JSON objects act as views and not serializers
- Storage errors inconsistent with exceptions hidden under more general exceptions
- LocalFiesystemStorage does a bit too much, some responsibilities should be split or delegated like serialization
| * | ||
| * @return | ||
| */ | ||
| public abstract String asLongString(); |
There was a problem hiding this comment.
the method names might be a bit procedural since they describe how. Intent would be made more clear with names like e.g. canonical() and displayName().
Changing to use path based identifiers could help here, since these might not be required at all.
| * An Identifier that uses a Path to uniquely identify resources | ||
| */ | ||
|
|
||
| public class PathIdentifier implements Identifier { |
| import java.util.List; | ||
| import java.util.Objects; | ||
|
|
||
| // Represents a single Directory that can contain Filesystem objects. |
There was a problem hiding this comment.
The unit test seems to indicate that this is not a real domain level Directory object rather it is a snapshot of a directory, if that is the case would a DirectorySnpashot or DirectoryState be a more accurate name?
|
|
||
| // Constructor for a stub notebook that can be loaded from file. | ||
| public Notebook() { | ||
| this.title = ""; |
There was a problem hiding this comment.
could use this(""); here to chain constructors
| } | ||
|
|
||
| @Override | ||
| public String readFile(final Identifier identifier) throws FileNotFoundException, IOException { |
There was a problem hiding this comment.
FileNotFoundException hidden by general exception
|
|
||
| @Override | ||
| public List<Identifier> listFiles(final Identifier identifier) | ||
| throws FileNotFoundException, MalformedRequestException, IOException { |
There was a problem hiding this comment.
FileNotFoundException hidden by general exception
| return files; | ||
| } | ||
|
|
||
| private void delete(final Path path) throws NoSuchFileException, IOException { |
There was a problem hiding this comment.
NoSuchFileException hidden by general exception
There was a problem hiding this comment.
Identifier is immediately turned back to a Path, making them just wrappers essentially.
| } | ||
|
|
||
| @Override | ||
| public String readDirectory(final Identifier identifier) throws IOException, MalformedRequestException { |
There was a problem hiding this comment.
should this return a structure like Directory and not a JSON representation?
2 participants
This PR is separated from #4 to make code review more manageable.
Note that this PR by itself will likely contain references to objects that are not present in this PR. Please refer to PR #4, which contains all objects, in case you need to see how some objects interact.
This PR contains objects related to how notebooks and directories are serialized, and where they are saved.
closes #21