6

Using C++ I built a Class that has many setter functions, as well as various functions that may be called in a row during runtime. So I end up with code that looks like:

A* a = new A();
a->setA();
a->setB();
a->setC();
...
a->doA();
a->doB();

Not, that this is bad, but I don't like typing "a->" over and over again.
So I rewrote my class definitions to look like:

class A{
public:
    A();
    virtual ~A();

    A* setA();
    A* setB();
    A* setC();
    A* doA();
    A* doB();

    // other functions

private:

    // vars
};

So then I could init my class like: (method 1)

A* a = new A();
a->setA()->setB()->setC();
...
a->doA()->doB();

(which I prefer as it is easier to write)
To give a more precise implementation of this you can see my SDL Sprite C++ Class I wrote at http://ken-soft.com/?p=234

Everything seems to work just fine. However, I would be interested in any feedback to this approach. I have noticed One problem. If i init My class like: (method 2)

A a = A();
a.setA()->setB()->setC();
...
a.doA()->doB();

Then I have various memory issues and sometimes things don't work as they should (You can see this by changing how i init all Sprite objects in main.cpp of my Sprite Demo).
Is that normal? Or should the behavior be the same?
Edit the setters are primarily to make my life easier in initialization. My main question is way method 1 and method 2 behave different for me?

Edit: Here's an example getter and setter:

Sprite* Sprite::setSpeed(int i) {
    speed = i;
    return this;
}

int Sprite::getSpeed() {
    return speed;
}
David Thornley
  • 56,304
  • 9
  • 91
  • 158
Kenny Cason
  • 12,109
  • 11
  • 47
  • 72
  • 4
    Is there a reason why you don't use initialization lists or the constructor body to set values ? – DumbCoder Aug 04 '10 at 13:17
  • I'm not too sure what "initialization lists" is, so i'll look into that. As for using the constructor, There are simply too many variables of which may or may not be set (in many cases the defaults are fine). And making various constructors, or a single constructor with many fields are both more difficult to maintain. Though I guess it's really just a matter of prefference. This is the primary way I build my class objects in Java. Though depending on how I do it in C/C++ I have run into errors (as mentioned above) – Kenny Cason Aug 04 '10 at 13:25
  • 3
    Having many setters is normally considered A Bad Thing in C++, as is having classes containing many variables. –  Aug 04 '10 at 13:25
  • Why is it bad, assuming the all member variables are relevant to the class? Or specifically, why is it A Bad Thing in C++? – Kenny Cason Aug 04 '10 at 13:28
  • 2
    @Kenny http://stackoverflow.com/questions/2747721/getters-and-setters-are-bad-oo-design –  Aug 04 '10 at 13:37
  • Thanks Neil, I also just talked to my office mate about that :) It's not so much the setters/getters being bad, it's more of the "are you just treating your class as a data structure or container? instead of setting some initial state and then allowing the class to 'behave'." – Kenny Cason Aug 04 '10 at 14:03
  • If you want advice, show us one of the setters so we can see exactly what you're doing. – David Thornley Aug 04 '10 at 14:26
  • Keep in mind that the generally accepted C++ best practices are different from other languages, such as Java and C#. I know a lot of Java developers love (and imo abuse) getters and setters. In C++, getters and setters are more looked down upon, unless changing or retrieving a piece of data has some side effects. If the getter/setter is only a one line return or assignment, it's probably considered 'wrong' by most developers. – Collin Dauphinee Aug 04 '10 at 14:40
  • @dauphic thanks, i'll keep all that in mind. I will admit to using Java primarily. so I've applied a lot of my habits to C++. @David There is a link in the post to the source code, Though I think dauphic's answer (below) was at least one of the major problems. I fundamentally misunderstood how A a = A() functions – Kenny Cason Aug 04 '10 at 14:47
  • 1
    I tracked down the source (there was a link in the post to a page with a link to the source code) and edited the question to include a sample getter and setter. – David Thornley Aug 04 '10 at 14:58
  • 1
    Note: I checked the class and "fluent" interfacing for transform felt relatively natural: "sprite->flip()->rotate(..)->translate(...)", other mixed usage less so: "sprite->animate()->draw()" or "sprite->rotate(...)->setTransaprency(...)" – MaR Aug 04 '10 at 15:00
  • @David thanks, I tend to be bad at wording my questions properly :) @MaR thanks for your input: I agree the the last one isn't very natural. the second one ("sprite->animate()->draw()") to me at first seemed natural because animate() is only setting the index of the next frame to be drawn, though actually i guess that could be a part of draw(). hmmm.. – Kenny Cason Aug 04 '10 at 15:14
  • 1
    @kenny: I spent a bit of time looking through the code. While I didn't spot any specific problems with memory management, I have to say it looked pretty messy to me. There are *lots* of magic numbers everywhere. The Sprite class looks to me like it should be at least three separate classes: Sprite, Bitmap, and Animation (and quite possibly a fourth, something like AnimationPath). It looks to me like you're also allowing a sprite to exist in an essentially uninitialized state, with no attached bitmap. This state appears to be useless, and probably shouldn't be allowed at all. – Jerry Coffin Aug 04 '10 at 15:14
  • @Jerry Thanks for the honesty :) I have to admit to just rushing through it. I spent most of time time learning how the low level stuff worked :P I will try to sit down and refactor it later – Kenny Cason Aug 04 '10 at 15:43

