is this code is good?

This is a discussion on is this code is good? within the C++ Programming forums, part of the General Programming Boards category; Code: void AddItem(int A) //the arug' the amount you want to add { int B; //used to contain the subscribed ...

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

    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)

  2. #2
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    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
    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
    Registered User
    Join Date
    May 2009
    Posts
    84
    Quote Originally Posted by matsp View Post
    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

  4. #4
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    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
    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.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 23
    Last Post: 04-20-2009, 08:35 AM
  2. Writing Code
    By ILoveVectors in forum C++ Programming
    Replies: 4
    Last Post: 06-13-2005, 01:27 AM
  3. Any good places for free source code?
    By VegasSte in forum A Brief History of Cprogramming.com
    Replies: 8
    Last Post: 11-19-2002, 04:22 AM
  4. << !! Posting Code? Read this First !! >>
    By kermi3 in forum Linux Programming
    Replies: 0
    Last Post: 10-14-2002, 02:30 PM
  5. Replies: 4
    Last Post: 01-16-2002, 12:04 AM

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