Refactoring: reducing duplication without creating flags

I don’t know how to refactor a piece of code that differs from other in the number of conditions checked on an if clause. Let me show you a real world example I’m facing right now.

The only difference between the two methods is that on the inner elseif, it checks an additional condition on the new parameter.

Method 1:

public function getKeysToInvalidateOnCreation($entity, $allKeys)
{
    $result = [];
    foreach ($allKeys as $key) {
        $entities_in_key = explode('+', substr($key, 0, strpos($key, '_')));
        $key_type = substr($key, strrpos($key, '_'));
        foreach ($entities_in_key as $entity_in_key) {
            if ($entity_in_key == $entity) {
                if ($key_type == self::GET_ALL_KEY || $key_type == self::CUSTOM_KEY) {
                    $result[] = $key;
                } elseif (strpos($key, 'id:') === false) {
                    $result[] = $key;
                }
            }
        }
    }
    return $result;
}

Method 2:

public function getKeysToInvalidateOnModification($entity, $allKeys, $modifiedId)
{
    $result = [];
    foreach ($allKeys as $key) {
        $entities_in_key = explode('+', substr($key, 0, strpos($key, '_')));
        $key_type = substr($key, strrpos($key, '_'));
        foreach ($entities_in_key as $entity_in_key) {
            if ($entity_in_key == $entity) {
                if ($key_type == self::GET_ALL_KEY || $key_type == self::CUSTOM_KEY) {
                    $result[] = $key;
                } elseif (strpos($key, 'id:') === false ||
                          strpos($key, 'id:'.$modifiedId) !== false) {
                    $result[] = $key;
                }
            }
        }
    }
    return $result;
}

What I would do is to extract all the inner code to a new protected method that would have the third parameter as optional, and then do the second check only if that parameter is set. But that has kind of bad smell for me: it seems like adding a flag to the method.

What would be the appropriate refactoring strategy?

3

PHP isn’t a language I have much experience with, but as I understand it both loops are trying to filter the set of keys, so you can extract the structure of the loop into another method that accepts the conditions for filtering as an argument in the form of a function:

private function getKeysWhere($entity, $allKeys, $condition)
{
    $result = [];
    foreach ($allKeys as $key) {
        $entities_in_key = explode('+', substr($key, 0, strpos($key, '_')));
        $key_type = substr($key, strrpos($key, '_'));
        foreach ($entities_in_key as $entity_in_key) {
            if ($entity_in_key == $entity) {
                if ($key_type == self::GET_ALL_KEY ||
                    $key_type == self::CUSTOM_KEY ||
                    $condition($key)) {
                    $result[] = $key;
                }
            }
        }
    }
    return $result;
}

This requires you use PHP 5.3 or above, in order to do things like pass functions as arguments and declare anonymous functions.

This is much more general and powerful than a boolean flag, because instead of picking between two set conditions, you can move the logic for whether to include the key into a separate function and separate the concern of filtering the keys from the concern of what conditions to filter them by.

Now, we define what the conditions for a key to invalidate on modification and on creation are as private helper methods:

private function keyIsInvalidOnCreation($key) {
    return strpos($key, 'id:') === false;
}

private function keyIsInvalidOnModification($key, $modifiedId) {
    return strpos($key, 'id:') === false || strpos($key, 'id:'.$modifiedId) !== false;
}

And finally, we can define our two methods to get invalid keys in terms of this general method and these two predicates:

public function getKeysToInvalidateOnCreation($entity, $allKeys) {
    return getKeysWhere($entity, $allKeys, 'keyIsInvalidOnCreation');
}

public function getKeysToInvalidateOnModification($entity, $allKeys, $modifiedId) {
    return getKeysWhere($entity, $allKeys, function($key) use ($modifiedId) {
        return keyIsInvalidOnModification($key, $modifiedId)
    });
}

The case for modification needs to be wrapped in an anonymous function in order to properly pass in $modifiedId to the predicate and turn it into a function of one argument to work with getKeysWhere. If you’re using a library with support for common functional programming tools such as currying and partial application, you can use that here as a cleaner alternative to the anonymous function wrapper. Not sure how possible some of that stuff is in PHP though.

This is fairly heavily decomposed and may be difficult to grok if you’re not familiar with functional programming patterns, but it cuts down on the duplication and generalizes well.

1

So just for other PHP guys that may be struggling with functional programming I want to register here the steps I did following Jack’s answer.

First I refactored each method to extract the condition so I could have an identical code base in both (note how the condition was extracted in both):

public function getKeysToInvalidateOnCreation($entity, $allKeys)
{
    $condition = function($key) {
        return strpos($key, 'id:') === false;
    };
    $result = [];
    foreach ($allKeys as $key) {
        $entities_in_key = explode('+', substr($key, 0, strpos($key, '_')));
        $key_type = substr($key, strrpos($key, '_'));
        foreach ($entities_in_key as $entity_in_key) {
            if ($entity_in_key == $entity) {
                if ($key_type == self::GET_ALL_KEY || $key_type == self::CUSTOM_KEY || $condition($key)) {
                    $result[] = $key;
                }
            }
        }
    }
    return $result;
}

public function getKeysToInvalidateOnModification($entity, $modifiedId, $allKeys)
{
    $condition = function($key) use ($modifiedId) {
        return strpos($key, 'id:') === false || strpos($key, 'id:' . $modifiedId) !== false;
    };
    $result = [];
    foreach ($allKeys as $key) {
        $entities_in_key = explode('+', substr($key, 0, strpos($key, '_')));
        $key_type = substr($key, strrpos($key, '_'));
        foreach ($entities_in_key as $entity_in_key) {
            if ($entity_in_key == $entity) {
                if ($key_type == self::GET_ALL_KEY || $key_type == self::CUSTOM_KEY || $condition($key)) {
                    $result[] = $key;
                }
            }
        }
    }
    return $result;
}

