2

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.

  • How does a static method manage to retrieve the promo code? – CodesInChaos Feb 10 '14 at 21:24
7

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.
}
  • It most likely accesses the database or a configuration file ("which is set in the backend admin system"). So you need to inject the db somehow, or pass it in explicitly. Implicit access the the db is a bad idea. – CodesInChaos Feb 10 '14 at 21:16
  • @CodesInChaos: Presumably, there's a Repository object somewhere. In any case, that doesn't seem to be relevant here. – Robert Harvey Feb 10 '14 at 21:17
  • @RobertHarvey If you need DB access (no matter if it's through a repository), you'll need to get access to a db related service somehow. Passing it in explicitly is a bit ugly, accessing it implicitly invokes all the singleton badness, so the least bad solution is constructor based DI together with the usual IoC containers. – CodesInChaos Feb 10 '14 at 21:23
  • 1
    @CodesInChaos: There's usually an easily editable configuration somewhere, like connection strings in ASP.NET's configuration XML. – Robert Harvey Feb 10 '14 at 21:26
3

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
}
  • Since this class is essentially a policy, it probably deserves its own interface as well. – CodesInChaos Feb 10 '14 at 21:27
  • @CodesInChaos Agreed, but based on the question, I wasn't going to assume knowledge of the policy pattern. – Aaron Hawkins Feb 10 '14 at 21:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.