1
\$\begingroup\$

I know this is a long code and hard to read but when i started working on this it was just a few if-s, and as time passed i added more and more and came to this. Tried refactoring it by my self and anything i tried to do had no effect on code. Switch statements don't work here( doesn't lower amount of lines needed for it to work, or i couldn't find a way for switch to work, which is more likely). If any one can help I would be grateful.

if (isset($request)) {
    $page = $request->get('page');
}
if ($pages->pageCount == 1 || $pages->pageCount == 0) {
    if (\Yii::$app->controller->action->id == 'category') {
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::to(['blog/category/' . $category], true)]);
} else if (\Yii::$app->controller->action->id == 'index') {
    $this->registerLinkTag(['rel' => 'canonical', 'href' => Yii::$app->urlManager->hostInfo . Yii::$app->urlManager->baseUrl . '/blog']);
} else if (\Yii::$app->controller->action->id == 'archive') {
    $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::to(['blog/' . $year . '/' . $month], true)]);
} else if (\Yii::$app->controller->action->id == 'author') {
    if ($page != null) {
        $this->title = $user_name->username . ' - Travel blog | Page ' . $page . ' | Clickstay';
    } else {
        $this->title = $user_name->username . ' - Travel blog | Clickstay';
    }
    $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::to(['blog/author/' . $name], true)]);
} else if (\Yii::$app->controller->action->id == 'search') {
    $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::current([], true)]);
}
}else {
if ($currentPage == 1) {
    if (\Yii::$app->controller->action->id == 'category') {
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::to(['blog/category/' . $category], true)]);
        $this->registerLinkTag(['rel' => 'next', 'href' => Url::to(['blog/category/' . $category . '/' . $next], true)]);
    } else if (\Yii::$app->controller->action->id == 'index') {
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Yii::$app->urlManager->hostInfo . Yii::$app->urlManager->baseUrl . '/blog']);
        $this->registerLinkTag(['rel' => 'next', 'href' => Url::to(['blog/' . $next], true)]);
    } else if (\Yii::$app->controller->action->id == 'archive') {
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::to(['blog/' . $year . '/' . $month], true)]);
        $this->registerLinkTag(['rel' => 'next', 'href' => Url::to(['blog/' . $year . '/' . $month . '/' . $next], true)]);
    } else if (\Yii::$app->controller->action->id == 'author') {
        if ($page != null) {
            $this->title = $user_name->username . ' - Travel blog | Page ' . $page . ' | Clickstay';
        } else {
            $this->title = $user_name->username . ' - Travel blog | Clickstay';
        }
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::to(['blog/author/' . $name], true)]);
        $this->registerLinkTag(['rel' => 'next', 'href' => Url::to(['blog/author/' . $name . '/' . $next], true)]);
    } else if (\Yii::$app->controller->action->id == 'search') {
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::current([], true)]);
        $this->registerLinkTag(['rel' => 'next', 'href' => Url::to(['blog/search/' . $next . '?q=' . $search . '/'], true)]);
    }
} else if ($currentPage > 1 && $currentPage != $pages->pageCount) {
    if (\Yii::$app->controller->action->id == 'category') {
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::to(['blog/category/' . $category . '/' . $currentPage], true)]);
        if ($page != 2) {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/category/' . $category . '/' . $prev], true)]);
        } else {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/category/' . $category], true)]);
        }
        $this->registerLinkTag(['rel' => 'next', 'href' => Url::to(['blog/category/' . $category . '/' . $next], true)]);
    } else if (\Yii::$app->controller->action->id == 'index') {
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::to(['blog/' . $currentPage], true)]);
        if ($page != 2) {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/' . $prev], true)]);
        } else {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/'], true)]);
        }
        $this->registerLinkTag(['rel' => 'next', 'href' => Url::to(['blog/' . $next], true)]);
    } else if (\Yii::$app->controller->action->id == 'archive') {
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::to(['blog/' . $year . '/' . $month . '/' . $currentPage], true)]);
        if ($page != 2) {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/' . $year . '/' . $month . '/' . $prev], true)]);
        } else {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/' . $year . '/' . $month], true)]);
        }
        $this->registerLinkTag(['rel' => 'next', 'href' => Url::to(['blog/' . $year . '/' . $month . '/' . $next], true)]);
    } else if (\Yii::$app->controller->action->id == 'author') {
        if ($page != null) {
            $this->title = $user_name->username . ' - Travel blog | Page ' . $page . ' | Clickstay';
        } else {
            $this->title = $user_name->username . ' - Travel blog | Clickstay';
        }
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::to(['blog/author/' . $name . '/' . $currentPage], true)]);
        if ($page != 2) {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/author/' . $name . '/' . $prev], true)]);
        } else {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/author/' . $name], true)]);
        }
        $this->registerLinkTag(['rel' => 'next', 'href' => Url::to(['blog/author/' . $name . '/' . $next], true)]);
    } else if (\Yii::$app->controller->action->id == 'search') {
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::current([], true)]);
        if ($page != 2) {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/search/' . $prev . '?q=' . $search], true)]);
        } else {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/search/?q=' . $search], true)]);
        }
        $this->registerLinkTag(['rel' => 'next', 'href' => Url::to(['blog/search/' . $next . '?q=' . $search], true)]);
    }
} else if ($currentPage == $pages->pageCount) {
    if (\Yii::$app->controller->action->id == 'category') {
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::to(['blog/category/' . $category . '/' . $currentPage], true)]);
        if ($page != 2) {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/category/' . $category . '/' . $prev], true)]);
        } else {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/category/' . $category], true)]);
        }
    } else if (\Yii::$app->controller->action->id == 'index') {
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::to(['blog/' . $currentPage], true)]);
        if ($page != 2) {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/' . $prev], true)]);
        } else {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/'], true)]);
        }
    } else if (\Yii::$app->controller->action->id == 'archive') {
        if ($page != null) {
            $this->title = $user_name . ' - Travel blog | Page ' . $page . ' | Clickstay';
        } else {
            $this->title = $user_name . ' - Travel blog | Clickstay';
        }
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::to(['blog/' . $year . '/' . $month . '/' . $currentPage], true)]);
        if ($page != 2) {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/' . $year . '/' . $month . '/' . $prev], true)]);
        } else {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/' . $year . '/' . $month . '/'], true)]);
        }
    } else if (\Yii::$app->controller->action->id == 'author') {
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::to(['blog/author/' . $name . '/' . $currentPage], true)]);
        if ($page != 2) {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/author/' . $name . '/' . $prev], true)]);
        } else {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/author/' . $name], true)]);
        }
    } else if (\Yii::$app->controller->action->id == 'search') {
        $this->registerLinkTag(['rel' => 'canonical', 'href' => Url::current([], true)]);
        if ($page != 2) {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/search/' .$prev. '?q=' . $search], true)]);
        } else {
            $this->registerLinkTag(['rel' => 'prev', 'href' => Url::to(['blog/search' . '?q=' . $search], true)]);
        }
    }
}
}
\$\endgroup\$
2
\$\begingroup\$

