16

I had an interesting discussion today with another developer about how to approach a class with a method that accepts a string and outputs string.

Imagine something like the following which is completely made up for the purpose of example

public string GetStringPart(string input)
{ 
   //Some input validation which is removed for clarity

   if(input.Length > 5)
        return input.Substring(0,1);

   if(input.Substring(0,1) == "B")
        return input.Substring(0,3);

   return string.empty;
}

A function which has some logic based on it's string input is added to a project using DI and has a DI Container in place. Would you add this new class with an interface and inject it where needed, or would you make it a static class? What are the pros and cons of each? Why would you (or not) want to make this something used with constructor injection rather than just accessed when required anywhere.

  • 1
    @Ewan For helper methods that has no use for abstraction. Example : Apache FileUtils. – Walfrat Nov 9 '17 at 15:15
  • 3
    @Ewan: static methods without side effects are the best kind of methods, because they are simple to understand and simple to test. – JacquesB Nov 9 '17 at 18:33
  • 1
    but they make it impossible to unit test things that depend on them – Ewan Nov 9 '17 at 18:38
  • 3
    @Ewan Eh, not really. Is Math.Max() bad because it is static? If you test your static method and it is working, you can use it safely on your other methods without issues. If the static one is failing, its test will catch it. – T. Sar Nov 10 '17 at 16:53
  • 1
    if 'static' guaranteed no side effects I might be able to see the argument. but it doesnt. – Ewan Nov 11 '17 at 8:29
19

There is no reason why this needs to be injected. This is just a function, it has no dependencies, so just call it. It can even be static if you want as it looks to be pure. One can write unit tests against this with no difficulty. If it is used in other classes, unit tests can still be written.

There is no need to abstract away functions with no dependencies, it's overkill.

If this becomes more complex then maybe passing an interface into a constructor or method is warranted. But, I wouldn't go down that road that unless I had complex GetStringPart logic based on location, etc.

  • 2
    This is the correct answer! To bad the cargo-cult answer have been accepted. – JacquesB Nov 9 '17 at 18:30
  • 3
    that the function has no dependencies isnt the point. the problem is when other things depend on the function and become tightly coupled to it – Ewan Nov 9 '17 at 18:37
  • 2
    What if the function changes in 6 months and isn't so pure, but it's already used in lots of places. It's not extensible as a static and it's also not something that can be isolated from consuming classes unit tests. If there is even a slight chance at change I'd go for injecting it. That said, I'm open to listening to opposing arguments, that's the point of this post. What are the downsides to this being injected and positives of a static? – James Nov 9 '17 at 18:49
  • 1
    Generally speaking, people discussing dependency injection on this web-site, don't have a need for dependency injection. Hence the bias toward unnecessary complexity. – Frank Hileman Nov 10 '17 at 0:38
  • 2
    @James If the function becomes less pure, you're doing something wrong and the function was probably not well defined from the start. Calculating the area of a square shouldn't suddenly need a database call or deploy a visual update for the UI. Can you imagine a scenario where a "Substring" method of sorts suddenly gets impure? – T. Sar Nov 10 '17 at 17:06
11

Here's why

class DOSClient {
    OrderParser orderParser;
    string orderCode;

    DOSClient(OrderParser orderParser, string ordercode) { 
        this.orderParser = orderParser; 
        this.ordercode = ordercode;
    }
    void DisplayOrderCode() {
        Console.Write( "Prefix: " + orderParser.GetStringPart(ordercode) ); 
        ...
    }
}

class GUIClient {
    OrderParser orderParser;
    string orderCode;
    GUI gui;

    GUIClient(OrderParser orderParser, string ordercode, GUI gui) { 
        this.orderParser = orderParser; 
        this.ordercode = ordercode;
        this.gui = gui;
    }

    void DisplayOrderCode() {
        gui.Prefix( orderParser.GetStringPart(ordercode) ); 
        ...
    }
}

 

class OrderParserUS : IOrderParser {

    public string GetStringPart(string input)
    { 
        //Some input validation which is removed for clarity

        if(input.Length > 5)
            return input.Substring(0,1);

        if(input.Substring(0,1) == "B")
            return input.Substring(0,3);

        return string.empty;
    }
}

class OrderParserEU : IOrderParser {

    public string GetStringPart(string input)
    { 
        //Some input validation which is removed for clarity

        if(input.Length > 6)
            return input.Substring(0,1);

        if(input.Substring(0,1) == "#")
            return input.Substring(0,3);

        return string.empty;
    }
}

If you had gone with a static method there would be no way to change the behavior of GetStringPart without either destroying the old behavior or polluting it with conditional logic. It's true that statics are evil globals in disguise but the fact that they disable polymorphism is my chief complaint about them. Static methods aren't first class in OOP languages. By giving the method an object to live in, even one with no state, we make the method portable. Its behavior can be passed around like the value of a variable.

Here I've imagined a system that needs to behave slightly differently when deployed in Europe then when deployed in the US. Rather then force either system to contain code only needed by the other we can change the behavior by controlling what order parsing object is injected in the clients. This allows us to contain the spread of the region detail. It also makes it easy to add OrderParserCanada without having to touch existing parsers.

If that means nothing to you then there really isn't a good argument for this.

BTW, GetStringPart is a terrible name.

  • *Cringe at brackets code style* I'm guessing you're a Java programmer trying to write C# code? Takes one to know one I suppose. :) – Neil Nov 9 '17 at 15:29
  • The question is not specific to a language, more about design. My problem with the static methods are they prevent isolation of code for testing. Developer I was talking to thinks I rely too heavily on DI and sometimes extension methods / static methods are relevant. I think possibly in rare instances but not as general purpose. Ongoing discussion that I felt was a good one to discuss further. I also agree with your case for polymorphism. – James Nov 9 '17 at 15:47
  • Testing is a reason but there are more. I don't like to blame this on testing to much because it just starts people criticizing testing. The simple fact is procedural programmers don't like it if you don't program procedurally. – candied_orange Nov 9 '17 at 15:54
  • Couldn't you simply say static getStringPartEU()? Your example only makes sense if there are other methods in that class that also requires the specialized EU treatment and they have to be treated as a single unit. – Robert Harvey Nov 9 '17 at 16:28
  • 2
    You are definitely arguing in favor of unnecessary complexity. Everything can have more and more code added to it. Things that need to change, should be easy to modify, but you should not try to predict what should need to change in the future. Static fields can be identical to global variables, but static methods can be pure, the best possible type of method. – Frank Hileman Nov 10 '17 at 0:36

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.