"Die" class

[mahurshi]

I am new to c++ classes. I defined this "cDie" class that would return
a value between 1 and 6 (inclusive) It runs fine and gives no warnings
during compilation.

I was wondering if you guys can pick up any mistakes/"don't do"s from
this code:

#include <iostream>
#include <cstdlib>

using namespace std;

class cDie
{
public:
cDie();
int roll();
};
cDie :: cDie()
{
srand(time(NULL));
}
int cDie :: roll()
{
return rand()%6 + 1;
}

int main()
{
cDie die;

for (int i = 0; i < 10; i++)
{
cout << "Die Rolled: " << die.roll() << endl;
}
return 0;
}

[W Marsh]

ma******@gmail.com wrote:

I am new to c++ classes. I defined this "cDie" class that would return
a value between 1 and 6 (inclusive) It runs fine and gives no warnings
during compilation.

I was wondering if you guys can pick up any mistakes/"don't do"s from
this code:

#include <iostream>
#include <cstdlib>

using namespace std;

class cDie
{
public:
cDie();
int roll();
};
cDie :: cDie()
{
srand(time(NULL));
}
int cDie :: roll()
{
return rand()%6 + 1;
}

int main()
{
cDie die;

for (int i = 0; i < 10; i++)
{
cout << "Die Rolled: " << die.roll() << endl;
}
return 0;
}

Immediate "don't do" is the global "using namespace std;". Bringing std
into the global namespace defeats the point of namespacing entirely.
Reference items in std explicitly (e.g. "std::cout << "Die Rolled:
"..."), bring in the namespace locally, or only bring in the items you
are going to use from the namespace.

Consider returning EXIT_SUCCESS from main instead of 0.

I would srand in main, not the constructor of cDie. If you choose to
remove it from the constructor of cDie, you wouldn't need to write your
own constructor at all (although I understand that you may be doing this
as an assignment which instructs you to write a constructor to show that
you /can/).

As you're only using i as a loop counter, consider changing it to ++i,
which may be faster as no temporary is generated.

Instead of using 6 as the modulo in the roll method, define a constant
such as "MAX_ROLLS". It could be a private member of cDie. Having "magic
numbers" in code decreases readability, and makes it harder to alter the
code should you want an 87-sided die.

[Luke Meyers]

Hi, here's my list:

