5
\$\begingroup\$

I'm in very early stages of learning some Java coding. I'm asking for this review because, although the code below works, I'd like to know if it's an ugly way of achieving it. What I want the code to do is take input from Scanner and reprint it, except if the input is empty, ie, just hit the return key, in which case the Scanner should close and the program end.

import java.util.Scanner;


public class Morse {

static boolean truefalse = true;

public static void main(String[] args) {
    // TODO Auto-generated method stub
Scanner input = new Scanner(System.in);
while (truefalse)
{
String line = input.nextLine();
if (line.equals(""))
truefalse = false;
else
System.out.println(line);
}
input.close();
}

}
\$\endgroup\$
7
\$\begingroup\$

There are a number of ways your code can be improved:

  1. Use nice indentation using 4 spaces. This improves the readability a lot. See the official guidelines on indentation here.

    Basically every time you open a new curly brace you should start a new indentation depth. (See code sample below)

  2. Use braces around all kinds of blocks, e.g. here even for one-lined if/else blocks. Also, for Java it is the official convention to place the braces like this:

    if (line.equals("")) {
        truefalse = false;
    } else {
        System.out.println(line);
    }
    
  3. You can use a trick to assign the value of the scanner input to a variable inside the while condition and then use the String's isEmpty() function to check if the input was empty. This way you can spare the boolean variable indicating whether something was read:

    while (!(line = input.nextLine()).isEmpty())
    
  4. Use try() to create instances of Types that implement java.lang.AutoCloseable to make sure they are automatically closed when the block ends (Note this only works with Java 7 or newer):

    try (Scanner input = new Scanner(System.in))
    

    See here for reference.

Putting that all together:

public class Morse {

    public static void main(String[] args) {
        try (Scanner input = new Scanner(System.in)) {
            String line;
            while (!(line = input.nextLine()).isEmpty()) {
                System.out.println(line);
            }
        }
    }
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ That's great. Many thanks for all the information. \$\endgroup\$ – Alan Nov 20 '14 at 14:01
5
\$\begingroup\$

I realy dont know why noone uses for.

import java.util.Scanner;
/** The main program to read and write what the user inserts. */
public class Morse {
    public static void main(final String[] args) {
        final Scanner scanner = new Scanner(System.in);
        for (String line = scanner.nextLine(); !line.isEmpty(); line = scanner.nextLine()) {
            System.out.println(line);
        }
    }
}

Because

  1. close() is called automatically on JVM end (see #7).
  2. truefalse is in default scope. This is bad because noone should be able to have control about the inner work of the class in runtime - may match the Open-Close-Principle.
  3. truefalse is a very bad variable name, input too. line is a good variable name.
  4. Its ugly to have absolute no JavaDoc.
  5. for has a variable-part, a condition-part and a increaser-part. Why dont shall we not use them?
  6. isEmpty() is mutch better.
  7. Close a scanner is a very very bad idea, even close indirectly using try()! You will never know when some one else replace the System.in by a Closeable-Stream, let the JVM decide when to close system streams!!!
\$\endgroup\$
4
  • \$\begingroup\$ Compare what you wrote to the not while loop that @Nithathriel wrote. "No one" uses the for loop because it's barely comprehend-able in comparison. Still you have some other valid points, so I refrain from voting one way or the other. \$\endgroup\$ – RubberDuck Nov 20 '14 at 14:53
  • \$\begingroup\$ The while loop that @Nithrathriel have written makes a functioncall, a value-assignment and an empty-check in one single part. for uses 3 parts whear everyone knows the parts's responses. But this is not so important at all. \$\endgroup\$ – Grim Nov 20 '14 at 15:25
  • 4
    \$\begingroup\$ Another advantage of for is that it limits the scope of the loop variable String line to the loop. It's always good to limit variables to the smallest scope where they belong. +1 \$\endgroup\$ – janos Nov 20 '14 at 16:47
  • \$\begingroup\$ Thank you all for your input. It definitely seems like the thrust of my original question, ie, using a boolean, was a clumsy way of achieving what I wanted \$\endgroup\$ – Alan Nov 20 '14 at 18:05
3
\$\begingroup\$

A petty little thing that the other answer didn't mention...

// TODO Auto-generated method stub

Is this really still a Todo? Imagine yourself coming back to this code six months from now. Would you be absolutely positive that you completed the work here if you saw this? It's also important to note that most IDE's will pick up that // TODO comment in some kind of task list. This is just cluttering that up now. If you've implemented the code, remove the Todo.

\$\endgroup\$

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.