DotNetAnalyzers / NullParameterCheckRefactoring

Null Parameter Check Refactoring for Reference Type Parameters
MIT License
14 stars 4 forks source link

don't suggest if there is already a string.IsNullOrEmpty or string.IsNullOrWhitespace check #4

Closed tugberkugurlu closed 9 years ago

tugberkugurlu commented 9 years ago

Maybe we should not suggest this refactoring if there is already a string.IsNullOrEmpty or string.IsNullOrWhitespace checks for the string parameter.

sharwell commented 9 years ago

These checks are typically separate. A null string throws ArgumentNullException, where an empty string (or whitespace when that is invalid) throws an ArgumentException.

tugberkugurlu commented 9 years ago

@sharwell This came up in a twitter discussion. What I assume from these usages is that: the developer already covered null cases. Wrong? I have never seen both null and IsNullOrEmpty check usages for a string parameter together.

sharwell commented 9 years ago

@tugberkugurlu I believe there are more than 100 examples in this repository:

https://github.com/openstacknetsdk/openstack.net/search?q=isnullorempty

tugberkugurlu commented 9 years ago

Why do you check for null two times :smile:? IMO, it should be this:

if(content == null)
{
    throw new ArgumentNullException("content");
}

// or "" instead of string.Empty
if(content == string.Empty)
{
    throw new ArgumentException("content cannot be empty");
}

Or, only with string.IsNullOrEmpty check with an unspecific error message which is not my favorite.

It is too illogical to me but I get it. Let's leave it as is for now.