You could try to solve this using polymorphism but it probably would get worse before it got better. There is an excellent site I just found with Martin Fowlers catalogue of refactorings: https://refactoring.guru

First I can see that you have 5 types of actions (category, index, archive, author, search). I would try to separate those actions to a single conditional, something like this:

$action = \Yii::$app->controller->action->id;

if($action == "category") {
    if ($pages->pageCount == 1 || $pages->pageCount == 0) {
        ...
    } else {
        if ($currentPage == 1) {
            ...
        } else if ($currentPage > 1 && $currentPage != $pages->pageCount) {
            ...
        } else if ($currentPage == $pages->pageCount) {
            ...
        }
    }
}

if($action == "index") {
    ...

I would do that for each of the actions. There will be more code for sure but it will still be a little bit more simple.

Next you could try to refactor it out to different classes, that might be a little bit cumbersome and you would probably have to pass in alot of variables to the class. This depends a little bit how far you want to take it and if you feel it is worth it.

You would want to have a parent class so you can pull up methods from your subclasses. Personally I would probably use the factory pattern for creating the new objects and and introduce parameter object (https://refactoring.guru/introduce-parameter-object)

But ideally, your code in the end would probably look something like this:

$actionLink = ActionLinkFactory::create(\Yii::$app->controller->action->id);
$actionLink->setParameters(new ActionLinkParameters([
    "pageCount" => $pages->pageCount,
    "currentPage" => $currentPage,
    ...
]));

$actionLink->registerTag();

Ther is no silver bullet unfortunately, but you dont have to do everything at once either.

\$\endgroup\$
1
  • \$\begingroup\$ I was afraid u would say something like that... I'll try with that one \$\endgroup\$ – Hajrudin Hajrulahović Aug 3 '16 at 7:48

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.