3
\$\begingroup\$

This Question now has a follow-up question

After learning a lot about applying OO principles, interfaces and a bit about argument-handling I post a revised version of my previous code. This is still an exercise for me to improve my general coding for a simple program. As such I would like to ask to pay special attention towards the application of OO programming principles and Exception handling, because those are the areas I feel weakest at.

The code is tested and - just like before - works properly.

Here a quote of the still relevant sections of the previous question:

The task itself is to go through a large .txt file that contains lines with ">" as their first character and to remove all spaces in these lines. I am using this program to modify files in FASTA format, a format often used in biology.

Algorithm:

  1. Read next line from input file and store in "line"
  2. If "line" contains ">" as first character, remove all spaces in "line"
  3. Print "line" to output file
  4. If next line is not null, go back to Step 1

Step 2 was changed from "If line contains ">" " to "If Line contains ">" as first character", since that reduces the number of comparisons the if-statement has to make - I think.

Code structure

The code structure changed a lot compared to last time and is now a lot more complex, which is why it has its own section.

Argument parsing is no longer handled by ArgumentHandler, which was split into a class ArgumentCollection - which does the argument parsing now - and a class ArgumentHandler - which triggers the argument parsing and contains the code that slowy previously suggested putting into a createArgumentHandler() method. ArgumentCollection implements an interface ArgumentManager that provides a bunch of method to manipulate, set and test arguments of the various flags. ArgumentHandlernow also implements an Interface Configuration that provides the methods getSourceFile()and getSinkFile()that returns the files to read from/write to. For the sake of some semblance of brevity 'ArgumentCollection' is not shown (It has over 250 lines of code).

Besides Configuration, the Client also works with the classes 'FileLineReader' , and 'FileLineWriter' (both implementing AutoCloseable) , which have the methods readLine()/writeLine().

The Client

public interface Configuration {
    /**
     * Returns the name and path of a file to read from in form of a String.
     */
    String getSourceFile();

    /**
     * Returns the name and path of a file to write to in form of a String.
     */
    String getSinkFile();

    /** Prints all arguments */
    void printArguments();
}

public class RemoveSpaces_Client {

    private static void printProgramProgress(int i) {
        if (i % 1000000 == 0) {
            System.out.println(i / 1000000 + " * 10^6 lines written.");
        }
    }

    public static void main(String[] args) {
        Configuration arguments = new ArgumentHandler(args);
        System.out.println("Starting Program with the following arguments: ");
        arguments.printArguments();

        try (FileLineReader inputReader = new FileLineReader(arguments.getSourceFile());
                FileLineWriter outputWriter = new FileLineWriter(arguments.getSinkFile());) {

            /*
             * Write every line from input file to output file. If the line is a
             * name (contains ">"), remove all spaces in it before writing.
             */
            int i = 0;
            for (String line = inputReader.readLine(); line != null; line = inputReader.readLine()) {
               if (line.charAt(0) == '>') {

                    line = line.replace(" ", "");
                }
                outputWriter.writeLine(line);
                printProgramProgress(++i);
            }
        } catch (IOException e) {
            System.out.println("Reader or Writer caused an IOException!");
            e.printStackTrace();
            throw new UncheckedIOException(e);
        }
        System.out.println("Finished!");
    }
}

FileLineReader

public class FileLineReader implements AutoCloseable {
    private BufferedReader reader;

    public FileLineReader(String filename) throws FileNotFoundException {
        this.reader = new BufferedReader(new FileReader(filename));
    }

    public String readLine() throws IOException {
        return reader.readLine();
    }

    public void close() throws IOException {
        reader.close();
    }
}

FileLineWriter

public class FileLineWriter implements AutoCloseable {
    private Writer writer;

    public FileLineWriter(String filename) throws IOException {
        try {
            this.writer = new BufferedWriter(new FileWriter(filename));
        } catch (IOException e) {
            System.out.println(
                    "Output file name " + filename + " was not accessible or could not be created. Printing to "
                            + (filename + ".nospace.txt instead"));
            this.writer = new BufferedWriter(new FileWriter(filename + ".nospace.txt"));
        }
    }

    public void writeLine(String line) throws IOException {
        this.writer.write(line);
        this.writer.write(System.lineSeparator());
    }

    public void close() throws IOException {
        this.writer.close();
    }
}

ArgumentHandler

public class ArgumentHandler implements Configuration {
    private ArgumentManager arguments;

