28

I'm going to save some string payload in the database. I have two global configurations:

  • encryption
  • compression

These can be enabled or disabled using the configuration in a way that either only one of them is enabled, both are enabled or both are disabled.

My current implementation is this:

if (encryptionEnable && !compressEnable) {
    encrypt(data);
} else if (!encryptionEnable && compressEnable) {
    compress(data);
} else if (encryptionEnable && compressEnable) {
    encrypt(compress(data));
} else {
  data;
}

I'm thinking about the Decorator pattern. Is it the right choice, or is there perhaps a better alternative?

  • 5
    What's wrong with what you currently have? Are the requirements likely to change for this functionality? IE, are there likely to be new if statements? – Darren Young Jan 1 '16 at 14:33
  • No, I'm looking at any other solution to improve the code. – Damith Ganegoda Jan 1 '16 at 14:35
  • 46
    You're going about this backwards. You don't find a pattern then write code to fit the pattern. You write the code to fit your requirements, then optionally use a pattern to describe your code. – Lightness Races with Monica Jan 1 '16 at 14:43
  • 1
    note if you believe that your question is indeed a duplicate of this one, then as an asker you have an option to "override" recent reopen and singlehandedly close it as such. I did that to some of my own questions and it works like a charm. Here is how I did it, 3 easy steps - the only difference with my "instructions" is that since you have less than 3K rep, you will have to go through flag dialog to get to "duplicate" option – gnat Jan 1 '16 at 20:40
  • 8
    @LightnessRacesinOrbit: There's some truth in what you say, but it's perfectly reasonable to ask if there's a better way to structure one's code, and it's perfectly reasonable to invoke a design pattern to describe a proposed better structure. (Still, I agree that it's a bit of an XY problem to ask for a design pattern when what you want is a design, that may or may not strictly follow any well-known pattern.) Also, it's legitimate for "patterns" to affect your code slightly, in that if you're using a well-known pattern, it often makes sense to name your components accordingly. – ruakh Jan 2 '16 at 7:42
15

When designing code, you alaways have two options.

  1. just get it done, in which case pretty much any solution will work for you
  2. be pedantic and design a solution which exploits the quirks of the language an its ideology (OO languages in this case - the use of polymorphism as a mean to provide the decision)

I am not going to focus on the first of the two, because there is really nothing to be said. If you just wanted to get it to work, you could leave the code as it is.

But what would happen, if you chose to do it the pedantic way and actually solved the problem with design patterns, in the way you wanted it do?

You could be looking at the following process:

When designing OO code, most of the ifs which are in a code do not have to be there. Naturally, if you want to compare two scalar types, such as ints or floats, you are likely to have an if, but if you want to change procedures based on configuration, you can use polymorphism to achieve what you want, move the decisions (the ifs) from your business logic to a place, where objects are instantiated - to factories.

As of now, your process can go through 4 separate paths:

  1. data is neither encrypted nor compressed (call nothing, return data)
  2. data is compressed (call compress(data) and return it)
  3. data is encrypted (call encrypt(data) and return it)
  4. data is compressed and encrypted (call encrypt(compress(data)) and return it)

Just by looking at the 4 paths, you find a problem.

You have one process which calls 3 (theoretically 4, if you count not calling anything as one) different methods that manipulate the data and then returns it. The methods have different names, different so called public API (the way through which the methods communicate their behaviour).

Using the adapter pattern, we can solve the name colision (we can unite the public API) that has occured. Simply said, adapter helps two incompatible interfaces work together. Also, adapter works by defining a new adapter interface, which classes trying to unite their API implement.

This is not a concrete language. It is a generic approach, the any keyword is there to represent it may be of any type, in a language like C# you can replace it with generics (<T>).

I am going to assume, that right now you can have two classes responsible for compression and encryption.

class Compression
{
    Compress(data : any) : any { ... }
}

class Encryption
{
    Encrypt(data : any) : any { ... }
}

