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 just would like to know if I am heading in the right direction with this. I am trying to learn and understand the concept of MVC, and I am doing that while making my own webpage, so I can learn a little bit more each time. I do not want to put too much code here, but I would simply like to ask, am I doing this right, and if not, what would you do different, and most importantly, why?

This little piece of code is in the controller for "Tracks".

public function viewForm()
{
    if($_SERVER['REQUEST_METHOD'] == 'POST') 
    {
        $artistID       = $_POST["artistID"];
        $artistName     = $_POST["artistName"];
        $trackName  = $_POST["trackName"];
        $trackLink  = $_POST["trackLink"];

        $tracks = $this->loadModel('TracksModel');
        $tracks->addTrack($artistID, $artistName, $trackName, $trackLink);

        header('location: ' . URL . 'tracks/viewform?submit=true');
    }
    require 'application/views/tracks/viewform.htm';
}

And this is in the model for it:

public function addTrack($artistID, $artistName, $trackName, $trackLink)
{
    $sql    = "INSERT INTO music_tracks (artistID, artistName, trackName, trackLink) VALUES (:artistID, :artistName, :trackName, :trackLink)";
    $query  = $this->db->prepare($sql);

    $query->bindParam(':artistID', $artistID, PDO::PARAM_INT);
    $query->bindParam(':artistName', $artistName, PDO::PARAM_STR);
    $query->bindParam(':trackName', $trackName, PDO::PARAM_STR);
    $query->bindParam(':trackLink', $trackLink, PDO::PARAM_STR);

    $query->execute(array('artistID' => $artistID, ':artistName' => $artistName, ':trackName' => $trackName, ':trackLink' => $trackLink));
}
share|improve this question

1 Answer 1

  • You have some spacing issues. This may be because you are improperly mixing tabs and spaces in your indentation.
  • Your use of PDOStatement::bindParam() renders the array passed to PDOStatement::execute() redundant. You only need one or the other.
  • Typically, after issuing a header("Location:"), you use an immediate die or exit to prevent needlessly executing any other script.
share|improve this answer
    
Thanks, that second thing you said, I didn't know, makes sense, thanks! :) –  Ron Brouwers Aug 8 '14 at 6:46

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.