OOP question in C++

Started by
4 comments, last by Zahlman 18 years, 5 months ago
Hey, I'm working on a sudoku puzzle interface. If you are not familiar with these puzzles, it is sufficient to say that they consist of a 9x9 grid of numbers (range 1-9). In order to implement this, I have created a class "board" to represent the entire puzzle. I have also created a class "cell" that represents each individual cell. Thus, class board has a private data member called cell_array that contains all of the cells needed: #ifndef BOARD_H #define BOARD_H #include "cell.h" class board { public: board(); ~board(); void print() const; bool check_row( cell, int ); bool check_col( cell, int ); bool check_grid( cell, int ); cell get_cell( int, int ) const; private: cell cell_array[9][9]; void print_numrow( int i ) const; }; #endif The problem lies in my attempt to change the value contained in the cell. Class "cell" has a private data member "value" that can be modified with member function: void set_value( int ) So when I try the following: board mainboard; mainboard.get_cell[1][1].set_value(1); I am trying to change cell[1][1]'s value to 1. However, this is not the case and the value of this cell remains unchanged. The board constructor automatically instantiates 99 cells properly in the array and gives them a value of 0. What should I do to enable class "board" to change the values in "cell"? Thanks!
Advertisement
Instead of:

cell get_cell(int, int) const;

Try:

cell& get_cell(int, int);

I believe this will return a reference to the cell, which will allow you to modify it.
-----------------------------Play Stompy's Revenge! Now!
Exactly. Basically this:

cell get_cell( int, int ) const;

Tells that you're copying by value, your return value.

This:

cell &get_cell( int, int ) const;

Says that you're returning a reference *to* a cell, rather than copying the one you're returning.

Make sense?
You should also be aware that (and you may be aware of most or all of these, but evidence suggests you might not):

1) you would call the function with get_cell(1, 1), not get_cell[1][1]. The latter should not compile. It is possible to define an 'operator[]' for a class instance, but making a pair of []'s work properly is not trivial (basically you need to make the board's operator[] return some kind of 'row' such that operator[] on a row yields a cell).
2) there are 9*9 = 81 cells on the board, not 99.
3) cell_array[1][1] is not the top-left-most cell of the array; that is cell[0][0]. Similarly, an index like cell_array[9][9] is *not valid*.
4) Creating a separate cell class is probably useless. Any checks on validity can and should be done by the board, and you can represent the cell values by plain ints.
5) Having get and set methods for the same value is generally a bad idea; gets and sets tend to do very little if any actual work (even in finished products), and having this interface adds extra complexity (vs. a public member) while pretending to practice good OO (it is effectively a lie). You don't really want to add extra, deceiving things to your code?

However, offering direct access is still probably not the best idea.

For something like Sudoku, I don't see a reason why one would ever need to check the value in a specific cell; one is only concerned with a specific cell in order to change it (either to initialize the puzzle or write in a value). The reading of cell values is limited to display purposes.

6) There are some standards and idioms surrounding print methods in C++ that most beginners are not aware of...

So I would do things like this:

