49

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.

Lotok
  • 1,759
  • 2
  • 17
  • 27
  • is there any argument at all for static? – Ewan Nov 09 '17 at 14:55
  • 2
    @Ewan For helper methods that has no use for abstraction. Example : Apache FileUtils. – Walfrat Nov 09 '17 at 15:15
  • @candiedOrange sums it up. abstraction is always useful because it prevents coupling. YAGNI is never a good argument if you are asking 'what is technically the best' – Ewan Nov 09 '17 at 15:26
  • 7
    @Ewan: static methods without side effects are the best kind of methods, because they are simple to understand and simple to test. – JacquesB Nov 09 '17 at 18:33
  • 1
    but they make it impossible to unit test things that depend on them – Ewan Nov 09 '17 at 18:38
  • Agree with JacquesB. Anything that can be static should be. – Frank Hileman Nov 10 '17 at 00:34
  • 5
    @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
  • say my function needs the max of 1 and maxint +1 and ive not covered that in my max tests. Your argument is YAGNI again. choosing a super simple case and ignoring the problems – Ewan Nov 11 '17 at 08:20
  • 2
    if 'static' guaranteed no side effects I might be able to see the argument. but it doesnt. – Ewan Nov 11 '17 at 08:29
  • 1
    @Ewan Static guarantees no side-effects when implemented properly, in the same fashion that Unit testing doesn't guarantees code coverage if not implemented properly nor DI guarantees loose coupling if not implemented properly. No language feature, coding practice, pattern or architecture can protect you from shooting yourself on the foot if you're really motivated to do so. – T. Sar Nov 13 '17 at 10:28
  • 1
    must be some new meaning of 'guarantee' which means 'the same as an instance method but tightly coupled' – Ewan Nov 13 '17 at 11:39
  • My take would be that It it's a pure function but contains bussiness logic that may change, then make it a singleton dependency instead. If it's something as obvious as Math.Max it's probably already in the framework somewhere, unless you are doing some very specific maths then why not just static? The point is, you never know with logic, it may change. You may, for example, need some scope during version transitions for feature flags, that are received in the context, so you can decide wich version of the function to use. – Aridane Álamo Oct 28 '22 at 17:56
  • Do your real life cases need to be able to change it without having to recompile it, or provide it later as you don't know yet? – Thorbjørn Ravn Andersen Jan 18 '23 at 04:59

2 Answers2

46

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.

Robert Harvey
  • 198,589
  • 55
  • 464
  • 673
Jon Raynor
  • 10,905
  • 29
  • 47
  • 4
    This is the correct answer! To bad the cargo-cult answer have been accepted. – JacquesB Nov 09 '17 at 18:30
  • 5
    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 09 '17 at 18:37
  • 3
    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? – Lotok Nov 09 '17 at 18:49
  • 2
    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 00:38
  • 1
    @Ewan Tight coupling is good, not bad. Bad coupling is coupling that is not needed or makes life more difficult. There is no evidence of that here; you are trying to predict future changes. – Frank Hileman Nov 10 '17 at 16:49
  • 1
    "_What if the function changes in 6 months and isn't so pure,_" - I think if you implemented a piece of logic that might change as a static method, then perhaps it was a wrong approach. I guess static methods have their cases, eg operations that will not change and are well defined such as **String.IsNullOrEmpty** (which could be used in lots of places). In other words, for very specific cases IMO you can apply static methods without having problems with it. – Emerson Cardoso Nov 10 '17 at 16:53
  • 3
    @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
  • 1
    there is _trying_ to predict future changes and predicting future changes. I do the later – Ewan Nov 10 '17 at 19:07
15

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 than when deployed in the US. Rather than force either system to contain code only needed by the other, we can instead change the behavior by controlling which 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.

candied_orange
  • 102,279
  • 24
  • 197
  • 315
  • 2
    \*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 09 '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. – Lotok Nov 09 '17 at 15:47
  • 1
    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 09 '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 09 '17 at 16:28
  • 1
    @RobertHarvey you could. But then the clients KNOW they are in EU. I'm trying to restrict that knowledge to only the composition root. – candied_orange Nov 09 '17 at 16:34
  • You're saying that DI only works with classes. I would counter that first-class functions solve that problem. It's not the fashion nowadays, of course, but since functional programming is becoming more in vogue... – Robert Harvey Nov 09 '17 at 16:37
  • 1
    @RobertHarvey First class functions certainly solve this. All I want is the polymorphism. If that's not an issue the only reason to put this in an object is to group it with other methods you want to travel around with it and/or whose behavior you want to change together as the state changes. Without that it's just a bunch of needless extra code. – candied_orange Nov 09 '17 at 16:42
  • Nice answer, BTW. You're getting better at this. Note that, in C# anyway, lambda expressions, anonymous methods, events and delegates are all considered first-class functions. – Robert Harvey Nov 09 '17 at 16:44
  • Let us [continue this discussion in chat](http://chat.stackexchange.com/rooms/68470/discussion-between-candiedorange-and-robert-harvey). – candied_orange Nov 09 '17 at 16:47
  • 4
    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 00:36
  • @FrankHileman I double your words. Pure, static methods, when implemented with the proper design, can be a valuable tool on the toolset. – T. Sar Nov 10 '17 at 16:56