Classes, why is this considered bad coding?

[maskanio]

My program compiles but my teacher said this was considered bad coding?

class Dice{
                //Constructor
        public:
                Dice();
                Dice(int);      
                double score();
                double randPoints;                    
                int pointTotal, a;
                double getRolls();
                void setRolls(double&,double&);               
                
				//methods       
                int roll();
                void resetNumRolls();
                int getNumRolls();              
                int playerPoints();
                int compPoints();
                int computerTotal();//this OBJECT called DICE should behave like a real dice
                        
        private:
                //properties
                int numRolls;
                int numSides;   
                int numPoints;
                int rollTotal;
                double rolls;   
                       
};

[Albatross]

I think your teacher was complaining about line 8, where you have public member variables that should be private. Just a shot in the dark, though.

-Albatross

[guestgulkan]

Ask your teacher what he/she meant - that's the point of a teacher.

[richardn413]

1. You also ought to include a destructor:

public:
    Dice();
    Dice(int);
    ~Dice(); // destructor 

2. I'm not too sure Dice(int) works as a constructor - it should be Dice(int number) where "number" is a parameter name. Same applies to void setRolls (try using these functions/constructors - I expect it'll throw errors as soon as you try using it).

3. Albatross is right, as a rule of thumb put all variables in private scope.

Hope this helps.

[clanmjc]

it should be Dice(int number) where "number" is a parameter name

You can declare member and non-member function with anonymous parameters (no parameter name). You just need to name them in the implementation.

To OP - I'm going to guess it's because this Dice class has a lot going on.

int playerPoints();
int compPoints();
int computerTotal();
double score();
double randPoints;
int pointTotal

These members don't belong in the Dice class, what does a score have to do with the dice. Think of it this way, lets say you use these dice in different games, Dice by itself is very generic, it has sides which have a value on each face of the side. Dice can be rolled, that is about it. I say you either have another class that handles these or you make some non member functions do these jobs, otherwise you limit the re-usability of a Dice.

[bboy212]

richardn413: 1. You also ought to include a destructor:

Aren't destructors only for freeing up dynamic memory?

[maskanio]

clanmjc (503) May 2, 2012 at 8:49am
it should be Dice(int number) where "number" is a parameter name
You can declare member and non-member function with anonymous parameters (no parameter name). You just need to name them in the implementation.

To OP - I'm going to guess it's because this Dice class has a lot going on.

1
2
3
4
5
6
int playerPoints();
int compPoints();
int computerTotal();
double score();
double randPoints;
int pointTotal


These members don't belong in the Dice class, what does a score have to do with the dice. Think of it this way, lets say you use these dice in different games, Dice by itself is very generic, it has sides which have a value on each face of the side. Dice can be rolled, that is about it. I say you either have another class that handles these or you make some non member functions do these jobs, otherwise you limit the re-usability of a Dice.

You hit it right on the nail for me. Thankyou for all the responses. Solved.