private function keyIsInvalidOnModification($modifiedId, $key)
{
    return strpos($key, 'id:') === false || strpos($key, 'id:' . $modifiedId) !== false;
}

private function keyIsInvalidOnCreation($key)
{
    return strpos($key, 'id:') === false;
}

At these moment, all tests are still green, so I can proceed to extract the common code to a private method:

public function getKeysToInvalidateOnCreation($entity, $allKeys)
{
    $condition = function($key) {
        return strpos($key, 'id:') === false;
    };
    return $this->getKeysToInvalidate($entity, $allKeys, $condition);
}

public function getKeysToInvalidateOnModification($entity, $modifiedId, $allKeys)
{
    $condition = function($key) use ($modifiedId) {
        return strpos($key, 'id:') === false || strpos($key, 'id:' . $modifiedId) !== false;
    };
    return $this->getKeysToInvalidate($entity, $allKeys, $condition);
}

private function getKeysToInvalidate($entity, $allKeys, callable $condition)
{
    $result = [];
    foreach ($allKeys as $key) {
        $entities_in_key = explode('+', substr($key, 0, strpos($key, '_')));
        $key_type = substr($key, strrpos($key, '_'));
        foreach ($entities_in_key as $entity_in_key) {
            if ($entity_in_key == $entity) {
                if ($key_type == self::GET_ALL_KEY || $key_type == self::CUSTOM_KEY || $condition($key)) {
                    $result[] = $key;
                }
            }
        }
    }
    return $result;
}

And everything still green. Thanks Jack

Trang chủ Giới thiệu Sinh nhật bé trai Sinh nhật bé gái Tổ chức sự kiện Biểu diễn giải trí Dịch vụ khác Trang trí tiệc cưới Tổ chức khai trương Tư vấn dịch vụ Thư viện ảnh Tin tức - sự kiện Liên hệ Chú hề sinh nhật Trang trí YEAR END PARTY công ty Trang trí tất niên cuối năm Trang trí tất niên xu hướng mới nhất Trang trí sinh nhật bé trai Hải Đăng Trang trí sinh nhật bé Khánh Vân Trang trí sinh nhật Bích Ngân Trang trí sinh nhật bé Thanh Trang Thuê ông già Noel phát quà Biểu diễn xiếc khỉ Xiếc quay đĩa Dịch vụ tổ chức sự kiện 5 sao Thông tin về chúng tôi Dịch vụ sinh nhật bé trai Dịch vụ sinh nhật bé gái Sự kiện trọn gói Các tiết mục giải trí Dịch vụ bổ trợ Tiệc cưới sang trọng Dịch vụ khai trương Tư vấn tổ chức sự kiện Hình ảnh sự kiện Cập nhật tin tức Liên hệ ngay Thuê chú hề chuyên nghiệp Tiệc tất niên cho công ty Trang trí tiệc cuối năm Tiệc tất niên độc đáo Sinh nhật bé Hải Đăng Sinh nhật đáng yêu bé Khánh Vân Sinh nhật sang trọng Bích Ngân Tiệc sinh nhật bé Thanh Trang Dịch vụ ông già Noel Xiếc thú vui nhộn Biểu diễn xiếc quay đĩa Dịch vụ tổ chức tiệc uy tín Khám phá dịch vụ của chúng tôi Tiệc sinh nhật cho bé trai Trang trí tiệc cho bé gái Gói sự kiện chuyên nghiệp Chương trình giải trí hấp dẫn Dịch vụ hỗ trợ sự kiện Trang trí tiệc cưới đẹp Khởi đầu thành công với khai trương Chuyên gia tư vấn sự kiện Xem ảnh các sự kiện đẹp Tin mới về sự kiện Kết nối với đội ngũ chuyên gia Chú hề vui nhộn cho tiệc sinh nhật Ý tưởng tiệc cuối năm Tất niên độc đáo Trang trí tiệc hiện đại Tổ chức sinh nhật cho Hải Đăng Sinh nhật độc quyền Khánh Vân Phong cách tiệc Bích Ngân Trang trí tiệc bé Thanh Trang Thuê dịch vụ ông già Noel chuyên nghiệp Xem xiếc khỉ đặc sắc Xiếc quay đĩa thú vị
Trang chủ Giới thiệu Sinh nhật bé trai Sinh nhật bé gái Tổ chức sự kiện Biểu diễn giải trí Dịch vụ khác Trang trí tiệc cưới Tổ chức khai trương Tư vấn dịch vụ Thư viện ảnh Tin tức - sự kiện Liên hệ Chú hề sinh nhật Trang trí YEAR END PARTY công ty Trang trí tất niên cuối năm Trang trí tất niên xu hướng mới nhất Trang trí sinh nhật bé trai Hải Đăng Trang trí sinh nhật bé Khánh Vân Trang trí sinh nhật Bích Ngân Trang trí sinh nhật bé Thanh Trang Thuê ông già Noel phát quà Biểu diễn xiếc khỉ Xiếc quay đĩa
Thiết kế website Thiết kế website Thiết kế website Cách kháng tài khoản quảng cáo Mua bán Fanpage Facebook Dịch vụ SEO Tổ chức sinh nhật