Basic CRUD in Laravel 5.2.37

Question

Controller class

namespace App\Http\Controllers\Website\SportsType;
use App\Classes\Contract\SportsType\ISportsType;
use \App\Http\Requests\SportsType\SportsTypeRequest as SportsTypeRequest;
class SportsTypeController extends \App\Http\Controllers\BaseController
{
    private $AllSportsTypes = "SportsTypes";
    private $sportstype;

    public function __construct(ISportsType $_sportstype) {
        $this->sportstype = $_sportstype;
        parent::__construct();
    }

    public function index() {
        $SportsTypes = $this->sportstype->All();
        return view('SportsType.List')->with('SportsTypes', $SportsTypes);
    }

    public function create() {
        return view('SportsType.Create');
    }

    public function edit($SportsTypeID) {

        $SportsType = $this->sportstype->Get($SportsTypeID);
        return view('SportsType.Edit', array('SportsType' => $SportsType));
    }

    public function save(SportsTypeRequest $request) {
        $data = [
            'SportsType'    =>  $request['SportsType'],
            'SportsTypeID'  =>  $request['SportsTypeID'],
        ];
        $result = $this->sportstype->Save($data);
        return redirect()->route($this->AllSportsTypes);
    }
}

Business logic class

namespace App\Classes\BusinessLogic\SportsType;
use App\Classes\DatabaseLayer\SportsType\SportsTypeDb;
use App\Classes\Contract\SportsType\ISportsType;

class SportsTypeBL implements ISportsType {       

    public function All() {
        $SportsTypes = (new SportsTypeDb())->All();
        return $SportsTypes;
    }

    public function Get($SportsTypeID) {
        $SportsType = (new SportsTypeDb())->Get($SportsTypeID);
        if($SportsType == null) {
            \App::abort(404);
            return;
        }
        return $SportsType;
    }

    public function Save($data) {
        return (new SportsTypeDb())->Save($data);
    }
}

Database class

namespace App\Classes\DatabaseLayer\SportsType;

class SportsTypeDb {

    public function All() {
        $SportsTypes = \App\Models\SportsType\SportsTypeModel::all();
        return $SportsTypes;
    }

    public function Get($SportsTypeID) {
        $SportsType = \App\Models\SportsType\SportsTypeModel
                ::where('SportsTypeID', $SportsTypeID)
                ->first();
        return $SportsType;
    }

    public function Save($data) {
        if($data["SportsTypeID"] == 0) {
            $SportsType = new \App\Models\SportsType\SportsTypeModel();
        }
        else {
            $SportsType = $this->Get($data["SportsTypeID"]);
        }

        $SportsType->SportsType     = $data["SportsType"];
        $SportsType->save();
        return true;
    }
}

1. Which class should have code for caching?
2. Am I writing bad or very bad code? Can you please suggest good ways to improve it?

Answer 1

Which class should have code for caching?

I recommend a separate Cache class that handles storing and retrieving from the cache. The argument for a separate class is simple: you want each class to have a narrow purpose. If you choose a particular cache driver/server today, but change your mind in a year, you should be able to change just one class -- the one that interacts with the driver -- to update your code. Start with this basic framework for a Cache class:

//reference to the only allowed instance of this class. It's private so
//outside scripts and objects cannot see it
private static $instance=null;

//private constructor will prevent outside code from instantiating objects.
private function __construct(){/*connect to the cache driver*/}
private function __destruct(){/*disconnect from cache driver*/}

//only way to get an instance (since constructor is private). We return
//the previously created instance instead of making a new one. As a result
//there is at most one instance of the class in memory.  This is a singleton.
public static function getInstance(){
    if(!self::$instance) self::$instance = new Cache();
    return self::$instance;
}

//takes key, returns cached object, or FALSE if not found
public function get(string $cache_key){}

//saves $value to the cache under $cache_key. Overwrite previous value
public function set(string $cache_key, mixed $value){}

//deletes whatever is stored under $cache_key, if it exists
public function delete(string $cache_key)

//connect to and disconnect from the cache server
private function connect(){}
private function disconnect(){}

Notice that this is a singleton . There can never be more than one Cache object in memory. This is to ensure that all your scripts are storing and retrieving data to and from the same cache. You would use the static method getInstance to grab a reference to the cache. Since the method is static, you call it on the class itself (Cache::getInstance()).

Once you have this in place, when you need a value that may be cached, you check for it in the cache first. For instance, suppose you previously saved the sport type data under cache key "sportsType_123". In your controller's edit function, change your code from:

$SportsType = $this->sportstype->Get($SportsTypeID);

To:

$SportsType = Cache::getInstance()->get('sportsType_'.$SportsTypeID)? :
              $this->sportstype->Get($SportsTypeID);

Likewise when you're saving data that you want cached, cache it after you've successfully saved to the DB. I've been using Memcache as my PHP cache driver with no problem, but Memcached may be even more feature rich. Because I have a Cache class as I described above, it wouldn't be too hard to switch. The rest of my scripts don't care because they interact with my Cache class, never directly with the cache driver.

Am I writing bad or very bad code? Can you please suggest good ways to improve it?

I have a couple of concerns with your code:

1. From what you posted, I cannot tell whether you're sanitizing the data before saving it to the database. Hopefully you're not just inserting strings you received from the client into your DB queries (this would expose you to SQL Injection attacks), but are instead using parameterized queries.
2. How do you handle a SELECT query not returning any result? For instance when I look at SportsTypeDb::Get, you call ->first() on your results without checking whether there is such a result. I'm not familiar with Laravel so maybe it handles this for you? Be sure to test how your script handles getting a non-existent resource.
3. In your SportsTypeBL class, you create a new instance of SportsTypeDb every time a method is called. This is a waste because you will have a lot of parallel connections to your database that you never close and all those objects must be kept in memory. A better approach would be to instantiate a new SportsTypeDb() in the constructor of SportsTypeBL, assign it to a class variable, and use it for all DB queries. Then in the __destruct() function of SportsTypeBL, disconnect from the DB.