#ifndef BOARD_H#define BOARD_H#include <iostream>// no more cell class.class Board {  // I like to put the private stuff first to make use of the fact that  // the class defaults to private.  int cell_array[9][9];  // Here's a hint: Use 0 to represent "empty cell". When printing, check  // for 0's and output a space instead.  // Another option is to store a char[][] array - one good use for using  // such things instead of std::strings - and store ' ' for empty cells and  // '1' through '9' for numeric values.  // I probably wouldn't have the printing of rows factored out to a separate  // function because the whole process is still not too complicated, but:  void printRowTo(ostream& os) const;  // This would output a line of the Board to the specified output stream,  // by doing 'os << whatever' just like you are presumably currently doing  // with cout. That gives you useful flexibility for the future - e.g. to  // save/load puzzles from file - with very little coding overhead.  // The checking functions go up here - really!  // The idea is that we want to prevent a change to a cell when the rules  // don't allow it. That is an 'invariant', and is something the class is  // supposed to enforce.  bool check_row(int x, int y, int value) const;  bool check_col(int x, int y, int value) const;  bool check_grid(int x, int y, int value) const;  // That also simplifies the interface for calling code (like the main loop).  public:  board();  // The destructor doesn't need to do anything, so we don't need to  // write or even declare one at all - the default will suffice.  // This is because we have a static array for the cells - no  // memory allocation via 'new'.  // Here's where we show the board...  void printTo(ostream& os) const;  // That will call printRowTo() for each row (or do the work itself, and  // then you can get rid of printRowTo()).   // And here's where we play the game:  bool set(int x, int y, int value);  // The implementation will look something like:  // - bounds check on x and y up here first; if out of range, return false  // - If the value is already set, return false  // - Then check the row, column and grid; if any of the helpers returns  // false, return false  // - If all our "checks" passed, we are allowed to set the value:  // grid[x][y] = value;  // return true;};// Now here's some magic that will let us "cout a board" in the same way that// one handles primitive types or library classes:ostream& operator<<(ostream& os, const Board& b) {  b.printTo(os);  return os;}// This defines an operation (ostream object) << (Board object).// ('cout' is an ostream object.)#endif


The operator is defined to return the object on the left side, which lets us "chain" calls, e.g. cout << myBoard << endl << "Now what?" << endl;. See here and here.

By the way, the author of that FAQ doesn't like the method of separating out a printTo() (he calls it printOn()) function, instead preferring to define a 'friend' operator<< which accesses the classes' internals directly (making use of the C++ provision for offering special-case access to 'private' members).

I will let you hear his side of the argument... and this is my response:

Re 1: I support doing it this way for aesthetic reasons rather than reasons of maintainability. Of course, one person's aesthetic sense will always have the chance to be incomprehensible to another.

Re 2: Using the print method directly doesn't cause a problem here; it simply doesn't respect the usual C++ idiom for outputting things. Besides which, noone complains about things in Java like the public-ness of run() methods in objects extending Thread (or implementing Runnable), where calling it directly would cause a problem (i.e. not actually creating a new thread of execution; that is done via start()).
Hey,

THANKS for such a thorough reply! Actually, yeah I was just writing quickly and carelessly when I posted that. Numbers 1, 2, and 3 I was aware of (they are so basic I feel I have to justify myself a little haha). get_cell[1][1] was a typo as was 99 cells. As for cell_array[1][1] I was just picking "some random cell" so... I'm not great, but I'm not as bad as it might seem ;]

Also, instead of passing the cell my reference, I got the class to work by instead creating an array of pointers to cells and then in the board constructor using "new" to create a cell for each pointer.

As for eliminating the cell class.... What if I want to do things like give each cell a private data member like "vector< int > valids" that contains a list of every POSSIBLE value in the cell. If you know sudoku, then you are probably aware that the "brute force" way of solving a tricky puzzle is to go through every square and write down every POSSIBLE number that works in that square. If you reach squares where only one number could possibly fit, then that is the correct number. Thus, I could do something like:

void fill_cell( cell * j ) {

for ( int i = 1; i < 10; i++ ) {
if ( /* i works in *j */ )
j->add_valid( i ); //Would call a function that would push_back(i) on the cell's valids vector
}

if ( j->get_valids.length() == 1 ) {
j->set_value(i);
}

/* clear j->valids after this */

return;

}

This way I could run through each cell in a double-nested loop and try to find squares with one value only and fill them in as such. There will be a couple other methods of filling the board that I will implement, but this is just one example where it seems convenient to have a cell class. I suppose I could do something like an array of vectors that has 81 elements and corresponds to each cell, but this seems as if it would take up more space (albeit by today's standards not much, but more nonetheless).

As for #5...
5) Having get and set methods for the same value is generally a bad idea; gets and sets tend to do very little if any actual work (even in finished products), and having this interface adds extra complexity (vs. a public member) while pretending to practice good OO (it is effectively a lie). You don't really want to add extra, deceiving things to your code?

