Opinions about formatting code + comments

[heebleworp]

First question is about what to do with long conditional tests. For example:

if (puzzle.one_possible(row, col) && (!(puzzle.two_possible(row,col) || puzzle.three_possible(row,col)
|| puzzle.four_possible(row,col) || puzzle.five_possible(row,col) || puzzle.six_possible(row,col)
|| puzzle.seven_possible(row,col) || puzzle.eight_possible(row,col) || puzzle.nine_possible(row,col))))
{
	return val;
}

The above is the best I could come up with. Would like to see alternate methods of formatting such code. Also opinions on the method I've used, mainly is it easily readable given what has to be in the conditional test. Plus am I just being overly nitpicky?

Also have questions about comments. How often do you put them in code? Mostly in header files or in both headers and definitions? Also would like opinions on Doxygen, is it complicated and/or worthwhile?

[giblit]

What are the N_possible functions? Seems like you could use an array but not positive.

[Chervil]

I don't think there are any real rules here. I tend to format code in a way which helps me to understand it. But that doesn't necessarily mean it will work for any other reader.

On another day, I might approach this differently, but I do think the emphasis should be on conveying the meaning, in whichever way seems to make sense.

Below, I removed one level of parentheses which appeared unnecessary,

if (puzzle.one_possible(row, col) 
    && !( puzzle.two_possible(row,col) 
       || puzzle.three_possible(row,col)
       || puzzle.four_possible(row,col) 
       || puzzle.five_possible(row,col) 
       || puzzle.six_possible(row,col)
       || puzzle.seven_possible(row,col) 
       || puzzle.eight_possible(row,col) 
       || puzzle.nine_possible(row,col) ))
{
    return val;
}

[heebleworp]

@Chervil: Was thinking kind of the same, but part of the issue was to condense the code as well. I like your format but here's a larger example of what I was dealing with:

int set_known_check(const Sudoku& puzzle, int row, int col)
{
	if (!puzzle.is_known(row, col)) //only do stuff if the value isn't already known
	{
	for (int val = 1; val <= 9; ++val) //TODO: fix indentation
	{
		switch (val)
		{
		case 1:
			if (puzzle.one_possible(row, col) && (!(puzzle.two_possible(row,col) || puzzle.three_possible(row,col)
			|| puzzle.four_possible(row,col) || puzzle.five_possible(row,col) || puzzle.six_possible(row,col)
			|| puzzle.seven_possible(row,col) || puzzle.eight_possible(row,col) || puzzle.nine_possible(row,col))))
			{
				return val;
			}
			break;
		case 2:
			if (puzzle.two_possible(row, col) && (!(puzzle.one_possible(row,col) || puzzle.three_possible(row,col)
			|| puzzle.four_possible(row,col) || puzzle.five_possible(row,col) || puzzle.six_possible(row,col)
			|| puzzle.seven_possible(row,col) || puzzle.eight_possible(row,col) || puzzle.nine_possible(row,col))))
			{
				return val;
			}
			break;

So you can see why I'd like to limit the number of lines per if(). Probably just being neurotic though. Also you're right about the parentheses, wish I'd noticed that sooner.


@giblit: For this program a bit field would probably be best, but I don't know how to use one yet. The Sudoku class (puzzle is a Sudoku object) contains a data member defined like this:

	struct cell {
		bool known; //true if the value of the cell is known
		int square; //the 3x3 square the cell lies in, with 1 being upper left and 9 lower right
		bool one; //true if one is a possible value, false otherwise
		bool two; //see above
		bool three;
		bool four;
		bool five;
		bool six;
		bool seven;
		bool eight;
		bool nine;
	};

with the N_possible functions returning the values of one, two etc.

[giblit]

Yeah I figured. What I would do is an array for those then have one function and add a 3rd param for the possible choice.

bool value[9]; //instead of one , two , three ,four... 

[Chervil]

I think giblit is right, or at least pointing in the right direction. When code starts to become long and cumbersome and especially repetitive with a block of code repeated multiple times with slight variations, it points to a need for taking another look at the design.

Sometimes it is simply a matter of putting the repeated code into its own separate function, and indeed that may work here, but the key point is the use of an array, which makes it much easier to make a single function which can handle any of the possibilities.

And very likely that array, introduced to help with this part, will also simplify the rest of the code too, so its all positive really.

[Glandy]

Crazy stuff incoming:

You can define an Array of Member functions of a Class
see the following:

typedef bool (Sudoku::*SudokuFunctions)(int, int);

// now you somewhere create an Array with all those "one_possible(), two_possible methods

SudokuFunctions su_funcs[] = {&Sudoku::one_possible, &Sudoku::two_possible, ... , &Sudoku::nine_possible };

Allright we got that.
Now instead of this crazy switch case stuff:

for (int val = 1; val <= 9; ++val) //TODO: fix indentation
{
   bool currentVal = false;
   bool others = false;
   for(int i = 0; i < 9; i++) {

      if(i+1 == val) {
         currentVal = (puzzle.*su_funcs[i])(row, col);
         if(currentVal == false)
            return -1; // val doesn't work no need to check anything else
      }
      else {
         others = (puzzle.*su_funcs[i])(row, col);
         if(others == true)
            return -1; //There is another number that is possible abort everything! ;)
      }
   }
// if our currentVal was true (possible to insert the number there)
// and none of the others ever evaluated to true
// if(currentVal && !others)  //edit this if is unncessary 
// 'cause we return -1 already before when anything is not right
   return val;
}

I think this is shorter ;)
Have fun !

[heebleworp]

Thanks for recommendations guys.

When code starts to become long and cumbersome and especially repetitive with a block of code repeated multiple times with slight variations, it points to a need for taking another look at the design.

Pretty much sums up what I've got so far :/

@Glandy: Didn't even consider that, though the modified code you posted doesn't capture the intent of the test (which I can't blame you for as my code looks hideous). For example assuming one is not possible for the cell we have on the first run:

val = 1;
currentVal = false;
others = false;
i = 0;
i+1 == val is true so set currentVal to false (assume one not possible)
currentVal == false so exit function

Instead of exiting the function the program should proceed to the next val and test. If one was possible I think the code would work fine though. If I'm following the code correctly just changing line 10 to break; fixes everything (others will always be false as we test higher values). Your code is much more readable/compact/easier to modify than what I've got, may implement it. Though I'll probably just end up using the bool array for other reasons (for example I might want to make the Sudoku variable-sized).