7
\$\begingroup\$

I made a Java program that asks for a word, then a screen width, then whether you want it to go fast or slow. Then it outputs that word and bounces it across the screen. Is this code efficient for what it does? Is there a better way to do it (without a pre-defined method/class)? And does it look professional and easy to read?

/*
  This program was designed for use with command prompt on an ASCII
  character set.  Output is run through CMD.
  This was designed for CMD with only an 80 ASCII character width.

  str1 is the string containing the spaces.
  str2 is the string containing the input word.
*/

import java.util.Scanner;

class Wave6 {


  //Causes the output to lag for a set amount.

  public static void xwait(long L) {
    for(long c=0; c<L; c++);
  }


  //Asks for a string to "bounce" across the screen.

  public static String input_string(char chr, String str2)
    throws java.io.IOException {

    do {
      System.out.print("\n"
                     + "What word do you what to input? \n"
                     + "Must be less than 81 characters: ");

      /*
        Reads a list of characters and
        inputs it into a string (to allow
        for spaces and other non-letter chars).
      */
      do {
        chr = (char) System.in.read();

        if(chr != '\n') str2 += chr;

      } while(chr != '\n');

    //Checks if the string is in the acceptable range.
    } while((str2.length() < 1) | (str2.length() > 80));

    return str2;
  }


  //Asks for a screen width (this is arbitrary).

  public static int input_width(int width, String str2){

    Scanner input = new Scanner(System.in);

    do {
      System.out.print("\n"
                     + "What is the width of the screen? \n"
                     + "Must be less than 81, and more than " + str2.length() + " characters: ");

      width = input.nextInt();

    } while((width <= str2.length() ) | (width >= 81));

    return width;
  }


  //Asks if the user wants the program to run
  //at full speed, or slower.

  public static long input_speed(char ch, char ign, long L)
    throws java.io.IOException {

    do {
      System.out.print("\n
                     + "Do you want it slow or fast? \n"
                     + "S or F: ");

      ch = (char) System.in.read();

      //Gets rid of unwanted chars in
      //the char buffer.

      do {
        ign = (char) System.in.read();
      } while(ign != '\n');

    //checks if ch == F or S
    } while( !(ch == ('F')) ^ (ch == ('S')) );


    //Returns a large or small number
    //to slow down the program, or not.

    L = 0;

    if(ch == 'S') 
      L = 25550000L;
    else if(ch == 'F')
      L = 0;

    return L;
  }


  public static void main(String args[])
    throws java.io.IOException {

    String str1 = "", str2 = "";
    int width = 0, i;
    char ch = '\0', chr = '\0', ign = '\0';
    long L = 0L;

    str2 = input_string(chr, str2);

    width = input_width(width, str2);

    L = input_speed(ch, ign, L);





    /*
      Part of the program that actually makes the word
      bounce around.
    */


    for(;;) {

      for(;;) { //Right bound

        /*
          If the two strings added length is equal to the
          Input width +1, do not output a character return.
          This is the end of the right bound, break out
          of the infinite loop.
          Based on the code, between the conversion from 
          right bound to left bound, str1 has to equal width.
          Otherwise the word only goes to the width -1 space. 
          So str1 + str2 must equal width +1
        */

        if( width + 1 == (str1.length() + str2.length()) ) {
          System.out.print(str1 + str2);
          xwait(L);
          break;
        }
        else {
          System.out.println(str1 + str2);
          xwait(L);
        }

        //If the two strings are not equal to width +1
        //add one space to str1

        if(width + 1 != ( str1.length() + str2.length()) )
          str1 += " ";
      }


      for(;;) { //Left bound

        /*
          Right bound handles the code when str1 and str2
          are equal to width +1, so left bound doesn't have to.
          Similarly Right bound handles the output for when
          str1 equals 0, but left bound makes it equal to 0.
        */

        if( width + 1 != (str1.length() + str2.length()) ) {
          System.out.println(str1 + str2);
          xwait(L);
        }

        //Take a space off of str1.
        str1 = str1.substring(0, str1.lastIndexOf(" "));

        if(str1.length() == 0)
          break;
      }
    }
  }
}

