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.

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

Code:

---

ID = NULL;
mainClass = NULL;
//...

---

NULL is for pointers (although in C++ it is defined as 0).

Quote:

---

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?

Quote:

---

Originally Posted by matsp

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?

Quote:

---

Originally Posted by anon

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

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

All these things produce compiler warnings, so they are not completely OK.

Quote:

---

well if it is the same as 0 its ok

---

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

Quote:

---

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.

Quote:

---

Originally Posted by anon

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.

Quote:

---

Originally Posted by anon

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.

Quote:

---

Originally Posted by ExDHaos

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!

Quote:

---

Originally Posted by iMalc

may God have mercy on your soul!

---

what? NO!, if that's the case then he deserves whats coming to him

Quote:

---

Originally Posted by iMalc

"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

Quote:

---

Originally Posted by matsp

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!

*facepalm*

Quote:

---

Originally Posted by iMalc

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

@grumpy: Declare it static const.