开发者

Rewrite this code in a neater fashion, without so many else's

I want to rewrite this code without so many "else's", but still keep it efficient in terms of not checking things or running queries if not needed.

Can someone suggest a better way to write this function?

public static function fetch($content) {

    products_library::init();
    self::$cache = $cache = url::assetsPath() . '../cache/soldout_cache';

    //check the cache
    if (file_exists($cache)) {
        $cache_date = filectime($cache);
        db::select('date_modified');
        db::orderBy('date_modified DESC');
        db::limit(1);
        $mod_date = db::get('sc_module_products')->fetch(PDO::FETCH_ASSOC);
        if ($mod_date) {
            $mod_date = strtotime('date_modified');
            if ($cache_date >= $mod_date) { //serve the cache
                try {
                    $soldout = filewriter::read($cache);
                    $soldout = unserialize($soldout);
                } catch (Exception $e) {
                    $soldout = self::query();
                }
            }
            else
                $soldout = self::query();开发者_JAVA技巧
        }
        else
            $soldout = self::query();
    }
    else
        $soldout = self::query();

    $data['items'] = $soldout; // print_r($items); exit;

    $html = view::load('Product_Display', $data, true);
    return $html;
}

Thanks


Refactored it into a method that returns instead of else statements

private static function getSoldout() {
    self::$cache = $cache = url::assetsPath() . '../cache/soldout_cache';

    //check the cache
    if (!file_exists($cache)) {
      return self::query();
    }

    $cache_date = filectime($cache);
    db::select('date_modified');
    db::orderBy('date_modified DESC');
    db::limit(1);
    $mod_date = db::get('sc_module_products')->fetch(PDO::FETCH_ASSOC);
    if (!$mod_date) {
      return self::query();
    }

    $mod_date = strtotime('date_modified');
    if ($cache_date < $mod_date) {
      return self::query();
    }

    try {
      //serve the cache
      $soldout = filewriter::read($cache);
      $soldout = unserialize($soldout);
      return $soldout;
    } catch (Exception $e) {
      return self::query();
    }
}

public static function fetch($content) {

    products_library::init();

    $soldout = self::getSoldout();

    $data['items'] = $soldout; // print_r($items); exit;

    $html = view::load('Product_Display', $data, true);
    return $html;
}

I don't understand this line, is there a bug there?

$mod_date = strtotime('date_modified');


Set $soldout to NULL. Then remove the else $soldout = self::query() statement.

After the if statement test $soldout for NULL and it true run the query.


A switch-case block would work wonders here. You'd just have a break statement that would point to a default case. However, if I were in your shoes, I'd attempt to refactor the whole thing, which would take more than a quick fix.


Something like this might work. I'm not sure what's happening inside all the ifs and why you need so many, it might be more compact.

public static function fetch($content) {

    products_library::init();
    self::$cache = $cache = url::assetsPath() . '../cache/soldout_cache';

    $soldout = self::fetchCache($cache);
    if ($soldout === false)
    {
        $soldout = self::query();
    }

    $data['items'] = $soldout; // print_r($items); exit;

    $html = view::load('Product_Display', $data, true);
    return $html;
}

public static function fetchCache($cache) {
    if (file_exists($cache)) {
        $cache_date = filectime($cache);
        db::select('date_modified');
        db::orderBy('date_modified DESC');
        db::limit(1);
        $mod_date = db::get('sc_module_products')->fetch(PDO::FETCH_ASSOC);
        if ($mod_date) {
            $mod_date = strtotime('date_modified');
            if ($cache_date >= $mod_date) { //serve the cache
                try {
                    $result = filewriter::read($cache);
                    $result = unserialize($soldout);
                    return $result;
                } catch (Exception $e) {
                    return false;
                }
            }
        }
    }
    return false;
}


Seems to me as if you could default $soldout to be self::query() by setting it to that before the first if check then remove all the elses, so if the conditions doesn't match it will still be self::query(). Might not work depending on what self::query() does.

0

上一篇:

下一篇:

精彩评论

暂无评论...
验证码 换一张
取 消

最新问答

问答排行榜