What is best software design in creating methods?

Question

I had created a extension method which extended the string type in C#.

// actually checks if the string is empty or null and then looks up the default
// promotion code which is set in the backend admin system. 
promocode.HasPromode(); 

I thought that was fine and dandy, but another colleague thought I should inject the method through constructor injection, which should implement a interface. I think this is overkill to have a simple lookup function to do that.

Another colleague thought that extending the string type makes it simpler to use it wrong and exposes it for the wrong purposes.

Which one is right? All solutions work fine; but some of these solutions should be the best one, or does it matter actually? It should matter, because we are having those discussions and we're trying to make the code better.

Answer 1

The problem that I have with your method is that it ties a method to the string class that has nothing to do with strings in general.

It would make more sense if your extension method had something to do with strings in general, not a particular field. IsPalindrome() would make more sense as an extension method on string than HasPromode() [sic], because IsPalindrome() is applicable to any string.

Now, then...

Another colleague thought I should inject the method through constructor injection, which should implement a interface.

Er, what?

This fellow sounds like an Architecture Astronaut. While his suggestion might make sense in some context, a simple Enum.TryParse() call would suffice.

PromoCode outValue;
if (Enum.TryParse(promoCode, out outValue))
{
    // string contains valid promo code.
}

Answer 2

I agree with Robert Harvey on everything except the solution. This is mainly because, more than likely your promotion codes are stored in the database and could change. In this case, I wouldn't opt for an enum simply because any additions/changes to the list of available promotion codes would require a build in order to update the enum with the correct values.

Instead, I would opt for a solution like this:

public class PromotionCodeManager //or some other meaningful name
{
    public bool IsPromotionCode(string value)
    {
        //TODO Attempt to lookup the promotion code in the backend system 
        //and return true if it's a valid promotion code that hasn't expired.
    }
}

And, if you want to get really fancy and satify the desires of your architecture astronaut, you could use dependency injection in the constructor of this class which would take an IPromotionCodeRepository that the class would use to lookup the valid promotion codes and determine if the value passed in is valid.

Then, the client code could look something like this:

if (new PromotionCodeManager(promotionCodeRepository).IsPromotionCode(myValue))
{
    //Valid promotion code
}
else
{
    //Invalid promotion code
}