What is wrong with this code?

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::operator= (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;

}

re: What is wrong with this code?

"datastructure" <bruss125@juno.com> wrote...[color=blue]
> Copyright (c) 2003 by James J. Perry. All Rights Reserved.[/color]

The line above is not C++
[color=blue]
>
> char Square::validList[4] = {'r', 'g', 'b'}; //missing an element, is 0[/color]

'Square' is undefined.
[color=blue]
> int Square::numValues = 3;
>
> Square::Square()
> {
> value = validList[0];[/color]

Initialisation should be preferred over assignment.
[color=blue]
> }
> Square::Square(char val)
> {
> value = val;[/color]


Initialisation should be preferred over assignment.
[color=blue]
> }
>
> Square::Square(Square& s)
> {
> value = s.getValue();[/color]


Initialisation should be preferred over assignment.
[color=blue]
> }
>
> void Square::generate()
> {
> for (int x = 0; x < numValues; x++)
> {
> if (validList[x] == value)
> {
> if (x == numValues){[/color]

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

if (x == numValues - 1)

???
[color=blue]
> value = validList[0];
> }
> else {
> value = validList[x+1];
> }
> return;
> }
> }
> }
>
> char Square::getValue()[/color]

This function should probably be declared 'const'.
[color=blue]
> {
> 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];[/color]

Using % with the result of a call to 'rand' is often a BAD IDEA(tm).
[color=blue]
> return value;
> }
>
> int Square::isValid(char c)[/color]

This function should probably be declared 'const' if it is not
static.
[color=blue]
> {
> for (int x = 0; x < numValues; x++)
> {
> if (validList[x] == c)
> {
> return true;
> }
> }
>
> return false;
> }
>
> const Square &Square::operator= (const Square &s)
> {
>
> value = s.value;
> return s;[/color]

Traditionally, operator= returns '*this', the left side. Why does
yours return 's', the right side? Any particular reason for that?
[color=blue]
> }
>
> 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();[/color]

You really don't need two loops here. One, from 0 to 81 would be
more than fine.
[color=blue]
> }
> }
> height = 9;
> width = 9;[/color]

Initialisation should be preferred over assignment.
[color=blue]
> }
>
> Board::Board(int h, int w)[/color]

This constructor can be merged with the default one if you give
'h' and 'w' default values (9 in your case).
[color=blue]
> {
> 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;[/color]

Initialisation should be preferred over assignment.
[color=blue]
> }
>
> Board::Board(const Board& b)
> {
> height = b.height;
> width = b.width;[/color]

Initialisation should be preferred over assignment.
[color=blue]
> 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];[/color]


You don't really need the nested loops here, do you? A simple one
from 0 to the height*width would suffice.
[color=blue]
> }
> }
>
> }
>
> Board::~Board()
> {
>
> //delete [] Values;[/color]

Why is this commented out? You allocate them, you should delete
them.
[color=blue]
> }
>
> 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;[/color]

'true' is not an int. Did you mean to make this function 'bool'?
[color=blue]
> }
>
> char Board::getSquareValue(int x, int y)[/color]

This function should probably be declared 'const'.
[color=blue]
> {
> if (height*width < x*y) return '\0';
> return Values[(y)*width+x-1].getValue();
> }
> int Board::getWidth()
> {[/color]


This function should probably be declared 'const'.
[color=blue]
> return width;
> }
> int Board::getHeight()
> {[/color]


This function should probably be declared 'const'.
[color=blue]
> 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();[/color]

You don't really need the nested loops here, do you?
[color=blue]
>
> }
> }
> }
>
> int Board::generate(int x, int y)
> {
>
> int score;[/color]

What's that variable for?
[color=blue]
> int position = (y)*width+x;
> int size = height*width-1;
>
> Values[position].generate();
> char newValue = Values[position].getValue();
>
> return 0;
>
> }[/color]

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

re: What is wrong with this code?

