Thread: How 'far off' is this code?

  1. #1
    Registered User
    Join Date
    Jul 2005
    Posts
    23

    Cool How 'far off' is this code?

    I wrote a class for simply creating sets of dice, and rolling them. This could obviously be pretty useful for anything involving dice. I also have the concept of an open-ended roll, which is simply a roll with no "hard" cap. For example, on a d10, if you rolled a 10, you would roll the die again, and subtract 1 from that roll and add it. If you get another 10, you'd add yet another roll(being0-9 for the secondary rolls.) I first saw a reference to this on the forums for a game called Dominions 2, but I am unsure where it originates. I am not claiming it was my idea at any rate
    I haven't added the ability to remove dice from the dice pool yet, I'll consider it if I find I need that ability (I suspect I will simply build and allow the scope to eat them.) It has the ability to add new dice at any time, and it has a Variadic *yay for new words * constructor. I am also fairly certain it doesn't leak, but I am too new to claim 100% certainty.
    I like defining all my functions inside the class, but I am aware that can cause the compiler to inline them, which isn't always good. What I am driving at here is, how much would this code stick out as poor in a "real"(as in I was getting paid) program. I am not naive enough to think it is up to pro snuff, I just was curious how far off it is, and what is *not* up to snuff about it.

    Here is the class itself: dice.cpp
    Code:
    #include<cstdlib>
    #include<stdarg.h>
    class Dice
    {
     private:
     //Hidden DIE class for use ONLY with DICE pools
     class Die
     {
      public:
      unsigned char size,face;bool open;Die * next;
      //Rolls the DIE
      int Roll(void)
      {
       if(open)//If open go through the motions for an OE dice roll
       {
        int tempRoll=0,tempFace;tempFace=(rand()%size)+1;tempRoll+=tempFace;
        if(tempFace==size){do{tempFace=(rand()%size);tempRoll+=tempFace;}while(tempFace==size-1);};
        face=tempRoll;
       }
       else//Otherwise, just roll and report
       {
        face=(rand()%size)+1;
       }
      }
     };
     Die * root;//Root pointer for lists of DIE
     public:
     Dice(){;} 
     Dice(char noDice...)//Handy-dandy Variadic constructor
     {
      va_list ap;
      va_start(ap,noDice);
      char size;bool open;
      while(noDice-->0)
      {
       size=(char)va_arg(ap,int);open=(bool)va_arg(ap,int);
       this->Add(size,open);
      }
      va_end(ap);
     }
     ~Dice()//Destructor. I hope it's airtight:)
     {
      Die * finger=root;Die * nullNext;
      while(!(finger==NULL)){nullNext=finger->next;delete finger;finger=nullNext;}
     }
     void Roll(int times)//Rolls all DIE a certain number of times. Will eventually
     {                   //add ability to roll X times and take highest/lowest
          
      Die * finger=root;
      for(int x=1;x<=times;x++){while(!(finger==NULL)){finger->Roll();finger=finger->next;};};
     }
     int Roll(void)//Overloaded to show what roll the DICE pool currently contains
     {
      Die * finger=root;
      int total=0;
      while(!(finger==NULL)){total+=finger->face;finger=finger->next;}
      return total;
     }
     void Add(char Size,bool Open=false)//Allows the addition of DIE after the constructor
     {
      Die * newDie;newDie = new Die;newDie->size=Size;newDie->open=Open;
      if(root==NULL){root=newDie;newDie->next=NULL;}
      else
      {
      Die * finger;finger=root;
      while(!(finger->next==NULL)){finger=finger->next;}
      finger->next = newDie;newDie->next=NULL;
      }
     }
    };
    Here is a sloppy (not interested in comments on THIS part, but just posted it to show a use, I know it's *super* ghetto) MAIN to go with it: main.cpp
    Code:
    //500,000 roll dice statistic test
    #include<iostream.h>
    #include"dice.cpp"
    
    int main(void)
    {
     bool addMore;
     Dice * Derka = new Dice();
     do
     {
      srand((unsigned)time(0)); 
      int size; char t_f;
      printf("Enter a die size(1-255):");
      scanf("%d",&size);
      printf("Is this die open ended(T/F)?:");
      cin>>t_f;
      if (t_f=='f' || t_f=='F'){Derka->Add((unsigned char)size);}
      else {Derka->Add((unsigned char)size,true);}
      printf("Add more dice?(T/F):");
      cin>>t_f;
      if (t_f=='f' || t_f=='F'){addMore=false;}else{addMore=true;};
     }while(addMore==true);
     //******Dice Statistics******
     int stats[500000];
     int largest=0,smallest=999999;
     for(int x=1;x<=500000;x++)
     {
      Derka->Roll(1);
      if(Derka->Roll()>largest){largest=Derka->Roll();}
      if(Derka->Roll()<smallest){smallest=Derka->Roll();}
      stats[x-1]=Derka->Roll();
      //printf("%3d|",Derka->Roll());
      //if(x%10==0){printf("\n");printf("----------------------------------------\n");}
      //if (x%100000==0){printf(".");};
     } 
     printf("\n%3d - %3d Range\n",smallest,largest);
     for(int x=smallest;x<=largest;x++)
     {
      int rollTemp=0;
      for (int y=1;y<=500000;y++)
      {
       if(stats[y-1]==x){++rollTemp;}
      }
      printf("%3d rolled %6d times and %2d%% of the time.\n",x,rollTemp,(rollTemp/5000));  
     }
     system("PAUSE");
     delete Derka;
    }
    Remember, I KNOW the main.CPP is sloppy, just want comments on dice.cpp

  2. #2
    I am me, who else?
    Join Date
    Oct 2002
    Posts
    250
    Actually both are hard to read... if you could reformat dice.cpp a bit it would help anyone greatly. I know it seems silly but reading someone else's code is a pain, and the less impediments the better.

  3. #3
    Registered User
    Join Date
    Jul 2005
    Posts
    23

    Good point

    Hrmm, I suppose it is kind of crazy looking. I apologize to everyone trying to read it, I will try to clarify and edit it as soon as I have time. And no, it doesn't seem silly at all. This stuff is wild looking if you didn't write it(or even if you didn't write it RECENTLY )

  4. #4
    Registered User
    Join Date
    Apr 2003
    Posts
    2,663
    Code:
    rand()%size
    That's not "professional". Read up on random numbers. Beware of the tutorials on this site--they have errors

  5. #5
    Registered User
    Join Date
    Jul 2005
    Posts
    23

    Yeah

    I guess I should have figured that was childish at best. I was just kind of stuck with "I have a random from 0 to someReallyHugeNumber" and that seemed like a quick way out. Noted.

  6. #6
    I am me, who else?
    Join Date
    Oct 2002
    Posts
    250
    What would you consider as "pofessional" using a library to generate a randomly seeded number? Or what? I realize rand() is not the all encompassing all wonderful random number generator, however for right now its a decent start... and the code probably needs more work than rand(). Anyway, please post more formatted code and we can help you

  7. #7
    Registered User
    Join Date
    Jul 2005
    Posts
    23

    Ok I *thought* I had this down but..

    Ok, I *thought* I had this, but something went horribly wrong.

    I am trying to straighten this mess out, but now I get errors.

    Multiple definition of `Dice::_______ for ALL methods.

    Now, I don't claim to be some kind of old hand at this, and I realize this might be some kind of RTFM error, but seriously I can't get what I am doing wrong from any of my sources.

    Code:
    #include<cstdlib>
    #include<stdarg.h>
    //**********************************************
    //*Dice class used for pooling and rolling DIE.*
    //**********************************************
    class Dice
    {
     public: 
     Dice(){};
     Dice(char...);//Variadic constructor
     ~Dice(void);
     int Roll(void);
     void Roll(int);
     void Add(char,bool);
     private:
     //*********************************************
     class Die//Hidden DIE class for use ONLY with DICE pools
     {
      public:
      void roll(void);//An internal function for reading DIE.
      unsigned char size;//Size is the d<x>(d6,d10,d20 etc)
      unsigned char face;//Face is the current rolled number of the die.
      bool open; //Open determines if the roll is OPEN
                 //An open roll has no firm limit, but is
                 //unlikely to roll *TOO* much higher than
                 //a standard roll.
      Die * next;//A pointer to the next DIE for linking lists.
     };
     //*********************************************
     Die * root;//Root pointer for lists of DIE
    };
    
    
    //**********************************************
    //*Methods**************************************
    //**********************************************
    Dice::Dice(char noDice...)//Variadic constructor
    {
     va_list ap;
     va_start(ap,noDice);
     char size;bool open;
     while(noDice-->0)
     {
      size=(char)va_arg(ap,int);open=(bool)va_arg(ap,int);
      this->Add(size,open);
     }
     va_end(ap);
    }
    Dice::~Dice(void)//Destructor.
    {
     Die * finger=root;Die * nullNext;
     while(!(finger==NULL)){nullNext=finger->next;delete finger;finger=nullNext;}
    }
    int Dice::Roll(void)//Overloaded to show what roll the DICE pool currently contains
    {
     Die * finger=root;
     int total=0;
     while(!(finger==NULL)){total+=finger->face;finger=finger->next;}
     return total;
    }
    void Dice::Roll(int times)//Rolls all DIE a certain number of times. Will eventually
    {                         //add ability to roll X times and take highest/lowest.
     Die * finger=root;
     for(int x=1;x<=times;x++){while(!(finger==NULL)){finger->roll();finger=finger->next;};};
    }
    void Dice::Add(char Size,bool Open=false)//Allows the addition of DIE after the constructor
    {
     Die * newDie;newDie = new Die;newDie->size=Size;newDie->open=Open;
     if(root==NULL){root=newDie;newDie->next=NULL;}
     else
     {
     Die * finger;finger=root;
     while(!(finger->next==NULL)){finger=finger->next;}
     finger->next = newDie;newDie->next=NULL;
     }
    }
    void Dice::Die::roll(void)
    {
     if(open)//If open go through the motions for an OE dice roll
     {
      int tempRoll=0,tempFace;tempFace=(rand()%size)+1;tempRoll+=tempFace;
      if(tempFace==size){do{tempFace=(rand()%size);tempRoll+=tempFace;}while(tempFace==size-1);};
      face=tempRoll;
     }
     else//Otherwise, just roll and report
     {
      face=(rand()%size)+1;
     }
    }
    I feel kind of dirty asking for help, knowing this is probably borderline RTFM, but what went wrong when I decided to move these out of inline? Yes, I know INLINE is a compiler hint and doesn't necessarily mean the same thing as "in the {} for the class," but I know that makes them INLINE by default.

    EDIT: I figured it out. Apparently #including and adding the same file to the project is a bad idea
    Last edited by tol; 07-28-2005 at 12:10 PM.

  8. #8
    I am me, who else?
    Join Date
    Oct 2002
    Posts
    250
    Hehe yeah that would do it, I couldn't tell since you didn't have your includes in there

    Also a design thing... you seem to like to put multiple statements on one line, you might wanna move them to separate lines, would make it easier to spot things... in this case however it wasn't the problem

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Extended ASCII Characters in an RTF Control
    By JustMax in forum C Programming
    Replies: 18
    Last Post: 04-03-2009, 08:20 PM
  2. Enforcing Machine Code Restrictions?
    By SMurf in forum Tech Board
    Replies: 21
    Last Post: 03-30-2009, 07:34 AM
  3. Obfuscated Code Contest
    By Stack Overflow in forum Contests Board
    Replies: 51
    Last Post: 01-21-2005, 04:17 PM
  4. Interface Question
    By smog890 in forum C Programming
    Replies: 11
    Last Post: 06-03-2002, 05:06 PM
  5. Replies: 0
    Last Post: 02-21-2002, 06:05 PM