* Avoid "using namespace std" like the plague. Either explicitly
qualify members of that namespace, or use individual using-declarations
for them. It's alright to use it within a single function scope, if
you like, though -- just not anything broader.
* Don't use "magic numbers" in your code -- create symbolic constants
(with const, not #define, of course).
* Formatting's a bit odd, but that's subjective, and as you learn
you'll be exposed to plenty of norms to choose among or hybridize. Are
you really using eight space tabs, or is that just my newsreader doing
that to me? Oh, and I recommend un-indenting "public:" so it stands
out more; that's typical to do.
* Use ++i rather than i++ whenever you can. Post-incrementing is less
efficient (it requires an additional temporary variable, think about
it) and has ordering ambiguity.
* Hungarian notation is the goddamn devil. Seriously, just don't. It
doesn't scan well and it's a maintainability nightmare. Variable names
should describe the role of the variable, not the implementation
thereof.
* Use const for methods and variables as often as possible ("and no
more" -- Sutter (?)). In this case, the roll() method should be const,
since it does not modify the state of the enclosing object.
* Since roll() does not depend on class state, it should be a static
method.
* Often, it's a good idea not to tie yourself to a particular primitive
data type for no reason, as in the case of the return value from
roll(). Maybe later you'd discover that you'd rather use a shorter
representation for efficiency reasons (e.g. unsigned char, a single
byte). Best thing to do is to create a nested public typedef and use
that for your return value. For example:
class Die {
public:
typedef int result_type;
result_type roll() const;
};

Cheers,
Luke

[osmium]

"W Marsh" writes:

Instead of using 6 as the modulo in the roll method, define a constant
such as "MAX_ROLLS". It could be a private member of cDie. Having "magic
numbers" in code decreases readability, and makes it harder to alter the
code should you want an 87-sided die.

I find that magic numbers are almost always more readable than some ugly,
long winded name. People know there are 7 days in a week, 365 days in a
year, 52 cards in a deck and 6 sides on a die. There is only one way to
write 7, there are tons of ways to write DaysInTheWeek, a lot of them annoy
me. I know this is a religious war so don't bother posting a rebuttal for
my benefit.
To the OP. Looks pretty good to me, but I would be inclined to put srand in
main too. I can't really put my finger on why I prefer it that way, though.
I assume the original had indentation, it got stripped before I saw it.

[red floyd]

osmium wrote:

"W Marsh" writes:
To the OP. Looks pretty good to me, but I would be inclined to put srand in
main too. I can't really put my finger on why I prefer it that way, though.

Simple.

Consider this:

#include <iostream>

int f()
{
cDie die;
return die.roll;
}

int main()
{
std::cout << "roll 1: " << f() << std::endl;
std::cout << "roll 2: " << f() << std::endl;
}
srand() gets called twice.

[red floyd]

red floyd wrote:

Ooops! forgot "assume OP's cDie class included here".

[W Marsh]

osmium wrote:

"W Marsh" writes:

Instead of using 6 as the modulo in the roll method, define a constant
such as "MAX_ROLLS". It could be a private member of cDie. Having "magic
numbers" in code decreases readability, and makes it harder to alter the
code should you want an 87-sided die.

I find that magic numbers are almost always more readable than some ugly,
long winded name. People know there are 7 days in a week, 365 days in a
year, 52 cards in a deck and 6 sides on a die. There is only one way to
write 7, there are tons of ways to write DaysInTheWeek, a lot of them annoy
me. I know this is a religious war so don't bother posting a rebuttal for
my benefit.

I'm posting a rebuttal for the OP's benefit.

* I don't think anybody would find 6 clearer to read than a named
constant. In more complex code where the context is less clear, who
knows what 6 is? What if I want to write a method returning 6 rolls of a
6-sided die? The code would not be clear.

* If 6 needed to be changed (don't assume that it won't!) then only a
single line of code would be changed if we're using named constants.
Without named constants, we would have to hunt for 6s, decide whether
they mean what we think they mean, and change them. This is just asking
for trouble.

I don't understand why you wouldn't use constants here - your reason
seems to be that you just don't like the look of them.
To the OP. Looks pretty good to me, but I would be inclined to put srand in
main too. I can't really put my finger on why I prefer it that way, though.

Because constantly reseeding the pseudo-random number generator makes
the output a function of the seed, which won't generate as useful data
(I believe).

[W Marsh]

W Marsh wrote:

Instead of using 6 as the modulo in the roll method, define a constant
such as "MAX_ROLLS". It could be a private member of cDie. Having "magic
numbers" in code decreases readability, and makes it harder to alter the
code should you want an 87-sided die.

Whoops, but ignore me and don't call it "MAX_ROLLS", which it clearly
isn't! My brain is half asleep today. You could call it "DIE_SIDES" or
something.

[Howard]

"osmium" <r1********@comcast.net> wrote in message
news:41*************@individual.net...

I find that magic numbers are almost always more readable than some ugly,
long winded name. People know there are 7 days in a week, 365 days in a
year, 52 cards in a deck and 6 sides on a die. There is only one way to
write 7, there are tons of ways to write DaysInTheWeek, a lot of them
annoy me. I know this is a religious war so don't bother posting a
rebuttal for my benefit.

I can't help but respond here (but not just for your benefit):

I have sets of Dungeons&Dragons dice, and they include a variety of dice
with different numbers of sides (4,6,8,10,12,20,30 and even 100). So _my_
preference would be to include the number of sides as a parameter of the
class' constructor (perhaps with 6 as the default), which is assigned to a
member variable for use in "rolling" the dice later on.

(But really, the calculation is so simple that employing a class for this in
the first place seems a waste to me. I'd probably just use a global
function, with the number of sides passed to it as a parameter.)

-Howard

[Jonathan Mcdougall]

W Marsh wrote:

W Marsh wrote:

> Instead of using 6 as the modulo in the roll method, define a constant
such as "MAX_ROLLS". It could be a private member of cDie. Having "magic
numbers" in code decreases readability, and makes it harder to alter the
code should you want an 87-sided die.
Whoops, but ignore me and don't call it "MAX_ROLLS", which it clearly
isn't! My brain is half asleep today. You could call it "DIE_SIDES" or
something.

Funny:

osmium wrote: I find that magic numbers are almost always more readable than some ugly,
long winded name. People know there are 7 days in a week, 365 days in a
year, 52 cards in a deck and 6 sides on a die. There is only one way to
write 7, there are tons of ways to write DaysInTheWeek, a lot of them annoy
me.

I tend to agree though that's hard on the habits.
Jonathan

[Peter Jansson]

ma******@gmail.com wrote:

I am new to c++ classes. I defined this "cDie" class that would return
a value between 1 and 6 (inclusive) It runs fine and gives no warnings
during compilation.

I was wondering if you guys can pick up any mistakes/"don't do"s from
this code:

#include <iostream>
#include <cstdlib>

using namespace std;

class cDie
{
public:
cDie();
int roll();
};
cDie :: cDie()
{
srand(time(NULL));
}
int cDie :: roll()
{
return rand()%6 + 1;
}

int main()
{
cDie die;

for (int i = 0; i < 10; i++)
{
cout << "Die Rolled: " << die.roll() << endl;
}
return 0;
}

Hello,

I agree with the replies you have so far but I would like to add the
follwing comment which is a quote from the man-pages of rand() to a
quote in numerical recipies:

In Numerical Recipes in C: The Art of Scientific Computing (William H.
Press, Brian P. Flannery, Saul A. Teukolsky, William T. Vetterling; New
York: Cambridge University Press, 1992 (2nd ed., p. 277)), the
following comments are made:
"If you want to generate a random integer between 1 and 10, you
should always do it by using high-order bits, as in

j=1+(int) (10.0*rand()/(RAND_MAX+1.0));

and never by anything resembling

j=1+(rand() % 10);

(which uses lower-order bits)."
With best regards,
Peter Jansson

[Howard]

"W Marsh" <wayneDOTmarshATgmailDOTcom@decipher> wrote in message
news:43***********************@news.zen.co.uk...

osmium wrote:

"W Marsh" writes:

To the OP. Looks pretty good to me, but I would be inclined to put srand
in main too. I can't really put my finger on why I prefer it that way,
though.

Because constantly reseeding the pseudo-random number generator makes the
output a function of the seed, which won't generate as useful data (I
believe).

The pseudo-random-number generator has been formulated to provide some
measure of randomness, and re-seeding constanlty changes its output to
something entirely different. Since the output of such a system is not
predictable (in advance), there is no way to know whether or not it will
exhibit the required randomness, thus making it a poor choice for any series
of trials where such randomness is desired.

-Howard

[roberts.noah]

osmium wrote:

"W Marsh" writes:

Instead of using 6 as the modulo in the roll method, define a constant
such as "MAX_ROLLS". It could be a private member of cDie. Having "magic
numbers" in code decreases readability, and makes it harder to alter the
code should you want an 87-sided die.

People know there are ... 6 sides on a die.

In D&D there are dice with any number of sides including 4, 6, 10, 12,
20, 100, etc... Other RPG systems have odd sided dice as well. There
is nothing that says a die has 6 sides...that is just the most common.

A better approach might be a templated class

template<int sides>
class die
{
int roll() { return rand() % sides + 1; }
};

die<6> six_sided;

Or simply a parameter to the constructor or maybe even roll...

[Howard]

"osmium" <r1********@comcast.net> wrote in message
news:41*************@individual.net...

People know there are ... 365 days in a year,

What about leap year? :-)

