What is wrong with this code?

[datastructure]

Copyright (c) 2003 by James J. Perry. All Rights Reserved.

char Square::validList[4] = {'r', 'g', 'b'}; //missing an element, is 0
int Square::numValues = 3;

Square::Square()
{
value = validList[0];
}
Square::Square(char val)
{
value = val;
}

Square::Square(Square& s)
{
value = s.getValue();
}

void Square::generate()
{
for (int x = 0; x < numValues; x++)
{
if (validList[x] == value)
{
if (x == numValues){
value = validList[0];
}
else {
value = validList[x+1];
}
return;
}
}
}

char Square::getValue()
{
return value;
}

char Square::setValue(char val)
{
for (int x = 0; x < numValues; x++)
{
if (validList[x] == val)
{
value = val;
return value;
}
}
value = validList[0];
return value;
}

char Square::setValueRand()
{
value = validList[rand()%numValues];
return value;
}

int Square::isValid(char c)
{
for (int x = 0; x < numValues; x++)
{
if (validList[x] == c)
{
return true;
}
}

return false;
}

const Square &Square:perator= (const Square &s)
{

value = s.value;
return s;
}

Board::Board()
{
Values = new Square[9*9];
for (int Row = 0; Row<9; Row++){
for (int Col = 0; Col<9; Col++){
Values[Row*9+Col].setValueRand();
}
}
height = 9;
width = 9;
}

Board::Board(int h, int w)
{
Values = new Square[h*w];
for (int Row = 0; Row<w; Row++)
{
for (int Col = 0; Col<h; Col++)
{
Values[Row*w+Col-1].setValueRand();

}
}
height = h;
width = w;
}

Board::Board(const Board& b)
{
height = b.height;
width = b.width;
Values = new Square[height*width];
for (int Row = 0; Row<width; Row++)
{
for (int Col = 0; Col<height; Col++)
{
Values[Row*width+Col]= b.Values[Row*width+Col];
}
}

}

Board::~Board()
{

//delete [] Values;
}

int Board::setSquare(Square s, int x, int y)
{
if (height*width < x*y) return 0;
Values[(y)*width+x].setValue(s.getValue());
return true;
}

char Board::getSquareValue(int x, int y)
{
if (height*width < x*y) return '\0';
return Values[(y)*width+x-1].getValue();
}
int Board::getWidth()
{
return width;
}
int Board::getHeight()
{
return height;
}

void Board::setRand()
{
for (int Row = 0; Row<width; Row++)
{
for (int Col = 0; Col<height; Col++)
{
Values[Row*width+Col+1].setValueRand();

}
}
}

int Board::generate(int x, int y)
{

int score;
int position = (y)*width+x;
int size = height*width-1;

Values[position].generate();
char newValue = Values[position].getValue();

return 0;

}

 

[Victor Bazarov]

Copyright (c) 2003 by James J. Perry. All Rights Reserved.

The line above is not C++

char Square::validList[4] = {'r', 'g', 'b'}; //missing an element, is 0

'Square' is undefined.

int Square::numValues = 3;

Square::Square()
{
value = validList[0];

Initialisation should be preferred over assignment.

}
Square::Square(char val)
{
value = val;

Initialisation should be preferred over assignment.

}

Square::Square(Square& s)
{
value = s.getValue();

Initialisation should be preferred over assignment.

}

void Square::generate()
{
for (int x = 0; x < numValues; x++)
{
if (validList[x] == value)
{
if (x == numValues){

'x' can _never_ be equal numValues in this loop. Did you mean to
write

if (x == numValues - 1)

???

value = validList[0];
}
else {
value = validList[x+1];
}
return;
}
}
}

char Square::getValue()

This function should probably be declared 'const'.

{
return value;
}

char Square::setValue(char val)
{
for (int x = 0; x < numValues; x++)
{
if (validList[x] == val)
{
value = val;
return value;
}
}
value = validList[0];
return value;
}

char Square::setValueRand()
{
value = validList[rand()%numValues];

Using % with the result of a call to 'rand' is often a BAD IDEA(tm).

return value;
}

int Square::isValid(char c)

This function should probably be declared 'const' if it is not
static.

{
for (int x = 0; x < numValues; x++)
{
if (validList[x] == c)
{
return true;
}
}

return false;
}

const Square &Square:perator= (const Square &s)
{

value = s.value;
return s;

Traditionally, operator= returns '*this', the left side. Why does
yours return 's', the right side? Any particular reason for that?

}

Board::Board()
{
Values = new Square[9*9];
for (int Row = 0; Row<9; Row++){
for (int Col = 0; Col<9; Col++){
Values[Row*9+Col].setValueRand();

You really don't need two loops here. One, from 0 to 81 would be
more than fine.

}
}
height = 9;
width = 9;

Initialisation should be preferred over assignment.

}

