0
\$\begingroup\$

I'm working on my first large CodeIgniter project (and one of my first MVC projects), and I want to get some feedback on the techniques I've used so far in one of my models:

<?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');
class Model_programs extends SOA_Model{

    function __construct(){
        parent::__construct();
    }

    /**
     *
     * Returns an array of all programs owned by a user.
     *
     * @param int         $user_id User ID to query.
     * @return bool|array          Array of user's programs or FALSE.
     */
    function user_programs($user_id){
        if(!is_numeric($user_id)) return FALSE;
        $sth = $this->dbh->prepare("SELECT programs.id, programs.name
                                    FROM programs
                                    INNER JOIN program_users
                                    ON program_users.program_id = programs.id
                                    WHERE program_users.user_id = :user_id");

        $sth->bindParam(':user_id', $user_id);
        $sth->execute();
        $user_programs = $sth->fetchall(PDO::FETCH_ASSOC);

        if(!$user_programs) return FALSE;

        $return = array();

        foreach($user_programs as $program){
            $return[$program['id']] = $program['name'];
        }

        return $return;

    }

    /**
     *
     * Creates a new program owned by a user.
     *
     * @param int       $user_id              ID of user to own the program.
     * @param string    $program_name         Name of program to create, max 255 chars. Must be unique to the user.
     * @param string    $program_description  Description of program, max 2000 chars.
     * @param string    $program_logo         Location and filename of uploaded logo.
     * @return bool|int                       Program ID or FALSE.
     */
    function create_program($user_id, $program_name, $program_description, $program_logo){
        if(!is_numeric($user_id)) return FALSE;

        //Check if the user already owns a program of that name
        $sth = $this->dbh->prepare("SELECT programs.id
                                    FROM programs
                                    INNER JOIN program_users
                                    ON program_users.program_id = programs.id
                                    WHERE program_users.user_id = :user_id
                                    AND programs.name = :program_name");
        $sth->bindParam(':user_id', $user_id);
        $sth->bindParam(':program_name', $program_name);
        $sth->execute();

        if($sth->fetch()) return FALSE;

        //Create a new record in the programs table
        $sth = $this->dbh->prepare("INSERT INTO programs (name, description, logo)
                                    VALUES (:name, :description, :logo)");
        $sth->bindParam(':name', $program_name);
        $sth->bindParam(':description', $program_description);
        $sth->bindParam(':logo', $program_logo);

        if(!$sth->execute()) return FALSE;

        //Get the ID of the new program
        $program_id = $this->dbh->lastInsertId();

        if(!$this->add_program_user($program_id, $user_id, 1)){ //Problem here
            $sth = $this->dbh->prepare("DELETE FROM programs
                                        WHERE program_id = :program_id");
            $sth->bindParam(':program_id', $program_id);
            $sth->execute();

            return FALSE;
        }

        return (int)$program_id;
    }

    /**
     * Add a user to a program. If user already exists on
     * program, their user type will be updated.
     *
     * @param int $program_id   ID of program to add user to.
     * @param int $user_id      ID of user.
     * @param int $user_type_id ID of user type for user on program.
     * @return bool
     */
    function add_program_user($program_id, $user_id, $user_type_id){
        if(!is_numeric($program_id) || !is_numeric($user_id) || !is_numeric($user_type_id)) return FALSE;

        $sth = $this->dbh->prepare("SELECT *
                                    FROM program_users
                                    WHERE program_id = :program_id
                                    AND user_id = :user_id");
        $sth->bindParam(':program_id', $program_id);
        $sth->bindParam(':user_id', $user_id);
        $sth->execute();

        if($sth->fetch()){
            //Update existing record in the program_users table
            $sth = $this->dbh->prepare("UPDATE program_users
                                        SET user_type = :user_type_id
                                        WHERE program_id = :program_id
                                        AND user_id = :user_id");
            $sth->bindParam(':user_type_id', $user_type_id);
            $sth->bindParam(':program_id', $program_id);
            $sth->bindParam(':user_id', $user_id);

            if(!$sth->execute()) return FALSE;
        }else{
            //Create a new record in the program_users table
            $sth = $this->dbh->prepare("INSERT INTO program_users (program_id, user_id, user_type)
                                        VALUES (:program_id, :user_id, :user_type)");
            $sth->bindParam(':program_id', $program_id);
            $sth->bindParam(':user_id', $user_id);
            $sth->bindParam(':user_type', $user_type_id);

            if(!$sth->execute()) return FALSE;
        }

        return TRUE;
    }

    /**
     * Remove specified user from program.
     *
     * @param  int $program_id Program to remove user from.
     * @param  int $user_id    User to remove.
     * @return bool
     */
    function remove_program_user($program_id, $user_id){
        if(!is_numeric($program_id) || !is_numeric($user_id)) return FALSE;

        #$sth = $this->dbh->prepare("DELETE FROM program_users
        #                            WHERE program_id = :program_id
        #                            AND user_id = :user_id");
    }

    /**
     * Gets program name from program ID.
     *
     * @param int         $program_id ID of program to query.
     * @return bool|string             Program name or FALSE.
     */
    function program_name($program_id){
        if(!is_numeric($program_id)) return FALSE;
        $sth = $this->dbh->prepare("SELECT name
                                    FROM programs
                                    WHERE id = :id");
        $sth->bindParam(':id', $program_id);
        $sth->execute();

        $program = $sth->fetch();

        if(!$program) return FALSE;

        return $program['name'];
    }

    /**
     * Gets program ID from program name and user ID.
     *
     * @param string    $program_name Name of program to look up.
     * @param int       $user_id      ID of user who owns the program.
     * @return bool|int               ID of program owned by user.
     */
    function program_id($program_name, $user_id){
        if(!is_string($program_name) || !is_numeric($user_id)) return FALSE;
        $sth = $this->dbh->prepare("SELECT programs.id
                                    FROM programs
                                    INNER JOIN program_users
                                    ON program_users.program_id = programs.id
                                    WHERE programs.name = :program_name
                                    AND program_users.user_id = :user_id");
        $sth->bindParam(':program_name', $program_name);
        $sth->bindParam(':user_id', $user_id);
        $sth->execute();
        $program = $sth->fetch();

        if(!$program) return FALSE;
        return (int)$program['id'];

    }

    /**
     *
     * Get users associated with a program ID.
     *
     * @param  int        $program_id ID of program.
     * @return bool|array             Array of user IDs.
     */
    function program_users($program_id){
        if(!is_numeric($program_id)) return FALSE;
        $sth = $this->dbh->prepare("SELECT user_id
                                    FROM program_users
                                    WHERE program_id = :program_id");
        $sth->bindParam(':program_id', $program_id);
        $sth->execute();
        $program_users = $sth->fetchall(PDO::FETCH_ASSOC);

        return $program_users;
    }

    /**
     * @param int       $program_id ID of program.
     * @param int       $user_id    ID of user.
     * @return bool|int             ID of user type or FALSE if user does not exist
     *                              on program.
     */
    function program_user_type($program_id, $user_id){
        if(!is_int($program_id) || !is_numeric($user_id)) return FALSE;
        $sth = $this->dbh->prepare("SELECT user_type
                                    FROM program_users
                                    WHERE program_id = :program_id
                                    AND user_id = :user_id");
        $sth->bindParam(':program_id', $program_id);
        $sth->bindParam(':user_id', $user_id);
        $sth->execute();
        $program = $sth->fetch();

        if(!$program) return FALSE;
        return (int)$program['user_type'];
    }

}

This is still a work in progress so many things are subject to change, but overall am I utilizing MVC correctly (as far as you can tell from this code, at least)? I'm sure I'm doing a few things that I don't need to do, and possibly not doing some things that I should be doing?

\$\endgroup\$
2
\$\begingroup\$

Is there any specific reason that you don't use Active Record?

For example:

$sth = $this->dbh->prepare("SELECT programs.id, programs.name
                            FROM programs
                            INNER JOIN program_users
                            ON program_users.program_id = programs.id
                            WHERE program_users.user_id = :user_id");

$sth->bindParam(':user_id', $user_id);
$sth->execute();
$user_programs = $sth->fetchall(PDO::FETCH_ASSOC);

This can be done like the following with AR, and it looks cleaner to me. This is probably personal preference.

$user_programs = $this->dbh->select('programs.id, programs.name')
                           ->from('programs')
                           ->join('program_users', 'programs.id = program_users.id', 'inner')
                           ->where('program_users.user_id', $user_id)
                           ->get()
                           ->result();

You can use result_array() as well if you want the results in array format.

\$\endgroup\$
  • 1
    \$\begingroup\$ I prefer PDO mainly because I have more experience with it, and I like to write actual SQL queries. Thanks for the input though. \$\endgroup\$ – DC_ Mar 22 '13 at 7:15

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.