-Howard

[Luke Meyers]

> I find that magic numbers are almost always more readable than some ugly,

long winded name. People know there are 7 days in a week, 365 days in a
year, 52 cards in a deck and 6 sides on a die. There is only one way to
write 7, there are tons of ways to write DaysInTheWeek, a lot of them annoy
me. I know this is a religious war so don't bother posting a rebuttal for
my benefit.

Religious, indeed... please god, let me never be forced to maintain
code written by someone with this mindset.

If not for your benefit, then for others -- magic numbers are, yes, a
Bad Thing. It's more than a question of knowing the meaning of obvious
numeric constants. There are other issues:
* What happens when the value changes? When you decide to use an
87-sided die, if you have defined a symbolic constant, you can make the
change in one place and be done. If you haven't, you have to search
your codebase for "6" and decide for each case whether to change it, or
whether it's some other 6 that has nothing to do with dice.
* Not all constants are obvious -- in the real world, I'd say the
obvious ones are in the minority. Regardless of the proportions, there
are certainly a significant number of cases where a symbolic constant
is essential, based on all the classical criteria. Since at least some
constants are unworkable without a symbolic representation, one must
either settle on a policy of letting everyone (within an organization,
or within a coding standard, or whatever) determine for themselves
what's obvious, or simply adopt a simple, unambiguous policy in line
with accepted industry wisdom.
* Magic numbers throw out many benefits of C++'s strong static typing
system. A numeric literal will always have the same type behavior,
implicit conversions, etc. A symbolic constant can be associated with
a typedef for maintainability.
* A symbolic constant gives you something meaningful to grep for.
* Multiple constants may carry the same value -- without symbolic
names, obvious ambiguity and confusion arises.
* Magic numbers may negatively impact portability -- system A may fit a
particular number into int, where system B uses long. And then perhaps
you change the value of the numeric literal, which may cause A and/or B
to once again use a different type... is this really what you want?
* If you're not comfortable with symbolic names, reconsider your
decision to be a programmer. "Ugly, long-winded names" are bad style,
but that's a straw man because constants should have concise, obvious
names that are consistent with a common naming scheme. And it's not
like the compiler won't tell you if you use an undefined symbol. If
you use a proper editor, it will complete symbols for you in most cases
anyway.

But I do agree with putting srand in main. I wouldn't want to re-seed
the RNG every time I created a new Die -- unnecessary overhead, and bug
potential.

Luke

[Luke Meyers]

Peter Jansson wrote:

j=1+(int) (10.0*rand()/(RAND_MAX+1.0));

I would disagree with:
* Using floating-point values to compute an integer.
* Using C-style casts in C++.
* Accessing higher-order bits via multiplication rather than shifting
bits.

Luke

[Mike Smith]

red floyd wrote:

osmium wrote:

"W Marsh" writes:

To the OP. Looks pretty good to me, but I would be inclined to put
srand in main too. I can't really put my finger on why I prefer it
that way, though.

Simple.

Consider this:

#include <iostream>

int f()
{
cDie die;
return die.roll;
}

int main()
{
std::cout << "roll 1: " << f() << std::endl;
std::cout << "roll 2: " << f() << std::endl;
}
srand() gets called twice.

You could make cDie a singleton...

--
Mike Smith

[Mike Smith]

Howard wrote:

"osmium" <r1********@comcast.net> wrote in message
news:41*************@individual.net...

People know there are ... 365 days in a year,

What about leap year? :-)

365.242199 days. Google sez so.

--
Mike Smith

[mahurshi]

after reading the discussions, i have made some changes to the code.
#include <iostream>
#include <cstdlib>
class cDie
{
private:
int SIDES;
public:
cDie(int sides);
int roll();
};

cDie :: cDie(int sides)
{
SIDES = sides;
}
int cDie :: roll()
{
return 1 + rand() % SIDES;
}

int main()
{
srand(time(NULL));

cDie die(6);

for (int i = 0; i < 10; ++i)
{
std::cout << "Die Rolled: " << die.roll() << "\n";
}
return EXIT_SUCCESS;
}
this is probably something i should look up somewhere, but endl started
whining as soon as i removed namespace std, so i am using "\n"
(another reason why i used namepsace std at the top was that i never
used stuff other than "std"..however, i see what you guys mean.. i
shouldn't make it global or it will screw things up)
also, i haven't made any changes to the rand() (using higher order
stuff)