    ArgumentHandler(String[] args) {
        /*
         * Define all allowed Flags and their associated arguments in an
         * ArgumentCollection. If possible, assign arguments their default
         * values. "null" are arguments that must be specified through parsing
         * of args or the program can't run. "" are arguments that are optional
         * or whose default value depends on an argument whose value needs to be
         * parsed.
         */
        String[] argumentList = { "-i", null, "-o", "", "-h", "Text to display if -h is called" };
        arguments = new ArgumentCollectionImplementation1(argumentList);

        arguments.parseArguments(args);
        /*
         * If flag "-o" does not have an argument associated with it after
         * argument parsing, set its argument to its default value.
         */
        if (!arguments.flagHasArguments("-o")) {
            arguments.setFlagArgument("-o", 0, arguments.getFlagStringArgument("-i") + ".nospace.txt");
        }
    }

    public String getSourceFile() {
        return arguments.getFlagStringArgument("-i");
    }

    public String getSinkFile() {
        return arguments.getFlagStringArgument("-o");
    }

    public void printArguments() {
        arguments.printArguments();
    }
}

Final Comment

One major thing this definitely taught me - If you can avoid it, don't code easy tasks in java. This program totals over 300 lines of code (not counting comments and package/import statements) to do something, as Vogel612 pointed out, that you can do in 1 line in sed in a linux-terminal.

\$\endgroup\$
  • \$\begingroup\$ Oh, and please do code easy tasks in Java. Java is good for easy tasks. Please don't try to make easy tasks difficult because, just, you know, it's Java. \$\endgroup\$ – Tamoghna Chowdhury May 19 '17 at 18:18
  • 1
    \$\begingroup\$ @TamoghnaChowdhury none of the two comments are constructive. I posted in the initial statement "This is still an exercise for me to improve my general coding for a simple program.". They key in here as in the previous version, which you also read, is to improve general coding for when I inevitably have to solve more complex problems and want a stable and easily test-able program for it. You start with comparatively easy tasks, not with difficult ones. As such this gives me invaluable insight into applying OO principles and learning more about error handling. \$\endgroup\$ – Isofruit May 19 '17 at 18:29
  • 2
    \$\begingroup\$ I find @TamoghnaChowdhury's comments to be the start of a review. I do think there is a valid point in there but I can remind about it doesn't only matter what you say but also how you say it. \$\endgroup\$ – Simon Forsberg May 19 '17 at 18:32
  • 1
    \$\begingroup\$ I'm writing an answer, hopefully that'll make my point clearer. My point being, Java has a large, wondrous API, try using it. \$\endgroup\$ – Tamoghna Chowdhury May 19 '17 at 18:32
  • \$\begingroup\$ @SimonForsberg I agree that it's indeed a bit strongly phrased, and I apologize if it offended anyone. It's because I've noticed that recently many people over-engineer just because the language is verbose. \$\endgroup\$ – Tamoghna Chowdhury May 19 '17 at 18:35
3
\$\begingroup\$

I'll handle the big thing first.

Over-Abstraction

Your current code is quite a bit over-engineered (the rest of the answer elaborates on that). In my opinion, if something is meant to be used for only a very specific purpose, there is no reason to make it more general than it needs to be, adding complexity and reducing maintainability in the process. Take my word for it - I learnt it the hard way.

In your case, BufferedReader is already sufficiently general for your needs - why bother implementing a class which just wraps it within another layer of indirection without providing any different or extra functionality?

BufferedReader and BufferedWriter implement AutoCloseable, so it's noise to abstract that behind classes which do not add significant functionality. (Yeah, I'm mostly an FP programmer now so I don't really care about what OOP purists say about decoupling interface from implementation, etc., etc.). But, you know, those OOP purists are (mostly) right. So what are we missing here?

The Java Standard Library

PrintWriter to the rescue!

However, there's an easier way out. You want PrintWriter. It'll completely, utterly replace FileLineWriter (of course it also implements AutoCloseable and is buffered). You can even use outputWriter.println(...)!

You want this constructor: public PrintWriter(File file, String csn). Why the csn bit?

Always specify the output charset!

csn is the output charset name. You want it, if, say, you move this code from macOS to Windows and the default charset changes from say MacRoman to CP-1252 or whatever. Don't make your code essentially single-platform when it's Java. Java will choose the default charset when outputting, so... you'll probably want "UTF-8" for csn.

Interface-Implementation design

getSourceFile and getSinkFile should return java.io.File (as their names indicate). You probably don't want them to return Strings containing the file path (principle of least surprise). If you do, name them appropriately (getSourceFilePath, etc.).

