# Please Critique this class and code

This topic is 3446 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

HI, I am writing this class to act as a message buffer between subsystems regarding what should be loaded and unloaded. The problem is that I am not loading the entire level before runtime. rather, during runtime chunks of the file become pertinent or lose pertinency, and get loaded and unloaded accordingly. [update: forgot to mention that it is the objects within these chunks, the materials, sounds, rigid bodies etc, which go onto the list. The point to this is that the class is agnostic of all the subsystems in the application, instead relying on names to tell the objects apart. if two objects have the same name, it will choose between them based on name/type pair. In this case, erhaps I should use a std::map with std::pair for the index, rather than a std::vector?
#ifndef CLASS_ResourceShoppingList_h
#define CLASS_ResourceShoppingList_h

#include <string>
#include <vector>

namespace Resources
{
enum ResourceType
{
StaticMesh,
Material,
Sound,
Vehicle,
Character,
AnyType

};

enum ResourceState{
AnyState
};

class ShoppingListEntry
{
public:
ShoppingListEntry(ResourceType type, ResourceState state, const std::string& name)
:type(type),state(state),name(name){}
private:
ResourceType type;
ResourceState state;
std::string name;
};

typedef std::vector<ShoppingListEntry> t_entry_list;

class ResourceShoppingList
{
public:
ResourceShoppingList(){}
~ResourceShoppingList(){}

void pushDirective(const ShoppingListEntry& entry);
//feed one into the list

bool popDirective(ShoppingListEntry& entry);
//just get the top one

void getAllOf(ResourceType _type, ResourceState _state, t_entry_list& _entries);
//get list of all resources which match those properties

private:
t_entry_list items;
};
}


I'd also like to know your thoughts on how I coded this. If I were an employee, would you be satisfied or would I be getting my P45? I need peer review. Be as critical as you like, I have a thick skin. Thanks alot. [Edited by - speciesUnknown on August 14, 2008 12:04:22 AM]

Is it that bad?

##### Share on other sites
style: Names beginning with an _ can be reserved by the implementation in some cases, so I like to avoid them.
Quote:
 17.4.3.1.2 Global names [lib.global.names]1 Certain sets of names and function signatures are always reserved to the implementation: * Each name that contains a double underscore (_ _) or begins with an underscore followed by an upper case letter (2.11) is reserved to the implementation for any use. * Each name that begins with an underscore is reserved to the implementation for use as a name in the global namespace.
There's nothing wrong with doing this:
ShoppingListEntry(ResourceType type, ResourceState state, const std::string& name)				:type(type),state(state),name(name){}
Performance: _name and entry should be passed by const reference here:
ShoppingListEntry(ResourceType _type, ResourceState _state, std::string _name)
and here:
void pushDirective(ShoppingListEntry entry);

Function: Should the pop function return boolean in case the vector is empty?

Documentation: Are the comments for the push/pop functions reversed?
Does getAllOf remove the returned items from the member vector?

Typo: I assume that "std::vector<ShoppingListEntry>t_entry_list;" is supposed to be "t_entry_list entries;" ;)

##### Share on other sites
Quote:
Original post by Hodgman
style: Names beginning with an _ can be reserved by the implementation in some cases, so I like to avoid them.
Quote:
 17.4.3.1.2 Global names [lib.global.names]1 Certain sets of names and function signatures are always reserved to the implementation: * Each name that contains a double underscore (_ _) or begins with an underscore followed by an upper case letter (2.11) is reserved to the implementation for any use. * Each name that begins with an underscore is reserved to the implementation for use as a name in the global namespace.
There's nothing wrong with doing this:
ShoppingListEntry(ResourceType type, ResourceState state, const std::string& name)				:type(type),state(state),name(name){}
Performance: _name and entry should be passed by const reference here:
ShoppingListEntry(ResourceType _type, ResourceState _state, std::string _name)
and here:
void pushDirective(ShoppingListEntry entry);

Function: Should the pop function return boolean in case the vector is empty?

Documentation: Are the comments for the push/pop functions reversed?
Does getAllOf remove the returned items from the member vector?

Typo: I assume that "std::vector<ShoppingListEntry>t_entry_list;" is supposed to be "t_entry_list entries;" ;)

Thanks alot, I was considering taking it to a more specific place. Some changes have been made.

