good habit or bad habit?

Started by
8 comments, last by ekrax 19 years, 7 months ago
hello again, i just have a few questions related to style that i want to clear up becasue i dont want to develope any bad habits ... 1) if i only want 1 instance of a class (ie. i have a resourceManger or something) should i do this ... class { // ... } resourceManager; or should i declare everything static withen the class? 2) is it considered bad habit to do something along these lines? class something { public: void foo(); }; class biggerClass { public: something getSomething(); } then i do this later on in my code biggerClass stuff; stuff.getSomething().foo() is it bad coding to do this, call a function withen returns a class and then use this to call a function withen that returned class? thanks for any suggestions in advance.
Advertisement
I think what you want is a singleton.

class Thingy{public:    static Thingy * Create( );    static void Release( );protected:private:    Thingy( );    Thingy( const Thingy & rhs );    ~Thingy( );    static Thingy * s_pInstance;    static unsigned int s_uiCount;};


First notice that the constructors and destructor are private. That means that only this object can instantiate itself. That might make someone ask "Well then how do I get an instance of the object?" Glad I asked!

The Create( ) function is just for that. Notice this function must be static. That way we don't need an instance to call the function. Now then...lets look at what the .cpp might have.

Thingy * Thingy::s_pInstance = NULL;unsigned int Thingy::s_uiCount = 0;Thingy * Thingy::Create( ){    if( s_pInstance == NULL )    {        s_pInstance = new Thingy;    }    ++s_uiCount;    return s_pInstance;}void Thingy::Release( ){    if( --s_uiCount == 0 )    {        delete s_pInstance;        s_pInstance = NULL;    }}


Lets first look at the function Create( ). It begins by checking if s_pInstance is NULL. Unfortunatly static initialization isn't garunteed at any specific time so I can't say that the line "Thingy * Thingy::s_pInstance = NULL;" ever executed. Dealing with this might be a bit beyond our scope right now, so I'll move on. Anyway...if we don't have an instance yet, it creates one. Any time later when we come to this part of code we will have an instance and thus there is only ever one.

We also keep track of how many times Create( ) was called with s_uiCount. I'll explain why in a bit.

The next function we have is Release( ). It begins by decreasing our s_uiCount. The reason why is...sooner or later we have to make sure we delete our instance. Otherwise we leak memory. That number keeps track of how many people requested that our singleton be created and released...if everyone cleaned up after themselfs we should be able to delete when s_uiCount = 0. Thus that is exactly what the code does.

In multithreaded apps you will want to insert locks and the such.

You mentioned having a bigger class. The problem is then that bigger class needs to only have one instance, thus somewhere down the line you need a singleton.

There are several ways that this singleton code can be cleaned up but it should give you a general idea.
Heh, not that I'm perhaps the ideal person to answer this...

1) the first method is what I use, and is probably sufficient. Others will suggest using something more complex that has the class create itself should it ever be accessed [and the instance isn't already created]. For a beginner, I think that's a little complex. Even for non-beginner, I think that it's overkill for a little resource manager [but I'm likely wrong]

2) Probably? Or rather, it's probably inefficient design to make the classes so dependant upon one another. It's likely frowned upon as far as code clarity goes too.

But then again I do absolutely horrendous things with anonymous objects [not sure if that's the proper name]:

(new vine(KEYALIAS,new keyalias(KB_F,0,new guiaction(dofarm,new vine(UNIT,cinv->data),0))))->insert(&nkb->aliases);


But I can almost guarantee that line of code will cause some people out there to gouge out their eyes, and then hunt me down and do something nasty... So umm, perhaps it's best not to get into the habit. :D
Indeed that line of code is ugly. Have you heard of exceptions? They are quite nice.

Each platform implements them a little differently and they are garunteed to slow your program down a tiny bit. For this reason several game devs attept to avoid them. I think they are good. There is a reason why every language invented within the last 20 years uses them. Anyway...let me explain how they work.

Suppose I have the function CreateWindow( ). I may want to pass a few parameters like how wide and how tall that window should be. The function should return a pointer to a window. This design is nice and simple...

return = Function( params );


Easy to understand. Nice and clean.

Now say CreateWindow( ) could have some errors. Perhaps we are running in a command prompt and I can't create a window. I could change the function to look like:

error_code = Function( & return, params );


This is a little lame because some functions might not return an error_code (IE the function is garunteed to work) or there wouldn't have been a return value anyway...so how do I know if the first param is really the return value or a param?

Another way is to keep our clean method and have errors stored elsewhere. For example:

return = Function( params );error_code = GetLastError( );// interpret error_code here to check if it errored


The problem with this is that 1.) You must have almost all parts of the problem include code for SetLastError( ), and 2.) You will have about 30 lines of error handling code for every function call. This makes your code almost impossible to read or understand.

So while these do address the problem...they aren't very nice. This is where exceptions come in. The idea is CreateWindow( ) is intended to create windows...that is the rule. Anything else is an exception to the rule. Imagine this:

try{    return = Function( params );    // other code here...it is all in one place so it is easy to read} catch( ... ) {    // somewhere in our try block an exception was thrown. Handle it here.}


The advantages to this are numerous. First my code is all located in the same area. Thus it is easy to read and understand. Also the ... is for a generic exception. I could handle specific exceptions like this:

} catch( memory_exception ) {   // some memory error} catch( window_excpetion ) {   // some window error} catch( ... ) {   // some exception...wasn't a memory or window error}


This helps structure your error handling into more logical units. Again...clean, easy to read.

Someone may ask "Where did memory_exception and window_exception come from?" Alright! I'm a smart question asker.

Your code can throw any type of exception. Most of the time people do something like this:

class myException{};class memory_exception : public myException{};class window_exception : public myException{};


Thus they are building their own exceptions. They could contain information about what caused the error or maybe other important recovory information. Anyway the idea is you can throw anything.

Okay...that was a very LONG introduction to exceptions...anyway now to address why I brought it up.

Telastyn had the line of code:
(new vine(KEYALIAS,new keyalias(KB_F,0,new guiaction(dofarm,new vine(UNIT,cinv->data),0))))->insert(&nkb->aliases);


Suppose we broke that up:

vine * vTemp = new vine( UNIT, cinv->data);guiaction * gTemp = new guiaction( dofarm, vTemp ), 0 );keyalias * kTemp = new keyalias( KB_F, 0, gTemp );vine * vTemp2 = new vine( KEYALIAS, kTemp );kTemp2->insert( &nkb->aliases );


new is supposed to throw the exception std::bad_alloc if it cannot allocate memory. That means any of those new calls could throw an exception. Also the vine, guiaction, or keyalias contructors could throw an exception. What I'm trying to show is that there are several things that could throw exceptions there.

Now what happens if an exception is thrown? It goes to the all the way back through the stack until it finds a try block. It then attempts to place the exception in the catch block. If it can't, it goes further until it gets to another try block. It keeps doing this until it gets to the OS which crashes your program because of an unhandled exception.

So the first problem is that your program could crash. Now lets say you placed catch blocks in smart locations. You could prevent this crash. Now you are leaking memory. For example if I allocate memory for the first three objects and then an exception is thrown, I don't have a pointer to the memory. Thus I cannot delete that memory.

So in general, that code is terrible. I can definatly see why people would be gouging their eyes when they see it. Any employer would fire you right then and there to minimize their losses.

If you want to learn more about making that code correct just let me know or google Exception Safety.
Heh, not to get too offtopic, the first step would be to impliment keybinding as some sort of message system instead of the nasty function pointer stack it is now. Remove the need for such contortions, and they won't really occur. The second step would probably be giving a rat's ass about code quality. At this point I'm simply thrilled my hobbyist project isn't in the circular file with oh so many others.

But thank you for pointing out that code I noted was terrible and an example of what not to do is in fact, terrible. And thank you for the explination on exceptions, that was perhaps one of the clearest descriptions of it I've seen.
getSomething() should return a pointer or reference, otherwise it's fine. It's not pure OO design, but IMNSHO that's a good thing.
- The trade-off between price and quality does not exist in Japan. Rather, the idea that high quality brings on cost reduction is widely accepted.-- Tajima & Matsubara
If you want to hear the opinions of the people that don't like stuff.getSomething().foo() (and especially if it gets to be a longer chain), search for "Law of Demeter".