i did return 1 + SIDES*rand()/(RAND_MAX + 1) but it was always
returning 1 (because the second part is always 0 (i never casted floats
and ints).. however, i can fix this by using the floating point stuff
but i haven't yet.

oh.. and the reason why i put srand() in the constructor was that i
didn't want to remember to include it in main, as main doesn't have
anything to do with srand() (although, i see what you mean by srand()
being called twice when i create a 2nd instance of cDie)

[Thomas Jespersen]

On 28 Dec 2005 14:55:19 -0800, ma******@gmail.com wrote:

after reading the discussions, i have made some changes to the code.

Ahem, another newbie is reading this thread (me) :)

I am using VC++ Express, and I couldn't get this to compile unless I

#include <ctime>

Is it VC++ that is in error here, or was it a minor bug in the program?

[Jay Nabonne]

On Wed, 28 Dec 2005 14:55:19 -0800, mahurshi wrote:

after reading the discussions, i have made some changes to the code.
#include <iostream>
#include <cstdlib>
class cDie
{
private:
int SIDES;
public:
cDie(int sides);
int roll();
};

cDie :: cDie(int sides)
{
SIDES = sides;
}
int cDie :: roll()
{
return 1 + rand() % SIDES;
}

int main()
{
srand(time(NULL));

cDie die(6);

for (int i = 0; i < 10; ++i)
{
std::cout << "Die Rolled: " << die.roll() << "\n";
}
return EXIT_SUCCESS;
}
this is probably something i should look up somewhere, but endl started
whining as soon as i removed namespace std, so i am using "\n"
(another reason why i used namepsace std at the top was that i never
used stuff other than "std"..however, i see what you guys mean.. i
shouldn't make it global or it will screw things up)
Then use std::endl.


oh.. and the reason why i put srand() in the constructor was that i
didn't want to remember to include it in main, as main doesn't have
anything to do with srand() (although, i see what you mean by srand()
being called twice when i create a 2nd instance of cDie)

class cDie
{
// Your stuff, and add:
static bool initedSrand;
static bool InitSrand()
{
srand(time(0));
return true;
}
};

bool cDie::initedSrand = cDie::InitSrand();

- Jay

[Simon Biber]

Luke Meyers wrote:

* Use ++i rather than i++ whenever you can. Post-incrementing is less
efficient (it requires an additional temporary variable, think about
it) and has ordering ambiguity.

For plain old data (especially integers or pointers), in a context where
the value of the expression is not used, the compiler can and will
optimise away any temporary storage. In this context, ++i and i++ are
almost certain to produce exactly the same code.

I understand what the problem is when you're talking about a class with
overloaded operators, but that is simply not the case here.

I consciously and deliberately switch between i++ and ++i depending on
the type of the object I'm incrementing.

Compare:
vector<int> v;

size_t n = v.size();
for(size_t i = 0; i < n; i++)
{
// code using v[i]
}

With:
for(vector<int>::iterator i = v.begin(); i != v.end(); ++i)
{
// code using *i
}

This way it helps me keep straight whether I'm using an integer or an
iterator.

--
Simon.

[Luke Meyers]

Looking better. A few more tweaks to consider:

* Using all caps to represent constants is great. However, you should
make sure that they actually are constants! You didn't declare SIDES
as const; you should do so.

* Use initializer lists when possible for constructors. Your cDie
constructor would look like:
cDie::cDie(int sides) :
SIDES(sides)
{
// NULL
}

* roll() still needs to be a const member function. Put the const
keyword after the closing paren, like so:
int roll() const;
int cDie::roll() const { ... }

* Regarding endl; you can still use it, but you have to do one of the
following:
- prefix with std, i.e. std::endl
- Put a using-declaration for it in an enclosing scope, i.e. using
std::endl;
- Bring back "using namespace std," but only do it inside a
function scope, rather than polluting the global namespace.

* The difference between \n and endl is, AFAIK, that endl flushes the
stream. Sometimes this is what you want, sometimes not.

* Floating point arithmetic is way more expensive than integral
(integers, not integration) arithmetic, and furthermore brings
questions about precision to the fore. I do not recommend going down
that path for this program.

Keep coding!

Luke

[roberts.noah]

Thomas Jespersen wrote:

On 28 Dec 2005 14:55:19 -0800, ma******@gmail.com wrote:

after reading the discussions, i have made some changes to the code.

Ahem, another newbie is reading this thread (me) :)

I am using VC++ Express, and I couldn't get this to compile unless I

#include <ctime>

Is it VC++ that is in error here, or was it a minor bug in the program?

ctime includes the C time functions like time(). Since that function
is used it would certainly be necessary. Bug in the program...

[Marcin Kalicinski]

In Java world everything is a class, but in C++ fortunately it is not. Your
class is artificial, and the constructor implicitly modifies global state -
this is evil. What you need is a function:

int roll()
{
return rand() % 6;
}

It has better functionality than your class (no global state modification),
is about 10 times simpler to write and 2 times simpler to use.

[Luke Meyers]

Simon Biber wrote:

For plain old data (especially integers or pointers), in a context where
the value of the expression is not used, the compiler can and will
optimise away any temporary storage. In this context, ++i and i++ are
almost certain to produce exactly the same code.
In cases where I expect the compiler to produce an optimized result, if
I can write code literally equivalent to what the compiler will do
without obfuscating, I prefer to do so. Pre-incrementing is also more
helpful to the sensitive reader, who won't have to stop and ponder
(however briefly) whether you're using the pre- or post- value.
I consciously and deliberately switch between i++ and ++i depending on
the type of the object I'm incrementing.

snip example<

This way it helps me keep straight whether I'm using an integer or an
iterator.

