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.
精彩评论