"datastructure" <bruss125@juno.com> wrote in message
news:2519ed30.0402011931.4e9a4c61@posting.google.c om...

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

--
Phlip
http://www.xpsd.org/cgi-bin/wiki?Tes...UserInterfaces

re: What is wrong with this code?

"datastructure" <bruss125@juno.com> wrote in message
news:2519ed30.0402011931.4e9a4c61@posting.google.c om...
[color=blue]
> Re: What is wrong with this code?[/color]
[color=blue]
> Copyright (c) 2003 by James J. Perry. All Rights Reserved.[/color]

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

re: What is wrong with this code?

"Mike Wahler" <mkwahler@mkwahler.net> wrote in message news:<AemTb.8954$uM2.7949@newsread1.news.pas.earth link.net>...[color=blue]
> "datastructure" <bruss125@juno.com> wrote in message
> news:2519ed30.0402011931.4e9a4c61@posting.google.c om...
>[color=green]
> > Re: What is wrong with this code?[/color]
>[color=green]
> > Copyright (c) 2003 by James J. Perry. All Rights Reserved.[/color]
>
> 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[/color]

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++.

re: What is wrong with this code?

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.

"Victor Bazarov" <v.Abazarov@comAcast.net> wrote in message news:<%ZjTb.159608$sv6.877482@attbi_s52>...[color=blue]
> "datastructure" <bruss125@juno.com> wrote...[color=green]
> > Copyright (c) 2003 by James J. Perry. All Rights Reserved.[/color]
>
> The line above is not C++
>[color=green]
> >
> > char Square::validList[4] = {'r', 'g', 'b'}; //missing an element, is 0[/color]
>
> '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[/color]

re: What is wrong with this code?

Victor Bazarov wrote:[color=blue]
> "datastructure" <bruss125@juno.com> wrote...
>
>[color=green]
>>int Square::numValues = 3;
>>
>>Square::Square()
>>{
>>value = validList[0];[/color]
>
>
> Initialisation should be preferred over assignment.[/color]

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

-matt

re: What is wrong with this code?

"Matt Graham" <mdg149@rcn.com> wrote...[color=blue]
> Victor Bazarov wrote:[color=green]
> > "datastructure" <bruss125@juno.com> wrote...
> >
> >[color=darkred]
> >>int Square::numValues = 3;
> >>
> >>Square::Square()
> >>{
> >>value = validList[0];[/color]
> >
> >
> > Initialisation should be preferred over assignment.[/color]
>
> What does initialization mean in this sense?
> How would it be done?[/color]

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

re: What is wrong with this code?

Matt Graham wrote:
[color=blue]
> Victor Bazarov wrote:
>[color=green]
>> "datastructure" <bruss125@juno.com> wrote...
>>
>>[color=darkred]
>>> int Square::numValues = 3;
>>>
>>> Square::Square()
>>> {
>>> value = validList[0];[/color]
>>
>>
>>
>> Initialisation should be preferred over assignment.[/color]
>
>
> What does initialization mean in this sense?
> How would it be done?[/color]

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

re: What is wrong with this code?

"Matt Graham" <mdg149@rcn.com> wrote...[color=blue]
> Matt Graham wrote:
>[color=green]
> > Victor Bazarov wrote:
> >[color=darkred]
> >> "datastructure" <bruss125@juno.com> wrote...
> >>
> >>
> >>> int Square::numValues = 3;
> >>>
> >>> Square::Square()
> >>> {
> >>> value = validList[0];
> >>
> >>
> >>
> >> Initialisation should be preferred over assignment.[/color]
> >
> >
> > What does initialization mean in this sense?
> > How would it be done?[/color]
>
> A post further kind of clears up what my question was.
> So is what initialization would be?
>
> Square::Square() {
> value( validList[0] );
> }[/color]

No, this is not initialisation. It's probably a syntax error.
[color=blue]
> And this is ok to do with simple data types like char and float?[/color]

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

Victor