4 Answers4

4

What you have implemented there is called fluent interface. I have mostly encountered them in scripting languages, but there is no reason you can't use in C++.

soulmerge
  • 73,842
  • 19
  • 118
  • 155
  • Thanks, I couldn't think of the name for the initialization style. Do you by chance have any idea why A* a = new A() works fine but when I used A a = A(), then my initialization method seems to have some memory problems. :/ – Kenny Cason Aug 04 '10 at 13:27
  • @KennyCason: It's nearly impossible to guess about what might go wrong with memory management unless you show us some of the relevant code. – Jerry Coffin Aug 04 '10 at 13:39
  • @Jerry there is a link in the question. And to be honest it may be tough to look through as there is a lot there. So that's why I edited my question to reflect "method 1" and "method 2", should they function the same? And if not, then why, and if they should, then I will know the problem lies deeper in the code. – Kenny Cason Aug 04 '10 at 13:52
  • @Jerry Specifically note in main.cpp I Initialize my Sprite objects like Sprite* sprite = new Sprite("file.bmp",frames,animSpeed); then sprite->setTransparency(255,0,255)->flipVertical()->otherstufF() That works fine, no apparent memory leaks, but if I switch all the sprite object so Sprite sprite = Sprite(...) and sprite.setTransparency(255,0,255)->flipVertical()->otherstufF() Then I get strange behavior like one effect is applied to another sprite, etc. – Kenny Cason Aug 04 '10 at 13:56
  • @KennyCason: Unfortunately, the link in your question goes to where there's a small fragment of code, less than you've posted above. – David Thornley Aug 04 '10 at 14:27
  • hmm. that should not be the case there is a Zip file containing the whole source as well as four links to the individual source files on that page (look above the code fragment). Thanks for looking. – Kenny Cason Aug 04 '10 at 14:31
4

One note unrelated to your question, the statement A a = A(); probably isn't doing what you expect. In C++, objects aren't reference types that default to null, so this statement is almost never correct. You probably want just A a;

A a creates a new instance of A, but the = A() part invokes A's copy constructor with a temporary default constructed A. If you had done just A a; it would have just created a new instance of A using the default constructor.

If you don't explicitly implement your own copy constructor for a class, the compiler will create one for you. The compiler created copy constructor will just make a carbon copy of the other object's data; this means that if you have any pointers, it won't copy the data pointed to.

So, essentially, that line is creating a new instance of A, then constructing another temporary instance of A with the default constructor, then copying the temporary A to the new A, then destructing the temporary A. If the temporary A is acquiring resources in it's constructor and de-allocating them in it's destructor, you could run into issues where your object is trying to use data that has already been deallocated, which is undefined behavior.

Take this code for example:

struct A {
    A() { 
        myData = new int;
        std::cout << "Allocated int at " << myData << std::endl;
    }
    ~A() { 
        delete myData; 
        std::cout << "Deallocated int at " << myData << std::endl;
    }
    int* myData;
};

