4

I have been working on refactoring old code and found a lot of instances of the following type of situation: There is a master object which we call "Application" and there is only one of these. The Application will create other objects called "Features" inside it depending on user input etc. The Feature object may have some methods that take void parameters but actually operate on or use some variables in the scope of Application. This is because the functions defined in class Feature are defined in the same project and include the header from Application (this is C++). Here is a rudimentary attempt to show a simple example pseudocode:

int main()
{
    Application theApp = new Application()

    the.App.someSetting = foo;
    the.App.someVariable = bar;

    the.App.RunSomeFeature();
}
// RunSomeFeature function def in class Application
void Application::RunSomeFeature()
{
    // create object of type feature
     Feature feature = new Feature();

    feature.SomeFunction(void)
}
// SomeFunction def in class Feature
void Feature::SomeFunction(void)
{
    // global variable from Application "creeping in"
    someLocalVariable = theApp.someSetting;
    otherLocalVariable = theApp.someVariable;
}  

I hope this illustrates somewhat what I mean. My goal is essentially to move the class "Feature" to some location outside the namespace of "Application" (to a dll for example) and I can not include Application header (since that would defeat the purpose of the separation). But since there are these types of globals everywhere in the old code this task is rather complicated. This may be a C++ thing for all I know that it is easy (or posible) to make this coding mistake but I am not sure.

This made me think of a few questions regarding OOP design. Well designed OOP code would only have completely autonomous classes and functions with all outside variables explicitly passed in? Even if one object will be created inside another? I can see that way the entire code would be much easier to internally "reshuffle". But sometimes there may be a lot of parameters to be passed and since "Feature" is created by "Application" anyway I see why my situation might arise. But code written as in the example implicitly joins class Application and Feature to the same overall "block" or scope and it takes line by line work sometime to separate them.

However the other extreme would be that each class is essentially almost an autonomous application in its own right (and this may go somewhat against the idea of inheritance). When I read about OOP design principles, I usually don't find this particular nuance talked about much or I just get the idea that everything should be basically autonomous but I find that in practice this is rarely truly so.

I see that this may be a bit open ended but any rules of thumb about at what level would we "completely encapsulate" in order to have pieces of code we can reshuffle easily? Or any design philosophy anyone would share about this type of situation?

4

What you are after is not very specific for OOP, and has absolutely nothing to do with inheritance. You are after proper modularization. Each of your features should be a component, that means either a single class, or a group of classes, with a well defined interface, and not directly dependent on the Application object.

In the current situation each feature depends on Application, and Application depends on each feature, so every feature depends essentially on every other feature, which makes it impossible to test single features in isolation. Eliminating the direct dependencies of the features from the Application object and passing data in and out through the components interfaces will change that situation. If you do this right, you will end up with lots of independent components, where you can change and test each of it independently from each other, with a much lower risk of breaking your program when you are going to change something.

  • This is a good answer and you explained the concepts I was having trouble articulating very well. I am currently having a huge problem testing anything in isolation inside this code base because of this interdependency problem. – virtore Mar 31 '15 at 18:38
2

I think it's perfectly OK for a "feature" to need an "application", but the use of globals is nauseating. Constructor dependency injection can solve this problem, and allow a gradual refactoring of code.

(I'm not a C++ developer, so any corrections/clarifications are welcome)

First, the Application class:

class Application
{
public:
    void run_blog_feature();
};

Application::run_blog_feature() {
    BlogFeature blog(this);

    blog.run();
}

The BlogFeature class has a constructor allowing the application to pass itself in:

class BlogFeature
{
private:
    int some_setting = 0;
public:
    BlogFeature(Application app);
    void run();
};

BlogFeature::BlogFeature(Application app) {
    this->some_setting = app.get_some_setting();

    // more initialization
};

BlogFeature::run() {
    // do stuff
};

That way the "features" can configure themselves based on the application. Furthermore, you can refactor the application feature by feature in isolation. Gradually you refactor the code relying on globals into code relying on constructor injection.

You could then refactor it further and split the application configuration into its own class, and then just pass the config object into the features to provide further decoupling.

As a final step, you can create a "factory object", and provide it with some configuration. The factory object would have a method that returns the "feature" objects. All configuration of the features happens within the factory object, leaving your "features" and the application ignorant of the configuration layer.

  • Thank you for the answer. What I am trying to understand is that if the Application passes itself in, then Feature will be tied to Application maintaining the same internal format (at least the parts used by Feature). In other words if Feature will be ported to another Application with much different internal logic this setup will not be flexible enough to handle that. In such case a standard separate configuration class that is in common between all parts would work? – virtore Mar 31 '15 at 18:30
  • Yes, a standard configuration object can provide some separation. Ideally, you want a "factory class" that can take this configuration object, and then manually set the values of fields or call "setter" methods on the Feature objects to configure them. This would keep the configuration, application and features completely decoupled. The application provides the config to the factory, the factory creates the feature and calls the appropriate methods to configure each individual feature using data from the config object. – Greg Burghardt Mar 31 '15 at 18:40
1

I can not include Application header (since that would defeat the purpose of the separation). But since there are these types of globals everywhere in the old code this task is rather complicated.

You just need to split the task into the correct steps, and then it is simple (though it may not be easy :) )

But sometimes there may be a lot of parameters to be passed and since "Feature" is created by "Application" anyway I see why my situation might arise.

Rule: things that are processed together should be grouped together.

Here's an example, for your feature code:

Original code

// RunSomeFeature function def in class Application
void Application::RunSomeFeature()
{
    // create object of type feature
     Feature feature = new Feature(this); // added this as a parameter
                                          // assuming Feature reads lots of data
                                          // from Application, to construct itself

    feature.SomeFunction(void)
}

Step 1

Create a constructor that receives already constructed values for it's data members. If Feature is constructed using Application::arg1, Application::arg2, ..., Application::arg_n_, then you will need a Feature constructor that receives n arguments.

Step 2

Extract Feature into a separate class, and switch client code from calling Feature::Feature(const Application&) to calling Feature::Feature(arg1, arg2, ..., argn).

note: Because things that are processed together should be grouped together, consider writing feature like this:

struct FeatureData // or class
{
    // feature args are passed together, so we group them together, here:

    Arg1 arg1;
    Arg2 arg2;
    ...
    Argn argn;
};

class Feature
{
    Feature(FeatureData&& data); // call std::move on data.arg1, ... data.argn to populate feature

};

(instead of having a Feature that receives n arguments).

Step 3

Update Application to either be a FeatureData specialization or construct a FeatureData instance when needed.

Step 4

Move Feature and FeatureData to a separate library.

I see that this may be a bit open ended but any rules of thumb about at what level would we "completely encapsulate" in order to have pieces of code we can reshuffle easily? Or any design philosophy anyone would share about this type of situation?

SOLID principles: In this case, the "I" stands for Interface Segregation (that is, do not use all of Application's public interface to pass arguments to constructors; separate the interface - into FeatureData and rest of Application, so you can use them separately).

Demeter's Law: a class should not know/have-access-to more than it needs to complete it's job (in this case, a Feature needs to know "a set of n values", not an Application object and "a set of n values").

0

The source of the confusion here may be the fact that you are viewing the application object as being one and the same thing as the domain model of your application. They are not. The application contains the domain model, and instantiates the features that operate on it.

So, my recommendation would be to take all these global variables out of the application object and move them into a domain model object which gets instantiated by and contained within the application object. Then, pass a reference to this domain model object as a constructor parameter to each and every feature that needs to operate on it.

In order for this to work well, the domain model object should offer change notification events, so that when one feature changes it, other features can take notice of this change if they need to.

It is perfectly fine if a feature needs to make use of only a small subset of the domain model; there is no need to break the domain model down into smaller chunks to feed them to individual features. Such a reduction of the available information would be too artificial, too much hassle to maintain, too fragile, and it would not yield any considerable benefits. On the other hand, if an entire group of features only need a small common subset of the domain model, then you may wish to consider having a subdomain model for use by that group of features only.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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