Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite
1

Just gave this as a response to an absolute beginner on SO, wondering how terrible it is.

odd number of lines output:

 >
 >>>
 >>>>>
 >>>
 >

even number of lines output:

 >
 >>>
 >>>>>
 >>>>>
 >>>
 >

the code...

    int lines = 7;
    int half = lines/2;
    String carat = ">";
    int i;

    //better to declare first or does it not matter?         

    if(lines%2==0){boolean even = true;}
    else{boolean even = false;}

    for(i=1;i<=lines;i++){
            System.out.println(carat + "\n");
            if(i==half && even){System.out.println(carat+"\n");}
            if(((i>=half && even) || (i>=half+1)) && i!=lines){
                    carat = carat.substring(0,carat.length()-2);
            }else{
                    carat = carat + ">>";
            }
    }

This code just seems so ugly. Not speaking to the fact that there is no method that takes number of lines and that it is abusing integer rounding.

share|improve this question

4 Answers

up vote 12 down vote accepted

better to declare first or does it not matter?

No. These days most people believe variables should be declared as late as possible, right by where they are used.

This code just seems so ugly.

It is. A little whitespace in there would help a lot. Which is easier to read:

// This:
if(i==half && even){System.out.println(carat+"\n");}

// Or this:
if (i == half && even) {
  System.out.println(carat+"\n");
}

that it is abusing integer rounding.

There's no abuse, and it's not rounding. Truncation on integer arithmetic is a specified part of the language. Feel free to use it. In this case, it does exactly what you want.

If I were writing this, I'd do:

printCarats(int numLines) {    
  for (int line = 0; line < numLines; line++) {
    // Mirror vertically across center line.
    int halfLine = line;
    if (line > numLines / 2) halfLine = numLines - line - 1;

    int numCarats = 1 + halfLine * 2;
    for (int i = 0; i < numCarats; i++) {
      System.out.print(">");
    }
    System.out.println();
  }
}
share|improve this answer
8  
I would rephrase "should be declared as late as possible" rather as "should be declared in as narrow a scope as possible". Perhaps the meaning is the same to most people, but to me, the talking about scope is clearer. For example, the benefit of declaring int i in the for loop statement is that its scope is limited to the loop statement and block. – Bert F Mar 3 '11 at 2:48
up vote 8 down vote 
    if(lines%2==0){boolean even = true;}     
    else{boolean even = false;}

Does this work for you? First, it looks like this makes the even variable only have scope in the decision block. You shouldn't be able to reference this further down in the code if you are declaring it in the braces.

Also, you can probably get rid of the if/else by just using the logic test to define the even variable:

boolean even = (lines%2 == 0); //parens for clarity
share|improve this answer
up vote 4 down vote

Know your libraries, use your libraries...

int lines = 7;

for (int i = 0; i < lines; i++) {
    int k = Math.min(i, lines-i-1);
    char[] line = new char[2*k+1];
    java.util.Arrays.fill(line, '>');
    System.out.println(new String(line));
}
share|improve this answer
nice, except it's java.util.Arrays – barjak Oct 14 '11 at 21:41
Darn... thanks for the catch! – Landei Oct 15 '11 at 10:31
I really like this. But it could be even better - you should initialize a String of length (lines + 1) / 2 before the for loop and print it with substring(...) inside. – Arne Aug 7 '12 at 15:56
up vote 0 down vote

Since there are not any requirements listed for this simple program, except some output that is lacking detail and since this was given to a beginner, I would start with a template like the one included and start the new programmer off with good practices like, code indentation, comments use of CONSTANTS etc.

 /*
 * this program will print the > greater than sign when the loop index is even
 * and the < less than sign when the loop index is odd
 * the program will test up to the integer value 7
 * 
 * programmer name
 * date
 */

//begin class definition
public final class PrintStrings {

 //required main method
    public static void main(String[] args){
      String ODDCARAT = "<"; // ODDCARAT constant
      String EVENCARAT = ">";// EVENCARAT constant
      int i;
      //loop through 7
      for(i=1;i<=7;i++){
        //test if integer is odd or even by % by 2 and checking for remainder
        if(i %2 == 0){
            System.out.println(EVENCARAT);
        }else{
            System.out.println(ODDCARAT);
        }//end if
      }//end for
    }//end main

}//end class
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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