CODEWITHSUNDEEP
phpif-statement
Jason
Jason

Reputation: 15378

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();
        }
        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

Upvotes: 0

Views: 102

Answers (5)

dbellizzi
dbellizzi

Reputation: 713

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');

Upvotes: 2

Arend
Arend

Reputation: 3772

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;
}

Upvotes: 1

user677526
user677526

Reputation:

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.

Upvotes: 1

Richard Schneider
Richard Schneider

Reputation: 35464

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.

Upvotes: 1

Rickard
Rickard

Reputation: 7277

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.

Upvotes: 0

Related Questions