is this code is good?

Code:

	void AddItem(int A)                                  //the arug' the amount you want to add
	{
		int B;                                           //used to contain the subscribed amount
        amount += A;                                     //adds to the amount

		if( amount > stackSize )                         //if the amount is greater then the stacksize
		{
			B = amount - stackSize;                      //it subscribes stackSize from the amount to find out by how much greater
			amount -= B;                                 //and then it subscribes the subscribed amount from the amount
			GetItem(B);                                  //which after then it takes the subscribed amount and calls and calls GetItem so itll add the subscribes amount to a new slot
		}
	}

    void GetItem(int A)                                   //the arug' indicates the amount of the item
	{
		for( int iii=0; iii != MAX_INV; iii++)            //a "for" loop to travel around the slots(arrays)

			if( charInv[iii].id == 0 )                    //if it finds an empy slot(array with NULL id)
			{ 
				charInv[iii] = *this;                     //it copies the item into the slot (representing the item with "*this")
				charInv[iii].additem(A);                  //and the addd the amount of the item
				break;                                    //which after this it breaks the loop so it won't add the same item multiple items
			}                                             //otherwise if the slot isn't empty itll start looking for another slot

			else if( charInv[iii].id == id)               //if it finds slot with the same item(same id)
				if( charInv[iii].amount < stackSize)      //if theres still space left in the stackSize(the amount is less then the stacksize)
				{ 
					charInv[iii].AddItem(A);              //it adds the value to the item(by calling AddItem)
					break;                                //which after this it breaks the loop so it won't add the same item multiple items
				}                                         //otherwise if theres no space left in stackSize itll look for another free slot
	}

is this code is good to follow the comments, is there any possible flaws? (like breaking from the fixed loop of calling and stuf ehich may cause problems)

I have no idea what the purpose of this code is.

What if all slots are full up.

You call addItem twice. You could probably do something like this:

Writing a comment for every line is often "too much". Certainly saying something like "for-loop to walk through the array" - well, doh!, it's generally what for-loops do. Comments aren't there to explain what the code itself means, but the overall idea of the code.

I would also add teh rest of the braces, even if they aren't strictly needed.

I also wonder if you should ALWAYS add new elements if there is a zero-id, even is element of that ID with a value in it.

You use both addItem and AddItem - I presume there is only one variant of the name.

Here's something like what you do, but with error checking and only ONE line that does AddItemI().

Code:

void GetItem(int A)
{
    for( int iii=0; iii != MAX_INV; iii++)
    {
	int tmpid = charInv[iii];
	if( tmpid == 0 || (tmpid == id && charInv[iii].amount < stackSize))
	{ 
	    break;
	}
    }

    if (iii != MAX_INV)
    {
	if (charInv[iii] == 0)
	{
	    charInv[iii] = *this; 
	}
	charInv[iii].Additem(A);
    }
    else
    {
	// Handle error : no slots free. 
    }
}

--
Mats

Originally Posted by matsp

I have no idea what the purpose of this code is.

What if all slots are full up.

You call addItem twice. You could probably do something like this:

Writing a comment for every line is often "too much". Certainly saying something like "for-loop to walk through the array" - well, doh!, it's generally what for-loops do. Comments aren't there to explain what the code itself means, but the overall idea of the code.

I would also add teh rest of the braces, even if they aren't strictly needed.

I also wonder if you should ALWAYS add new elements if there is a zero-id, even is element of that ID with a value in it.

You use both addItem and AddItem - I presume there is only one variant of the name.

Here's something like what you do, but with error checking and only ONE line that does AddItemI().

Code:

void GetItem(int A)
{
    for( int iii=0; iii != MAX_INV; iii++)
    {
	int tmpid = charInv[iii];
	if( tmpid == 0 || (tmpid == id && charInv[iii].amount < stackSize))
	{ 
	    break;
	}
    }

    if (iii != MAX_INV)
    {
	if (charInv[iii] == 0)
	{
	    charInv[iii] = *this; 
	}
	charInv[iii].Additem(A);
    }
    else
    {
	// Handle error : no slots free. 
    }
}

--
Mats

the purpose is just to add item to inventory

oh yeah, thanks for remaining, i was planning to write the code for that but i guess i forgot

Code:

		for( int iii=0; iii <= MAX_INV; iii++)
			if ( iii == MAX_INV)
			{
				cout << "The inventory is full" << endl;
				break;
			}

is calling the same function of separate objects is bad?

i just got carried away with writing the comments(first time writing comments)

what braces?

about the zero id, there's no such item with zero id, each has his own id, so its impossible for a zero id(only if i was daydreaming while coding but even after that i can run a program that checks if there no zero id)

the addItem is just a typo

Starting from the back: Yes, I thought the addItem was a typo - which means that you have certainly not tested the code before you posted it. Which means that it may have all sorts of other problems too. Code-review is better done when you have code that YOU think is working.

Braces: If an inner block has braces, then I put braces on the outer block too. It avoid problems like this:

Code:

if (x) 
    if (y) 
    {
    ...
    }
    else if (z) 
        ... 
else
   // meaning to do something if x wasn't set.

Well, doh, someone didn't look at it properly, and the else is actually done when z is not set.

If there is a way to write code so that it reduces the likelyhood of making mistakes, use it. You may think that you don't make mistakes like that, but we had a case recently where it took 6 months to find a fault like this:

Code:

   if (x) ;
   {
       // Code to be done if x is true ... 
   }

Mistakes happen. Making them less likely to happen by using strict rules, then you help yourself write better code!

And finally, any code-duplication is bad. Since you had ALMOST the same code in the two cases, I thought it was easier to move it out of the loop - it also makes the loop itself simpler, which is NEVER a bad thing.

Finally, I prefer to go to test the case of "did we not find anything" after the loop, rather than inside it. Particularly: Intentionally going one beyond the intended loop, and then checking that inside the loop is bad, for two reasons:
1. It makes the loop look "wrong" (experienced programmers look specifically for a <= instead of < in for-loops to spot potential buffer overflow problems)
2. You have more code inside the loop. The if-statement is only going to be true if there was no space in the loop, so only on the last iteration of the loop [which is actually one iteration more than what you need to perform]. So if MAX is 1000, you test a value that you KNOW can't be true 1000 times, only to find it true the 1001st time. Modern processors do not like jumping/branching, so adding more if-statements in the loop will definitely not help it run faster. Whilst each of these arguments range from "trivial" to "nearly premature optimization", I still think the sum of all the arguments against this idea, and the lack of any real "for" argument says that it's a bad idea.

--
Mats