I'm not really sure I understand what you mean. Why is practicing good OO a lie?

In any case, thanks again for the great reply. If there are errors in my code, sorry, I just wrote it on the spot in this post so you could get an idea of what I'm trying to do.

Thanks!
Quote:Original post by matrisking
As for eliminating the cell class.... What if I want to do things like give each cell a private data member like "vector< int > valids" that contains a list of every POSSIBLE value in the cell.


(etc.)

Sure, that's valid. One option you may explore is to make the cell an 'inner class' of the board. The board needs to know pretty intimate details of each cell, but none of the rest of the code cares that cells even exist - if you make the cell an inner class with everything public (you should make it a struct in this case - there is no difference in C++ except that structs default to making things public, then the board gets to manipulate individual cells directly, while noone else can even instantiate cells or get references to them (with a little work).

Instead of the vector of remaining choices, another option would be an array of bools - the setting of value i indicates whether i+1 is a valid value for that cell. You could then either cache the count of true values in a separate member of the Cell, or recalculate it each time it's needed.

Quote:
5) Having get and set methods for the same value is generally a bad idea; gets and sets tend to do very little if any actual work (even in finished products), and having this interface adds extra complexity (vs. a public member) while pretending to practice good OO (it is effectively a lie). You don't really want to add extra, deceiving things to your code?

I'm not really sure I understand what you mean. Why is practicing good OO a lie?


It isn't. But putting a 'plain' get and set method on a value isn't good OO; it lies and pretends to be. It says "oh, we have good encapsulation here because the data is private". But:

1) Users are free to manipulate the data *as if* it were public, in a great majority of cases. Additionally, the functions are normally named after the data member, so you don't generally really hide much about how the class is implemented. This kind of stuff is written to insulate against future changes, but in practice (at least, in my practice) these changes just don't happen.
2) You write extra code that doesn't *do* anything.

The idea behind OO is to associate code with your little bits of data - to breathe life into your objects. So those bits of code should have a function, a purpose - something you can assign a descriptive name to. It's also so that you can put the code for some specific manipulation in one place, so that it doesn't get distributed throughout the code.

// Badstruct Thingy {  int x, y;  // other stuff};Thingy t;// lots of times in the calling code// Some Thingy's aren't really meant to be classes. However, the patterns// of access and use can tell a lot about what ought to be... :)t.x += 3;t.y += 3;if (t.x > 100) t.x = 100;if (t.y > 100) t.y = 100;//////////////////// Worse! And lots of people write this kind of thing 'automatically'// because they don't understand OO at all.class Thingy {  int x, y;  // other stuff  public:  void setX(int x) { this->x = x; }  int getX() { return x; }  void setY(int y) { this->y = y; }  int getY() { return y; }};Thingy t;// lots of times in the calling codet.setX(t.getX() + 3);t.setY(t.getY() + 3);if (t.getX() > 100) t.setX(100);if (t.getY() > 100) t.setY(100);////////////////// Significantly better, but still missing the point:class Thingy {  int x, y;  // other stuff  public:  void setX(int x) { this->x = x; if (this->x > 100) this->x = 100; }  int getX() { return x; }  void setY(int y) { this->y = y; if (this->y > 100) this->y = 100; }  int getY() { return y; }};Thingy t;// lots of times in the calling codet.setX(t.getX() + 3);t.setY(t.getY() + 3);////////////////// What we really want:class Thingy {  int x, y;  // other stuff  public:  void move(int dx, int dy) {    x += dx; if (x > 100) x = 100;    y += dy; if (y > 100) y = 100;    // maybe we also clip things at 0, and the other examples didn't    // make that clear (they were assuming that increasing the value wouldn't    // require a check on the lower bound). If we wrap that code up here then    // yes, we lose some tiny amount of speed, but we write *much* less code,    // and remove the chance of messing it up somewhere.    // if (x < 0) x = 0;    // if (y < 0) y = 0;  }};Thingy t;// lots of times in the calling codet.move(3, 3);

This topic is closed to new replies.

Advertisement