views:

147

answers:

4

I have class with lots of conversion functions:

class Something {
  public:

    string toXml();
    string toJson();
    ...

    static Something fromXml(string);  // factory
    static Something fromJson(string); // factory
    ...
};

Because static functions can be called on instance, it is easy to write code like this:

Something sss;

... initializing sss ...

string xml1 = sss.toXml();
sss.fromXml(xml1); // does nothing
string xml2 = sss.toXml();
assert(xml1 == xml2); // always true

So I want to forbid calling fromXXX on objects, or at least make them do something different.

Is there a way to do this?

+3  A: 

Revise your design instead. Split your static methods into a separate SomethingFactory class. This is just confusing.

Pontus Gagge
That could be SomethingFactory namespace as well. But then functions have to be friends with Something class. Code will be more complex.If I could forbid confusing call syntax, it would be completely OK.
Ivan
@Ivan: If you could redesign the C++ language so your specific problem was trivial, then you'd no longer have any problem. (And that's sometimes the best solution.) However, it's unlikely switching language meets your requirements and so your ideal solution just isn't possible in C++.
Roger Pate
@Roger: I am not sure C++ standard directly says that I can't identify what call form was used. If C++ evaluates "object" in "object.staticMethod()" call anyway, it could provide it to static function as "this". Or do something like it.
Ivan
@Ivan: I am sure that it doesn't provide a way. ("Directly"? *\*shrug\**, it doesn't, and that's good enough here.)
Roger Pate
@Ivan: Not quite, while you do not see it in the code, in non-static member functions there is a hidden parameter that is the 'this'. Static member functions have signatures equivalent to those of free functions, so `class X { static void f(); void g(); }; void h();` you can write: `void (*ptr)() = X::f;` and `ptr = h;`, but the signature is not compatible with member functions, so `ptr = X::g;` is an error. What you are saying is that you would like the compiler to change the type of the function depending on the usage and not the definition.
David Rodríguez - dribeas
+7  A: 

Do they really need to be class members? The obvious way to prevent this is to make them free functions.

