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.

I have this old implementation of the Command pattern. It is kind of passing a Context through all the DIOperation implementation, but I realized later on, in the process of learning and learning (that never stops), that is not optimal. I also think that the "visiting" here doesn't really fit and just confuses.

I am actually thinking of refactoring my code, also because a Command should know nothing about the others and at the moment they all share the same key-value pairs. It is really hard to maintain which class owns which key-value, sometimes leading to duplicate variables.

An Example of use case: let's say CommandB requires UserName which is set by CommandA. Should CommandA set the key UserNameForCommandB=John? Or should they share a common UserName=John key-value? What if the UserName is used by a third Command?

How can I improve this design? Thanks!

class DIParameters {
public:
   /**
    * Parameter setter.
    */
    virtual void setParameter(std::string key, std::string value) = 0;
    /**
    * Parameter getter.
    */
    virtual std::string getParameter(std::string key) const = 0;

    virtual ~DIParameters() = 0;
};

class DIOperation {
public:
    /**
     * Visit before performing execution.
     */
    virtual void visitBefore(DIParameters& visitee) = 0;
    /**
     * Perform.
     */
    virtual int perform() = 0;
    /**
     * Visit after performing execution.
     */
    virtual void visitAfter(DIParameters& visitee) = 0;

    virtual ~DIOperation() = 0;
};
share|improve this question

migrated from stackoverflow.com Mar 4 '13 at 17:50

This question came from our site for professional and enthusiast programmers.

3  
I have never had luck using command to set properties (like name). It starts to become very dependent. If your setting properties try using an event architecture or an observer pattern. –  ahenderson Mar 4 '13 at 17:46
1  
1. Why pass the parameters through a separate visitor? What's wrong with passing a context as argument of perform? 2. Context is for the 'common' part of the command (eg. Current session/document). All operation-specific parameters are better passed through the operation's constructor. –  Kris Van Bael Mar 4 '13 at 18:18
    
@KrisVanBael that's the confusing part that I am trying to change. I am passing it as a Visitor while it actually is a Context... –  Kap Mar 4 '13 at 19:14
    
@ahenderson Do you mean events between my commands right? Would you put your key-values there (similar to what Android does with Parcel)? Would it be the same in the sense that CommandA should build an Event with the key-value pairs that CommandB accepts? –  Kap Mar 4 '13 at 19:52
add comment

4 Answers

up vote 2 down vote accepted

I am a bit worried about the mutability of your command parameters. Is it really necessary to create a command with constantly changing parameters?

Problems with your approach:

Do you want other threads/commands to change your parameters while perform is going on?

Do you want visitBefore and visitAfter of the same Command object to be called with different DIParameter objects?

Do you want someone to feed parameters to your commands, that the commands have no idea about?

None of this is prohibited by your current design. While a generic key-value parameter concept has its merits at times, I do not like it with respect to a generic command class.

Example of consequences:

Consider a concrete realization of your Command class - something like CreateUserCommand. Now obviously, when you request a new user to be created, the command will need a name for that user. Given that I know the CreateUserCommand and the DIParameters classes, which parameter should I set?

I could set the userName parameter, or username.. do you treat parameters case insensitively? I wouldn't know really.. oh wait.. maybe it's just name?

As you can see the freedom you gain from a generic key-value mapping implies that using your classes as someone who did not implement them is unwarrantedly difficult. You would at least need to provide some constants for your commands to let others know which keys are supported by that command.

Possible different designs approaches:

  • Immutable parameters: By turning your Parameter instances immutable you can freely reuse them among different commands.
  • Specific parameter classes: Given a UserParameter class that contains exactly the parameters I would need for commands that involve a user, it would be much simpler to work with this API. You could still have inheritance on the parameters, but it would not make sense any longer for command classes to take arbitrary parameters - at the pro side of course this means that API users know which parameters are required exactly.
  • One command instance per context: If you need your commands to have things like visitBefore and visitAfter, whilst also reusing them with different parameters, you will be open to the problem of getting called with differing parameters. If the parameters should be the same on multiple method calls, you need to encapsulate them into the command such that they cannot be switched out for other parameters in-between the calls.
share|improve this answer
    
Yes, I got rid of visitBefore and visitAfter. I am basically passing my DIParameter interface in the perform method. The problem with undesired DIParamters instances is always going to be there, because I chose to have the flexibility of passing the interface. I really like the idea of being able to subclass and make DIParameters children immutable once they are filled. However, a "central authority" still needs to pass the correct DIParameter to the Command. This is probably why I started implementing a Visitor pattern..I wanted to have inversion of control in some way... –  Kap Mar 5 '13 at 16:37
add comment

What is nice about design principles is that sooner or later, they conflict with each other.

