Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring of Parser.java #429

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactoring of Parser.java #429

wants to merge 1 commit into from

Conversation

drkarl
Copy link

@drkarl drkarl commented Sep 2, 2016

On this PR:

  • Make file field final and create constructor that accepts a File to make it immutable, then remove setter.
  • Use try-with-resources to handle automatically closing of resources, that is, the file. The original code didn't even bother closing the resources.
  • Use FileReader and FileWriter instead of FileInputStream and FileOutputStream as a higher level and better abstraction to read a file.
  • Use BufferedReader to read the file line by line instead of char by char since IO operations are a bottleneck and that makes it faster.
  • Process the each line by the BufferedReader to strip out non-ASCII characters since that's done in memory and is faster than reading char by char and ignoring non-ASCI characters because IO involving disk is slower.
  • The method getFile() doesn't need to be synchronized since the file field is immutable
  • Change Javadoc since the original one was useless
  • Since BufferedReader and BufferedWriter are thread safe and we have an immutable instance of a File, we don't need to do anything else to ensure that this class is thread safe.

What else would I had done if I had more than 15 minutes to spend:

  • Add a build system like Maven, or even better, Gradle, and add dependencies like Junit.
  • Write unit tests (positive and negative cases reading a text file with and without non-ascii characters) before starting any refactoring. After the refactoring the tests should pass.
  • The name of the file is not completely correct, since it allows not only to parse a text ASCII file, but also to write text to it. It would require to know the use case, but in order to improve encapsulation and assign the right responsibilities to each class, it might make sense (again depending on the use case that would be required for) to make the class extend java.ioFile, and also rename the class.
  • In order to make the code to read and write from and to the file non-blocking and asynchronous, we could return a CompletableFuture<String> or a RxJava Observable like `Single instead of a String. That would perform better when handling multiple large files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant