Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added non-generic DehumanizeTo - addresses #92 #93

Merged
merged 2 commits into from
Feb 18, 2014
Merged

Conversation

MehdiK
Copy link
Member

@MehdiK MehdiK commented Feb 17, 2014

No description provided.

@JeremyThomas
Copy link

Yeah looks fine.

Though you could DRY it out by simply making the generic version call the non-generic then casting the result.

Thanks for adding.

MehdiK added a commit that referenced this pull request Feb 18, 2014
added non-generic DehumanizeTo - addresses #92
@MehdiK MehdiK merged commit 4ff641a into master Feb 18, 2014
@MehdiK MehdiK deleted the DehumanizeTo branch February 18, 2014 08:25
@MehdiK
Copy link
Member Author

MehdiK commented Feb 18, 2014

This is now released as v1.11.3 on NuGet

@JeremyThomas
Copy link

Actually I spoke too soon, I can't use this as throws an exception if not found, where I need it to return null.
It makes sense for the generic version to throw the exception since the return value can't be null but why do the same for the non-generic?
BTW I've changed mine to be a one-liner.

    private static Enum DehumanizeTo(string input, Type enumType)
    {
       return Enum.GetValues(enumType).Cast<Enum>().FirstOrDefault(value => string.Equals(value.Humanize(), input, StringComparison.OrdinalIgnoreCase));
    }

On a side note: EnumHumanizeExtensions.Humanize throws a null exception if the enum is null, but that looks consistent with all the other Humanize methods so fairplay

@MehdiK
Copy link
Member Author

MehdiK commented Feb 19, 2014

I personally think DehumanizeTo should throw exception if it cannot match. It's kinda like you're asking the framework to do a wrong thing and it complains.

One solution is to keep the current behavior as the default and also allow it to return null by passing a second optional parameter to the method (bool throwIfCannotMatch = true); but then it becomes inconsistent with the generic method and I think that inconsistency is going to be very surprising. Assume the case when you use both variations: one known at compile time and one used through reflection. The usage then looks completely different where one is wrapped in try-catch and another is null-checked!!

What do you think?

@JeremyThomas
Copy link

I agree its hard to see the best way to go. If we look at the equivalent methods in the BCL:
Enum.Parse throws ArgumentException when no match
Enum.TryParse the generic version obviously doesn't
the BCL also provides Enum.IsDefined for which you don't have an equivalent but that seems inefficient to use to avoid throwing an exception

    if (Enum.IsDefined(coreType, strOfEnum))
        return Enum.Parse(coreType, strOfEnum, true);

But I don't like how the BCL does it anyway i.e., I would prefer it if Enum.Parse returned null, but there are a lot of places where, IMHO, the BCL throws exceptions when it would be better if it didn't.
I'm fine with the throwIfCannotMatch idea which is similar to the throwOnError param to Type.GetType() in BCL.
If consistency is the main concern you could give the non-generic version a slightly different name - e.g. DehumanizeToOrNull

Some opinions here: http://stackoverflow.com/questions/175532/should-a-retrieval-method-return-null-or-throw-an-exception-when-it-cant-prod

BTW why did you roll your own exception rather than use ArgumentException like Enum.Parse?

@MehdiK
Copy link
Member Author

MehdiK commented Feb 20, 2014

Nah, I guess it's cool to add the optional param to the non-generic method (keeping the same name); but instead of a bool we should use an enum so instead of DehumanizeTo(sometype, false) users get something like DehumanizeTo(sometype, NoMatch.ReturnsNull) which explains the behavior and prevents any surprises.

WRT ArgumentException vs custom exception, the methods are both throwing ArgumentException if the target type is not an Enum which is different from when a match cannot be found. I prefer explicit exceptions for each error as opposed to hey if you get this exception it could mean one of many errors, like what happens with ArgumentException in Enum.Parse:

enumType is not an Enum.
-or-
value is either an empty string or only contains white space.
-or-
value is a name, but not one of the named constants defined for the enumeration.

That's too vague IMO.

@MehdiK
Copy link
Member Author

MehdiK commented Feb 20, 2014

Do you want to send me a PR with the optional param? :)

I guess before it's too late we could also rename the custom exception to something a bit more generic like NoMatchFoundException to go with the NoMatch enum?!

@JeremyThomas
Copy link

PR. is this what you were after?

public static class EnumDehumanizeExtensions
  {
    /// <summary>
    /// Dehumanizes a string into the Enum it was originally Humanized from!
    /// </summary>
    /// <typeparam name="TTargetEnum">The target enum</typeparam>
    /// <param name="input">The string to be converted</param>
    /// <exception cref="ArgumentException">If TTargetEnum is not an enum</exception>
    /// <exception cref="NoMatchFoundException">Couldn't find any enum member that matches the string  + input</exception>
    /// <returns></returns>
    public static TTargetEnum DehumanizeTo<TTargetEnum>(this string input)
        where TTargetEnum : struct, IComparable, IFormattable, IConvertible
    {
      return (TTargetEnum)DehumanizeToPrivate(input, typeof(TTargetEnum), NoMatch.ThrowsException);
    }

    /// <summary>
    /// Dehumanizes a string into the Enum it was originally Humanized from!
    /// </summary>
    /// <param name="input">The string to be converted</param>
    /// <param name="targetEnum">The target enum</param>
    /// <param name="whatToDoWhenNotMatched">What to do when input is not matched to the enum.</param>
    /// <returns></returns>
    /// <exception cref="NoMatchFoundException">Couldn't find any enum member that matches the string  + input</exception>
    /// <exception cref="ArgumentException">If targetEnum is not an enum</exception>
    public static Enum DehumanizeTo(string input, Type targetEnum, NoMatch whatToDoWhenNotMatched = NoMatch.ThrowsException)
    {
      return (Enum) DehumanizeToPrivate(input, targetEnum, whatToDoWhenNotMatched);
    }

    private static object DehumanizeToPrivate(string input, Type targetEnum, NoMatch whatToDoWhenNotMatched)
    {
      var match = Enum.GetValues(targetEnum).Cast<Enum>().FirstOrDefault(value => string.Equals(value.Humanize(), input, StringComparison.OrdinalIgnoreCase));

      if (match == null && whatToDoWhenNotMatched == NoMatch.ThrowsException)
        throw new NoMatchFoundException("Couldn't find any enum member that matches the string " + input);
      return match;
    }
  }

  public enum NoMatch
  {
    ThrowsException,    
    ReturnsNull
  }

  /// <summary>
  /// This is thrown on String.DehumanizeTo enum when the provided string cannot be mapped to the target enum
  /// </summary>
  public class NoMatchFoundException : Exception
  {
    public NoMatchFoundException()
    {
    }

    public NoMatchFoundException(string message)
      : base(message)
    {
    }

    public NoMatchFoundException(string message, Exception inner)
      : base(message, inner)
    {
    }
  }

@MehdiK
Copy link
Member Author

MehdiK commented Feb 20, 2014

Thanks.

This looks nice. Although it is missing the unit tests, explaining the feature in readme and adding the PR and broken change to release_notes :p

@JeremyThomas
Copy link

Ahh, I've just read https://github.com/MehdiK/Humanizer#how-to-contribute, which I missed before, I don't suppose you've thought about adding a table of contents to your readme.md?
Anyway, I guess it's time I learnt to use Git, won't be anytime soon but.

WRT ArgumentException vs custom exception, what does it matter if there are different ArgumentExceptions for different situations? As long as the exception message is different its just as clear as having a custom exception I would think.

@MehdiK
Copy link
Member Author

MehdiK commented Feb 24, 2014

ToC for readme - good idea. I guess it's big enough to need that.

Well, I guess this is a good opportunity to learn git then :)

Reading message is easy for humans but hard to do programatically. I hate to catch an exception and have to put an if condition around the message to find out what exactly went wrong. Catching different types of exceptions helps avoid that.

@MehdiK
Copy link
Member Author

MehdiK commented Feb 24, 2014

Implemented in #95 and released as v1.12.4 to NuGet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants