Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

Sometimes I find myself writing comments on class methods like this:

class A : public Base
{
public:
    /**
      * Sets variable;
      * should be called before ImplementsInterfaceMtehod(),
      * else has no effect.
      */
  void SetSomeVariable( var_type value );

  virtual void ImplementsInterfaceMethod();
}

The callers of Base::ImplementsInterfaceMethod obviously do not know about the variable, and should not. But the users of A should set the variable if they want it to take effect. It is not required to set the variable (else it could be a parameter for the constructor), so I cannot throw exceptions in ImplementsInterfaceMethod if it is not set.

Is this a sign of some typical bad practice? Is there a better way than writing a comment as shown to deal with this?

edit here's a more concrete example resembling part of some realtime 3D image processing code; because of the realtime part and the fact images can get huge, everything has to be preallocated.

class DataProcessor
{
public:
    /**
      * Sets dimensions of the data for the next call to Process().
      * Can be used by implementations to setup internals, eg preallocate.
      * Upon returning out contains the dimensions that will be returned
      * by the Process call, given dims as input dimensions.
      * (A)
      */
  virtual bool SetInputDimensions( const Dimensions& dims, Dimensions& out ) = 0;

  virtual bool Process( const Data& in, Data& out ) = 0;
};

class ImageStitcher : public DataProcessor
{
    /**
      * Set size of the output image returned by Process() as a factor,
      * eg when size == 2, image will be twice as big.
      * (B)
      */ 
  void SetOutputSize( const double size );

    /**
      * Set the position at which the next Data object passed to Process()
      * will be stitched.
      * (C)
      */ 
  void SetStitchPosition( double x, double y, double alpha );
}

So here (A) must be called at least once before Process() is called. Which I enforce by returning an error in Process() if that was not the case. Most of the time the dimensions are not known when constructing the object though so a constructor parameter is not an option. Two main components use the DataProcessor interface: all processors are in a network of processors, and that network is unaware of the type of processor, it just takes care of transferring data between them by calling Process() multiple time. Another component takes care of setting up the network by calling SetInputDimensions(), again unaware of what type the processor is.

(B) should be called before SetInputDimensions() gets called, as it defines the size of the resulting image.

(C) should be called before each call to process else the stitcher doesn't know where to put the image

Now this whole processing network is actually used in commercial applications and works extremely well since I made sure everything is called nicely in order. Yet each time I write 'must/should be called before' bells start ringing all over the place..

share|improve this question
1  
Perhaps you need a construction pattern: Builder or Factory. This allows you to enforce 'SomeVariable' to be set before the class is used. –  Bringer128 Jun 8 '12 at 7:00
    
@Bringer128 it is not really required for the class, it only affects beahviour of ImplementsInterfaceMethod. For exxample there might be a default value that's fine, and other methods can be called before the variable has been set because they are not affected by it. –  stijn Jun 8 '12 at 7:06
3  
It looks like you have two entirely separate branches of behaviour here, in this case I would consider whether your class needs to be split into two? –  MattDavey Jun 8 '12 at 7:27
    
"It is not required to set the variable (else it could be a parameter for the constructor)" - so make it an optional parameter, or declare more than one constructor. –  Mike Seymour Jun 8 '12 at 8:30
1  
What you face here is called sequential coupling. You'll find more informations googling that. –  deadalnix Jun 8 '12 at 11:48
show 1 more comment

4 Answers

up vote 3 down vote accepted

As always, it depends.

If the object exists some of the time in state when the variable is not set and another time in state when it is set and in both cases the ImplementsInterfaceMethod does the thing one expects from looking at Base alone, than it's right.

In the object has to have the variable set before it's usable as implementation of Base, than it's a code smell. I would probably call it contract violation, since the object claims to implement an interface, but only implements it sometimes.

In this case it would be preferred to either require the value in constructor, or to create an object with parametric method and wrapper which implements the interface, again taking the value in constructor. But it might easily be best of bad solutions if you are constrained by some existing interfaces.

share|improve this answer
add comment

As always it depends on context. If your class was an XML parser, and SetSomeVariable was a flag for modifying whether whitespace nodes are returned from parsing then there's no problem.

The only times when this is an issue is when there is a requirement for SetSomeVariable to be invoked before the other method. This would cause tight coupling.

As you asked, perhaps a better comment is required:

/**
  * Modifies the behaviour of the ImplementsInterfaceMethod() method in way xyz.
  */
void SetSomeVariable( var_type value );

Of course, in this example you could probably use an optional parameter:

virtual void ImplementsInterfaceMethod(int i = 1);
share|improve this answer
add comment

It probably indicates that you should have taken that value as an argument in an overload of ImplementsInterfaceMethod, as the setter serves no other purpose. However, if it sets general state good for many functions, and it's optional, then I don't believe it is bad design.

share|improve this answer
    
the overload wouldn't work out, the value makes sense for a single class A only, whereas there are multiple implementations of Base –  stijn Jun 8 '12 at 9:29
    
@stijn: You can make overloads in A that aren't in Base, you know. –  DeadMG Jun 8 '12 at 10:52
    
off course, but the code calling Base::ImplementsInterfaceMethod won't call them so in my case they are of no use –  stijn Jun 8 '12 at 11:18
add comment

Here's an excerpt from Curly's Law..

"A variable should mean one thing, and one thing only. It should not mean one thing in one circumstance, and carry a different value from a different domain some other time. It should not mean two things at once. It must not be both a floor polish and a dessert topping. It should mean One Thing, and should mean it all of the time."

The same principle applies to objects and their behaviour (replace the word 'variable' with 'object', and the word 'mean' with 'do').

"An object should do one thing, and one thing only. It should not do one thing in one circumstance, and exhibit a different behaviour from a different domain some other time. It should not do two things at once. It must not be both a floor polisher and a dessert topper. It should do One Thing, and should do it all of the time."

share|improve this answer
    
Please explain the downvote? –  MattDavey Jun 8 '12 at 15:07
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.