Is it good?

This is a discussion on Is it good? within the C++ Programming forums, part of the General Programming Boards category; Code: #include <string> #include <vector> using std::string; using std::vector; using std::cout; using std::endl; #define MAX_ITEMS 100 #define MAX_INV 30 class ...

  1. #1
    Registered User
    Join Date
    May 2009
    Posts
    84

    Is it good?

    Code:
    #include <string>
    #include <vector>
    using std::string;
    using std::vector;
    using std::cout;
    using std::endl;
    
    #define MAX_ITEMS 100
    #define MAX_INV 30
    
    class Items
    {
    		int ID;
    		int mainClass;
    		int subClass;
    		int grade;
    		int quality;
    		int degree;
    		int bonding;
    		int price;
    		int stackSize;
    		int primDmg[3];
    		int sconDmg[3];
    		int resist[3];
    		int bnsOpt[10];
    	    string name;
    	    string desc;
    		double attSpeed;
    		double defSpeed;
    
    public:
    
    	Items(){
    		ID = NULL; 
    		mainClass = NULL;
    		grade = NULL;
    		quality = NULL;
    		degree = NULL;
    		bonding = NULL;
    		price = NULL;
    		attSpeed = NULL;
    		defSpeed = NULL;}
    
    	void ItemsInfo (int A, int B, int C, int D, int E, int F, int G, int H, int I, int J, int K,
    		int L, int M, int N, int O, int P, int Q, int R, int S, int T, int U, int V, int W, int X,
    		int Y, int Z, int AA, int AB, string AC, string AD, double AE, double AF)
    	    {
    		    ID = A;
    		    mainClass = B;
    		    subClass = C;
    		    grade = D;
    		    quality = E;
    		    degree = F;
    		    bonding = G;
    		    price = H;
    		    stackSize = I;
    		    primDmg[0] = J;  // first element directly adds to the needed stat
    		    primDmg[1] = K;
    		    primDmg[2] = L;
    		    sconDmg[0] = M;  // first element directly adds to the needed stat
    		    sconDmg[1] = N;
    		    sconDmg[2] = O;
    		    resist[0] = P;
    		    resist[1] = Q;
    	    	resist[2] = E;
    	    	bnsOpt[0] = S;       //each array here is directly added to the player stats
    	    	bnsOpt[1] = T;
    	    	bnsOpt[2] = S;
    	    	bnsOpt[3] = U;
    	    	bnsOpt[4] = V;
    	    	bnsOpt[5] = W;
    	    	bnsOpt[6] = Y;
    	    	bnsOpt[7] = Z;
    	    	bnsOpt[8] = AA;
    	    	bnsOpt[9] = AB;
    	        name = AC;
    	        desc = AD;
    		    attSpeed = AE;
    		    defSpeed = AF;};
    
    		double Quality()
    		{
    			if (quality==0) return 0.7;
    			if (quality==1) return 1.0;
    			if (quality==2) return 1.3;
    			if (quality==3) return 1.7;
    			if (quality==4) return 2.1;
    			if (quality==5) return 2.6;
    			if (quality==6) return 3.1;}
    
    		double Grade()
    		{
    			if (grade==0) return 1.0;
    			if (grade==1) return 1.1;
    			if (grade==2) return 1.2;
    			if (grade==3) return 1.4;
    			if (grade==4) return 1.6;
    			if (grade==5) return 2.0;
    			if (grade==6) return 2.4;
    		    if (grade==7) return 3.2;}
    
    		double Degree()
    		{
    			if (degree==0) return 1.0;
    			if (degree==1) return 1.1;
    			if (degree==2) return 1.2;
    			if (degree==3) return 1.4;
    			if (degree==4) return 1.6;
    			if (degree==5) return 1.9;
    			if (degree==6) return 2.2;
    		    if (degree==7) return 2.6;
    			if (degree==8) return 3.0;
    			if (degree==9) return 3.5;
    			if (degree==10) return 4.0;
    			if (degree==11) return 4.6;
    			if (degree==12) return 5.2;
    			if (degree==13) return 5.9;
    			if (degree==14) return 6.6;}
    
    		int ArmRes(){return (resist[0]*(int)Quality())*(int)Grade();}
    		int EleRes(){return (resist[1]*(int)Quality())*(int)Grade();}
    		int MagRes(){return (resist[2]*(int)Quality())*(int)Grade();}
    
    		int MinDmg(){return (primDmg[1]*(int)Quality())*(int)Grade();}
    		int MaxDmg(){return (primDmg[2]*(int)Quality())*(int)Grade();}
    		int Min2Dmg(){return (primDmg[1]*(int)Quality())*(int)Grade();}
    		int Max2Dmg(){return (primDmg[2]*(int)Quality())*(int)Grade();}
    
    		void showOpts()
    		{
    			string type;
    			for (int iii=0; iii!=10; iii++)
    				if (bnsOpt[iii]!=0)
    				{
    					if (iii==0)
    						type = "DEX";
    					else if (iii==1)
    						type = "STR";
    					else continue;
    					
    					cout << "Adds" << bnsOpt[iii] << type << endl;} }
    
    		void descrWin()
    		{
    			/*showName();
    			showInf1();
    			showDesc();
    			showStts();*/
    			showOpts();
    			/*showInf2();*/}
    
    
    }itemsDB[MAX_ITEMS], charInv[MAX_INV], charEqup[10];
    Well the good think it works fine, but im still wondering if i can improve it somehow, any suggestion? but don't mention the argu's.

  2. #2
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Definitely, having 20+ arguments to a function is BAD. Not naming them something sensible makes it DOUBLY bad.

    Since some of those are arrays, why not pass in an array/vector instead?

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  3. #3
    The larch
    Join Date
    May 2006
    Posts
    3,573
    Code:
    ID = NULL; 
    mainClass = NULL;
    //...
    NULL is for pointers (although in C++ it is defined as 0).

    but don't mention the argu's.
    My compiler says that parameters R and X are unused.

    Code:
    		double Grade()
    		{
    			if (grade==0) return 1.0;
    			if (grade==1) return 1.1;
    			if (grade==2) return 1.2;
    			if (grade==3) return 1.4;
    			if (grade==4) return 1.6;
    			if (grade==5) return 2.0;
    			if (grade==6) return 2.4;
    		    if (grade==7) return 3.2;}
    This and similar functions don't handle the case where grade is not one of the values. (assert / throw an exception / return some default value like 0.0)

    Code:
    int ArmRes(){return (resist[0]*(int)Quality())*(int)Grade();}
    In this and similar functions. Why make Quality and Grade return a double in the first place, if you are always truncating it to int before using? Perhaps the intention was to make the calculation with doubles, and only cast the final result?
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  4. #4
    Registered User
    Join Date
    May 2009
    Posts
    84
    Quote Originally Posted by matsp View Post
    Definitely, having 20+ arguments to a function is BAD. Not naming them something sensible makes it DOUBLY bad.

    Since some of those are arrays, why not pass in an array/vector instead?

    --
    Mats
    well i did something like that in the past but i didn't like the fact that i had to use multiple functions for one object, and is it the only thing i should improve?

  5. #5
    Registered User
    Join Date
    May 2009
    Posts
    84
    Quote Originally Posted by anon View Post
    Code:
    ID = NULL; 
    mainClass = NULL;
    //...
    NULL is for pointers (although in C++ it is defined as 0).



    My compiler says that parameters R and X are unused.

    Code:
    		double Grade()
    		{
    			if (grade==0) return 1.0;
    			if (grade==1) return 1.1;
    			if (grade==2) return 1.2;
    			if (grade==3) return 1.4;
    			if (grade==4) return 1.6;
    			if (grade==5) return 2.0;
    			if (grade==6) return 2.4;
    		    if (grade==7) return 3.2;}
    This and similar functions don't handle the case where grade is not one of the values. (assert / throw an exception / return some default value like 0.0)

    Code:
    int ArmRes(){return (resist[0]*(int)Quality())*(int)Grade();}
    In this and similar functions. Why make Quality and Grade return a double in the first place, if you are always truncating it to int before using? Perhaps the intention was to make the calculation with doubles, and only cast the final result?
    well if it is the same as 0 its ok

    oh, then maybe its just some little typo i had

    the value is not user-input, its a fixed value, so theres no need to do the assert IMO

    the double value id for some other purpose, in that case i need a whole number

  6. #6
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Anon gives a few other items that you should improve.

    My point is that I have worked with functions that take 8-10 parameters (Win32 CreateProcess, StretchBitBlt for example), and those are hard to work with, when it comes to keeping track of which parameter is which and what to pass. This function has over 30 parameters. None of which are called anything remotely meaningful.

    I'd say, generally, if a function takes more than about 5 arguments, it's likely good to either split the function, or pass the arguments in groups (struct, vector, array) so that the number of things to pass are fewer.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  7. #7
    The larch
    Join Date
    May 2006
    Posts
    3,573
    All these things produce compiler warnings, so they are not completely OK.

    well if it is the same as 0 its ok
    It takes more typing and it doesn't mean the same thing semantically.

    the value is not user-input, its a fixed value, so theres no need to do the assert IMO
    The value comes from the outside and is not checked anywhere. Asserts like this help you catch accidental errors when your mind wonders.
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  8. #8
    Registered User
    Join Date
    May 2009
    Posts
    84
    Quote Originally Posted by anon View Post
    All these things produce compiler warnings, so they are not completely OK.



    It takes more typing and it doesn't mean the same thing semantically.



    The value comes from the outside and is not checked anywhere. Asserts like this help you catch accidental errors when your mind wonders.
    well i think i will change it to 0 then (tho i always loved the NULL keyword)

    hmm i guess ill use it then, it makes more sense, well thanks for the help, I really appreciate it.

  9. #9
    Registered User
    Join Date
    Jun 2005
    Posts
    5,856
    Quote Originally Posted by anon View Post
    Code:
    		double Grade()
    		{
    			if (grade==0) return 1.0;
    			if (grade==1) return 1.1;
    			if (grade==2) return 1.2;
    			if (grade==3) return 1.4;
    			if (grade==4) return 1.6;
    			if (grade==5) return 2.0;
    			if (grade==6) return 2.4;
    		    if (grade==7) return 3.2;}
    This and similar functions don't handle the case where grade is not one of the values. (assert / throw an exception / return some default value like 0.0)
    An easier way would be to have the function that sets grade enforce the condition (grade >= 0 && grade <= 7). That allows Grade() to assume the values are in range. Or, in other words, enforce your class invariants in setters rather than getters.

    The philosophy is to detect an error as early as possible - and then either report an error, ignore it, recover from it - as early as possible. Practically, in many circumstances, the earliest point where that is possible is when changing a value, not retrieving it.

    A simpler implementation of Grades() - again assuming the error check is done elsewhere - is
    Code:
    double Grade() const
    {
         const double data[] = {1.0, 1.1, 1.2, 1.4, 1.6, 2.0, 2.4, 3.2};
         return data[grade];
    }
    Since the function doesn't actually change the object it acts on, it is appropriate that it be const.
    Last edited by grumpy; 05-22-2009 at 05:59 PM.
    Right 98% of the time, and don't care about the other 3%.

  10. #10
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,262
    Quote Originally Posted by ExDHaos View Post
    Well the good think it works fine, but im still wondering if i can improve it somehow, any suggestion? but don't mention the argu's.
    "You can't polish a turd!"

    If you don't want to fix the fact that your function has 32 OBFUSCATINGLY NAMED ARGUMENTS then may God have mercy on your soul!
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  11. #11
    Banned ಠ_ಠ's Avatar
    Join Date
    Mar 2009
    Posts
    687
    Quote Originally Posted by iMalc View Post
    may God have mercy on your soul!
    what? NO!, if that's the case then he deserves whats coming to him
    ╔╗╔══╦╗
    ║║║╔╗║║
    ║╚╣╚╝║╚╗
    ╚═╩══╩═╝

  12. #12
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by iMalc View Post
    "You can't polish a turd!"
    Going completely off-topic here, but the US TV program(me) Mythbusters showed that you can. I think Lion poo was one of the better ones to work on... [the principle is one of a japanese art of polishing balls of mud/clay or some such].

    The fact that a turd MAY be polished to a gloss doesn't fix the problems with too many arguments tho'.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  13. #13
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,262
    Quote Originally Posted by matsp View Post
    Going completely off-topic here, but the US TV program(me) Mythbusters showed that you can. I think Lion poo was one of the better ones to work on... [the principle is one of a japanese art of polishing balls of mud/clay or some such].

    The fact that a turd MAY be polished to a gloss doesn't fix the problems with too many arguments tho'.

    --
    Mats
    Interesting, LOL!!!
    I didn't see that episode!

  14. #14
    int x = *((int *) NULL); Cactus_Hugger's Avatar
    Join Date
    Jul 2003
    Location
    Banks of the River Styx
    Posts
    902
    *facepalm*
    long time; /* know C? */
    Unprecedented performance: Nothing ever ran this slow before.
    Any sufficiently advanced bug is indistinguishable from a feature.
    Real Programmers confuse Halloween and Christmas, because dec 25 == oct 31.
    The best way to accelerate an IBM is at 9.8 m/s/s.
    recursion (re - cur' - zhun) n. 1. (see recursion)

  15. #15
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by iMalc View Post
    Interesting, LOL!!!
    I didn't see that episode!
    It appears to be on Youtube here:
    YouTube - MYTHBUSTERS are going to Polish ........ they have gone nuts
    I haven't looked through it.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. In a game Engine...
    By Shamino in forum Game Programming
    Replies: 28
    Last Post: 02-19-2006, 10:30 AM
  2. Good books for learning WIN32 API
    By Junior89 in forum Windows Programming
    Replies: 6
    Last Post: 01-05-2006, 04:38 PM
  3. Good resources for maths and electronics
    By nickname_changed in forum A Brief History of Cprogramming.com
    Replies: 8
    Last Post: 12-22-2004, 03:23 PM
  4. what is good for gaphics in dos
    By datainjector in forum Game Programming
    Replies: 2
    Last Post: 07-15-2002, 03:48 PM
  5. i need links to good windows tuts...
    By Jackmar in forum Windows Programming
    Replies: 3
    Last Post: 05-18-2002, 11:16 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21