ArgumentHandler should probably be a private static inner class of RemoveSpaces_Client, as it is specific to it (if otherwise, justify in the documentation, and name ArgumentHandler more specifically (IOArgumentsHandler should be enough, a help function can be assumed to be common)).

Core code:

Yes, you're very much right that checking the initial character directly is faster (in fact, much faster). Just in case, here's why:

contains would have iterated the String in order, and since the character in question is constrained by the format to always be at the start, the loop would have run for only 1 iteration before returning if the String did indeed start with the character in question, but it would have run till the end if it didn't.

When you're measuring file sizes in millions of lines, these things add up. I project about a \$n\times\$ performance improvement on average, if each line had \$n\$ characters.

Also, if you're going to be using the source/sink terminology, try to be consistent about it - the names should be sourceReader and sinkwriter.

I'd prefer a while loop for this case, as "loop while progression condition is satisfied" is more like a while loop. Of course, there's nothing wrong with using a for loop, it's just a matter of personal preference. I use whichever is convenient under the circumstances. However, as always, better to be consistent.

Note:

Where is UncheckedIOException implemented? I suspect it's a subclass of RuntimeException encapsulating the parent IOException in its cause field, and the message associated with the parent in its message field.

Edit: I have since realised thanks to the comments that UncheckedIOException is very much part of the library, and I retract my below statement and appreciate the OP simulating the logging of the occurrence of an exception.

However, I still think that the entire wrap-and-rethrow business seems convoluted in this context, however, it would benefit in the long run by reducing the burden on consumers of the OP's API by not enforcing the throws clause in all their method signatures. This is now an appreciated effort.

Then, if you're going to notify the user that an exception has occurred, print a stack trace, and exit, why bother catching the exception at all and wrapping it? The default JVM uncaught exception handler does exactly what you do manually, minus a vague error message printed to STDOUT.

Syntax

Nitpick: To keep in line with standard Java camelCase identifiers, RemoveSpaces_Client should be named RemoveSpacesClient, without the underscore.

All together now

