J
Jay Nabonne
Then use std::endl.
class cDie
{
// Your stuff, and add:
static bool initedSrand;
static bool InitSrand()
{
srand(time(0));
return true;
}
};
bool cDie::initedSrand = cDie::InitSrand();
- Jay
Then use std::endl.
class cDie
{
// Your stuff, and add:
static bool initedSrand;
static bool InitSrand()
{
srand(time(0));
return true;
}
};
bool cDie::initedSrand = cDie::InitSrand();
- Jay
S
Simon Biber
Luke said:
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
}
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.
L
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
* 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
R
roberts.noah
Thomas said: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...
M
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.
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.
L
Luke Meyers
Simon said:
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 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
D
deane_gavin
Luke said:
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
L
Luke Meyers
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
D
deane_gavin
Luke said:
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.
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.
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
L
Luke Meyers
[email protected] said: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
N
Niklas Norrthon
Mike Smith said:
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
H
Howard
Marcin Kalicinski said:
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
C
Christopher Benson-Manica
Seriously - it doesn't take a +4 potion of intellect to know that!
M
Mike Smith
Niklas said:
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.
M
Mike Smith
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).
O
osmium
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.
L
Luke Meyers
osmium said:
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
P
porcelli
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;
}
G
Gavin Deane
Why?
No. And using unsigned in preference to signed didn't offer any help
there either.
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
K
Kaz Kylheku
That is a function, not a class. Classes don't return anything, but C++
classes can have member functions that return values.
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.
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?