In an enterprise world, even these specific classes are very likely to be replaced by interfaces, such as the class keyword would be replaced with interface (should you be dealing with languages like C#, Java and/or PHP) or the class keyword would stay, but the Compress and Encrypt methods would be defined as a pure virtual, should you code in C++.

To make an adapter, we define a common interface.

interface DataProcessing
{
    Process(data : any) : any;
}

Then we have to provide implementations of the interface to make it useful.

// when neither encryption nor compression is enabled
class DoNothingAdapter : DataProcessing
{
    public Process(data : any) : any
    {
        return data;
    }
}

// when only compression is enabled
class CompressionAdapter : DataProcessing
{
    private compression : Compression;

    public Process(data : any) : any
    {
        return this.compression.Compress(data);
    }
}

// when only encryption is enabled
class EncryptionAdapter : DataProcessing
{
    private encryption : Encryption;

    public Process(data : any) : any
    {
        return this.encryption.Encrypt(data);
    }
}

// when both, compression and encryption are enabled
class CompressionEncryptionAdapter : DataProcessing
{
    private compression : Compression;
    private encryption : Encryption;

    public Process(data : any) : any
    {
        return this.encryption.Encrypt(
            this.compression.Compress(data)
        );
    }
}

By doing this, you end up with 4 classes, each doing something completely different, but each of them providing the same public API. The Process method.

In your business logic, where you deal with the none/encryption/compression/both decision, you will design your object to make it depend on the DataProcessing interface we designed before.

class DataService
{
    private dataProcessing : DataProcessing;

    public DataService(dataProcessing : DataProcessing)
    {
        this.dataProcessing = dataProcessing;
    }
}

The process itself could then be as simple as this:

public ComplicatedProcess(data : any) : any
{
    data = this.dataProcessing.Process(data);

    // ... perhaps work with the data

    return data;
}

No more conditionals. The class DataService has no idea what will really be done with the data when it is passed to the dataProcessing member, and it does not really care about it, it is not its responsibility.

Ideally, you would have unit tests testing the 4 adapter classes you created to make sure they work, you make your test pass. And if they pass, you can be pretty sure they will work no matter where you call them in your code.

So doing it this way I will never have ifs in my code anymore?

No. You are less likely to have conditionals in your business logic, but they still have to be somewhere. The place is your factories.

And this is good. You separate the concerns of creation and actually using the code. If you make your factories reliable (in Java you could even go as far as using something like the Guice framework by Google), in your business logic you are not worried about chosing the right class to be injected. Because you know your factories work and will deliver what is asked.

Is it necessary to have all these classes, interfaces, etc.?

This brings us back to the begining.

In OOP, if you choose the path to use polymorphism, really want to use design patterns, want to exploit the features of the language and/or want to follow the everything is an object ideology, then it is. And even then, this example does not even show all the factories you are going to need and if you were to refactor the Compression and Encryption classes and make them interfaces instead, you have to include their implementations as well.

In the end you end up with hundreds of little classes and interfaces, focused on very specific things. Which is not necessarily bad, but might not be the best solution for you if all you want is to do something as simple as adding two numbers.

If you want to get it done and quickly, you can grab Ixrec's solution, who at least managed to eliminate the else if and else blocks, which, in my opinion, are even a tad worse than a plain if.

Take into consideration this is my way of making good OO design. Coding to interfaces rather than implementations, this is how I have done it for the past few years and it is the approach I am the most comfortable with.

I personally like the if-less programming more and would much more appreciate the longer solution over the 5 lines of code. It is the way I am used to designing code and am very comfortable reading it.


Update 2: There has been a wild discussion about the first version of my solution. Discussion mostly caused by me, for which I apologize.

I decided to edit the answer in a way that it is one of the ways to look at the solution but not the only one. I also removed the decorator part, where I meant facade instead, which I in the end decided to leave out completely, because an adapter is a facade variation.

  • 28
    I didn't downvote but the rationale might be the ridiculous amount of new classes/interfaces to do something the original code did in 8 lines (and the other answer did in 5). In my opinion the only thing it accomplishes is increasing the learning curve for the code. – Maurycy Jan 1 '16 at 18:46
  • 6
    @Maurycy What OP asked was to try to find a solution to his problem using common design patterns, if such solution exist. Is my solution longer than his or Ixrec's code? It is. I do admit that. Does my solution solve his problem using design patterns and thus answers his question and also effectively removing all the necessary ifs from the process? It does. Ixrec's does not. – Andy Jan 1 '16 at 19:03
  • 26
    I believe that writing code that is clear, reliable, concise, performant and maintainable is the way to go. If I had a dollar for every time someone cited SOLID or quoted a software pattern without clearly articulating their goals and their rationale, I'd be a rich man. – Robert Harvey Jan 1 '16 at 19:14
  • 12
    I think I have two issues I see here. First is that the Compression and Encryption interfaces seem totally superfluous. I'm not sure if you are suggesting that they are somehow necessary to the decoration process, or merely implying that they represent extracted concepts. The second issue is that making a class like CompressionEncryptionDecorator leads to the same kind of combinatorial explosion as the OP's conditionals. I'm also not seeing the decorator pattern clearly enough in the code suggested. – cbojar Jan 1 '16 at 19:31
  • 5
    The debate on SOLID vs. simple is kinda missing the point: this code is neither, and it also does not use the decorator pattern. Code is not automatically SOLID just because it uses a bunch of interfaces. Dependency injection of a DataProcessing interface is kinda nice; everything else is superfluous. SOLID is an architecture-level concern aimed at handling change well. OP gave no information about his architecture nor how he expects his code to change, so we can't really even discuss SOLID in an answer. – Carl Leth Jan 2 '16 at 3:05
120

The only problem I see with your current code is the risk of combinatorial explosion as you add more settings, which can be easily be mitigated by structuring the code more like this:

if(compressEnable){
  data = compress(data);
}
if(encryptionEnable) {
  data = encrypt(data);
}
return data;

I'm not aware of any "design pattern" or "idiom" that this could be considered an example of.

  • 18
    @DamithGanegoda Nope, if you read my code carefully you'll see it does exactly the same thing in that case. That's why there's no else between my two if statements, and why I'm assigning to data each time. If both flags are true, then compress() gets executed, then encrypt() gets executed on the result of compress(), just like you want. – Ixrec Jan 1 '16 at 15:37
  • 14
    @DavidPacker Technically, so does every if statement in every programming language. I went for simplicity, since this looked like a problem where a very simple answer was appropriate. Your solution is valid too, but personally I'd save it for when I have a lot more than two boolean flags to worry about. – Ixrec Jan 1 '16 at 15:55
  • 15
    @DavidPacker: correct is not defined by how well the code adheres to some guideline by some author about some programming ideology. Correct is "does the code do what it's supposed to do and was it implemented in a reasonable amount of time". If it makes sense to do it the "wrong way", then the wrong way is the right way because time is money. – whatsisname Jan 1 '16 at 18:56
  • 9
    @DavidPacker: If I was in the OP's position, and asked that question, Lightness Race in Orbit's comment is what I really need. "Finding a solution using design patterns" is already starting from the wrong foot. – whatsisname Jan 1 '16 at 19:18
  • 6
    @DavidPacker Actually if you read the question more closely, it doesn't insist on a pattern. It states, "I'm thinking about the Decorator pattern. Is it the right choice, or is there perhaps a better alternative?". You addressed the first sentence in my quote, but not the second. Other people took the approach that no, it is not the right choice. You can't then claim that only yours answers the question. – Jon Bentley Jan 2 '16 at 1:29
12

I guess your question is looking not for practicality, in which case lxrec's answer is the correct one, but to learn about design patterns.

Obviously the command pattern is an overkill for such a trivial problem as the one you propose but for the sake of illustration here it goes:

public interface Command {
    public String transform(String s);
}

public class CompressCommand implements Command {
    @Override
    public String transform(String s) {
        String compressedString=null;
        //Compression code here
        return compressedString;
    }
}

public class EncryptCommand implements Command {
    @Override
    public String transform(String s) {
        String EncrytedString=null;
        // Encryption code goes here
        return null;
    }

}

public class Test {
    public static void main(String[] args) {
        List<Command> commands = new ArrayList<Command>();
        commands.add(new CompressCommand());
        commands.add(new EncryptCommand()); 
        String myString="Test String";
        for (Command c: commands){
            myString = c.transform(myString);
        }
        // now myString can be stored in the database
    }
}

As you see putting the commands/transformation in a list allows for ther to be executed sequentially. Obviously it will execute both, or only one of them dependind og what you put in the list without if conditions.

Obviously the conditionals will end up in some kind of factory that puts together the command list.

EDIT for @texacre's comment:

There are many ways of avoiding the if conditions in the creational part of the solution, let's take for example a desktop GUI app. You can have checkboxes for the compress and encrypt options. In the on clic event of those checkboxes you instantiate the corresponding command and add it to the list, or remove from the list if you are deselecting the option.

  • Unless you can provide an example of "some kind of factory that puts together the command list" without code that essentially looks like Ixrec's answer then IMO this doesn't answer the question. This provides a better way of implementing the compress and encrypt functions, but not how to avoid the flags. – thexacre Jan 3 '16 at 19:53
  • @thexacre I added an example. – Tulains Córdova Jan 3 '16 at 21:47
  • So in your checkbox event listener you have "if checkbox.ticked then add command"? It seems to me like you're just shuffling the flag if statements around... – thexacre Jan 3 '16 at 22:45
  • @thexacre No, one listener for each checkbox. In the click event just commands.add(new EncryptCommand()); or commands.add(new CompressCommand()); respectively. – Tulains Córdova Jan 3 '16 at 22:48
  • What about handling unchecking the box? In just about every language / UI toolkit I've encountered you'll still need to check the state of the checkbox in the event listener. I agree this is a better pattern, but it doesn't avoid needing basically if flag do something somewhere. – thexacre Jan 3 '16 at 22:55
7

I think "design patterns" are unneccesarily geared towards "oo patterns" and completely avoiding much simpler ideas. What we're talking about here is a (simple) data pipeline.

I would try to do it in clojure. Any other language where functions are first-class is probably ok as well. Maybe I could a C# example later on, but it's not as nice. My way of solving this would be the following steps with some explanations for non-clojurians:

1. Represent a set of transformations.

(def transformations { :encrypt  (fn [data] ... ) 
                       :compress (fn [data] ... )})

This is a map, i.e. a lookup table/dictionary/whatever, from keywords to functions. Another example (keywords to strings):

(def employees { :A1 "Alice" 
                 :X9 "Bob"})

(employees :A1) ; => "Alice"
(:A1 employees) ; => "Alice"

So, writing (transformations :encrypt) or (:encrypt transformations) would return the encrypt function. ((fn [data] ... ) is just a lambda function.)

2. Get the options as a sequence of keywords:

(defn do-processing [options data] ;function definition
  ...)

(do-processing [:encrypt :compress] data) ;call to function

3. Filter all transformations using the supplied options.

(let [ transformations-to-run (map transformations options)] ... )

Example:

(map employees [:A1]) ; => ["Alice"]
(map employees [:A1 :X9]) ; => ["Alice", "Bob"]

4. Combine functions into one:

(apply comp transformations-to-run)

Example:

(comp f g h) ;=> f(g(h()))
(apply comp [f g h]) ;=> f(g(h()))

5. And then together:

(def transformations { :encrypt  (fn [data] ... ) 
                       :compress (fn [data] ... )})

(defn do-processing [options data]
  (let [transformations-to-run (map transformations options)
        selected-transformations (apply comp transformations-to-run)] 
    (selected-transformations data)))

(do-processing [:encrypt :compress])

The ONLY changes if we want to add a new function, say "debug-print", is the following:

(def transformations { :encrypt  (fn [data] ... ) 
                       :compress (fn [data] ... )
                       :debug-print (fn [data] ...) }) ;<--- here to add as option

(defn do-processing [options data]
  (let [transformations-to-run (map transformations options)
        selected-transformations (apply comp transformations-to-run)] 
    (selected-transformations data)))

(do-processing [:encrypt :compress :debug-print]) ;<-- here to use it
(do-processing [:compress :debug-print]) ;or like this
(do-processing [:encrypt]) ;or like this
  • How is funcs populated to only include the functions which need to be applied without essentially using a series of if statements in one way or another? – thexacre Jan 3 '16 at 19:57
  • The row funcs-to-run-here (map options funcs) is doing the filtering, thus choosing a set of functions to apply. Maybe I should update the answer and go into a bit more detail. – NiklasJ Jan 4 '16 at 15:55
5

[ Essentially, my answer is a follow-on to the answer by @Ixrec above. ]

An important question: is the number of distinct combinations that you need to cover is going to grow? You are better aware of your subject domain. This is your judgement to make.
Can the number of variants possibly grow? Well, that's not inconceivable. For example, you may need to accommodate more different encryption algorithms.

If you anticipate that the number of distinct combinations is going to grow, then Strategy pattern can help you. It's designed to encapsulate algorithms and provide an interchangeable interface to the calling code. You would still have a small amount of logic when you create (instantiate) the appropriate strategy for each particular string.

You have commented above that you don't expect the requirements to change. If you don't expect that the number of variants would grow (or if you can defer this refactoring), then keep the logic the way it is. Currently, you have a small and manageable amount of logic. (Maybe put a note to self in the comments about a possible refactoring to a Strategy pattern.)

1

One way to do this in scala would be:

val handleCompression: AnyRef => AnyRef = data => if (compressEnable) compress(data) else data
val handleEncryption: AnyRef => AnyRef = data => if (encryptionEnable) encrypt(data) else data
val handleData = handleCompression andThen handleEncryption
handleData(data)

Using decorator pattern to achieve the above goals (separation of individual processing logic and how they wire together) would be too verbose.

Wherein you would require a design pattern to achieve these design goals in an OO programming paradigm, functional language offers native support by using functions as first class citizens (line 1 and 2 in the code) and functional composition (line 3)

  • Why is this better (or worse) than the OP's approach? And/or what do you think about the OP's idea to use a decorator pattern? – Kasper van den Berg Jan 6 '16 at 14:08
  • this code snippet is better and is explicit about ordering (compression before encryption); avoids unwanted interfaces – Rag Jan 8 '16 at 8:51

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.