views:

40

answers:

3

I'm writing a 'Sudoku' program, and I need to write a method to check whether a number, the user wants to insert in a certain cell is already contained by the row, column or region. My code looks like this(I'm only checking the row for the number at this point, setNumber returns a boolean value indicating whether the number can be inserted or not):

public boolean setNumber(int row, int column, int number) {  
    if (this.isEmpty(row, column)) {  
     if (!this.rowContains(row, number)) {  
      return true;  
     } else {  
      return false;  
     }  
    } else {  
     return false;  
    }  
  }  

  private boolean rowContains(int row, int number) {  
   for (int i=0; i < this.cells[row].length; i++) {  
    if (this.cells[row][i].getNumber() == number) {  
     return true;  
    }  
   }  
   return false;

}

Now, the same number can be inserted multiple times, so apparently rowContains always returns false, but why?

A: 

Your code snippet looks like it should work to me.

Good old println debugging should be your saviour here. Just put the statements into the code that let you see what's going on (for example:)

private boolean rowContains(int row, int number) {  
 for (int i=0; i < this.cells[row].length; i++) {  
  if (this.cells[row][i].getNumber() == number) {  
   System.err.println("Row " + row + " contains target number " + number + " in cell " + i);
   return true;  
  }  
 }  
 System.err.println("Row " + row + " does not contain target number " + number);
 return false;
}

This won't solve your problem but it will give you a better idea of where your expectations are being invalidated.

EDIT: I've just had another look through your code to see if anything jumped out at me - it didn't (I suspect your problem is elsewhere - are you sure you're actually setting the number on each call?). However, one thing I did notice was the idiom:

if (condition)
    return true;
else
    return false;

which is better in just about every way as

return condition;

Doing things the "longhand way" tends to be considered (for better or worse) a sign of an inexperienced developer.

Andrzej Doyle
Yes, so it looks, but it doesn't :) Probably some other part of the program fails. I'll continue debugging.. :)
rize
A: 

If setNumber is not a misnomer for canSetNumner I would expect it to contain a

cells[row][column].setNumber(number);

line just before the

return true;
rsp
Yes, I forgot to actually set the number in the cell, if it is legal, since I thought another class does it, if this method returns true, but it wasn't so.
rize
A: 

setNumber isn't a very good name (would name it checkNumber, or canSetNumber, for example). 'set' should be used for methods having side effect (setting a value in an object).

You should ensure you actually set the number in cells array, it might be a cause why you can set several times in the same row.

PhiLho