I can see that being a valid practice. However, if we assume that
beginners are not equipped for such subtleties, I prefer to recommend a
safe, consistent policy. There are more overt ways of determining what
you're incrementing -- if it's difficult, you have bigger problems,
neh?

Luke

[deane_gavin]

Luke Meyers wrote:

* Using all caps to represent constants is great.

No it isn't. Always use all uppercase for macros and never use all
uppercase for anything else. Then you will never have a macro tramp all
over your code (at least not one of your own macros). So

class cDie
{
private:
const int sides;
public:
cDie(int sides_);
int roll();
};

cDie::cDie(int sides_) : sides(sides_) {}

Gavin Deane

[Luke Meyers]

de*********@hotmail.com wrote:

Luke Meyers wrote:

* Using all caps to represent constants is great.

No it isn't. Always use all uppercase for macros and never use all
uppercase for anything else. Then you will never have a macro tramp all
over your code (at least not one of your own macros). So

Hmm, I can see your point, but considering how few macros occur in the
code I work on (they're a last resort, restricted as much as possible),
I'm unwilling to reserve such a large chunk of my namespace for them.
How many macros have you got in your code? I generally only find a
need for macros when doing preprocessor tricks, and the symbols I
choose for such cases are unlikely to collide with constant names.

As long as you're being paranoid, there's nothing stopping someone from
defining a macro that isn't all-caps.

Anyone else adhere to the non-caps constants rule? It's novel to me.

Luke

[deane_gavin]

Luke Meyers wrote:

de*********@hotmail.com wrote:

Luke Meyers wrote:

* Using all caps to represent constants is great.
No it isn't. Always use all uppercase for macros and never use all
uppercase for anything else. Then you will never have a macro tramp all
over your code (at least not one of your own macros). So

Hmm, I can see your point, but considering how few macros occur in the
code I work on (they're a last resort, restricted as much as possible),
I'm unwilling to reserve such a large chunk of my namespace for them.
How many macros have you got in your code?

None at all if I can help it. But following the all caps naming
convention for macros guarantees that I will never create the problem
for myself. It's a separate issue, but I have never found the need for
a special naming convention for constants at all. The only time I've
seen any widespread convention for constants is in C style code where
the constants are #define'd rather than declared const. Then the all
caps macro convention is used. But get rid of the use of macros for
constants and you get rid of the need for all caps.
I generally only find a
need for macros when doing preprocessor tricks, and the symbols I
choose for such cases are unlikely to collide with constant names.

As long as you're being paranoid, there's nothing stopping someone from
defining a macro that isn't all-caps.
That's why I qualified the benefits of this rule by saying that you are
protected from your own macros but not necessarily anyone else's. The
more people who follow the rule, the greater the protection.
Anyone else adhere to the non-caps constants rule? It's novel to me.

The rule I am proposing is not "avoid all caps for constants". It is
"avoid all caps for everything except macros, which should always be
all caps". Granted, the second implies the first, but as far as the
rule is concerned, macros are the special case being given their own
'namespace', there is nothing special about constants. So, as to who
adheres to that rule...

Boost for one (and from what their web page says, the C++ standard
library for another): from http://www.boost.org/more/lib_guide.htm
<quote>
Use the naming conventions of the C++ Standard Library
<snip>
Macro (gasp!) names all uppercase and begin with BOOST_.
</quote>

He doesn't put it quite as strongly as me, but in "The C++ Programming
Language" (I have the 3rd edition) Stroustrup says "Also to warn
readers, follow the convention to name macros using lots of capital
letters". I just typed that in, so any errors are mine not his. It's
not exhaustive, but I've looked though the book - all the macros I saw
were all caps and nothing else was. He also mentions the convention on
his website in a section titled "So, what's wrong with using macros?".
After raising the obvious problem of macros and other symbols having
the same name, he says "Conventions such as having macros (and only
macros) in ALLCAPS helps, but there is no language-level protection
against macros." See
http://public.research.att.com/~bs/bs_faq2.html

The convention is recommended in this group regularly - not just by me
:-)

Gavin Deane

[Luke Meyers]

[e-mail] wrote:

The convention is recommended in this group regularly - not just by me
:-)

Gavin,

Excellent points. As I was writing the post to which you replied, I
did have to concede that perhaps constants do not really benefit so
greatly from all-caps. Readability, maybe, but not terribly much so.
And, of course, the compiler will tell you in short order if you get it
wrong.

I'll give your convention w.r.t. constants serious consideration. And
since there's nothing else I really want to use all caps for... well,
dammit, you may just convert me. I'll think on it a spell.

Cheers,
Luke

[Howard]

"Marcin Kalicinski" <ka****@poczta.onet.pl> wrote in message
news:T0*****************@newsfe3-win.ntli.net...

In Java world everything is a class, but in C++ fortunately it is not.
Your class is artificial, and the constructor implicitly modifies global
state - this is evil. What you need is a function:

int roll()
{
return rand() % 6;

You forgot to add 1. The mod (%) operator returns a value in the rang
[0..n-1], so you need to add 1 to make it [1..6].

(And I suggest passing the number of sides as a parameter, so you can handle
dice of different types.)

-Howard

[Christopher Benson-Manica]

ro**********@gmail.com wrote:

In D&D there are dice with any number of sides including 4, 6, 10, 12,
20, 100, etc... Other RPG systems have odd sided dice as well. There
is nothing that says a die has 6 sides...that is just the most common.

Seriously - it doesn't take a +4 potion of intellect to know that!

--
Christopher Benson-Manica | I *should* know what I'm talking about - if I
ataru(at)cyberspace.org | don't, I need to know. Flames welcome.

[Niklas Norrthon]

Mike Smith <mi*****************@acm.org> writes:

Howard wrote:

"osmium" <r1********@comcast.net> wrote in message
news:41*************@individual.net...

People know there are ... 365 days in a year,

What about leap year? :-)

