2

Here is two methods that does the same thing. One is repeating the call to the method Print where the other one does not but has an additional variable.

class Program
{
    static void Main(string[] args)
    {
        Snippet1(args);
        Snippet2(args);
    }

    private static void Snippet1(string[] args)
    {
        string message;
        if (args.Length == 0)
        {
            message = "No argument";
        }
        else
        {
            message = string.Format("Argument Count = {0}", args.Length);
        }
        Print(message);
    }

    private static void Snippet2(string[] args)
    {
        if (args.Length == 0)
        {
            Print("No argument");
        }
        else
        {
            Print(string.Format("Argument Count = {0}", args.Length));
        }
    }

    private static void Print(string s)
    {
        Console.WriteLine(s);
    }
}

I personally do not like having the same invocation twice and prefer to use a variable which make the if statement goal to define the message instead of defining + invoking. I also believe that it's more clear to have an unique invocation if this one need to be changed later.

What is the best practice/pattern that should be applied?

  • @jmoreno No. The OP isn't asking to have genuine working code reviewed; he has a question about one specific coding practice and has provided sample code to illustrate it. On topic here, off topic there. If this were posted on Code Review it would be closed – itsbruce Jul 17 '15 at 8:40
  • @jmoreno this is indeed not a real code but a sample as mentioned by itsbruce. – Patrick Desjardins Jul 17 '15 at 14:38
3

I'd clearly choose the extra variable approach, because DRY. The varaible name additionally conveys the meaning of the argument (e.g. it could be warning or log_entry, etc).

  • 1
    Sure, but you're repeating yourself with two assignments, too. Why not just Print(args.length == 0 ? "No arguments" : String.format(...)) for maximum conciseness? I do agree about the variable name having the potential to indicate purpose, though. – Joshua Taylor Jul 17 '15 at 2:25
  • @JoshuaTaylor: if the calculation is as trivial as in the example code, a ternary operator will indeed suffice. If the calculation is somehow longer, a final assignment to a variable in each branch will not be too much repetition. Even better, if each branch could be wrapped into a separate function, the terseness could be preserved: Print(predicate ? calculationOne() : calculationTwo()). This is not always practical because of the need to pass too much context into the functions, though. – 9000 Jul 17 '15 at 15:27
  • @Joshua the code sample is very simple. In fact, this could be something else than printing a String but at the end having to invoke something with the built object. – Patrick Desjardins Jul 17 '15 at 15:31
  • @9000 I completely agree. My point was much more that in code this size, it's difficult to make firm judgments about what should and shouldn't be duplicated. In practice, and on this scale, I'd probably do exactly what you suggest in your answer. – Joshua Taylor Jul 17 '15 at 15:40
3

I also believe that it's more clear to have an unique invocation if this one need to be changed later.

The uniqueness of the invocation can be subject to change, too.

Maybe you don't want to print anything any more if there is no argument and you don't want to call Print at all in this case.

I don't think there is a definite answer to your question and it always depends on the specific use case.

  • This is interesting, so would you say that it is better to have duplication in case of some possible future scenario or keep the code without any duplication for the present time and adjust in a limited scope (since we have 1 method to change)? The example provided is pretty simple, but the IF could had some else if too which would have produce a way bigger redundancy of the invocation. I am also curious about scenario that has more than 1 parameter too. – Patrick Desjardins Jul 16 '15 at 22:39
  • 1
    You can always return from the function to prevent the call to Print at the end. The point was that you expressed a preference for the solution based on assumptions about what possible changes look like. That's great and makes you prepared for those changes. The unexpected changes are the ones that cause trouble. "I do X to be prepared for changes." - "_ Cool, but we have to change X!_". For more complex cases, you probably wouldn't call the same function anyway. Again, I don't think the general definite answer you are looking for exists. – null Jul 16 '15 at 23:02
  • @PatrickDesjardins It really is situation dependent. I've had cases where I had the simple case, and could use a variable+invocation. Then, later, the surrounding code was changed, and I had to print a second value on one side of the if statement, but not the other. By then it was too late to refactor, so I ended up using string concatenation to combine both values into a single message to be printed! UGLY! – Cort Ammon Jul 17 '15 at 2:31
  • @CortAmmon I think you are right. The code sample provided may have simplify too much the reality. The perspective that I had whilst writing the question was that you need to build some of your variables and then call something. These variables can change depending of conditions. Then, you always call the same method. – Patrick Desjardins Jul 17 '15 at 15:40