In the situation described, I think I would prefer to go with some kind of context that each command can take information from and put information to (especially if those are key-value pairs). This is based on a trade off : I do not want separate commands to be coupled just because they are some kind of input to each other. Inside CommandB, I do not care how UserName was set - just that it is there for me to use. Same thing in CommandA : I set the information in, I do not want to know what other are doing with it - neither who they are.

This means a sort of passing context, which you can find bad. For me, the alternative is worse, especially if this simple key-value context (can be a simple bean with getters and setters, to limit a bit the "free form" factor) can allow the solution to be simple and testable, with well separated commands, each with its own business logic.

share|improve this answer
1  
Which principles do you find conflicting here? –  Jimmy Hoffa Mar 4 '13 at 18:31
    
Just to clarify, my problem is not choosing between Context or Visitor pattern. I am basically using a Context pattern called Visitor :) –  Kap Mar 4 '13 at 19:16
    
Ok, I probably misunderstood your exact question/problem. –  Martin Mar 4 '13 at 19:29
add comment

Lets assume you have a command interface:

class Command {
public:
    void execute() = 0;
};

And a subject:

class Subject {
    std::string name;
public:
    void setName(const std::string& name) {this->name = name;}
}

What you need is:

class NameObserver {
public:
    void update(const std::string& name) = 0;
};

class Subject {
    NameObserver& o;
    std::string name;
private:
    void setName(const std::string& name) {
        this->name = name;
        o.update(name);
    }
};

class CommandB: public Command, public NameObserver {
    std::string name;
public:
    void execute();
    void update(const std::string& name) {
        this->name = name;
        execute();
    }
};

Set NameObserver& o as a reference to CommandB. Now whenever CommandA changes the Subjects name CommandB can execute with the correct information. If name is used by more command use a std::list<NameObserver>

share|improve this answer
    
Thanks for the answer. The problem with this design imho is that we need a Setter + NameObserver per every parameter. I could pass a DIParameters (context) instance and notify, but again, I won't probably solve the fact that I am still coupling CommandA with CommandB, meaning CommandA has to put a key-value that only CommandB should know of...what I tried was also to have an external entity (ParameterHandler) which is the only one to know which Command needs which parameter and sets/gets accordingly on the DIParameters instance. –  Kap Mar 4 '13 at 21:45
    
@Kap "The problem with this design imho is that we need a Setter + NameObserver per every parameter" - parameter in this context is a bit confusing for me, I'm thinking you meant field. In which case you should already have a setter for each field that changes. From your example it seems ComamndA does change the name of the Subject. It should change the field via a setter. Note: you do not need an observer per field, just have a getter and pass the object to all observers. –  ahenderson Mar 6 '13 at 15:17
add comment

I don't know if this is the right way to handle this on Programmers (in which case I apologize), but, after having checked all the answers here (@Frank's in particular). I refactored my code this way:

  • Dropped DIParameters. I'll have individual (generic) objects as DIOperation's input (immutable). Example:
class RelatedObjectTriplet {
private:
    std::string const m_sPrimaryObjectId;
    std::string const m_sSecondaryObjectId;
    std::string const m_sRelationObjectId;

    RelatedObjectTriplet& operator=(RelatedObjectTriplet other);

public:
    RelatedObjectTriplet(std::string const& sPrimaryObjectId,
                         std::string const& sSecondaryObjectId,
                         std::string const& sRelationObjectId);

    RelatedObjectTriplet(RelatedObjectTriplet const& other);


    std::string const& getPrimaryObjectId() const;
    std::string const& getSecondaryObjectId() const;
    std::string const& getRelationObjectId() const;

    ~RelatedObjectTriplet();
};
  • New DIOperation class (with example) defined as:
template <class T = void> 
class DIOperation {
public:
    virtual int perform() = 0;

    virtual T getResult() = 0;

    virtual ~DIOperation() = 0;
};

class CreateRelation : public DIOperation<RelatedObjectTriplet> {
private:
    static std::string const TYPE;

    // Params (immutable)
    RelatedObjectTriplet const m_sParams;

    // Hidden
    CreateRelation & operator=(CreateRelation const& source);
    CreateRelation (CreateRelation const& source);

    // Internal
    std::string m_sNewRelationId;

public:
    CreateRelation(RelatedObjectTriplet const& params);

    int perform();

    RelatedObjectTriplet getResult();

    ~CreateRelation();
};
  • It can be used like this:
RelatedObjectTriplet triplet("33333", "55555", "77777");
CreateRelation createRel(triplet);
createRel.perform();
const RelatedObjectTriplet res = createRel.getResult();

Thanks for the help and I hope I haven't made mistakes here :)

share|improve this answer
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.