365.242199 days. Google sez so.

Google is wrong. In the western world a year is either 365 or
366 days, take or leave a second or two, but never 365.242199
days. One specific year a long time ago was a number of days
shorter when it was realized that the leap year had to be
skipped about once every century.

/Niklas Norrthon

[Mike Smith]

Niklas Norrthon wrote:

Mike Smith <mi*****************@acm.org> writes:

Howard wrote:

"osmium" <r1********@comcast.net> wrote in message
news:41*************@individual.net...
People know there are ... 365 days in a year,

What about leap year? :-)

365.242199 days. Google sez so.

Google is wrong. In the western world a year is either 365 or
366 days, take or leave a second or two, but never 365.242199
days. One specific year a long time ago was a number of days
shorter when it was realized that the leap year had to be
skipped about once every century.

The *calendar* contains 365 or 366 days for each year. The length of a
year is defined in terms of the motion of the earth relative to either
the sun or the background of faraway stars, and this length of time does
not come to a convenient round multiple of the rotational period of the
earth.

--
Mike Smith (Happy New Year Leap Second!)

[Mike Smith]

ro**********@gmail.com wrote:

Thomas Jespersen wrote:

On 28 Dec 2005 14:55:19 -0800, ma******@gmail.com wrote:

after reading the discussions, i have made some changes to the code.

Ahem, another newbie is reading this thread (me) :)

I am using VC++ Express, and I couldn't get this to compile unless I

#include <ctime>

Is it VC++ that is in error here, or was it a minor bug in the program?

ctime includes the C time functions like time(). Since that function
is used it would certainly be necessary. Bug in the program...

However, #including <ctime> should put the time() function into the
std:: namespace (as opposed to #including <time.h> which would put
time() into the global namespace). So, to be picky, there's also a bug
in Visual C++ (a well-known and long-standing one, and actually a
conscious choice on the part of the VS developers rather than a mistake).

--
Mike Smith

[osmium]

<ma******@gmail.com> wrote:

after reading the discussions, i have made some changes to the code.
#include <iostream>
#include <cstdlib>
class cDie
{
private:
int SIDES;
public:
cDie(int sides);
int roll();
};

cDie :: cDie(int sides)
{
SIDES = sides;
}
int cDie :: roll()
{
return 1 + rand() % SIDES;
}

int main()
{
srand(time(NULL));

cDie die(6);

for (int i = 0; i < 10; ++i)
{
std::cout << "Die Rolled: " << die.roll() << "\n";
}
return EXIT_SUCCESS;
}

Well, you have lets the abstruse crowd lead you up the creek and abandon
you. I have given them all day to come and rescue you and figure that is
long enough to wait. . AFAICT you used to have a working program. Now you
let the user toss a die with a negative number of sides. Also, the number
of sides could be greater than the range of rand. And the number of sides
should be a constant. The last item may have been mentioned, I'm not really
sure.

None of those were a problem in your initial post. 6 *is* a constant. It
*is* positive. It is less than any RAND_MAX that is allowed by the
standard.

[Luke Meyers]

osmium wrote:

Well, you have lets the abstruse crowd lead you up the creek and abandon
you. I have given them all day to come and rescue you and figure that is
long enough to wait. . AFAICT you used to have a working program. Now you
let the user toss a die with a negative number of sides. Also, the number
of sides could be greater than the range of rand. And the number of sides
should be a constant. The last item may have been mentioned, I'm not really
sure.

None of those were a problem in your initial post. 6 *is* a constant. It
*is* positive. It is less than any RAND_MAX that is allowed by the
standard.

The OP established first thing that the program worked fine -- this
thread has been about coding practices from the beginning, not program
correctness. Obviously, someone having a bunch of new, sometimes
conflicting ideas about coding practices heaped on all at once is going
to take a couple of iterations to sort them all out in a reasonable
fashion.

You're right about range checking. There are a number of strategies
that can be adopted. Certainly, an unsigned data type should be used.
Beyond that, you must first answer the question of whether you want to
do bounds checking at compile-time, or run-time. If the number of
sides is not going to be a compile-time constant (which could be
checked by static assertion, e.g. BOOST_STATIC_ASSERT), then of course
the value will have to be bounds-checked against RAND_MAX (and probably
0) at runtime. How errors are handled (exception, return value,
assertion failure, etc.) is an issue with multiple acceptable answers,
depending on context.

All that said, here's a sketch of what I might do to incorporate the
advice received thus far:

#include <iostream>
#include <cstdlib>

typedef unsigned int DieSides;
typedef unsigned int DieRoll;

const DieSides defaultSides(6);

DieRoll roll(DieSides sides = defaultSides) {
static bool rngSeeded(false);
if (!rngSeeded) {
srand(time(NULL)); // Seed the RNG, once only.
rngSeeded = true;
}
return 1 + rand() % sides;
}

int main() {
const unsigned int numRolls(10);

for(int i = 0; i < numRolls; ++i) {
using namespace std;
cout << "Die rolled: " << roll() << "\n";
}

return EXIT_SUCCESS;
}

