11
\$\begingroup\$

I am learning Java and this is my solution for a program that will read in a name from the command line and write it out 100 times furthermore words are never split up assuming that the console is 80 characters wide. Could someone please help me improve its quality?

/**
* Program to print command line argument 100 times
* Words are never split up. Assuming that the console is 80 characters wide.
*
**/
class Hundred {
    private static final int LINE_WIDTH = 80;

    public static void main(String[] args) {
        if(args.length == 0) {
            System.out.println("Argument missing");
        } else {
            String name = args[0];

            int wordsOnOneLine = LINE_WIDTH / (name.length() + 1);
            int i = 0;
            while(i < 100) {
                for(int j = 0; j < wordsOnOneLine && i < 100; j++) {
                    i++;
                    System.out.print(name);
                    System.out.print(" ");
                }
                System.out.println();
            }
        }
    }
}
\$\endgroup\$
  • 6
    \$\begingroup\$ For no-productive-value-school-assignment-code, I think this is really OK. One bug though: if you have an input of 8 chars width, you only use 72 chars of the line, whereas another 8 chars would fit (but NO following space) - see if you can fix that. \$\endgroup\$ – mtj Feb 7 '17 at 7:08
13
\$\begingroup\$

General Remarks

Well done. While there are some points of improvement, you created a functional program, with proper naming of arguments, and even some basic error handling. Asking for feedback is the best way to learn, so here you get some :). Keep up the good work.

Repeating/LineWidth logic

The repeating of the value 100 is suspicious, there probably is a better solution.

   while(i < 100) {
                for(int j = 0; j < wordsOnOneLine && i < 100; j++) {

If you see this, think more and try to approach the problem differently.

The main idea (that vp_arth already posted) is to keep track of the available space in the line, no need to calculate the wordsOnOneLine

Also, you can count-down the number of words you still have to output.

You will end up with two simple counters repetitions and availableSpace. See below for more detail.

Cleanness of output

While not really enforced, you should not add a space at the end of the line if not needed. Trailing spaces are a general pain in the ass, because you can't see them and might give adverse effects.

Validating arguments

And you need to take care your user does not apply an argument that will break your program (what if the user enters an 1000 character String?), so you need to check for that too.

Java conventions

You already follow most of the java conventions (casing, bracing, etc). But you are missing a package for your class. While not needed, it is good practice.

Exit codes

While not THAT interesting, it is good practice to let you program end with an error code (integer greater than zero) if it could not run succesfully.

Proposed solution

package hundred;

/**
 * Program to print command line argument 100 times Words are never split up. Assuming that the console is 80 characters
 * wide.
 **/
public class Hundred
{
    private static final int LINE_WIDTH = 80;

    public static void main( String[] args )
    {
        if ( args.length == 0 )
        {
            System.err.println( "Argument missing" );
            System.exit( 1 ); //error status
        }
        else
        {
            String word = args[0];
            int availableWidth = LINE_WIDTH;

            if ( word.length() > availableWidth )
            {
                System.err.println( "Argument too long for the line width" );
                System.exit( 1 ); //error status
            }

            int repetitions = 100;

            while ( repetitions > 0 )
            {
                if ( availableWidth < word.length() )
                {
                    System.out.println();
                    availableWidth = LINE_WIDTH;
                }
                System.out.print( word );

                availableWidth -= word.length();

                //We might need a space, but only if the word + space still fits
                if ( availableWidth > word.length() + 1 )
                {
                    System.out.print( " " );
                    availableWidth--;
                }

                repetitions--;
            }
        }
    }
}
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ I want to notice as you mentioned that "wordsOnOneLine" is not needed: It is important to derive the model we implement in code should be as near at reality as possible. You correctly stated that "wordsOnOneLine" is not the subject in this context and I want to support this semantic suggestion. \$\endgroup\$ – oopexpert Feb 7 '17 at 8:27
  • 2
    \$\begingroup\$ @RobAu when you exit in the first if where you check the args.length then the else is not necessary. :) \$\endgroup\$ – Dodge Feb 7 '17 at 9:37
  • 1
    \$\begingroup\$ You mentioned following Java conventions, yet you place opening curly braces on new lines and place spaces after opening and before closing parenthesis, which is certainly not Javaesque. \$\endgroup\$ – Mikhail Feb 7 '17 at 9:37
  • 1
    \$\begingroup\$ @Mikhail true, it is not the Oracle preference, but (amongst others) maven uses this style and I use it as default formatter for my pet-projects. Mainly for readability reasons. I know this might trigger holy wars :) With bracing I meant the single-statement-if in the example. \$\endgroup\$ – RobAu Feb 7 '17 at 11:42
  • 1
    \$\begingroup\$ I know using the default package is discouraged, but I dislike the redundancy that repeating the class' name brings. Supposing this is a student's assignment for a specific course, I'd suggest something along the line of <studentName>.<courseName>.assignments, with maybe another level to identify the specific assignment. Supposing OP decided by himself to do this exercise and won't share it with anyone or use it in conjunction with other code, I might prefer the default package (although it's obviously a good thing you mentioned it doesn't follow conventions). \$\endgroup\$ – Aaron Feb 7 '17 at 14:36
1
\$\begingroup\$

You should not reserve space for last space.
For example: 80char line fits 9 times of 'EightNam'(9 words*8+8 spaces) Also, you should check, that one instance of name not longer than available space.

class HelloWorld {
    private static final int LINE_WIDTH = 19;
    private static final int WORD_CNT = 20;

    public static void main(String[] args) {
        if(args.length == 0) {
            System.out.println("Argument missing");
            return;
        }
        String name = args[0];

        if (name.length > LINE_WIDTH) {
            System.out.println("Name is too long");
            return;
        }

        int availableWidth = LINE_WIDTH;
        for(int i = 0; i < WORD_CNT; i++) {
            System.out.print(name);
            availableWidth -= name.length();

            if (availableWidth < 1 + name.length()) {
                availableWidth = LINE_WIDTH;
                System.out.println();
            } else {
                System.out.print(" ");
                availableWidth -= 1;
            }
        }
        System.out.println();
    }
}
| improve this answer | |
\$\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.