Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

Say I have this code:

        if ($this->ship == "e") {
            $this->price->addLine($this->price->base * $this->price->e, 0, "Export");
            $this->price->total += $this->price->base * $this->price->e;

        } elseif ($this->ship == "d") {
            $this->price->addLine($this->price->base * $this->price->d, 0, "Domestic");
            $this->price->total += $this->price->base * $this->price->d;
        }

I have multiple identical blocks of code like the above. I also have other code (other than $this->ship) that is similar in structure. Basically, this code adds a Line to a PDF, where the line says "Domestic" or "Export" along with other options. The code above also computes price for that line and adds it to the total.

My thoughts on refactoring this:

I want to remove duplication. I also want to abide by SOLID principles, i.e. separation of concerns. I see that code above does two things (computes price and adds a line item to PDF). I can surely split this code into two separate functions, but then I still end up with duplicated code -- I need to check if $this->ship equals e or d in both places. That is no good. If I need to add a 3rd option, I have to change code in two places.

So then I can move my check for e or d into the main body of code, but still I have duplication because I will have

switch($this->ship)
{
    case 'd': $cost = $this->getShippingCost($this->price->base, $this->price->d);break;
    case 'e': $cost = $this->getShippingCost($this->price->base, $this->price->e);break;     
}

I get these two long lines of code for basically a single character change. While maybe it is kind of acceptable, I am not quite liking it. If I need to change the number of parameters for example, or base parameter or etc, I will need to change it in more than one place.

Basically, so far no matter what way I spin this code, I end up with some duplication somewhere. I mean if it is unavoidable, I guess it's okay. But here I come for advice on seeing if there is a way to separate the concerns, and remove duplication and have minimal number of places (ideally one) when I do need to change something. can this be done?

share|improve this question
    
What are the $this->price->d and $this->price->d attributes for? What class is $this referring to? –  valenterry May 6 at 21:56
    
$this->price->d is a ratio used for the type of shipping. $this is a god-object (en.wikipedia.org/wiki/God_object) of everything –  Dennis May 6 at 21:59
    
If $this is a god-object, then refactoring this small portion of code should be the least of your concerns. It seems that you're using a deeply flawed architecture. This problem should be easily solved with polymorphism (e.g. $price = $Ship->getPrice(); $this->addToTotal($price)). However, since you're mixing OOP ($this) with procedural style (getShippingCost()), it looks like a very fundamental rewrite and redesign is in order. –  exizt May 7 at 7:43
    
oopsie I meant $this->getShippingCost(). I didn't quite catch what you meant by polymorphism.. If I had a chance to redesign this class .. I would try to redesign the immediate set of classes instead. But since these are line items, and corresponding prices for line items, I'd have to think about how to represent them best, basically go over entities, how they interact, etc. A line item will probably be an array of objects sitting inside of a container class, and line item object will contain the price for that item. –  Dennis May 7 at 13:44

2 Answers 2

I think the easiest and most readable is, extracting the price and the label in the case, and keep the rest out of it:

switch($this->ship)
{
    case 'd': 
        $price = $this->price->d;
        $label = 'Domestic';
        break;
    case 'e': 
        $price = $this->price->e;
        $label = 'Export';
        break;
}
$cost = $this->price->base * $price;
$this->price->addLine($cost, 0, $label);
$this->price->total += $cost;

That also allows you to break this method up in separate methods that return price and label based on the type (yeah, that's still two concerns, unless you see it as returning 'type properties' or something), another method that calculates the cost, etcetera.

Alternative: variable property name

Personally I don't really like solutions like this, because I think they are fragile, poorly readable and hard to debug, but they have their uses and maybe you'll like it:

You might also 'abuse' the fact that the character is the same as the property name, so you can use it like this:

$ship = $this->ship;
$price = $this->price->$ship;

Maybe even this will work, but I'm not sure about this syntax. And even if it does, do you understand it? I don't. :)

$price = $this->price->($this->ship);

Alernative: arrays with keys:

You might store the prices in an array by key, so you can read $this->price->prices['e']. Then your code might look like this:

$cost = $this->price->base * $this->price->prices[$this->ship];
$this->price->addLine($cost, 0, $this->labels[$this->ship]);
$this->price->total += $cost;

I must say I can see the benefit of the array variant, although maybe the first variant is more readable and will require less work to refactor.

share|improve this answer
    
cool. I like it. Thanks. The concept here as I see it is "see what is being changed", and work that out first, and add the rest after. –  Dennis May 6 at 21:50
    
looking over the new code I can't help but notice that original code had 8 lines, and new code has about 14 :/ Also, superimposing it with existing code, it does not look as "nice" (I am thinking the switch statement adds the bulkiness). So while refactoring it lost its "niceness" of being concise as well as being 'similar' to its nearby code pieces. I am thinking of encapsulating the blocks into functions such as $this->addLine(..) and $this->getPrice(..)... except it's already done in the above code, and the switch statement still needs to be where it is... –  Dennis May 6 at 22:09
1  
Yes, they are 14 lines, but the lines are shorter, simpler and much easier to read. Also there is hardly any duplication, because the (relatively) complex operations are moved out of the switch and they are pulled apart, so you can read this code while merely scanning over it. Less lines is not always better, and personally I would be quite happy with the cleanliness of those 14 lines. –  GolezTrol May 6 at 22:16
    
But if you like less lines, then maybe you might like the array bases solution which I added at the end of the answer. That one has 3 lines instead of 8. Those lines are just as complex, but there is no duplication. Or you can just keep it as is. I was just suggesting some possibilities. If they helped you conclude that your current code is as good as it gets, then that's fine as well. :) –  GolezTrol May 6 at 22:18
    
soo.. at the moment I have decided to separate computing total cost and generating line item label. In my case this means two functions, and duplicating the switch statement in each. (aka the switch statement in this answer contains two concerns I now have decided to separate). I guess this means duplicating the switch statement headers, but so be it .. :) –  Dennis May 7 at 15:21

It is hard produce any really solid refactoring advice based on a few lines of code out of context, but in general don't worry too much about a few almost-identical lines, it is not a practical problem.

In some cases it may be a sign of bad design, simple duplication elimination will however only serve to hide the problem.

In your case, I can't imagine how you end up with that code, how can $this contain both the cumulative total and the price for individual items, with those items apparently not even being stored in an array? I think you have got way bigger problems than a few duplicate lines. It would seem that you have tried to use OOP without really being prepared for it. The typical result of that is spaghetti code with objects.

If you want a deeper analysis of your code I suggest that you write a post on Code Review.

share|improve this answer
1  
I do agree that redesign is needed. But I disagree that eliminating duplication will hide the problem. I think it will expose opportunities for further refactoring that are hidden now in vast quantities of code. God-refactoring (aka rewriting this class) will take time, but incremental refactoring is something that I can do now without much stalling. –  Dennis May 7 at 13:30

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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