Sign in to follow this  
speciesUnknown

Please Critique this class and code

Recommended Posts

speciesUnknown    527
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{
		Load,
		Unload,
		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]

Share this post


Link to post
Share on other sites
Hodgman    51234
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 this post


Link to post
Share on other sites
speciesUnknown    527
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 this post


Link to post
Share on other sites
Nitage    1107
The functions:

ResourceShoppingList(){}
~ResourceShoppingList(){}

are superfluous - the compiler will generate them for you.

Share this post


Link to post
Share on other sites
ToohrVyk    1595
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 this post


Link to post
Share on other sites
hoLogramm    170
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 this post


Link to post
Share on other sites
ToohrVyk    1595
Quote:
Original post by hoLogramm
As 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 filters
sequence_type get_filtered(resource_type type_filter, bool loaded_filter) const;

Share this post


Link to post
Share on other sites
speciesUnknown    527
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);
or
void 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 this post


Link to post
Share on other sites
ToohrVyk    1595
Quote:
Original post by speciesUnknown
This 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 this post


Link to post
Share on other sites
speciesUnknown    527
Quote:
Original post by ToohrVyk
Quote:
Original post by speciesUnknown
This 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 this post


Link to post
Share on other sites
CProgrammer    303
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 this post


Link to post
Share on other sites
ToohrVyk    1595
Quote:
Original post by CProgrammer
When 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 this post


Link to post
Share on other sites
CProgrammer    303
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 this post


Link to post
Share on other sites
fpsgamer    856
Quote:
Original post by CProgrammer
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.


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 CProgrammer
The 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.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this