Also, I use n++, and I have my tab set to two ASCII character widths, so when I copied and pasted all of it was set to four spaces, is there an easy way to copy it without having to go back and delete all of the tabs and replace them with spaces? It takes forever. (this isn't important to me, but it is a hassle when trying to ask questions about code on here)

This happened when I did this in Basic on my Ti 84, but what if I had made an output_leftBound() and output_rightBound() method, used main to call rightBound(), then had rightBound() and leftBound() call each other? Would that be efficient or would it cause the memory to build up? On the calculator, if you goto out of loops, the calculator builds up memory because it reserved it to look for an END command to signal the end of a loop, but it never finds one.

Someone mentioned that:

public static void main(String args[])
  throws java.io.IOExeption {

was a code smell. How (in an inquisitive tone)? The book I am using uses it whenever it uses System.in.read(). Though I did notice that when I didn't include it my other methods that use System.in.read(), the compiler reported an error stating:

Unreported exception IOExeption; must be caught or declared to be thrown

Is this a problem?

\$\endgroup\$
  • 1
    \$\begingroup\$ when it is about clean code, make sure that you get rid of unnecessary spaces. \$\endgroup\$ – Kinjal Sep 15 '13 at 4:32
  • \$\begingroup\$ Give some better names to variables/methods. You have some comments but I really prefer to see good names without comments (as Uncle Bob used to say) \$\endgroup\$ – Artiom Sep 18 '13 at 2:19
8
\$\begingroup\$

Major issues

This feels like C code more than Java. Part of the reason is the code structure — you're using class methods everywhere instead of instance methods. The bigger problem is your parameter passing. For example, in input_string(char chr, String str2), you are using chr and str2 as nothing more than local variables — you completely disregard their input values. Why not just take zero parameters and return a String?

Your input routines are unnecessarily complicated. You shouldn't have to read one character at a time. Just read a line at a time using BufferedReader.readLine().

Your xwait() function relies on busy-waiting, which you should never ever do. The first problem with busy-waiting is that it chews up CPU cycles which could be yielded to other processes running on the machine. Laptop users will hate you for wasting battery power warming up the CPU and spinning the fan. These days, you might even be running on a virtual machine on a multi-tenant host, which will cause other customers to complain! The other problem with busy-waiting is that the delay depends on the CPU's speed, which will differ wildly on various machines. Assumptions about the CPU speed are what forced early PCs to have a "Turbo" button — to slow down the CPU so that poorly written games could run at their intended speed. If you want to introduce a delay, call Thread.sleep().

Your right-bound and left-bound loops are excessively complex. The way you use an infinite for(;;) loop and break out of them is unconventional. Why not use the for-loops the way they are intended to be used? Then every programmer can tell at a glance what the structure of your loops are — what is being tested and what changes between iterations.

Nitpicks

Comments on your classes and functions might as well be JavaDoc comments. All you have to do is follow the JavaDoc formatting conventions.

Indentation should be at least four spaces. Two spaces is too narrow to read reliably, and it also encourages excessively deep nesting, which is a symptom of poorly organized code.

Method names in Java should lookLikeThis().

Your variables str1, str2, L, ch, chr, ign are all poorly named. None of them is self-descriptive. For the first three, I suggest lead, word, and delay instead. Avoid declaring all your variables at the beginning of the function. For tightest scoping and least mental workload, you should declare variables as close as possible to where they are used. Also, don't initialize all your variables to 0 or \0 frivolously — let the Java compiler tell you when you have failed to assign them meaningful values before usage.

You should use a boolean-or rather than a bitwise-or in while((str2.length() < 1) | (str2.length() > 80)) and similar expressions.

When shrinking str1, just call str1.substring(1). Chopping off the first character is much easier than finding the last one and removing it. Also, test .isEmpty() rather than .length() == 0.

Proposed Solution

import java.io.*;

/**
 * This program was designed for use with command prompt on an ASCII
 * character set.  Output is run through CMD.
 * This was designed for CMD with only an 80 ASCII character width.
 */
class Wave6 {
    /**
     * Asks for a string to "bounce" across the screen.
     */
    public static String inputString(BufferedReader in)
      throws IOException {
        String input;
        do {
            System.out.print("\n"
                             + "What word do you want to input? \n"
                             + "Must be less than 81 characters: ");
            input = in.readLine();
            if (null == input) throw new EOFException();
            //Checks if the string is in the acceptable range.
        } while (input.length() < 1 || input.length() > 80);
        return input;
    }


    /**
     * Asks for a screen width (this is arbitrary).
     */
    public static int inputWidth(BufferedReader in, String word)
      throws IOException {
        int width;
        do {
            System.out.print("\n"
                             + "What is the width of the screen? \n"
                             + "Must be less than 81, and more than "
                             + word.length() + " characters: ");
            String input = in.readLine();
            if (null == input) throw new EOFException();
            width = Integer.parseInt(input);
        } while ((width <= word.length() ) | (width >= 81));
        return width;
    }


    /**
     * Asks if the user wants the program to run at full speed, or slower.
     * @return The number of milliseconds of delay per output line
     */
    public static long inputDelay(BufferedReader in)
      throws IOException {
        while (true) {
            System.out.print("\n"
                             + "Do you want it slow or fast? \n"
                             + "S or F: ");
            switch (in.read()) {
                case -1: throw new EOFException();
                case 'S': return 100;
                case 'F': return 0;
            }
            // Swallow the rest of the line and the end-of-line terminator
            in.readLine();
        }
    }

    public static void bounce(String word, int width, long delay)
      throws InterruptedException {
        // lead is the string containing the leading spaces.
        String lead = "";

        while (true) {

            // Right-bound loop
            for(; lead.length() + word.length() <= width; lead += " ") {
                System.out.print(lead);
                System.out.println(word);
                Thread.sleep(delay);
            }

            // Take two spaces off of lead: one because a space was added
            // when leaving the loop above, and another because we want to
            // start moving left.
            lead = lead.substring(2);

            // Left-bound loop
            for(; !lead.isEmpty(); lead = lead.substring(1)) {
                System.out.print(lead);
                System.out.println(word);
                Thread.sleep(delay);
            }
        }
    }

    public static void main(String args[])
      throws IOException {
        BufferedReader br = new BufferedReader(new InputStreamReader(System.in));

        String word = inputString(br);
        int width = inputWidth(br, word);
        long delay = inputDelay(br);

        // Part of the program that actually makes the word bounce around.
        try {
            bounce(word, width, delay);
        } catch (InterruptedException timeToQuit) {
            // Sleep interrupted? Time to quit.
        }
    }
}

Optional Suggestion

Consider pre-allocating an array of 80 (or however many) lead strings instead of creating and tossing one for each line.

    private static String[] leads;
    public static void bounce(String word, int width, long delay)
      throws InterruptedException {
        // Pre-allocate all the leading space strings, such that
        // leads[i] is a string containing i spaces
        if (null == leads || leads.length < width - word.length()) {
             leads = new String[width - word.length() + 1];
             StringBuffer sb = new StringBuffer(leads.length);
             for (int i = 0; i < leads.length; i++) {
                 leads[i] = sb.toString();
                 sb.append(' ');
             }
        }

        while (true) {
            // Right-bound loop
            for(int i = 0; i < width - word.length(); i++) {
                System.out.print(leads[i]);
                System.out.println(word);
                Thread.sleep(delay);
            }

            // Left-bound loop
            for(int i = width - word.length(); i >= 1; i--) {
                System.out.print(leads[i]);
                System.out.println(word);
                Thread.sleep(delay);
            }
        }
    }

Addendum: Suggestion to use printf()

Another way to print a string with leading spaces is to use System.out.printf().

public static void bounce(String word, int width, long delay)
  throws InterruptedException {
    while (true) {
        for (int w = word.length(); w < width; w++) {
            System.out.printf("%" + w + "s\n", word);
            Thread.sleep(delay);
        }
        for (int w = width; w > word.length(); w--) {
            System.out.printf("%" + w + "s\n", word);
            Thread.sleep(delay);
        }
    }
}

It's less efficient, but I think that the resulting simplicity in your own code is worth it.

\$\endgroup\$
  • \$\begingroup\$ Thankyou, since I am reading from the book, it is difficult to know what is a good idea or not in my own programs. When I didn't initialize a variable, but I initialized it through if statements (in another pogram), the compiler had problems, so I think that's why I did that. Thanks on mentioning 0 parameters, I didn't think about returning to another variable. I did use buisy-waiting because I didn't know of a built in java method to do it; thanks for mentioning one. I did use the for loops with an iteration once before, but that felt like it added complexity to my code. \$\endgroup\$ – Lightfire228 Sep 15 '13 at 16:09
  • \$\begingroup\$ You're right, my advice about variable initialization was very poorly worded. I've corrected it in Revision 3. I've also corrected some input-handling issues in Revision 2. \$\endgroup\$ – 200_success Sep 15 '13 at 16:23
  • \$\begingroup\$ Though the way you implemented the for loops was a ton better than what I did. What does str1.substring(1) do exactly? One thing I noticed was many people commented that I should use boolean instead of bitewise operators, but in this book, bitewise operators are the same as logical operators (i.e. the symbols used to call them), so what is the difference between the two? I am using them as conditionals to break out of a do loop, not to change the bits of their values. And how do I implement a pre-allocating array? Thanks for your reply, I needed it (as you can see) \$\endgroup\$ – Lightfire228 Sep 15 '13 at 16:28
  • \$\begingroup\$ @Lightfire Edit I looked up substring in the java documentation, so i know what it does now. \$\endgroup\$ – Lightfire228 Sep 15 '13 at 16:44
  • \$\begingroup\$ See bitwise vs. logical operators. Also, I've added a pre-allocation implementation to the answer. \$\endgroup\$ – 200_success Sep 15 '13 at 17:11
1
\$\begingroup\$

One suggestion that I would make is to use better names for your variables. Typically your variable name should be long enough to describe what the variable represents (and not it's type). Short variable names work ok for loop indexes or in a very small scope (say a lambda expression) but generally you want the reader to be able to understand what the variable is without having to read the context. For example, in your code you might want to name str2 as userInput.

Another suggestion would be, perhaps, to use a BufferedReader to read the input line. For numeric input, where you want to handle incorrect input nicely, you might want to use Integer.parseInt and catch NumberFormatException.

For what it's worth, I'd typically prefer a do-while(true) loop over an infinite for loop, but that's my personal preference.

\$\endgroup\$
  • \$\begingroup\$ Thanks, I'll look into BfferedReader and Integer parseint. When I was writing this (if you noticed that it is called Wave6, it has four other versions; #4 I intended to use after working on #3, but that developed into #5), I used a string based code from my previous version and didn't think about changing the string variables to proper names. Thanks for the tip though. \$\endgroup\$ – Lightfire228 Sep 15 '13 at 4:15

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.