3

Tomorrow your boss will tell you that they want to have most of the messages in RED color and a few in BLUE with extra padding... and you are pretty much into major refactoring with both of your approaches.

Your Program class is responsible for decision making and printing at the same time. That way Single Responsibility Principle has been broken.

So, I would add even more separation of concerns:

class Program {
    public static void Main(string[] args) {
        var simplePrinter = new SimplePrinter();
        var errorPrinter = new ErrorPrinter();

        var objectWithError = new MyObject(errorPrinter);
        objectWithError.DoSomething();

        var simpleObject = new MyObject(simplePrinter);
        simpleObject.DoSomething();
    }
}

public interface IConsolePrinter {
    void Print(string s);
}

public class SimplePrinter : IConsolePrinter {
   public void Print(string s) {
       Console.WriteLine(s);
   }
}

public class ErrorPrinter : IConsolePrinter {
   public void Print(string s) {
       Console.WriteLine(String.Format("**** ERROR: {0}", s));
   }
}

public class MyObject {
    private ConsolePrinter _printer;

    public MyObjectThatUsesConsolePrinter(IConsolePrinter printer) {
        _printer = printer;        
    }

    public void DoSomething() {
        _printer.Print("Love OOP! Respect Single Responsibility Principle!");
    }
}
  • this is totally true, however, there is a balance between efficiency. The case I try to illustrate is between a very simple case and a large application. Having classes for every creation building is often an over head when it's related to simple invocation. For example, you are creating in JavaScript an Html Link with different classes, properties, etc. You may just want to create the link without having to build class, thus having the scenario illustrated in the question. – Patrick Desjardins Jul 17 '15 at 15:44
  • That's exactly what I am getting onto, right now, your application is simple, but unless you never plan to have it grow(Example: learning program that you wrote and ditched, that's the only non growing application I can think of), it's easier to put in place some basic application design early on than refactor it later. My answer is how I would have done it regardless of the size of the application. – Alexus Jul 17 '15 at 20:06
  • thank you for your feedback. This is really interesting. However, from my enterprise experience, their is a line between the theory and practice. Doing so in every situation is time consuming and complexity in some case the invocation . However, I agree with you that your solution is the one the scale the most in time with the grow of the application. – Patrick Desjardins Jul 17 '15 at 22:10
  • Absolutely agree with you as well, there is always a fine line between over engineering and just engineering. – Alexus Jul 17 '15 at 22:15
2

A competent production-quality industrial-strength compiler, with optimization turned on, will automagically transform Snippet2 into Snippet1. If your compiler falls into that category (and most do), there's no real reason for a preference.

Personally, being one who believes in being terse, I'd've coded it as:

private static void Snippet3(string[] args)
{
    Print((args.Length != 0)?string.Format("Argument Count = {0}",
                                           args.Length)
                            :"No argument");
}

(assuming that Java allows the ternary operator...)

  • Being pedantic: It's C#, I believe. Look at the CamelCase functions! – Kroltan Jul 17 '15 at 1:55
  • @Kroltan: I'm not a C# guy, so I wouldn't know the difference between C# and Java. It looks like Java to me. – John R. Strohm Jul 17 '15 at 14:09
  • @JohnRStrhohm Exactly, the languages at their basic level look so much the same. Don't take it offensively, though. :) – Kroltan Jul 17 '15 at 14:13
  • For me, that answer is the same as not repeating the Print method. You have it once. I wasn't concerned about the compiler but more about the maintainability of the code. – Patrick Desjardins Jul 17 '15 at 14:35

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.