3

I am in the process of resharpening my solution, using the just-released version of Resharper (2016.2.2)

It flags this line of code:

ReportRunnerConstsAndUtils.ConvertValueToAppropriateTypeAndAssign(totalPackagesCell, packages);

...intimating that I should "Invoke as extension method"

If I acquiesce, it changes that line to this:

totalPackagesCell.ConvertValueToAppropriateTypeAndAssign(packages);

Is this better? If so, how? why?

Here is the method being called, which is in a "ConstsAndUtils" class:

// Adapted from https://stackoverflow.com/questions/26483496/is-it-possible-to-ignore-excel-warnings-when-generating-spreadsheets-using-epplu
public static void ConvertValueToAppropriateTypeAndAssign(this ExcelRangeBase range, object value)
{
    string strVal = value.ToString();
    if (!String.IsNullOrEmpty(strVal))
    {
        decimal decVal;
        double dVal;
        int iVal;

        if (decimal.TryParse(strVal, out decVal))
            range.Value = decVal;
        if (double.TryParse(strVal, out dVal))
            range.Value = dVal;
        else if (Int32.TryParse(strVal, out iVal))
            range.Value = iVal;
        else
            range.Value = strVal;
    }
    else
        range.Value = null;
}
B. Clay Shannon-B. Crow Raven
  • 8,547
  • 144
  • 472
  • 862
  • 5
    It certainly seems cleaner and clearer to me. ReSharper makes many preference-type recommendations though. You should be able to turn that off in the options if you don't like it. – itsme86 Sep 13 '16 at 16:15
  • 1
    As itsme86 says, it's personal preference... ReSharper also has an option to perform a reverse refactor "Convert Extension Method to Plain Static" https://www.jetbrains.com/resharper/features/code_refactoring.html#Convert_Extension_Method_to_Plain_Static – Scott Perham Sep 13 '16 at 16:18
  • 1
    This is opinion based. There is no better way of doing it. Most people think extensión methods look cleaner but thats about it. About the code you've posted, when is `double.TryParse` going to be called succesfully? (your missing an `else` by the way). – InBetween Sep 13 '16 at 16:21
  • 2
    I would say that if a static method is made into an extension method then it would make sense to call it as such as it will result in more terse code. On the other hand if it doesn't make sense, for whatever reason, to call in in that manner then maybe the issue is that the method shouldn't be an extension method in the first place. – juharr Sep 13 '16 at 16:22
  • @InBetween: Thanks, but why is double.TryParse a problem? – B. Clay Shannon-B. Crow Raven Sep 13 '16 at 16:23
  • Unless you are using some extreme ranges, what number would fail `decimal.TryParse` but succeed wtith `doubke.TryParse` and why use the former if your going to store it in a range? – InBetween Sep 13 '16 at 16:24
  • Further to what @InBetween says, when would `Int32.TryParse` succeed when `double.TryParse` fails? Seems like you'd try the strictest type first then `else if` up to the least strict. – juharr Sep 13 '16 at 16:27

3 Answers3

4

As some of the comments have indicated, this is at least partially a preference issue. Personally, I think it's "cleaner" and clearer to use an extension method here but some people may disagree with this.

"Under the hood," of course, the extension method is a static method (not an actual instance method), it's just that the compiler's giving you some syntactic sugar here (but that's besides the point).

2

It's recommended that you invoke it as an extension method, because you (or someone) CREATED it as an extension method. The syntax this ExcelRangeBase range makes that method an extension method, and so for consistencies sake, it should be used as an extension method when it is invoked. Otherwise you have lines that read ReportRunnerConstAndUtils.ConvertValueToAppropriateTypeAndAssign(range) and lines that do the exact same thing that read range.ConvertValueToAppropriateTypeAndAssign().

C#6 introduced some new syntax, and now you can have using ReportRunnerConstAndUtils at the top of you file and then ConvertValueToAppropriateTypeAndAssign(range) at your call site.

jmoreno
  • 12,752
  • 4
  • 60
  • 91
  • Ok, thanks; what's interesting to me is that when I do this, I no longer need to specify where the extension method is. Doing it the "wrong" way, I have to prepend the method name with "ReportRunnerConstsAndUtils" (the .cs file that contains the public method); doing it the "right" way, that is unnecessary. I'm not really complaining, but I wonder why. – B. Clay Shannon-B. Crow Raven Oct 26 '16 at 14:50
  • 1
    @B.ClayShannon: the reason is because C#, unlike VB doesn't automatically pull all static classes into scope, so given the method(parameter) syntax the compiler literally can not find a matching method. That was a design choice, which they have slightly tweaked with C#6, you can now specify a class to pull into scope, and it will be able to find it without the class name. – jmoreno Oct 27 '16 at 10:04
1

Being this an opinion based question, its not really answerable but I'd like to point out the following.

In this particular case, I would not use an extension method, simply because the method returns void; methods that don't return something and simply cause side effects are not good extension method candidates and I find them more readable as standard static method calls.

I try to keep my extension methods ase "pure" as posible, but like I said, this is my personal opinion.

If you think about it, extension methods were implemented to make LINQ posible which is a very functional side of C#. I tend to keep the same "feeling" in any extension methods I implement.

InBetween
  • 32,319
  • 3
  • 50
  • 90
  • I didn't take the OP's question to be opinion based. OP asked why Re-Sharper made the suggestion. The fact that Re-Sharper makes a suggestion based on personal preference is not an opinion. – devlin carnate Sep 13 '16 at 17:00
  • @devlincarnate That Resharper is making the suggestion is obviously a fact, I'm not claiming that to be an opinion based question, but if your read the OP's question carefully you'll notice that he is also asking why, how and what makes Resharper's suggestion better and the answer to those more important questions are opinion based. – InBetween Sep 13 '16 at 17:39