1
\$\begingroup\$

The program itself reads 100 values from a text file named input.txt. Then sorts those values in ascending order and writes the values to a new file called output.txt.

I used two try-catches for the Scanner and the PrintWriter since to my very basic knowledge they can throw different exceptions. So if anyone can review the code and let me know if my style is bad/good and what could be better, that would be much appreciated.

    //Create new file in, used for input.
    File in = new File("input.txt");

    /* Used two try-catch blocks to narrow down the potential error.
    * Read online if you use try-catch you want to keep them as
    * narrow as possible to know what to catch.
    * If this is bad style let me know.
    */

    try {
        Scanner input = new Scanner(in);
        for (int i = 0; i < numbers.length; i++) {
            numbers[i] = input.nextInt();
        }
        input.close();
        Arrays.sort(numbers);
    } catch (FileNotFoundException e) {
        /*Assume the file is missing since new Scanner(in);
        * will implicitly check if the input file actually exists.
        */
        System.err.println("File missing. Exiting with code: 1");
        System.exit(1);
    }

    try {
        PrintWriter output = new PrintWriter("output.txt");
        for (int i = 0; i < numbers.length; i++) {
            output.println(numbers[i]);
        }
        output.close();
    } catch (IOException ex) {
            System.err.printf("File name does not denote an existing, writable regular file %n"
                            + "and a new regular file of that name cannot be created%n"
                            + "Or another error occured creating/writing to the specified file.%n"
                            + "Exiting with code: 2");
            System.exit(2);
    }
\$\endgroup\$
2
  • 1
    \$\begingroup\$ The declaration of numbers is missing. \$\endgroup\$ – Bobby Jan 30 at 19:19
  • 1
    \$\begingroup\$ The site standard is for the title to simply state the task accomplished by the code, not your concerns about it. Please see How do I ask a good question? for examples, and revise the title accordingly. \$\endgroup\$ – Martin R Jan 30 at 19:24
3
\$\begingroup\$
File in = new File("input.txt");

Do not shorten variable names, it makes the code harder to understand and maintain.


        for (int i = 0; i < numbers.length; i++) {
            numbers[i] = input.nextInt();
        }

