What's the down side of defining this extension method?

Question

I've got a bunch of duplicate code that looks like this:

If mValue is Nothing Return ""
Return mValue.ToUpper

I defined the following extension method to reduce duplicate code:

<System.Runtime.CompilerServices.Extension()>
Public Function EmptyIfNull(this As String) As String
    If String.IsNullOrEmpty(this) Then Return ""
    Return this
End Function

The duplicate code can be re-written as:

Return mValue.EmptyIfNull.ToUpper

Is there a downside to this?

I dont think. You are still using string.IsNullOrEmpty(), nothing more. Yes , there is a benefit i can see , you can use it in a single statement without any if and else condition. — , Dec 16 '11 at 16:05

score 2 · Accepted Answer · answered Dec 16 '11 at 16:02

2

The only downside is that you're essentially recreating what's already in the language (the null coalescing operator, or If function, as it's implemented in VB.NET)

Return If(mValue, "").ToUpper()

Should do what you're looking for.

As for your extension method, there's no need to call String.IsNullOrEmpty, as you only need to handle the case where it's Nothing.

answered Dec 16 '11 at 16:02

Adam Robinson

182,639
35
285
343

I prefer `mValue.EmptyIfNull.ToUpper` from a code readability standpoint. – Meta-Knight Dec 16 '11 at 16:03
Which is more readable? You can read what the extension method does, while you have to evaluate your code to know what it does. – jgauffin Dec 16 '11 at 16:04
I prefer ext method as more readable as well. Especially in C#, where `(mValue ?? String.Empty).ToUpper()` looks IMHO hideous. Also, by replacing `""` with the more correct `String.Empty` it starts to get long in typing as well. – Jon Dec 16 '11 at 16:05
@jgauffin: I'm not sure what that means. It sounds like you're making the case that you shouldn't use the types and functions provided by the framework (or, in this case, the language) because the code isn't part of your project. It seems wiser to be familiar with the language rather than reinventing what's already there without providing something meaningfully better. – Adam Robinson Dec 16 '11 at 16:06
1

@Jon: At the risk of getting side-tracked, `String.Empty` is no more "correct" than `""`. All string literals in code are interned, [and post .NET 2.0 any instance of `""` in your code actually refers to `String.Empty`](http://stackoverflow.com/questions/151472/what-is-the-difference-between-string-empty-and). – Adam Robinson Dec 16 '11 at 16:07
I like this better because I didn't have to write it and it's available everywhere. Thanks! – Seth Reno Dec 16 '11 at 16:08
@AdamRobinson: "More correct" in the sense that it's not a magic literal. Granted, `""` is a very special literal but a literal nonetheless. Personally I avoid using it, but granted this is a matter of preference. – Jon Dec 16 '11 at 16:14
@Jon: The point of avoiding literals is to use something meaningful instead. `String.Empty` is no more meaningful (in that its meaning is no clearer) than `""` any more than using something like `Count == int.Zero` (if such a constant existed) instead of `Count == 0`. Not everything needs to be a constant. – Adam Robinson Dec 16 '11 at 18:26
is "Return If" a keyword? any link to reference? I can't find anything about it. – ChatGPT Dec 29 '13 at 11:46
1

@MaxHodges: The `If()` function in VB.NET (when called with only two arguments) is its version of the null coalescing operator in C#. The result of the function is the first argument if it is not null, otherwise it is the second argument. Note that I'm referring to it as a function and that is what it looks like, when used in this way the second argument is only evaluated if the first argument is null (so if it's a function call, for example, the function will only be called if the first argument to `If` is null) – Adam Robinson Dec 29 '13 at 15:53
interesting! been using vb.net for years and never heard of this. I noticed it was illegal without the "return" so that confused me. Rather unconventional construct! Wouldn't this do the same thing? `return (mValue + "").ToUpper` – ChatGPT Dec 29 '13 at 16:19
1

@MaxHodges: I can't say for sure since I haven't used VB.NET in awhile, but that would depend on whether or not concatenating a `null` string substitutes an empty string or results in a `NullReferenceException`. But your substitution is only for this particular case; the `If` operator/function can operate on any nullable type, whereas the `+` operator could only operate on types (like string) that have defined it. – Adam Robinson Dec 29 '13 at 19:45
@AdamRobinson I see. makes sense. Thanks for clearing that up! – ChatGPT Dec 30 '13 at 05:59
here is the reference for the If() Operator is anyone needs it: http://msdn.microsoft.com/en-us/library/bb513985(v=vs.120).aspx – ChatGPT Dec 30 '13 at 06:03
@Jon: Re."String.Empty is no more meaningful ... than "" any more than using something like Count == int.Zero ... instead of Count == 0.": It's much easier to mistype and/or misread " " vs. "" or vice versa or "=" / "!=" / "<" vs. "==" or "1" vs. "0" than some other compile-able expression similar to "String.Empty" or "EmptyIfNull". Code is read much more often than it's written, and performance (i.e. in LOB vs. utility/STEM apps) usu. doesn't matter at least nowhere near as much as error/maintenance costs. With modern IDE's, no excuse not to type few extra chars. – Tom Dec 05 '17 at 01:24
@Jon: Actually, IMHO, I would say that every programming language should have an "isnull" keyword for Reference Types and every multi-element Type should have an "IsEmpty" Property. – Tom Dec 05 '17 at 01:26

score 1 · Answer 2 · answered Dec 16 '11 at 16:02

1

No, there is no downside to doing this. The method is simple, it covers a legitimate use case, and is well named.

The only thing to consider here is how you are actually using it: perhaps it would be more appropriate to throw an ArgumentNullException (or other exception of more appropriate type) if mValue is null -- but that depends on the caller.

answered Dec 16 '11 at 16:02

Jon

428,835
81
738
806

I agree. In many cases it's preferable to make sure that the value isn't null first, so that you don't need to call `EmptyIfNull` everywhere afterwards. – Meta-Knight Dec 16 '11 at 16:06

score 0 · Answer 3 · answered Dec 16 '11 at 16:02

0

It's easy to understand what it does (and that is the most important thing).

It also produces cleaner code.

answered Dec 16 '11 at 16:02

jgauffin

99,844
45
235
372

What's the down side of defining this extension method?

3 Answers3