How to refactor code that has 2 concerns

Question

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?

Answer 1

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.

Answer 2

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.