Board::Board(int h, int w)

This constructor can be merged with the default one if you give
'h' and 'w' default values (9 in your case).

{
Values = new Square[h*w];
for (int Row = 0; Row<w; Row++)
{
for (int Col = 0; Col<h; Col++)
{
Values[Row*w+Col-1].setValueRand();

}
}
height = h;
width = w;

Initialisation should be preferred over assignment.

}

Board::Board(const Board& b)
{
height = b.height;
width = b.width;

Initialisation should be preferred over assignment.

Values = new Square[height*width];
for (int Row = 0; Row<width; Row++)
{
for (int Col = 0; Col<height; Col++)
{
Values[Row*width+Col]= b.Values[Row*width+Col];

You don't really need the nested loops here, do you? A simple one
from 0 to the height*width would suffice.

}
}

}

Board::~Board()
{

//delete [] Values;

Why is this commented out? You allocate them, you should delete
them.

}

int Board::setSquare(Square s, int x, int y)
{
if (height*width < x*y) return 0;
Values[(y)*width+x].setValue(s.getValue());
return true;

'true' is not an int. Did you mean to make this function 'bool'?

}

char Board::getSquareValue(int x, int y)

This function should probably be declared 'const'.

{
if (height*width < x*y) return '\0';
return Values[(y)*width+x-1].getValue();
}
int Board::getWidth()
{

This function should probably be declared 'const'.

return width;
}
int Board::getHeight()
{

This function should probably be declared 'const'.

return height;
}

void Board::setRand()
{
for (int Row = 0; Row<width; Row++)
{
for (int Col = 0; Col<height; Col++)
{
Values[Row*width+Col+1].setValueRand();

You don't really need the nested loops here, do you?

}
}
}

int Board::generate(int x, int y)
{

int score;

What's that variable for?

int position = (y)*width+x;
int size = height*width-1;

Values[position].generate();
char newValue = Values[position].getValue();

return 0;

}

Now, I cannot verify the correctness of this code beyond those
remarks I made after a very quick glance, simply because the code
you posted is (a) a fragment and (b) no requirements have been
stated.

Good luck!

V

 

[Phlip]

Can you ask a complete question; about a narrow part of this code?

 

[Mike Wahler]

Re: What is wrong with this code?

Copyright (c) 2003 by James J. Perry. All Rights Reserved.

Don't worry, I doubt anyone will want to steal that code.

As far as what's wrong, only you can tell us. What is it
supposed to do, and what does it do instead?

-Mike

 

[datastructure]

Don't worry, I doubt anyone will want to steal that code.

As far as what's wrong, only you can tell us. What is it
supposed to do, and what does it do instead?

-Mike

It's a control class for the Koleurs gameboard and square values.
I had to go through code to find problems. I found a few problems,
just wanted to see if there were any more. I'm learning data
sructure, just trying to learn C++.

 

[datastructure]

Thanks for the comments. I din't post the *.h which contains my
function declarations. This was a code where I had to pick out some
of the problems, just trying to learn data structure.

Copyright (c) 2003 by James J. Perry. All Rights Reserved.

The line above is not C++

char Square::validList[4] = {'r', 'g', 'b'}; //missing an element, is 0

'Square' is undefined.

Now, I cannot verify the correctness of this code beyond those
remarks I made after a very quick glance, simply because the code
you posted is (a) a fragment and (b) no requirements have been
stated.

Good luck!

V

 

[Matt Graham]

int Square::numValues = 3;

Square::Square()
{
value = validList[0];

Initialisation should be preferred over assignment.

What does initialization mean in this sense?
How would it be done?

-matt

 

[Victor Bazarov]

int Square::numValues = 3;

Square::Square()
{
value = validList[0];

Initialisation should be preferred over assignment.

What does initialization mean in this sense?
How would it be done?

Too lazy to look at the original post, but I'll speculate
that 'validList' is some kind of static or global array, then
one may write

Square::Square() : value(validList[0]) {
...

Victor

 

[Matt Graham]

int Square::numValues = 3;

Square::Square()
{
value = validList[0];

Initialisation should be preferred over assignment.

What does initialization mean in this sense?
How would it be done?

A post further kind of clears up what my question was.
So is what initialization would be?

Square::Square() {
value( validList[0] );
}

And this is ok to do with simple data types like char and float?

thanks,
matt

 

[Victor Bazarov]

int Square::numValues = 3;

Square::Square()
{
value = validList[0];



Initialisation should be preferred over assignment.

What does initialization mean in this sense?
How would it be done?

A post further kind of clears up what my question was.
So is what initialization would be?

Square::Square() {
value( validList[0] );
}

No, this is not initialisation. It's probably a syntax error.

And this is ok to do with simple data types like char and float?

Absolutely. Even if there is no difference in the final result or
in performance, it promotes good coding style.

Victor