1
\$\begingroup\$

I've written a program all by myself, but I just want to make sure I did it right and if anybody has any suggestions on improving it in any way.

  1. Define and implement the Die class depicted by the following UML diagram to create an ADT for one die (singular of dice) with faces showing values between 1 and the numberOfFaces on the die. Define a two-parameter constructor with defaults of 6 faces and 1 for face value. Define and implement a roll() method to simulate the rolling of the die by generating a random number between 1 and numberOfFaces and storing that number in faceValue. Provide an accessor to return the die’s face value and a print() method to print the die’s face value.

  2. Use composition to design a PairOfDice class, composed of two six-sided Die objects. Create a driver program with a main() function to rolls a PairDice object 1000 times, counting then displaying the number of snake eyes (i.e. two ones) and box cars (i.e. two sixes) that occur.

die.h

#ifndef DIE_H
#define DIE_H

class die {

private:
    int numberOfFaces;
    int faceValue;
public:
    die();
    die(int, int);
    int roll();
    void print();
};
#endif

die.cpp

#include "die.h"
#include<iostream>
#include<ctime>
#include<random>
using namespace std;

die::die()
{
    numberOfFaces=6;
    faceValue=1;
}

die::die(int numOfFaces, int faceVal)
{
    numberOfFaces=numOfFaces;
    faceValue=faceVal;
}

int die::roll()
{
    faceValue=rand()%numberOfFaces;
    return faceValue;
}

void die::print()
{
    cout << faceValue << endl;
}

pairOfDice.h

#include "die.h"
#ifndef PAIROFDICE_H
#define PAIROFDICE_H

class pairOfDice :
    public die
{
private:
    die die1;
    die die2;
    int value1;
    int value2;
    int total;
public:
    pairOfDice();
    int roll();
};

#endif

pairOfDice.cpp

#include "pairOfDice.h"

pairOfDice::pairOfDice()
    :die(6,1)
{
    value1=1;
    value2=1;
    total=value1+value2;
}

int pairOfDice::roll()
{
    value1=die::roll();
    value2=die::roll();
    total=value1+value2;
    return total;
}

main.cpp

#include"pairOfDice.h"
#include<iostream>
using namespace std;

