Open Lewiscowles1986 opened 5 years ago
Thanks @Lewiscowles1986 - I'll take a look at your PRs.
Agreed - a JsonLoader and YamlLoader class would make more sense in this case than a simple RxLoader. Perhaps RxLoader could become an abstract class instead of a final class to enable this.
The loader class takes on deserializing files from a file-name to Objects currently. In #1 I suggested making sure it always returned objects. While looking at it and thinking about it writing #2 it seemed the class method is destined to grow and grow, taking on more responsibilities.
Currently the class takes on loading files without a file-exists check, it loads all bytes in one hit which uses all RAM, doesn't catch exceptions or errors so breaks encapsulation and seems fairly rigid. If I wanted to block files of 1MB or more, it would be difficult. If I wanted to load a json file which contained 3 hyphens, problems, etc.
It also takes on validation, which although I applaud, is similarly rigid. The class body will grow and grow under the current structure.
More interfaces would help to address this.
One could deal with loading a file, providing a string or stream for the json & yaml parsers to work with. If someone wanted to derive a class from this they would be able. They could also implement an interface allowing them to not receive a filename to the constructor, perhaps pulling JSON or YAML from a remote source. Perhaps someone just wants to check the file is there first, and check the size of the file without editing library code.
Another could deal with validating the output of the current libraries.
A real cherry on the cake would be having the parsing itself use an adaptor so that people not wanting to use the two implementations here could do so. That might be a larger work, or future task.