Regarding the typedef, im using that so I dont need to put the full container type declaration every time I use it, for example when writing an iterator. I do this for most types that use STL.
ahh, I see what you mean. I changed it to be sane. I'm puzzled as to why it allowed me to do that, naming a member after a typedef'd type.

##### Share on other sites
The functions:

ResourceShoppingList(){}
~ResourceShoppingList(){}

are superfluous - the compiler will generate them for you.

##### Share on other sites
A few remarks.

	enum ResourceType	{		// ...		AnyType	};
Note that AnyType is not a type of resource, since resources can't possibly be of 'any type'! So, you should remove this from your enumeration.

	enum ResourceState{		Load,		Unload,		AnyState	};
This sounds like the 'true', 'false', 'file_not_found' boolean definition. Again, if a resource can be either loaded or unloaded, use an 'is_loaded' boolean.

	class ShoppingListEntry	{		        // ...		private:			ResourceType type;			ResourceState state;			std::string name;	};
Not very useful, is it? You have an object but can't read anything about it. Since the ShoppingListEntry is quite obviously being used as a "record" data type, you can go ahead and make all its fields public and add an isValid() method that returns true (you can use this validation method later on if your semantics stop being "vector of properties" semantics). So, that's a struct with three members and a constructor. Don't forget to dump the default constructor here.

			ResourceShoppingList(){}			~ResourceShoppingList(){}
Never define empty constructors or destructors if they would have been generated the same by default.

			bool popDirective(ShoppingListEntry& entry);			//just get the top one
Why return a boolean? Why pass by reference (thereby forcing the user to pass a reference even if they just want to drop the entry?). In practice, one provides a 'pop' function which returns void and just drops the data, and a 'front' function which returns the front data without popping it. Both throw an exception if the list is empty (and you should, by the way, provide a function to determine if the list is empty!). Also, document your code: which directive, exactly, is the top one? The first one? The last one? A random one? Is this a stack or a queue or something else?

			void getAllOf(ResourceType _type, ResourceState _state, t_entry_list& _entries);			//get list of all resources which match those properties
First, a 'get' function which returns nothing is odd. You can either return a container (using RVO or NRVO to avoid a copy) or return a 'begin' input iterator (combined with an 'end' member function) that skips items which don't match the required predicates. As for the arguments, you don't want the user to force a choice. Therefore, I would suggest making those parameters optional: if they're provided, filter with them, otherwise don't. Optional, in this cased, would be implemented using overloading and forwarding all four filter combinations to a single private function.

Last but not least, as a general piece of advice, drop the warts. Those prefix underscores are not useful.

##### Share on other sites
Maybe you want to encapsulate your enums and the typedef vector in your resource class too, because those are only essential to this specific class. I would also use a global namespace for your project, so the Resources namespace does not clash with something else.

As my previouse poster stated the "_"-Parameters are better avoided. Instead you could use something like "InType, InState, InEntries" or the like.

h.

##### Share on other sites
Quote:
 Original post by hoLogrammAs my previouse poster stated the "_"-Parameters are better avoided. Instead you could use something like "InType, InState, InEntries" or the like.

The point is that adding a prefix (any prefix) is ugly and useless. If you need, for some reason, to separate your parameters from your other internal function variables, then you have a problem with code readability that would be best solved by refactoring, not by adding prefixes. Replacing '_' with 'In' doesn't really change anything.

In practice, I would give the arguments names that actually reflect what they are. So, the function signature would be
//! Return all the list elements that satisfy both filterssequence_type get_filtered(resource_type type_filter, bool loaded_filter) const;

##### Share on other sites
Lots of nice advice here, although some of it is conflicting.

Regarding the enums,

	enum ResourceType	{		StaticMesh,		Material,		Sound,		Vehicle,		Character,		AnyType		};	enum ResourceState{		Load,		Unload,		AnyState	};

This allows a search for all resources named charlie, for example, or all materials regardless of the directive to be performed. e.g.,
shopping_list.getAllOf(AnyType, Load, subset);orvoid getAllOf(Material, AnyState, subset);

I will change the name ResourceState to ResourceDirective, since that is what it is: load, unload, or either. The problem is that these enums are being used as search parameters as well. How can I deal with that duality?

Quote:
 Also, document your code: which directive, exactly, is the top one? The first one? The last one? A random one? Is this a stack or a queue or something else?

in that case, perhaps popFront() and pushBack() would be more appropriate names?

##### Share on other sites
Quote:
 Original post by speciesUnknownThis allows a search for all resources named charlie, for example, or all materials regardless of the directive to be performed.I will change the name ResourceState to ResourceDirective, since that is what it is: load, unload, or either. The problem is that these enums are being used as search parameters as well. How can I deal with that duality?
Duality is known as the "single responsibility principle violation", and is a very nasty thing to have. Give each of your types a single responsibility—in this case, either your ResourceDirective is to "load", or it is to "unload". You can't tell your resource loader to "either" your resource!

As I explained above, what you are trying to do is make the filters on your query optional. This can be achieved using an 'option' type:
// If you use boost :typedef boost::optional<ResourceDirective> directive_option;// Otherwise :typedef ResourceDirective *directive_option;

Then, hide this behind your overloaded functions. For instance, with pointers:
sequence_type get_all() const { return get(0, 0); }sequence_type get_filtered(ResourceDirective d) const { return get(0, &d); }sequence_type get_filtered(ResourceType t) const{ return get(&t, 0); }sequence_type get_filtered(ResourceType t, ResourceDirective d) const{ return get(&t, &d); }private: sequence_type get(type_option t, directive_option d) const { ... }

Or using boost option types, and avoding overloading:
static const directive_option any_directive;static const type_option any_type;sequence_type get_filtered(type_option t, directive_option d) const{ ... }

##### Share on other sites
Quote:
Original post by ToohrVyk
Quote:
 Original post by speciesUnknownThis allows a search for all resources named charlie, for example, or all materials regardless of the directive to be performed.I will change the name ResourceState to ResourceDirective, since that is what it is: load, unload, or either. The problem is that these enums are being used as search parameters as well. How can I deal with that duality?
Duality is known as the "single responsibility principle violation", and is a very nasty thing to have. Give each of your types a single responsibility—in this case, either your ResourceDirective is to "load", or it is to "unload". You can't tell your resource loader to "either" your resource!

As I explained above, what you are trying to do is make the filters on your query optional. This can be achieved using an 'option' type:
// If you use boost :typedef boost::optional<ResourceDirective> directive_option;// Otherwise :typedef ResourceDirective *directive_option;

Then, hide this behind your overloaded functions. For instance, with pointers:
*** Source Snippet Removed ***

Or using boost option types, and avoding overloading:
*** Source Snippet Removed ***

ahh, thats an excellent method, ill implement it now. Thanks alot.

##### Share on other sites
This may be personal preference but I dont like the use of 'typedef'. When I read a variable declaration using the typedef I immediately assume a custom made class is being used. And even if it is clear that you are using a typedef I still hve to scroll up to get the definition. Just a personal oppinion.

EDIT: I just saw your comment ontypedef in the original post but my oppinion still stands. templeted types are sometimes long and somewhat ugly optically but that doesnt excuse a solution such as typedef in my oppinion.

##### Share on other sites
Quote:
 Original post by CProgrammerWhen I read a variable declaration using the typedef I immediately assume a custom made class is being used.
The solution would be to stop assuming silly things.

Quote:
 And even if it is clear that you are using a typedef I still hve to scroll up to get the definition.
How is this different from any other type? Besides, the standard library uses a lot of typedefs (iterators, for instance, are often typedefs) yet this has never bothered me and (I suspect) neither has it bothered you.

##### Share on other sites
If its in the standard library then everyone knows the specific typedef, hence theres no problem. The problem only arises if everyone starts creating theirown typedefs which can make things confusing. Used in the right amount and the right way is ok I guess, but personally I prefer to just not have any typedefs. Obviously I don't think its a disaster if others do.

##### Share on other sites
Quote:
 Original post by CProgrammerI just saw your comment ontypedef in the original post but my oppinion still stands. templeted types are sometimes long and somewhat ugly optically but that doesnt excuse a solution such as typedef in my oppinion.

So I guess std::basic_string<charT, traits, Alloc> is so much clearer than using std::string?

Face it, this is partially what typedefs are for.

Quote:
 Original post by CProgrammerThe problem only arises if everyone starts creating theirown typedefs which can make things confusing.

Just do the same thing you always do when you encounter a variable of unknown type, seek out its declaration and class definition.