I'm a very persistent advocate that you're only allowed to use single-letter variable names if you're dealing with dimensions. Even for simple for loops one should use index or counter or something similar descriptive.


    try {
        Scanner input = new Scanner(in);

Could have used a try-with-resources instead.


System.exit(1);

Just as a note, that is not "let's exit the application" kind of call, it's "let's end the JVM abruptly" kind of call. When calling System.exit, not even finally blocks will be executed.


for (int i = 0; i < numbers.length; i++) {

You could use a for-each instead.


            System.err.printf("File name does not denote an existing, writable regular file %n"
                            + "and a new regular file of that name cannot be created%n"
                            + "Or another error occured creating/writing to the specified file.%n"
                            + "Exiting with code: 2");

That's completely unhelpful error handling and is on the same level as Microsoft Windows' infuriating "Your PC had a problem :)" message. Never throw away error information that you have, it will make debugging a lot harder. Printing the exception message would be a minimum that you should do because from that error message, even though you did put a lot of thought and work into it, it's completely impossible to deduce the actual error and what to do to remedy it.

Short story time: I've been working with a third-party service lately and that service tries to contact yet another service. Whenever there is a problem with the communication with the other service, the service will spew out the log message "Failed to communicate with that other service, is the other service really already started?" followed by the exception message that clearly shows that an HTTP response was received. Not helpful, because you know that somebody will get hung up at some point on that message even though the question is bogus.


Coming around to your actual question, if you put your logic into a method of its own, your question simply goes away.

public static final void sortContent(File inputFile, File outputFile) throws IOException {
    // TODO Add verification that inputFile is not null.
    // TODO Add verification that outputFile is not null.

    int[] numbers = new int[100];
    
    try (Scanner scanner = new Scanner(inputFile) {
        // Logic goes here.
    }

    // Rest of logic goes here.
}

Now all you need to worry about is closing the resources correctly, which the try-with-resources will do for you. The IOException will simply being thrown and bubble up until it is being handled. For example in your main method:

public static final void main(String[] args) {
    try {
        sortContent(new File("input.txt"), new File("output.txt"));
    } catch (IOException e) {
        ex.printStackTrace();
    }
}

If you feel especially inclined to provide more information, you can always wrap the thrown exception.

public static final void sortContent(File inputFile, File outputFile) throws IOException {
    // ...
    
    try (Scanner scanner = new Scanner(inputFile) {
        // ...
    } catch (IOException e) {
        throw new IOException("Failed to read from input file <" + inputFile.toString() + ">.", e);
    }

    // ....
}

Notice that we do not throw away the original exception, but instead use it as cause for the one we are throwing.


Optional appendix: Why are one letter variable names not so good and should be avoided if possible?

Good code is expressive, as it is with the spoken language, expressiveness tends to depend on three things:

  1. More letters
  2. More words
  3. The correct terms

If we only use few letters to communicate something, we are not very expressive, example "ltns gr8 cu 2d asap". Of course this is readable to some, but not as widely as if it would be a written as "long time no see, great idea, see you tonight as soon as possible".

If we only use few words to communicate something, we are not very expressive, example "tomorrow margaritas". Again, we can deduce the meaning if we are familiar with the context, but without the necessary context we have a hard time understanding "We'll meet tomorrow at your place, I bring the margaritas".

If we use the incorrect terms to communicate something, we are note very expressive, example "you can use our typewriter". Again, it will take use much longer to figure out that "typewriter" is used her interchangeably with "printer".

So we can establish the simple rule: The more expressive, the better.

Let's map this to code. We do not always have the luxury of being aware of the complete context of the code we are looking at. Here is a simple example:

a[i] = a[i] + 5;

We can deduce from this line the following assumptions:

  1. a is an array, very likely int.
  2. i is very likely a loop counter.
  3. 5 is added to the value at the index i.

There's a good chance that these assumptions would be correct. However, we still do not know what a is, or what range i has (spoiler: we'll never know, actually), or why 5 is being added to these values. So if we look at the same snippet with better naming, at least the first question becomes clear:

readValues[index] = readValues[index] + THE_MAGIC_VALUE_THIS_NAME_TELLS_YOU_SOMETHING;

Notice how index still doesn't tell us much besides "it's most likely a loop over the whole array, otherwise it would have a better name". But if we swap out index with counter, we suddenly have a completely different meaning of this line:

readValues[counter] = readValues[counter] + THE_MAGIC_VALUE_THIS_NAME_TELLS_YOU_SOMETHING;

We know that it is very likely not a loop over the whole array, but instead over a different range, maybe this is not even a for loop, but a while loop. So with a simple name change we can already transport some meaning. Let's build up a larger example:

// Assume a to be non-null int[]

for (int i = 0; i < a.length; i++) {
    for (int j = a.length - 1; j > i; j--) {
        if (a[i] == a[j]) {
            storeForLater(a, i, j);
        }
    }
}

Additionally, because the code was refactored the names carried over into the function that we call inside the loops, storeForLater(int[] a, int i, int j). And I believe at this point we can all agree that the names are not that good. Also notice how I sneakily extended the scope of the argument by introducing the refactored method. Let's see if we can improve that with proper names:

for (int leftIndex = 0; leftInex < readValues.length; leftIndex++) {
    for (int rightIndex = readValues.length - 1; rightIndex > leftIndex; rightIndex--) {
        if (readValues[leftIndex] == readValues[rightIndex]) {
            storeForLater(readValues, leftIndex, rightIndex);
        }
    }
}

Now that's quite a mouthful, but every line on its own has become much more expressive. Let's have an isolated look at the if:

if (a[i] == a[j]) {

if (readValues[leftIndex] == readValues[rightIndex]) {

Given the more expressive names in the second if, we can make quite a few assumptions more about the code and what it is supposed to do and what it is about.

Let's do another quick example, assume the following line of code:

    s[j][i] = getValueFromExternalSource(i, j);

Speed question: What is i and j?

Yes, "a loop counter" is also a correct answer I guess.

    spreadsheet[column][row] = getValueFromExternalSource(row, column);

As we can see, that one cleared it up quite nicely.

I earlier said that "you're only allowed to use single-letter names when dealing with dimensions", what I meant are "x", "y" and "z" (possible "w" and "v", if you need another two), because these are on their own expressive enough and there is not really a more expressive way:

for (int x = 0; x < width; x++) {
    for (int y = 0; y < height; y++) {
        process(data[x][y]);
    }
}

That's as good as it gets when you simply deal with two dimensional data, or more dimensions. So there is no really a point in enforcing "dimensionX" or "indexX" or something like that. Yet I still feel the need to point out that if you're working on data that is consisting of rows and columns, row and column would be the preferred names for the loop.

We've already seen that the gains are not world-changing, but still quite interesting and in some cases can be significant. Now let's move on the whole point of the exercise: Production code. Does it matter whether or not you use i in your exercise snippets? No, it does not. Does it matter whether you use i in your production code? Given that I have seen code that is 40 years old and still is maintained, I'd say yes, yes it does. The worst example in this regard was a 250 line logic that was something like this:

// Lots of setup code.

for (int x = 0; x < someArraySize; x++) {
    // Some logc goes here.
    
    for (int a = 0; a < someOtherValueCantRemember; a++) {
        // More logic goes here.
        
        for (int b = someValue; b < yetAnotherValue; b++) {
            // The main part of the logic.
        }
        
        // A little bit of logic here.
    }
    
    // Some more logic here.
}

Again, we can all agree that this code is already badly structured, I believe it was something around 8 years or so old and the programmer long since gone. But especially in this case every name counts to make sure that you're able to follow the code. On my 1920x1080 screen I can see ~65 lines of code at once, in this case it means the logic was 4 pages long. That's quite some scrolling you have to do if you need to look up what a variable does because "a" tells you exactly nothing. And no, x had nothing to do with a dimension, it was a normal index loop.

Short interjection for everyone who now wants to say "At my work we throw that away and rewrite it": Great for you! Our customers pay for features, and redoing work our own company has done is not a feature.

Short interjection for everyone who now wants to say "Maybe you should find a better place to work, yours sucks": Great, thank yo! In the mean time I'd like to pay rent and buy food.

Back on topic, code like this is very, very common in software that had some point a deadline and a release. There are heaps and heaps of it, and the likelihood that you will encounter this than properly structured code is very good. What we can do, what we should try every day, is not to add to this pile, by writing the best code we can. And in my opinion, we should do our best to make every single line stand on its own as could as we can, without as much context as we can, because if the project is non-trivial, this line might be the only thing another programmer sees from our logic. If somebody is for example following a stacktrace, they will not want to familiarize themselves with every single piece of code on the way (who's got the time?). So every single line must be as expressive as possible.

Last but not least, I said it does not matter what you use in your exercises, so why to I raise the point in this review? Habit. Humans are pieces of habit. Making exceptions to a rule is always a bad thing, because you now have a special case at your hand that you need to deal with, that you need to remember, that you need to honor. Not making exceptions to a habit is much easier.

\$\endgroup\$
1
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ – Mathieu Guindon Feb 2 at 12:47

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.