A grand total of 89 lines (that's not considering your supporting API, though; argument parsing in Java can be a pain - use Lists and Maps judiciously to make your job easier), without much lost functionality:

RemoveSpacesClient.java

import java.io.*;

public class RemoveSpacesClient {
    private static class ArgumentHandler implements Configuration {
    private ArgumentManager arguments;

    ArgumentHandler(String[] args) {
        /*
         * Define all allowed Flags and their associated arguments in an
         * ArgumentCollection. If possible, assign arguments their default
         * values. "null" are arguments that must be specified through parsing
         * of args or the program can't run. "" are arguments who are optional
         * or whose default value depends on an argument whose value needs to be
         * parsed.
         */
        String[] argumentList = { "-i", null, "-o", "", "-h", "Text to display if -h is called" };
        arguments = new ArgumentCollectionImplementation1(argumentList);

        arguments.parseArguments(args);
        /*
         * If flag "-o" does not have an argument associated with it after
         * argument parsing, set its argument to its default value.
         */
        if (!arguments.flagHasArguments("-o")) {
            arguments.setFlagArgument("-o", 0, arguments.getFlagStringArgument("-i") + ".nospace.txt");
        }
    }

    public File getSourceFile() {
        return new File(arguments.getFlagStringArgument("-i"));
    }

    public File getSinkFile() {
        return new File(arguments.getFlagStringArgument("-o"));
    }

    public void printArguments() {
        arguments.printArguments();
    }
}

    private static void printProgramProgress(int i) {
        // You can use underscores to group digits in numeric literals since Java 7
        if (i % 1_000_000 == 0) {
            System.out.println(i / 1_000_000 + " * 10^6 lines written.");
        }
    }

    public static void main(String[] args) {
        Configuration arguments = new ArgumentHandler(args);
        System.out.println("Starting Program with the following arguments: ");
        arguments.printArguments();

        try (BufferedReader sourceReader = new BufferedReader(new FileReader(arguments.getSourceFile()));
                PrintWriter sinkWriter = new PrintWriter(arguments.getSinkFile(), "UTF-8");) {

            /*
             * Write every line from input file to output file. If the line is a
             * name (contains ">"), remove all spaces in it before writing.
             */
            int i = 0;
            for (String line = sourceReader.readLine(); line != null; line = sourceReader.readLine()) {
               if (line.charAt(0) == '>') {
                    line = line.replace(" ", "");
                }
                sinkWriter.println(line);
                printProgramProgress(++i);
            }
        } catch (IOException e) {
            throw new UncheckedIOException(e);
        }
        System.out.println("Finished!");
    }
}

Configuration.java

import java.io.File;

public interface Configuration {
    /**
     * Returns the name and path of a file to read from in form of a String.
     */
    File getSourceFile();

    /**
     * Returns the name and path of a file to write to in form of a String.
     */
    File getSinkFile();

    /** Prints all arguments */
    void printArguments();
}
\$\endgroup\$
  • \$\begingroup\$ UncheckedIOException is a class in java.io that extends RuntimeException, as such doesn't need an implementation from me. I'll write another comment once I had a time to go through everything. In advance - thanks for introducing me to PrintWriter, didn't read anything about this class before, but it'll be convenient to have around. \$\endgroup\$ – Isofruit May 19 '17 at 19:35
  • \$\begingroup\$ Also small attachement: I ran the entire program over a ~1GB FASTA database - it takes ~ 8-13 seconds either way, regardless of the use of "contains" or "charAt". Those were not clean benchmark values, those were more to get a "feeling" for the performance and see if there are drastic changes. It seems the printing/writing process is in any case so expensive, that the if-clause in theses cases does not make a vast difference. \$\endgroup\$ – Isofruit May 19 '17 at 20:07
  • 1
    \$\begingroup\$ the catching, wrapping and throwing is because IOException is a checked exception and RuntimeException (or specifically in this case UncheckedIOException) isn't, which allows you to omit it from throws-clauses. \$\endgroup\$ – Vogel612 May 19 '17 at 21:26
  • \$\begingroup\$ @Isofruit I think this may come from branch prediction and the fact that IO is generally significantly more costly than just accessing the RAM to iterate over a String ... \$\endgroup\$ – Vogel612 May 19 '17 at 21:29
  • \$\begingroup\$ @Vogel612 You'll note that in the code, I rethrow the exception wrapped in an UncheckedIOException. As this is an application and not an API, there should be no problem in declaring main as throws IOException and being done with it. \$\endgroup\$ – Tamoghna Chowdhury May 20 '17 at 5:45
2
\$\begingroup\$

Philosophy lessons with slowy

a priori: OP and me talked about the the solution in the initial question, let me explain why he came up with this solution.

a prior II: I call it philosophy lesson, because it's less of a programming lesson ;)

I must highly disagree with the "over-engineered" statement of Tamoghna Chowdhury. Of course, it depends, but since it is a learning excercise, it's the best opportunity to apply object oriented principles, right? If not applied to an easy problem, I doubt, OP will have it easy, applied to a more complex problem. I agree, make the solution, as easy as possible, but not easier. I draw the line between easy and easier earlier, it seems.

And my philosphy - and the philosophy of most of the developers I talk to, which are developing large enterprise applications - is: Good code runs its test. Good code has a decent code coverage. That's how I made OP do this solution.

I highly disagree with the statement "reducing maintainability" / "adding complexity", I state the opposite - of course, always with unit testing in mind: Having unit tests not only verifies, it also - and that's imo more important - enforces good design - Because of one reason: If you can't test it, you have a design problem. Now, if you have code, which is not tested and you have to refactor it, you do not know if it still works. Having a set of unit tests, which can be executed after every step of the refactoring, will always help and give you confidence. Unit tests also are a very good documentation. My experience has been: Code with decent test cases, which may seem "a bit over engineered" are easier to maintain, than code without tests - and therefore more classes/abstraction/etc.

The single responsibility principle's goal - which we tried to apply here - is to reduce complexity: A class should only have one reason to change. I have catched myself too often, facing routines, which started easy as this one, but "historically grew", without having test cases and putting new requirements without thinking too much about future self. It often ends up in reverse engineering, trying to test it, introducing bugs and also - the worst - staying late friday night. In my opinion and my experience, the initial investment will pay off, sooner or later, when on the other hand, not applying the principles, will be expensive.

code review

Since I'm whining about test cases, I'm gonna start with that one. I applied the tdd approach as best as I could. I didn't care too much about naming (otherwise, I'd sit here all night) or exception handling, I try to explain my "ranting" from above. I used JUnit4 and mockito 2.7.22.

// This test class tests "two", the "remove spaces when line starts with >", and the "do not touch the line if not" aspect.
public class RemoveSpacesClientTest {
    /* LineReader and Writer are mocked - this can be easily done, since those are interface.
     * Unit tests by definition, do not touch file systems, services or other external components. */
    @Mock
    private LineReader lineReader;
    @Mock
    private LineWriter lineWriter;

    private RemoveSpacesClient removeSpacesClient;

