Need code optimization suggestions

[Netwatcher]

I just recently finished an Item class for a little project I'm working on.

Do you have any suggestions on how I can Optimize this code?
Do you see any common mistakes?


Item.h

Code: Select all

#ifndef _ITEM_H
#define _ITEM_H 1

#include <iostream>
#include <stdio.h>
#include <stdlib.h>
#include <vector>

#define Nofitems 10

//consider putting under namespace
class CItem
{
private:
	typedef struct
	{
		std::string name;
		std::string Description;
		char type;
		float AttSpeed;
		int AttDamage,DefArmor,RechargeTime;
	}ITEM;
	int ItemNum;
	std::vector<ITEM> items;

public:
	CItem();
	virtual ~CItem();


.... function defs....

extern CItem *G_Item;

#endif	

[short]

Where do you want a suggestion to improve? Do you believe anything is wrong?

Also, with all the comments it is kind of difficult to read through your code, maybe clean the unnecessary ones out if you get some time please/ty.

edit: I guess it's just the

Code: Select all

/*done&working*/

that is bothering me /shrug

[K-Bal]

You are making a class CItem which defines a struct ITEM and holds a std::vector of ITEMs? I don't know what you want to achieve but that sounds bad

[MarauderIIC]

Item.h

Code: Select all

#ifndef _ITEM_H
#define _ITEM_H 1

Can just be

Code: Select all

#ifndef _ITEM_H
#define _ITEM_H

Code: Select all

#include <iostream>
#include <cstdio> //use c++-compliant version of these libs
#include <cstdlib> //use c++-compilant version of these libs
#include <vector>
//I would really put "using namespace std;" here for code readability

//#define Nofitems 10
const int N_OF_ITEMS 10; //constants are uppercase; use const ints instead of #defines

//consider putting under namespace
class CItem
{
private:
	typedef struct
	{
		std::string name;
		std::string Description;
		char type;
		float AttSpeed;
		int AttDamage,DefArmor,RechargeTime;
	}ITEM;

Can't you just do

Code: Select all

class CItem {
private:
    struct ITEM {
        string name;
        string description;
        string type;
        float attSpeed;
        int attDamage, defArmor, rechargeTime;
    };
        int itemNum;
	vector<ITEM> items;

public:

I can't say that I've ever used structs in classes. Does it prevent the rest of your code from making items? Is that what you want? If not, make Item a class on its own.

You need to be consistent for capitalization. Typically, member variables are in "humpBack" form (and class names are "Capitalized", but you've got that).
You might wind up needing to do vector<Item*> items, if you store pointers to entries in the vector, because the vector is allowed to move around in memory when items are added or removed from it (which means your pointers would be invalid). Make your function parameter names lowercase, too.

There's not really any code that you provided that has opportunities for optimization.

[Falco Girgis]

How would one "optimize" code in a header file? There are no algorithms, just data structures. We also can't see how you're using the data structures, so there isn't a lot we can do to help.

[Netwatcher]

^
Why do I need to know if I've done anything wrong in the header, I know there are no algorithms, but that's what i USE to write the algorithms (which work, apparently),
I'm wondering if the struct is the best way to save the "Item" because I need something to save all of it's properties
and the struct and vector are doing it well.
(Compiles as a static library)


Mutators are so... (in pseudocode)

Code: Select all

	unsigned end = CItem::items.size();
	for(unsigned f=0;!CItem::items.empty();++f)
	{
		if(f<end)
        {
		if(CItem::items[f].name == Item_Name)
		{
			CItem::items[f].<property> = <value of property given to the function>;
			break;
		}
        }
	}
	

Accessors are so... (GetItemByName/Element follows the same code as above mostly, it just returns an ITEM)

Code: Select all

return CItem::GetItemByName(Item_Name).<property>;

And/or

Code: Select all

return CItem::GetItemByElement(Item_ID).<property>;

and example for use of this class is...

Code: Select all

#include "Items.h"
#include <iostream>
int main()
{
	CItem *G_Item;
	G_Item = new CItem;
	G_Item->CreateItem("Sword of the unfrogotten");
	G_Item->CreateItem("Misha the unforgotten");
	std::string d ="Sword of the unfrogotten";
	std::cout<<"Item:   "<<G_Item->GetItemByElement(2).name.c_str()<<"  \n";
	G_Item->SetItemDescription(d,"It sucks like shit\n");
	std::cout<<"Description:   "<<G_Item->GetItemDescription(d).c_str();
	
	G_Item->CreateItemFS("MOM","LOVELY",5.5f,500,400,30,NULL);
	std::cout<<"\n\n\n\n"<<"New Item: "<<G_Item->GetItemDescription("MOM").c_str()<<std::endl;
	system("PAUSE");
}

[dandymcgee]

I guess I have a similar question as Mar. Why are you encapsulating all of your CItem's properties inside of a struct? Why not just put them directly in the CItem class's data members? You are aware that a struct is the exact same structure as a class (exception: default public instead of private)? This is almost like defining a class withing a class, which doesn't seem to be helping you achieve anything. If you still want to encapsulate all of the properties I'd recommend defining a whole separate class in another header (for instance, "CItemProperties") and then just putting that into your class:

Code: Select all

class CItem
{
    private:
       CItemProperties properties;
       int itemNum;
       std::vector<ITEM> items;
    public:
       CItem();
       virtual ~CItem();
};

Dunno..

[Falco Girgis]

I stand by what I said. If we don't know how you are going to be using/accessing/manipulating your data structures, then how are we supposed to know if they are okay?

And "optimization" refers to processor time. O(anything) isn't happening in a header. There is no possible optimization to be done.

You are getting nothing but questions to go with your question, because we don't have enough information to work with.

[zodiac976]

A class within a class would be better than a struct inside a class
in my opinion but I am not of fond doing stuff like that.

I would just make a new class, use inheritance and use protected
variables instead of private so you don't have to use friends if you
need that particular class to use the other classes variables.

*I agree with GyroVorbis though .

[MarauderIIC]

^

Code: Select all

	unsigned end = CItem::items.size();
	for(unsigned f=0;!CItem::items.empty();++f)
	{
		if(f<end)
        {
		if(CItem::items[f].name == Item_Name)
		{
			CItem::items[f].<property> = <value of property given to the function>;
			break;
		}
        }
	}
	

You'll want to use iterators, here. They're faster.

Code: Select all

[code]for (vector<ITEM>::iterator item = items.begin(); item != items.end(); ++item) {
    if (item->name == itemName) {
        item->property = property;
        return SUCCESS;
    }
    return FAILURE;
}

dandy: the point of the encapsulation in NetWatcher's code is, looking at the use, so that only the item container (CItem) can make items. This enforces a constraint of "every item created must be in the item container", since the struct cannot be accessed outside of the class (since it's in the private section). Since this is the case, NetWatcher, it looks like you might want to make CItem follow a singleton pattern, or implement CItem inside your Game class. It wouldn't make much sense to me to enforce items to be in a container while also allowing more than one container.

[dandymcgee]

dandy: the point of the encapsulation in NetWatcher's code is, looking at the use, so that only the item container (CItem) can make items. This enforces a constraint of "every item created must be in the item container", since the struct cannot be accessed outside of the class (since it's in the private section). Since this is the case, NetWatcher, it looks like you might want to make CItem follow a singleton pattern, or implement CItem inside your Game class. It wouldn't make much sense to me to enforce items to be in a container while also allowing more than one container.

That makes perfect sense. I wasn't thinking of CItem as a container, but rather a single item with the struct simply storing a single item's properties. But if CItem is a container and the internal struct is defining the individual items within the container then the implementation is a bit more logical.

I've always wondered about the "C" in "CItem" habit (seen it a few other places), I'm guessing this is a general way of saying something is a container?

[Joeyotrevor]

I've always wondered about the "C" in "CItem" habit (seen it a few other places), I'm guessing this is a general way of saying something is a container?

Nah, it's just in case they forget it is a class. IMO it's kinda stupid.

[programmerinprogress]

Nah, it's just in case they forget it is a class. IMO it's kinda stupid.

yeah, and I find using the m prefix to explain datamembers to be a pretty soul destroying practice too, but I guess it's good to get into a consitent style for the sake of clarity...

[Netwatcher]

dandy: the point of the encapsulation in NetWatcher's code is, looking at the use, so that only the item container (CItem) can make items. This enforces a constraint of "every item created must be in the item container", since the struct cannot be accessed outside of the class (since it's in the private section). Since this is the case, NetWatcher, it looks like you might want to make CItem follow a singleton pattern, or implement CItem inside your Game class. It wouldn't make much sense to me to enforce items to be in a container while also allowing more than one container.

That makes perfect sense. I wasn't thinking of CItem as a container, but rather a single item with the struct simply storing a single item's properties. But if CItem is a container and the internal struct is defining the individual items within the container then the implementation is a bit more logical.

I've always wondered about the "C" in "CItem" habit (seen it a few other places), I'm guessing this is a general way of saying something is a container?

Container? waa.... no... just in case I want to use Item for something else...(C-class)

[dandymcgee]

I've always wondered about the "C" in "CItem" habit (seen it a few other places), I'm guessing this is a general way of saying something is a container?

Nah, it's just in case they forget it is a class. IMO it's kinda stupid.

Wow seriously? That's like starting variable names with "i", "f", and "d"...