3
\$\begingroup\$

Preface: I'm starting to learn C# and I don't want to port any of my bad habits from other languages, so I'm following convention wherever possible.

Using the default Visual Studio code formatting, this relatively simple function requires 14 lines of code.

public void add(int[] values, Func<int, int> transform = null)
{
    foreach (int v in values)
    {
        if (transform == null)
        {
            add(v);
        }
        else
        {
            add(transform(v));
        }
    }
}

My first thought was to use the ?? operator to do something like the following snippet, but apparently that's gibberish.

public void add(int[] values, Func<int, int> transform = null)
{
    foreach (int v in values)
    {
        add(transform(v) ?? v);
    }
}

Is there a more concise and/or readable way to write the following method?

\$\endgroup\$
3
\$\begingroup\$

There are at least two answers. The short one:

public void add(int[] values, Func<int, int> transform = null)
{
    foreach (int v in values)
        add( (transform != null) ? transform(v) : v);
}

The efficient one:

public void add(int[] values, Func<int, int> transform = null)
{
    if( transform != null) 
      foreach (int v in values)
        add( transform(v));
    else
      foreach (int v in values)
        add( v);
}

Not nice, but you wanted a short version.

Maybe someone has a LINQ at hand which does this even better.

\$\endgroup\$
  • \$\begingroup\$ Your first block of code is what I'm looking for. Also, your 'efficient' block may not be so efficient. I'm believe the JIT would use branch prediction to optimize your first approach. msdn.microsoft.com/en-us/magazine/dd569747.aspx#id0400010 \$\endgroup\$ – Lawrence Barsanti Jan 16 '14 at 23:09
  • \$\begingroup\$ The second snippet sounds very much like a premature optimization to me. A null check is very cheap, there is no reason to make your code twice as long just for that. \$\endgroup\$ – svick Jan 17 '14 at 8:42
2
\$\begingroup\$

It may not be more concise, but it is definitely more readable.

public void addMany(int[] values)
{
  foreach (int v in values)
    {
      add(v);
    }
 }

public void addTransformMany(int[] values, Func<int,int> transform)
{
  foreach (int v in values)
    {
      add(transform(v));
    }
 }

Maybe not what you are looking for, but I think having two methods that are explicit about what they do is much more readable than a single method with an execution branch that depends on an optional parameter. The method signature should make it clear what it does, without requiring the reader to inspect the code.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the response, however, I don't really agree in this specific case. It seems pretty logical to me, that, if you pass in a transform function, it will be used while adding the values. In general, I do agree with the idea, especially when the branches are unrelated. \$\endgroup\$ – Lawrence Barsanti Jan 16 '14 at 23:22
  • \$\begingroup\$ @LawrenceBarsanti I suppose my answer is somewhat subjective. I have found that optional parameters (with default values) can cause headaches in C# code in the long run. I think it is similar to having multiple public constructors for a class, rather than factory methods that describe the intent of the constructor definitions. \$\endgroup\$ – Sheldon Warkentin Jan 17 '14 at 5:12
1
\$\begingroup\$

Your original use of the null coalescing operator didn't work because it was testing the output of calling the method transform(v) and not testing whether transform is null.

If transform was null, the operation would throw an exception.

@JensG use of the conditional operator is the most concise.

Another option is to declare a NullTransform

Func<int,int> NullTransform = x=>x;

And use this when transform is not set.

public void add(int[] values, Func<int, int> transform = null)
{
  transform = transform ?? NullTransform; // Optionally test and set here.
  foreach (int v in values)
    add(transform(v));
}

Sadly, you cannot set the default in the method call to the NullTransform instance.

\$\endgroup\$
  • \$\begingroup\$ The code is from a simple Accumulator object. The other add() method adds a single value to the accumulator, while, the one in my question adds an array of values to to the accumulator. \$\endgroup\$ – Lawrence Barsanti Jan 17 '14 at 12: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.