    @Test
    public void linesWithGTSignRemovesEmptySpaces() throws Exception {
        String lineToRead = "> A A";
        String readLineWithoutSpaces = ">AA";
        when(lineReader.readLine()).thenReturn(lineToRead).thenReturn(null);

        removeSpacesClient.start();

        verify(lineWriter).writeLine(readLineWithoutSpaces);
    }

    @Test
    public void noGTSignWritesSameLine() throws Exception {
        String lineToRead = "A B C";
        when(lineReader.readLine()).thenReturn(lineToRead).thenReturn(null);

        removeSpacesClient.start();

        verify(lineWriter).writeLine(lineToRead);

    }

    @Before
    public void setUp() throws Exception {
        initMocks(this);
        removeSpacesClient = new RemoveSpacesClient(lineReader, lineWriter);
    }
}

Here's the actual program:

public class RemoveSpacesClient {
    private final LineReader lineReader;
    private final LineWriter lineWriter;

    public RemoveSpacesClient(LineReader lineReader, LineWriter lineWriter) throws IOException {
        // readers and writers get injected, so no need for a configuration here for now.
        this.lineReader = lineReader;
        this.lineWriter = lineWriter;
    }

    public void start() throws IOException {
        int i = 0;
        String line;
        // I transformed the for loop to a while loop, since imo it's easier to read
        while ((line = lineReader.readLine()) != null) {
            if (line.charAt(0) == '>') {
                line = line.replace(" ", "");
            }
            lineWriter.writeLine(line);
            printProgramProgress(++i);
        }
    }


    private void printProgramProgress(int i) {
        if (i % 1000000 == 0) {
            System.out.println(i / 1000000 + " * 10^6 lines written.");
        }
    }

    public static void main(String[] args) {
        // dedicate the main method to the creation and wiring of the needed components aka "dependency injection"

        // I don't have the argument handler code, so this just a hard coded configuration impl
        Configuration configuration = createArgumentHandlerConfiguration(args);

        // The creation of the LineReader and Writer are done by a LineFactory (aka static factory: hide implementation, provide a name to the object generating method (!)
        try (LineReader lineReader = LineFactory.createFileLineReader(configuration.getSourceFile());
        LineWriter lineWriter = LineFactory.createFileLineWriter(configuration.getSinkFile())) {
            new RemoveSpacesClient(lineReader, lineWriter).start();
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

The LineFactory is a plain static Factory which actually creates FileLineReader's and FileWriteReader's which use LineNumberReader's and FileWriter's - pretty straight forward.

You can call it overkill. I call it super sexy! :P

Hope this helps...

\$\endgroup\$
  • \$\begingroup\$ To add to the discussion - your approach seems to be the long-term flexible one (easily exchangeable classes) and in exchange demands higher development effort. Question for the static LineFactory class - Is there a way to write createFileLineReader/Writer that determines which class in LineFactory to use via a hashmap? E.g. you write in a key as argument to the method and depending on that key it chooses which specific implemented LineReader shall be used. It could be done via a switch statement alternatively, but that runs the risk of becoming spaghetti-code. \$\endgroup\$ – Isofruit May 23 '17 at 6:00
  • \$\begingroup\$ Also - you use try/catch to surround the creation of the LineReader and LineWriter, Is there a benefit to catching the FileNotFoundExceptions that'll be occuring there ? So far it seems to me the Client can't really do anything in this situation - if the file isn't there, the file isn't there and you can't assume where they put it or how they named it. Wouldn't it make more sense to have the main method throw the exception instead of ignoring it? Though you could force some user-program-interaction via terminal I guess. \$\endgroup\$ – Isofruit May 23 '17 at 6:03
  • \$\begingroup\$ 1): Nah, in that case, I'd just provide another create-method. Otherwise, the caller does have knowledge of the implementation detail again. Edit: If you have "problems" with your creation of objects, see GoF's "Factory Method" / "Abstract Factory" patterns. \$\endgroup\$ – slowy May 23 '17 at 8:35
  • \$\begingroup\$ 2) As I mentioned, I didn't take too much care about exception handling. I just put it there, mainly because I was lazy :P. The exception handling itself depends on the requirements of the app. \$\endgroup\$ – slowy May 23 '17 at 8:40
  • \$\begingroup\$ As I wrote in our chat before I realized this question also makes sense on the codedreview website: Setting up the RemoveSpacesClient as an actual object and than instantiating it, solely so that a method in the RemoveSpacessClient can then perform the actual main-function seems somewhat counter-intuitive. I think I see the benefit here - You never have to touch the Client-code like this, even when manipulating the main function of the code. But I'm not sure that's worth the effort. \$\endgroup\$ – Isofruit May 23 '17 at 17:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.