int main()
{
    int rolls=0, total=0, snakeEyes=0, boxCars=0;
    pairOfDice dice;

    while(rolls<1000)
    {
        total = dice.roll();

        if(total==2)
            snakeEyes++;
        else if(total==6)
            boxCars++;
        else
            rolls++;
    }

    cout << rolls << " " << snakeEyes << " " << boxCars << endl;
    system("pause");
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Where do you call srand()? \$\endgroup\$ – Jamal Nov 19 '13 at 5:54
  • 1
    \$\begingroup\$ Prefer std::uniform_int_distribution to rand when it is available. \$\endgroup\$ – Michael Urman Nov 19 '13 at 15:19
  • \$\begingroup\$ @Jamal i was calling it in the roll method of the dice class but i moved it right before the loop in the main program and that seemed to fix the problem. idk if thats the best solution though ... \$\endgroup\$ – Juan Battini Nov 19 '13 at 19:45
5
\$\begingroup\$

Some miscellaneous remarks unrelated to object-oriented design:

  • Die face numbering: Your die face values are 0, 1, 2, 3, 4, 5. That's not how most people expect a six-sided die to be labeled. It also makes an exceptionally weird definition of rolling snake eyes!
  • Biased die: Since 6 is not a power of 2, if you just do rand() % 6 then you have a slightly biased die.
  • Roll miscount: You aren't rolling a pair of dice 1000 times. You're rolling a pair of dice until you get 1000 results other than 2 or 6. You could probably have avoided this bug if you had used a for-loop instead of a while-loop, since for-loops impose a standard structure on your code.
  • Variable declaration: In main(), the variable total is only used within the while-loop, so it should be declared at the point of use: int total = dice.roll();
  • Seeding: You didn't actually call srand().
  • Portability: system("pause") is unportable.
  • ostream support: Consider implementing std::ostream &operator<<(std::ostream &out, ...) for printing.
\$\endgroup\$
  • \$\begingroup\$ Avoiding the roll miscount is a matter of putting rolls++ on its own line, not part of the if statement. It's not a matter of a for loop being better or worse than a while loop. \$\endgroup\$ – Michael Shaw Nov 19 '13 at 7:51
  • 1
    \$\begingroup\$ @MichaelShaw: In this context a for loop is better because it puts the loop invariants in one place where it's easy to see. \$\endgroup\$ – ChrisWue Nov 19 '13 at 8:10
  • \$\begingroup\$ @MichaelShaw Of course you can fix the bug, but it would be better to address the root cause of the bug, so that you would be less likely to make such mistakes in the future. \$\endgroup\$ – 200_success Nov 19 '13 at 8:18
  • \$\begingroup\$ @200_success: But the root cause of the bug was not lack of a for loop or presence of a while loop. \$\endgroup\$ – Michael Shaw Nov 19 '13 at 8:28
2
\$\begingroup\$

As mentioned, composition must be implemented in terms of members, not parents.

You could use default arguments and only have one constructor rather than two. It would make the code simpler.

The assignment also requested an accessor for face value. Make such side-effect free method const.

I suggest not putting Die and PairOfDice in the same class hierachy. The only interface they could possibly share is roll(), and that is only if the return type is void. Use class hierachies for things that are polymorphic, i.e. where you intend for instances of both classes to be used the same way in the same code.

\$\endgroup\$
2
\$\begingroup\$

That's not composition. That was inheritance, done poorly. The difference is, would you say that…

  • A pairOfDice has two dice, but is not a die (composition), or
  • A pairOfDice is a loaded die (inheritance)?

Since you declared pairOfDice to publicly inherit from die, you are treating a pairOfDice as a dodecahedron that is somehow weighted to produce a random distribution biased towards 6 or 7. I don't think you could produce such a physical object with just the right weight distribution. It could exist in your imagination though. Therefore, saying that pairOfDice is a die would be controversial modeling at best.

Composition

With composition, a pairOfDice contains two die members, but does not necessarily inherit from anything.

class pairOfDice {
private:
    die die1;
    die die2;
public:
    pairOfDice() {};
    int roll();
};

int pairOfDice::roll() {
    int value1 = die1.roll();
    int value2 = die2.roll();
    return value1 + value2;
}

That's all. Note that you should not store value1, value2, or total. All of that state is handled by the constituent die1 and die2 members.

Private Inheritance

C++ has a private inheritance feature, which could let you inherit the code for just one die, without letting anyone else know that you did so.

class pairOfDice : private die
{
private:
    die die2;
public:
    pairOfDice() {};
    int roll();
};

int pairOfDice::roll()
{
    int value2 = die2.roll();
    return ((die *)this)->roll() + value2;
}

The code above acts like the composition example. Attempting to do pairOfDice dice; die *diePtr = &dice; would be a compilation error, since pairOfDice is not publicly a subclass of die. Private inheritance is an odd feature, and you would be better off doing pure composition instead.

Public Inheritance

With public inheritance, you would be treating pairOfDice as a kind of die — a loaded 12-sided die.

class pairOfDice : public die
{
private:
    die die2;
public:
    pairOfDice() {};
    int roll();
};

int pairOfDice::roll()
{
    int value2 = die2.roll();
    return faceValue = ((die *)this)->roll() + value2;
}

Note that pairOfDice must now support every operation that die supports, including print(). To ensure that pairOfDice::print() works, you must assign faceValue when rolling, and therefore the faceValue member of die must be changed from private to protected to support that. That illustrates a kind of fragility that comes with sharing code through inheritance: the subclass might accidentally inherit code that doesn't work quite right, and in a sense the subclass is patching the superclass code.

The fragility of such an inheritance relationship is also a symptom of the fact that the inheritance was a long stretch of the imagination to start with (as discussed in the beginning of this review). Let's just say, inheritance, whether of the public or private sort, is not really appropriate for this situation.

A better hierarchy

Perhaps it would be best to have a virtual base class representing number generators.

template <class T>
class randgen {
public:
    virtual T roll() = 0;
    virtual void print() = 0;
};

class die : public randgen<int> {
    ...
};

class pairOfDice : public randgen<int> {
private:
    die die1;
    die die2;
public:
    ...
};

Everyone would agree that a die is a randgen<int> and that a pairOfDice also is a randgen<int>. With a common base class, it would be possible to have a randgen<int>* pointer that points to either a die or a pairOfDice. Finally, pairOfDice could be implemented using composition, which is the least error-prone way to reuse code.

\$\endgroup\$
  • 1
    \$\begingroup\$ Regarding "Therefore, you would be better off using composition instead of inheritance as a code-reuse mechanism whenever possible" - I'd say if you use inheritance for the sole purpose of code reuse your abstraction is broken \$\endgroup\$ – ChrisWue Nov 19 '13 at 8:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.