1
\$\begingroup\$

I have a Customer class that has the code below. I can refactor the two if condition clauses into the Memebership class. Apart from that, I am wondering how I should refactor the if clause within the Benefits set property, without over-complicating thing.

Does any one of these below apply?

Patterns of refacotring if to subclass/strategy/state/polymorphism.

  public class Customer
    {

            public Membership Membership { get; private set; }
            public bool Hasbenefits { get; private set; }

            private int benefits;

            public int Benefits
            {
                get
                {
                    return benefits;
                }

                set
                {
                    if (Membership.Name == "MembershipA")
                    {
                        this.Hasbenefits = false;
                    }
                    else if (Membership.Name == "MembershipB")
                    {
                        this.Hasbenefits = true;
                        this.benefits = value * 4 + 2;
                    }
                    else
                    {
                        this.Hasbenefits = true;
                        this.benefits = value;
                    }
                }
            }
    }

Any advice would be very much appreciated!

\$\endgroup\$
10
  • 1
    \$\begingroup\$ I'd be wary of this initial design from the get-go. That's all fine because you're asking about re-factoring, but the general consensus is that properties shouldn't really have adverse side-effects, and yet that's what is happening here. \$\endgroup\$
    – Moo-Juice
    Aug 1 '13 at 20:04
  • 1
    \$\begingroup\$ well, for one, Membership.Name doesn't look like a name so much as it looks like a type, and it should be probably defined as an enum of some sort instead of a string. \$\endgroup\$
    – Sam I am
    Aug 1 '13 at 20:05
  • \$\begingroup\$ This logic should be in its own method, since your setting two fields in this setter \$\endgroup\$
    – christiandev
    Aug 1 '13 at 20:05
  • 1
    \$\begingroup\$ Looks like HasBenefits belong to Membership object (likely to be extended) \$\endgroup\$
    – AD.Net
    Aug 1 '13 at 20:11
  • 1
    \$\begingroup\$ @Pingpong - I have notified a moderator of your wish to move this question to the Code Review site. I cannot automatically do it, because Code Review is not a choice for migration yet, it must be done by a moderator, which I am not. \$\endgroup\$ Aug 1 '13 at 20:17
4
\$\begingroup\$

Personally here's what I would do :

Use the Membership type to create derived class; this will remove the if chain completely. Avoid side effects in setters; bad practice in my opinion. Avoid using properties with private sets; bad practice in my opinion.

This results in the following code :

public abstract class Customer
{
     protected int benefits;

     public abstract void UpdateBenefits(int newBenefits);

     public bool HasBenefits()
     {
         return benefits != 0;
     }

     public int GetBenefits()
     {
         return benefits;
     }
 }

 public class MembershipACustomer : Customer
 {
     public override void UpdateBenefits(int newBenefits)
     {
     }
 }

 public class MembershipBCustomer : Customer
 {
     public override void UpdateBenefits(int newBenefits)
     {
         benefits = newBenefits * (4 + 2);
     }
 }

 public class MembershipSomethingElseCustomer : Customer
 {
     public override void UpdateBenefits(int newBenefits)
     {
         benefits = newBenefits;
     }
 }

IMO it would also be a good idea to make a hierarchy with a new Benefit class, instead of customers. You can pass the benefit type to the customer by DI and that way it is the Benefit class that is responsible for calculating the benefit, not the customer. This would respect SRP a little more.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks! This is pretty valid, which is also one of my refactoring choices. \$\endgroup\$
    – Pingpong
    Aug 1 '13 at 20:19
  • \$\begingroup\$ What does it mean to return true from HasBenefits() if the UpdateBenefits() has yet to be called so there is no defined benefit yet? \$\endgroup\$
    – djna
    Aug 1 '13 at 20:24
  • \$\begingroup\$ @djna Hmm.. indeed! Don't know why, but at first I saw it as "This membership can have benefits" instead of "This customer currently have benefits". Fixed the code, it simplified the things a lot more. \$\endgroup\$
    – Pierre-Luc Pineault
    Aug 1 '13 at 20:29
  • \$\begingroup\$ @Pierre-LucPineault is it newBenefits * (4 + 2) or (newBenefits * 4) + 2? Looks like the example uses the second. On a side note, it can be refactored into interfaces IBenefitable and completely removing the need of HasBenefits. But it can be an overkill for such scenario. \$\endgroup\$
    – Fendy
    Aug 2 '13 at 1:55
2
\$\begingroup\$

My solution is thus:

  1. Remove the setter, it fundamentally changes the object in ways that the "property" idiom would be violated:

    public class Customer
    {
        public Membership Membership { get; private set; }
        public bool Hasbenefits { get; private set; }
    
        private int benefits;
    
        public int Benefits
        {
            get
            {
                return benefits;
            }
        }
    

    }

Right, so what now. We're still left with refactoring that if, and (imho) we've made the concious decision to use a method so that the caller of the method understand that there may be fundamental changes. My solution involves putting a setter back in, but making it simple:

        public int Benefits
        {
            get
            {
                return benefits;
            }
            set 
            {
                benefits = value;
            }
        }

Now I would have another class. The one that is responsible for determining benefits. Let's call it a BenefitProvider.

 public sealed class BenefitProvider
 {
     public void ProvideBenefit(Customer customer) // You have ONE job to do
     {
         /*
          * Do you stuff here.  You could use some predicates to cut down on the 
          * if/else kind of stuff
          */
     }
 }
\$\endgroup\$
2
  • \$\begingroup\$ You said: "setter, it fundamentally changes the object in ways that the "property" idiom would be violated" Could you please elaborate on this idiom? \$\endgroup\$
    – Pingpong
    Aug 1 '13 at 20:24
  • 1
    \$\begingroup\$ Setting a property should just set the value of the property. It shouldn't do other stuff. If you want to do other stuff use a method or extract a class as in this answer. \$\endgroup\$ Aug 1 '13 at 22:09

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.