Doing a little quiz. #406
Description
I was too lazy to make some code but here are the things I noticed:
- Why is the class called parser? It does not parse anything, it provides functionality to read and write stuff from a file.
- The class violates SRP: It performs file IO and charset filtering.
- The class is mutable which is a major source for all sorts of weird bugs.
- Encapsulation problem: setFile/getFile seems bad design, immutable object would be better.
- Why is getFile needed at all? Should be removed if possible.
- I'm not sure why thread-safefy is announced in the javadoc. What exactly should be thread-safe here? Nothing prevents two threads from simultaneously write to the same file and f.. up things.
- Reading and writing does implicit binary-string conversion but I can not tell it which encoding to use.
- Reading large files will take a long time because (a) no byte buffer is used and (b) += operator is used which will create a new string instance each time IIRC.
- InputStreams are not closed and will leak resources. try-with-statement should be used.
- getContentWithoutUnicode(): Should this really be named unicode? I'm no expert in Unicode but I think this should be named something like getContentWithoutUTF8Characters() or something. Also, Unicode is a standard, UTF-8 is a character encoding :)
- I am not sure what the intent for getContentWithoutUnicode() is but filtering for 0x80 seems weird. My wtf sense is tingeling.
- Unit tests are missing.
- No way to mock the filesystem.
- Javadoc does not describe what the class should actually do.
For the refactoring, why not delete the class and use Files.readAllBytes() and Files.write() instead? Given that the code is so bad, the rest of the project might be too and if we are really lucky getContentWithoutUnicode() is not used at all and then we don't even have to build a new class. :-P