Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'd like to know if I'm doing this properly. Basically, I have 3 models: Student, Course, StudentCourse. I'm running into problems when I'm trying to sign a student up for a course (I've removed a lot of the non-important code - auth component, view, etc).

// Courses Controller (ie: /courses/signup/6)
class CoursesController extends AppController {
        public function signup($id = null) {
            $this->Course->id = $id;
            if (!$this->Course->exists()) {
                throw new NotFoundException(__('Invalid course'));
            }

            // Let's check to see if the user can signup for the course
            if($this->Course->canSignupForCourse($this->Auth->user('id'))) {

            } else {
                $this->Session->setFlash(__('You cannot signup for that course.'), 'flash_error');      
                $this->redirect(array('controller' => 'pages', 'action' => 'calendar'));
            }
    }   
}

// Course Model:
class Course extends AppModel {
    // Class 'hasMany' StudentCourse
    // Class 'habtm' Student

    public function isOwnedBy($course, $user) {
        return $this->field('id', array('id' => $course, 'student_id' => $user)) === $course;
    }

    public function isApproved() {
        $course = $this->read(null);

        if($course['Course']['approved_by'] != 0) {
            return true;
        }

        return false;
    }

    public function isComplete() {
        $course = $this->read(null);

        if($course['Course']['completed'] != 0) {
            return true;
        }

        return false;
    }   

    public function canSignupForCourse($studentId) {    

        if(! $this->StudentCourse->isSignedUpForCourse($studentId) && $this->isApproved() && $this->isComplete()) {
            return true;
        }       

        return false;
    }

}

// Student Model
class Student extends AppModel {
    // Student 'hasMany' Course
    // Student 'hasMany' StudentCourse
}

// StudentCourse Model
class StudentCourse extends AppModel {
    // StudentCourse 'belongsTo' Student
    // StudentCourse 'belongsTo' Course
    public function isSignedUpForCourse($studentId = null, $courseId = null) {
        return $this->find('count', array('conditions' => array('StudentCourse.course_id' => $courseId, 'StudentCourse.student_id' => $studentId))) != 0 ? TRUE : FALSE;
    }   
}

Basically, am I going about this the right way, are my relationships correct, or, is there a better/easier way to figure out if a student can signup for a course.

share|improve this question

3 Answers 3

I agree with Hugo, looks good.

I'd just like to add a little bit of best practice; Controllers and Models should have public $name="ControllersName"; and public $name="ModelName"; at the top respectively.

share|improve this answer

To me your code and relations looks good! You removed the redundancy in your database by splitting the tables up. Your cake really looks cakes way to me!

Just make sure you actually do something with the exception thrown and it should be all good.


Edit : I know this is a little late, but I wanted to complete my answer for the sake of completeness.

Your code style looks pretty good to me. The functions names are well chose. What I mean by that is by putting verb first you make the function a lot easier to find and understand.

Also, putting the logic in the model is the good choice. Continue to keep thin controllers.

share|improve this answer

Redundant bool returning:

public function isApproved() {
    $course = $this->read(null);

    return $course['Course']['approved_by'] != 0;
    /* if($course['Course']['approved_by'] != 0) {
        return true;
    }

    return false; */
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.