Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Is it recommended to combine two similar methods into one?

public void initialize_board(){
    for(int y = 0; y < board.length; y++){
        for(int x = 0; x < board.length; x++){
            if(x == 0)
                board[x][y] = (char)y;
            else
                board[x][y] = '~';
        }
    }
}

public void display_board(){
    for(int y = 0; y < board.length; y++){
        for(int x = 0; x < board.length; x++){
            System.out.print(board[x][y]);
        }
        System.out.print("\n");
    }
}
share|improve this question
3  
Use the camelCase for naming. –  Anton Dozortsev Apr 13 at 21:47
3  
Post rolled back. Please do not edit the original code based on answers; that will invalidate them. –  Jamal Apr 14 at 14:18
    
Small trick: board[x][y] = (x == 0) ? (char) y : '~'; But you may want to stick to the original version for readability. –  toto2 Apr 14 at 16:48

4 Answers 4

up vote 10 down vote accepted

By the Single Responsibility Principle, they should remain separate methods. Furthermore, initialize_board() should probably be a constructor for a Board class, and display_board() should be its toString().

share|improve this answer
    
Indeed, the only reason I turned my constructor into a method was to ask this question. Also, toString() wouldn't work because it's an array of char –  user89428 Apr 13 at 21:25
    
@user89428 What exactly prevents you from concatenating chars to a string? –  Vogel612 Apr 13 at 21:32
    
I don't know, it says : "The method toString() in the type Object is not applicable for the argument (int)". I haven't been able to use toString() in Java successfully yet. –  user89428 Apr 13 at 21:38

Not so similar...

Just because the method bodies "look similar" doesn't make them similar. It's better to think in terms of purpose instead. The two methods are for completely different things. They are not similar, and good to be separated.

In functional languages you could use a "walk" method to iterate over all cells of the board and use a callback function to act on each cell. You could emulate that behavior in Java, but the code will get more complex, and I'm not sure it will be worth it in this example.

Robustness

Do you intend to initialize the board more than once in the lifetime of the object? If not (probably), then move the initialization to the constructor.

Writing style

You should use camelCase for naming, for example initializeBoard instead of initialize_board.


It's recommended to use braces with ifs, for example:

if (x == 0) {
    board[x][y] = (char)y;
} else {
    board[x][y] = '~';
}

It's better to print a newline using println:

System.out.println();
// instead of: System.out.print("\n");
share|improve this answer

The two methods look similar because at some level they do the same thing : they traverse the board. What is repeated is the code to traverse the data structure. The gang of four identified isolating the traversal behaviour as the iterator pattern.

In object-oriented programming, the iterator pattern is a design pattern in which an iterator is used to traverse a container and access the container's elements.

(from wikipedia)

So you could apply the iterator pattern. If it's really just those two methods, it's probably overkill, but keep it in mind when you find yourself traversing the datastructure in other places as well.

share|improve this answer
  1. I guess this does not do what you really want:

    board[x][y] = (char)y;
    

    It sets the first few ASCII control characters and display_board() shows weird results. I'd try this:

    board[x][y] = Character.forDigit(y, 10); 
    
  2. Instead of

    System.out.print("\n");
    

    you should use println() (as @janos already mentioned) but if you want to put a line feed manually you should use

    System.out.print("%n");
    

    instead. %n outputs the correct platform-specific line separator.

share|improve this answer
    
Nice, your first solution fixed the problem I had. –  user89428 Apr 13 at 23:09

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.