2

I'm looking for an elegant way to choose a method signature (overloaded) and pass an argument based on a conditional. I have an importer that will either produce the most recent file to import, or take an explicit path for the data.

Currently my code looks like this:

if (string.IsNullOrEmpty(arguments.File))
{
    importer.StartImport();
}
else
{
    importer.StartImport(arguments.File);
}

I would like it to look like this (or conceptually similar):

importer.StartImport(string.IsNullOrEmpty(arguments.File) ? Nothing : arguments.File);

The idea being that a different method signature would be called. A few stipulations:

1) I will not rely on 'null' to indicate an unspecified file (i.e. anything besides null itself).
2) I will not pass the arguments struct to the importer class; that violates the single responsibility principle.

One solution I'm aware of, is having only one StartImport() method that takes a single string, at which point that method resolves the conditional and chooses how to proceed. I'm currently choosing to avoid this solution because it only moves the if-statement from one method to another. I'm asking this question because:

1) I would like to reduce "8 lines" of code to 1.
2) I'm genuinely curious if C# is capable of something like this.

Richard Pianka
  • 3,317
  • 2
  • 28
  • 36
  • 3
    Bad idea. Leave the `if...else` it is much easier to read. – Nate Jul 01 '11 at 22:42
  • The ternary operator here is probably too verbose, but I would prefer something closer to the null coalescing operator. – Richard Pianka Jul 01 '11 at 22:44
  • 1
    Do you get docked pay if you exceed some code line quota? I realize that's snarky, but terse code isn't always the way to go and I think this is one of those cases. Your intent is very clear in the first example. – Marc Jul 01 '11 at 22:54
  • Of course not :) but the more succinct code can be, while still being understandable, the better. – Richard Pianka Jul 01 '11 at 22:57

5 Answers5

4

I would like to reduce "8 lines" of code to 1.

I think you're asking the wrong question. It's not how many lines of code you have, it's how clear, maintainable, and debuggable they are. From what you've described, importing from a default location and importing with a known file are semantically different - so I think you're correct in separating them as two different overloads. In fact, you may want to go further and actually name them differently to further clarify the difference.

I'm genuinely curious if C# is capable of something like this.

Sure, we can use all sorts of fancy language tricks to make this more compact ... but I don't think they make the code more clear. For instance:

// build a Action delegate based on the argument...
Action importAction = string.IsNullOrEmpty(arguments.File) 
                            ? () => importer.StartImport() 
                            : () => importer.StartImport(arguments.File)
importAction(); // invoke the delegate...

The code above uses a lambda + closure to create a Action delegate of the right type, which is then invoked. But this is hardly more clear ... it's also slightly less efficient, as it requires creating a delegate and then invoking the method through that delegate. In most cases the performance overhead is completely negligible. The real problem here is the use of the closure - it's very easy to misuse code with closures - and it's entirely possible to introduce bugs by using closures incorrectly.

Community
  • 1
  • 1
LBushkin
  • 129,300
  • 32
  • 216
  • 265
  • Fair enough. I'm not sure your example is more 'compact' but your explanation is logical. I was hoping for something like importer.StartImport(??arguments.file); or something analogous. – Richard Pianka Jul 01 '11 at 23:16
  • @Richard: I formatted the example for clarity, you could easily collapse it into a single line and omit the temporary variable: `(string.IsNullOrEmpty(arguments.File) ? () => importer.StartImport() : importer.StartImport(arguments))();` However, you can't get rid of the overload resolution step. – LBushkin Jul 01 '11 at 23:20
  • so it's my fault for only specifying that I wanted to reduce 8 lines to 1. I could have done that by putting all of my code onto a single line. The point was to reduce the overall verbosity of the thing. – Richard Pianka Jul 02 '11 at 00:05
1

You can't do this using 'strong-typed' C#. Overload resolution is performed at compile time: the compiler will work out the static (compile-time) type of the argument, and resolve based on that. If you want to use one of two different overloads, you have to perform two different calls.

In some cases, you can defer overload resolution until runtime using the dynamic type. However, this has a performance (and clarity!) cost, and won't work in your situation because you need to pass a different number of arguments in the two cases.

itowlson
  • 73,686
  • 17
  • 161
  • 157
1

Yes, it is possible (using reflection). But I wouldn't recommend it at all because you end up with way more code than you had before. The "8-lines" which you have are quite simple and readable. The "one-liner" using reflection is not. But for completeness sake, here's a one-liner:

importer.GetType().GetMethod("StartImport", string.IsNullOrEmpty(arguments.File) ? new Type[0] : new Type[] { typeof(string) }).Invoke(importer, string.IsNullOrEmpty(arguments.File) ? new object[0] : new object[] { arguments.File) }));
carlosfigueira
  • 85,035
  • 14
  • 131
  • 171
  • Thanks! You've successfully dissuaded me from modifying this code. Does anybody feel the same way as I do about the existence of such an operator, or am I missing some coding principle? – Richard Pianka Jul 01 '11 at 22:55
  • Richard: It's an interesting idea, and there are cases where I could have used it, but the problem is it plays hob with compile-time overload resolution. That's not a 'coding principle' per se, but C# devs don't usually expect method resolution to be deferred until runtime unless they opt into it using `dynamic`. There could be performance and version-safety impacts (as with `dynamic`). Plus the need for a new keyword to represent "no arguments"... it's quite a bit of complexity for a bit of an edge case which can often be resolved using lambdas or an arguments type. – itowlson Jul 01 '11 at 23:05
0

Frankly, I think your code would be a lot more readable and modular if you combined the overloaded methods back into a single method and moved the conditional inside it instead of making the caller pre-validate the input.

importer.StartImport(arguments.File);

void StartImport(string File) {
  if (string.isNullOrEmpty(File)) {
      ...
  }
  else {
      ...
  }
}

Assuming you call the method from multiple places in your code you don't have the conditional OR ternary expression scattered around violating the DRY principle with this approach.

JohnFx
  • 34,542
  • 18
  • 104
  • 162
  • I call it only once (this snippet is literally from Main()) so I wasn't in violation of, or even worried about, DRY. Also, I believe that depending upon a string value from within the importer to determine how the import should happen is a violation of the SRP. It also becomes even more muddy during unit testing. – Richard Pianka Jul 01 '11 at 23:06
  • Not sure I agree on the SRP. The method still is responsible for the single responsibility principle. The method just does an import and takes a filename as a parameter, null is just another possible value for that parameter. However, if the null condition does complicated logic like prompting the user for a path, then I'd break that out into a sub-method, but still call it from within StartImport. – JohnFx Jul 01 '11 at 23:14
0
//one line 
if (string.IsNullOrEmpty(arguments.File)) {importer.StartImport();} else{importer.StartImport(arguments.File);}
Chris Catignani
  • 5,040
  • 16
  • 42
  • 49
Ryan Moran
  • 1