A a = A();
cout << "a.myData points to " << a.myData << std::endl;

The output will look something like:

Allocated int at 0x9FB7128
Deallocated int at 0x9FB7128
a.myData points to 0x9FB7128

As you can see, a.myData is pointing to an address that has already been deallocated. If you attempt to use the data it points to, you could be accessing completely invalid data, or even the data of some other object that took it's place in memory. And then once your a goes out of scope, it will attempt to delete the data a second time, which will cause more problems.

Collin Dauphinee
  • 13,664
  • 1
  • 40
  • 71
  • 1
    A a = A() equals to A a(A()). in both cases, temporal object get constructed first before the named object. – YeenFei Aug 04 '10 at 14:37
  • Oh wow. I definitely overlooked that. thanks for the clarification, I will try it out when I get home. but +1 because I know for sure that that was definitely one of the issues! – Kenny Cason Aug 04 '10 at 14:39
2

If you really, really hate calling lots of set functions, one after the other, then you may enjoy the following code, For most people, this is way overkill for the 'problem' solved.

This code demonstrates how to create a set function that can accept set classes of any number in any order.

#include "stdafx.h"
#include <stdarg.h>

// Base class for all setter classes
class cSetterBase
{
public:
    // the type of setter
    int myType;
    // a union capable of storing any kind of data that will be required
    union data_t {
        int i;
        float f;
        double d;
    } myValue;

    cSetterBase( int t ) : myType( t ) {}
};

// Base class for float valued setter functions
class cSetterFloatBase : public cSetterBase
{
public:
    cSetterFloatBase( int t, float v ) :
        cSetterBase( t )
        { myValue.f = v; }
};

// A couple of sample setter classes with float values
class cSetterA : public cSetterFloatBase
{
public:
    cSetterA( float v ) :
        cSetterFloatBase( 1, v )
        {}
};
// A couple of sample setter classes with float values
class cSetterB : public cSetterFloatBase
{
public:
    cSetterB( float v ) :
        cSetterFloatBase( 2, v )
        {}
};


// this is the class that actually does something useful
class cUseful
{
public:
    // set attributes using any number of setter classes of any kind
    void Set( int count, ... );

    // the attributes to be set
    float A, B;
};

    // set attributes using any setter classes
void cUseful::Set( int count, ... )
{
    va_list vl;
   va_start( vl, count );

     for( int kv=0; kv < count; kv++ ) {
        cSetterBase s = va_arg( vl, cSetterBase );
        cSetterBase * ps = &s;
        switch( ps->myType ) {
        case 1:
            A = ((cSetterA*)ps)->myValue.f; break;
        case 2:
            B = ((cSetterB*)ps)->myValue.f; break;
        }
     }
     va_end(vl);
}


int _tmain(int argc, _TCHAR* argv[])
{
    cUseful U;
    U.Set( 2,  cSetterB( 47.5 ), cSetterA( 23 ) );
    printf("A = %f B = %f\n",U.A, U.B );
    return 0;
}
ravenspoint
  • 19,093
  • 6
  • 57
  • 103
  • haha that is definitely overkill in my case :) but that is very insightful. At one point in time I had actually been curious about something like this. – Kenny Cason Aug 04 '10 at 15:47
1

You may consider the ConstrOpt paradigm. I first heard about this when reading the XML-RPC C/C++ lib documentation here: http://xmlrpc-c.sourceforge.net/doc/libxmlrpc++.html#constropt

Basically the idea is similar to yours, but the "ConstrOpt" paradigm uses a subclass of the one you want to instantiate. This subclass is then instantiated on the stack with default options and then the relevant parameters are set with the "reference-chain" in the same way as you do.

The constructor of the real class then uses the constrOpt class as the only constructor parameter.

This is not the most efficient solution, but can help to get a clear and safe API design.

IanH
  • 3,968
  • 2
  • 23
  • 26
  • Thanks, I have seen that method before in some Java code. I know my question was very long winded but the problem I was having with my method of initialization was towards the bottom :) – Kenny Cason Aug 04 '10 at 14:10