anon
On that note: http://www.drdobbs.com/cpp/184401197 Talks about how free functions is better for encapsulation.
Eld
I just wanted to keep global namespace clean - this object is visible everythere in huge program. Of course I could make "SomethingUtils" namespace for them. But they looked like "factory" functions. It actually made sense until I wasted 2 hours searching for non-existing "bug".
Ivan
Eld, free functions will be "friends" with class anyway. Incapsulation level does not change.
Ivan
@Ivan: It's really, really easy to pick your own namespace and use it consistently if that's a concern (and it's a decent concern), and put Something in it too, so they would still be together.
Roger Pate
@Ivan: The free functions do not necessarily need to be `friend` s of the class, they can depend on existing interfaces (I guess you can create `Something` objects in memory with a given state). There is also another design advantage: most users of `Something` probably do not quite care whether it can be serialized as XML, Json or any other technology, but if those operations are (static) members of the class, then the `Something` abstraction is 'leaking' how it can be serialized to the rest of the system.
David Rodríguez - dribeas
... that 'leak' implies that whenever you need to add a new type of serialization to the mix, all code that depended on No/XML/Json serialization will need to be recompiled --serialization mechanisms have 'leaked' and became part of your interface.
David Rodríguez - dribeas
Besides the link by @Eld, you could also take a look at this GOTW: http://www.gotw.ca/gotw/084.htm, that is not so closely related, but is worth reading and is at least somehow related.
David Rodríguez - dribeas
@David: This class has no other interfaces, just toXXX and fromXXX forms of functions. XXX is other class or serialization format.State is encapsulated in PIMPL idiom, so it is not visible.Moving half of functions from class looks inconsistent. If I move out all of them - class will be empty.
Ivan
@David: Free functions do not solve recompiling problem. If new formats appear, I will add generic function that will accept format name - that will solve it.But described problem will remain.
Ivan
What you are saying is that you have created a class whose only functionality is serialization/deserialization of other elements? If that is so, make the functions free, bind them in a namespace and take rid of the class. Utility classes are required in languages that have no free functions, but not so in C++.
David Rodríguez - dribeas
No, it is opaque container, like boost::any. I can't just get rid of it - then I will have to work with untyped pointers.
Ivan
+1  A: 

The standard actually requires all compliant compilers to allow that syntax in 9.4 [class.static]/2:

A static member s of class X may be referred to using the qualified-id expression X::s; it is not necessary to use the class member access syntax (5.2.5) to refer to a static member. A static member maybe referred to using the class member access syntax, in which case the object-expression is evaluated.

Now, there are some things you can do to avoid the pitfall, in no particular order

  • Convert them into free-functions disallowing the syntax
  • Improve the naming convention: createFromXml to make more explicit that it is a factory method
  • Convert the static method into a concrete method that will perform the operation in the object, and provide an external method factory that will reuse the code.

From a design point of view, the first option has the advantage of un-coupling the serialized formats from the class itself. Something (by a better name), represents an object with properties and operations and all those OO things. But in many cases Something is unrelated to the fact that it can be serialized for sending or storage in different formats.

Users of your Something class that only want to work with XML don't need to even know that your object can be serialized to Json. Neither users of Json or XML should be affected if you later add a database persistence option to the class.

David Rodríguez - dribeas
@David: these functions are not for serialization, but for conversion. Anyone using that header will want to convert Something to more useful form. It could be JSON, XML or other object. Keeping all this stuff in one header seems reasonable.So standard allows both call forms. But it does not say I can't identify form that was used. For example, it could set "this" to NULL in "Type::method" call, but to real object in "object.method" call.Still, I wonder why "object.method" syntax is allowed at all. It is confusing, especially in "this->staticMethod()" form that is required in templates.
Ivan
`this->` is rarely required, even in templates. It is sometimes more convenient than using a qualified name (`Name::member`), such as when accessing a member inherited from a base class which depends on a template parameter (and thus the member is a dependent name so you can't just name it alone).
Roger Pate
@Roger: It is different syntax. Qualified name will break if you rename class or move it to other namespace, and "this->" will not.And it is confusing anyway. Why C++ allows "object.staticMethod()" syntax? You could use "Type::staticMethod()" instead with trivial template helper function.
Ivan
Roger Pate
There is no `this` pointer in a static method call. Static methods are similar to free functions defined within the class name scope, and you cannot know how or where they have called a function from within the same function.
David Rodríguez - dribeas
@David: that is exactly what confuses me. Static methods don't have "this", but can be called "this->staticMethod()".Compilers could generate warnings on such calls anyway. Most of them scream on valid C++ like "if (a = b)", but do nothing in my case.
Ivan
There is no problem in calling `object.staticMethod()`, the problem you have is that the semantics of the operation and the name seem to imply to the casual reader that `object.fromXml()` will modify `object` instead of returning a value that you are ignoring (I bet there is some compiler warning for ignored return values). The problem is ignoring the value, not the operation being static. If the method had been called `createFromXml` instead of plain `fromXml`, then `object.createFromXml()` would seem funny and you would have detected it before
David Rodríguez - dribeas
@Roger: I can call static method on "this" with template helper function anyway. There is no real reason to allow confusing syntax. If it is just C++ design "misfeature" and can't be fixed, maybe specific compilers will generate warning on it?
Ivan
@David: I would be confused on object.createFromXml() too. In general, I think "object.staticMethod()" form is confusing. Just in my case it was obvious.
Ivan
@Ivan: You could certainly call it a problem, but I wouldn't say it's huge or worth a lot of effort or worry, just fix your code (make the functions non-members instead of static) and move on. I've been using C++ for almost 15 years and have maybe run into it once. I've certainly spent more time on this single question than it has ever cost me in my own code. :) (Of course, this is just my own experience.)
Roger Pate
A: 

How about making your static members private?

If you need to have member functions that are static this may be a way to solve this.

Why do you have static member function? Do they need access to something else in the class? If not, then you could make them free functions that are not in the header with Something, but in another header in a separate namespace.

quamrana
Being private would not work here, and really has nothing to do with it. They are factory methods, which is another way to say they are special constructors ("named constructors").
Roger Pate
This class has only toXXX/fromXXX functions and pointer to implementation. Moving functions out will make this class almost empty. And they still have to be friends.Without these functions Something is completely useless. I don't think they should be in separate header file.
Ivan