Note that I didn't do bounds checking against 0 and RAND_MAX in the
above. If I wanted to be as careful as that, I'd probably define
DieSides as a more robust type, convertible from an unsigned int, which
performs bounds checking on construction. That way, an invalid value
can never even exist, and you find out about the error as early as
possible.

Luke

[porcelli]

ma******@gmail.com wrote:

int cDie :: roll()
{
return rand()%6 + 1;
}

Not a C++ nitpick, but note that if rand() generates a number between 0
and RAND_MAX uniformly, this method will not be guaranteed to generate
a number between 1 and 6 uniformly. Of course, the difference in
probabilities gets smaller the larger RAND_MAX gets, but this is a
nitpick. Better is something like this:

int cDie::limit = (RAND_MAX / 6) * 6;

int cDie::roll()
{
int num;

do {
num = rand();
while (num >= limit);

return num % 6 + 1;
}

[Gavin Deane]

Luke Meyers wrote:

osmium wrote:

None of those were a problem in your initial post. 6 *is* a constant. It
*is* positive. It is less than any RAND_MAX that is allowed by the
standard.

You're right about range checking. There are a number of strategies
that can be adopted. Certainly, an unsigned data type should be used.
Why?

<snip code using unsigned int for the number of sides on the die>
Note that I didn't do bounds checking against 0 and RAND_MAX in the
above.
No. And using unsigned in preference to signed didn't offer any help
there either.
If I wanted to be as careful as that, I'd probably define
DieSides as a more robust type, convertible from an unsigned int, which
performs bounds checking on construction. That way, an invalid value
can never even exist, and you find out about the error as early as
possible.

Absolutely. And if you're going to do that, you are just as well off
using a singed type as an unsigned type.

Since using an unsigned type gains you nothing, I think it's a little
strong to say that "Certainly, an unsigned data type should be used."

Gavin Deane

[Kaz Kylheku]

ma******@gmail.com wrote:

I am new to c++ classes. I defined this "cDie" class that would return
a value between 1 and 6 (inclusive)
That is a function, not a class. Classes don't return anything, but C++
classes can have member functions that return values.
I was wondering if you guys can pick up any mistakes/"don't do"s from
this code:
The number one mistake would be creating a class whose instances don't
have any useful state. Because your cDie::roll() function just calls
the library's pseudo-random number generator, all of the random
generation state is actually held outside of the object. If you make
two or more instances of cDie, they will not generate independent
pseudo-random sequences.

A useful PRNG object would maintain its own state, and reproduce the
same sequence if given the same seed, even if calls to it are
interleaved with calls to other PRNG objects.

What is worse, the constructor of your class re-seeds the PRNG from the
time() variable. (I won't comment on the non-portability of casting a
time_t to unsigned int). Suppose that time_t has second granularity.
Suppose that within the span of one second, your program constructs
many cDie objects, and calls roll() on them. Each construction will
re-set the seed to the same time_t value, and so each new roll() will
produce the same PRNG value.
int main()
{
cDie die;

for (int i = 0; i < 10; i++)
{
cout << "Die Rolled: " << die.roll() << endl;
}
return 0;
}

Now change the program like this:

int main()
{
// <-- object definition removed from here ...

for (int i = 0; i < 10; i++)
{
cDie die; // <-- ... and inserted here!

cout << "Die Rolled: " << die.roll() << endl;
}
return 0;
}

For each iteration of the loop, a new cDie object is constructed. What
values does your program output now?

[Luke Meyers]

Gavin Deane wrote:

Since using an unsigned type gains you nothing, I think it's a little
strong to say that "Certainly, an unsigned data type should be used."

You really think so? I agree that, given a more robust solution, it
sort of becomes moot, but, still... a die with a negative number of
sides is pretty nonsensical, so why would I use a data type that allows
it? So, what I was saying was, *at the least* use an unsigned type,
preferably use a specially defined type that can handle all the
boundary cases.

Luke

[Hugo]

Marcin Kalicinski wrote:

In Java world everything is a class, but in C++ fortunately it is not. Your
class is artificial, and the constructor implicitly modifies global state -
this is evil. What you need is a function:

int roll()
{
return rand() % 6;
}

It has better functionality than your class (no global state modification),
is about 10 times simpler to write and 2 times simpler to use.

int roll(int sides) {
return rand() % sides;
}

[Kaz Kylheku]

Luke Meyers wrote:

Gavin Deane wrote:

Since using an unsigned type gains you nothing, I think it's a little
strong to say that "Certainly, an unsigned data type should be used."

You really think so? I agree that, given a more robust solution, it
sort of becomes moot, but, still... a die with a negative number of
sides is pretty nonsensical, so why would I use a data type that allows
it?

This is silly. There are high level languages in which integers behave
much more like mathematical integers than do C++ integers. There are no
unsigned types.

The reason unsigned types exist in C++ is because they exist in machine
language, and C++ is a machine-oriented language in its core.

Unsigned types are good for storing bitfields, and for portable
arithmetic: you can use them to emulate two's complement signed
arithmetic even on machines that don't have two's complement signed
integers.

An unsigned int is good for representing a nonnegative quanitty only if
you never need to subtract two such quantities, and compare the result
for inequality to some third quantity.

If you are going to have expressionsl like (x - y > z) then unsigned
types aren't a particularly good fit, because you then have to handle
cases when y is greater than x, and some big, positive number pops out
that is greater than z when it should not.

Thus code like if (buffer_space - requested_payload > header_size)
is broken if unsigned integers are used, and there was no prior check
that the requested_payload isn't larger than the buffer.

In other words, arithmetic even on small unsigned integers clustered
around zero does not behave like normal integer arithmetic, and you
have to be extra careful.

I don't know about you, but I like my integers to behave like
mathematical integers.
If my problem allows them to be sufficiently small in magnitude, I can
write the code with mathematical clarity, without obtrusive overflow
checks.

Are you sure, as the library designer, that nobody will ever need to
subtract two quantities that represent the sides of a die, and find it
convenient to get a negative number sometimes?

Lastly, zero is also a "nonsensical" number of sides for a die to have,
yet it's in the domain of an unsigned int. The same can be said about
65535.

[Gavin Deane]

Luke Meyers wrote:

Gavin Deane wrote:

Since using an unsigned type gains you nothing, I think it's a little
strong to say that "Certainly, an unsigned data type should be used."
You really think so? I agree that, given a more robust solution, it
sort of becomes moot, but, still... a die with a negative number of
sides is pretty nonsensical, so why would I use a data type that allows
it?

The minimum number of sides a die can have is 4. I don't know what the
maximum is. The most I've come across is 100. Certainly the maximum is
a lot less than, for example, 4294967295.

Both signed and unsigned permit values outside this range, so from the
point of view of restricting you to sensible values, they are both
useless.

The problem with unsigned is the way arithmetic works. Subtract one
positive integer from another in the real world and you don't
necessarily get a positive answer. Do the same with unsigned in the C++
world and you will always get a positive answer. 2 - 3 evaluating to a
huge positive number is pretty nonsensical, so why would I use a data
type that allows it?

There are other quantities, a person's age for example, where the
minimum sensible value could be 0. But the maximum sensible value is
again a lot less than the maximum unsigned int value. So using unsigned
protects you from going off one end of the sensible range, but not the
other. If this is of any benefit at all, and I'm not sure it is, that
benefit is far outweighed by the fact that your_age - my_age does not,
in general, do what you want it to.
So, what I was saying was, *at the least* use an unsigned type,
preferably use a specially defined type that can handle all the
boundary cases.

If you are willing to sacrifice the ability to do natural arithmetic
you can get away with unsigned, but it doesn't gain you anything.

If you really need a bounded range then a class for that purpose is the
only option. Unsigned does not provide a reasonable partial solution.

Gavin Deane

[Kaz Kylheku]

Gavin Deane wrote:

The minimum number of sides a die can have is 4. I don't know what the
maximum is. The most I've come across is 100. Certainly the maximum is
a lot less than, for example, 4294967295.

A coin can be considered a kind of die, so the minimum is two.

[Jim Langston]

"osmium" <r1********@comcast.net> wrote in message
news:41*************@individual.net...

"W Marsh" writes:

Instead of using 6 as the modulo in the roll method, define a constant
such as "MAX_ROLLS". It could be a private member of cDie. Having "magic
numbers" in code decreases readability, and makes it harder to alter the
code should you want an 87-sided die.

I find that magic numbers are almost always more readable than some ugly,
long winded name. People know there are 7 days in a week, 365 days in a
year, 52 cards in a deck and 6 sides on a die. There is only one way to

There are 6 sides on a 6 sided die. There are 20 on a 20 sided die. 8 on
an 8 sided die, etc.. Yes, these other die are actually produced and used
in various games (see dungeons and dragons for instance or D20).

[Howard]

"Hugo" <hu*****@gmail.com> wrote in message
news:11**********************@o13g2000cwo.googlegr oups.com...

Marcin Kalicinski wrote:

In Java world everything is a class, but in C++ fortunately it is not.
Your
class is artificial, and the constructor implicitly modifies global
state -
this is evil. What you need is a function:

int roll()
{
return rand() % 6;
}

It has better functionality than your class (no global state
modification),
is about 10 times simpler to write and 2 times simpler to use.
int roll(int sides) {
return rand() % sides;

return 1 + (rand() % sides);

// parens for my own use... I always get confused without them :-))
}

[Earl Purple]

Kaz Kylheku wrote:

ma******@gmail.com wrote:

I am new to c++ classes. I defined this "cDie" class that would return
a value between 1 and 6 (inclusive)

That is a function, not a class. Classes don't return anything, but C++
classes can have member functions that return values.

I was wondering if you guys can pick up any mistakes/"don't do"s from
this code:

The number one mistake would be creating a class whose instances don't
have any useful state. Because your cDie::roll() function just calls
the library's pseudo-random number generator, all of the random
generation state is actually held outside of the object. If you make
two or more instances of cDie, they will not generate independent
pseudo-random sequences.

A useful PRNG object would maintain its own state, and reproduce the
same sequence if given the same seed, even if calls to it are
interleaved with calls to other PRNG objects.

I would use a singleton class to randomise the seed, and then to return
random numbers. Thus something like:

class RandomNumberGenerator
{
private:
RandomNumberGenerator();
public:
static RandomNumberGenerator & instance();
unsigned int rand() const;
};

Now you have 3 choices:

- Make your die a class that uses this one, storing the maximum
possible roll.
- Write a free function that takes a min/max and returns a random
number between them.
- Extend the class above with a member function to return range-bound
random numbers, or even put these values into rand() using 0 and
RAND_MAX as defaults.

[Marcus Kwok]

osmium <r1********@comcast.net> wrote:

52 cards in a deck

Unless you're playing Pinochle.

--
Marcus Kwok