Note that I don't necessarily agree with them, but a lot of the time you can make something nicer-looking via delegation. The problem is that commonly used OO-supporting languages tend to make it difficult to do large amounts of delegation (since it's all manual) - which is about the only reason I personally use inheritance at all. :/

WRT singletons, they tend to be overused. Your setup won't prevent people from making other resourceManager instances, unless you also have a private constructor as well. Actually I'm not even sure you can make the instance like that; the usual approach is to have an instance of the class as a static member of the class. Making that public is good enough for my tastes usually, but the full Singleton pattern makes that instance private as well, and provides an accessor. Supposedly that provides a bit more encapsulation.

There also exists the "Monostate" pattern, whereby all data members are made static, so that every instance of the class has the same data (and you let the user make any instances they like, but they all behave as if they were the same instance). I tend to simplify that further (for reasons of YAGNI, plus the fact that I tend to work in Java, where even empty objects have some cost) into just having a collection of static methods and data. The class is a namespace; and if you really need a global, there isn't much point to shell games, except to namespace things so they don't interfere accidentally.

However - you should also consider that if you want to make a class with "Manager" in its name, that may already be a sign you should rethink your design. After all, there's the principle that classes should be named after what they do - and there is a cynical but surprisingly appropriate observation to make here, that "managers" are highly-paid and don't really do very much. :)

An object should be able to do most of its own management, although you may also want to control the objects of a class as a group - in particular, their creation process and management of the number of instances. So - consider adding that stuff to the class itself, as the static part. And read up on patterns like Factory.

E.g.:

class Resource {  static std::some_collection_or_other<Resource> all_resources_in_known_universe;  // and data members common to all Resource classes.  public:  // public interface of Resources, and:  static Resource getResource(args, ...) {    // possibly return an already existing resource from the    // collection, or create a new one. And in either case, it    // might actually be any subclass of Resource, an option you    // don't get when using constructors.  }}
wow thanks for everyones posts they were all helpful, and the exception handling description was very clear.

someone suggested i return a pointer, so something like this i think is what they mean.

class q {
void myFunction ();
};

class myClass {
q *get ();
} object;

// get would be something like this
q *myClass::get () {
q *p;
// ...
return p;
}

object.get() -> myFunction();

ok i get a little confused here, because get returns a pointer with the address of p in the function get, wont that memory be deleted as soon as get is out of scope? so that when i call get it passes the previous memory of p, but then p is immediatly deleted?

also if i do the basic way of creating one class ie

class {
// ..
} object;

does this mean it's immposible to define this then later implement it because it has no class name? so if i were to do this, then i would have to implement everything inside the class declaration?


Quote:Original post by ekrax
// get would be something like this
q *myClass::get () {
q *p;
// ...
return p;
}

object.get() -> myFunction();

ok i get a little confused here, because get returns a pointer with the address of p in the function get, wont that memory be deleted as soon as get is out of scope? so that when i call get it passes the previous memory of p, but then p is immediatly deleted?


Only if you pointed to a local (auto) 'q' object. Memory is only deleted "when it gets out of scope" if it's on the stack.

q *myClass::get () {  q *p = &q(args); // bad: q is a stack var and we take its address  return p; // at end of scope, that part of the stack isn't our  // memory any more. Boom.}q *myClass::get () {  q *p = new q(args); // good: we made a new object on the heap  // so it won't be deleted until we ask to delete it.  return p; // ok}// But, now the caller has to know that it's responsible for// deleting this thing later. In the general case this can get// tricky, which is why garbage collection is a good thing.namespace important_qs {  static q importantQ(args);}q *myClass::get () {  q *p = &important_qs::importantQ;  // good: this object is already on the heap.  // and chances are we will be ok with just leaving it around  // until the end of the program.  return p; // ok}// Of course, you want to avoid globals if you can ;)
ok thanks for clearing that up. now if i wanted to do 'garbage collecting' would it be as simple as creating my own class that records all the dynamic memory in use .......... i just wrote a bunch of code and conufsed myself with all the templates and blah, but yeah it didnt work at all, so im gonna go try and code some sort of "garbage collector" class.

EDIT: can you make a vector of pointers to a template class?

This topic is closed to new replies.

Advertisement