1
\$\begingroup\$

The following code will read a db, show the contents to the user and provide instruments to edit them. This has been tested and works well.

model.php

<?php
class Collega {
    private $host="localhost";
    private $user="root";
    private $pwd="";

    private $attiva=false;

    public function connetti () {
        if (!$this->attiva) {
            if (!mysql_connect($this->host, $this->user, $this->pwd)) {
                die (mysql_error());
                exit ();
            } else {
                $this->attiva=true;
            }
        }
    }
    public function selectdatabase ($dbname) {
        if (!$this->attiva) {
            return false;
            exit ();
        } else {
            if (!mysql_select_db($dbname)) {
                echo mysql_error();
                return false;
                exit ();
            } else {
                return true;
            }
        }
    }
    public function interroga($sql) {
        if ($this->attiva) {
            $query = mysql_query($sql) or die (mysql_error());
            return $query;
        }
    }
}
$connessione = new Collega ();
$connessione->connetti();
$connessione->selectdatabase("conoscenze");
$result = $connessione->interroga("SELECT * FROM conoscenti");
?>

view.php

<?php
include ("model.php");
include ("controller.php");


class Visualizza {
    public function tetris ($rex) {
        echo "<table>";
echo "<tr><td>Nome</td><td>Cognome</td><td>Data di nascita</td><td></td><td></td></tr>";
        while ($row=mysql_fetch_array($rex)) {
            $birth=date_parse($row['Nato']);
    echo "<tr><td>".$row['Nome']."</td><td>".$row['Cognome']."</td><td>".$birth['day']."/".$birth['month']."/".$birth['year']."</td><td><form action='".$_SERVER['PHP_SELF']."?action=edit&id=".$row['id']."' method='post'><input type='hidden' name='nome' value='".$row['Nome']."' /><input type='hidden' name='cognome' value='".$row['Cognome']."' /><input type='hidden' name='nato' value='".$row['Nato']."' /><input type='hidden' name='index' value='".$row['id']."' /><button>Modifica</button></form></td><td><form action='".$_SERVER['PHP_SELF']."?action=delete' method='post'><input type='hidden' name='id' value='".$row['id']."' /><button>Elimina</button></form></td></tr>";
        }
        echo "</table>";
        echo "<form action='".$_SERVER['PHP_SELF']."?action=new' method='post'>";
        echo "<input type='hidden' name='nome' value='' />";
        echo "<input type='hidden' name='cognome' value='' />";
        echo "<input type='hidden' name='nato' value='' />";
        echo "<button type='submit'>Crea nuova voce</button>";
        echo "</form>";
    }
    public function editordati () {
        echo "<form action='".$_SERVER['PHP_SELF']."?action=submit' method='post'>";
        echo "<input type='text' value='".$_POST['nome']."' name='nome' />";
        echo "<input type='text' value='".$_POST['cognome']."' name='cognome' />";
        echo "<input type='date' value='".$_POST['nato']."' name='nato' />";
        echo "<input type='hidden' value='".$_GET['id']."' name='id' />";
        echo "<input type='submit' />";
        echo "</form>";
    }
    public function nuovo () {
        echo "<form action='".$_SERVER['PHP_SELF']."?action=submitnew' method='post'>";
        echo "<input type='text' value='' name='nome' />";
        echo "<input type='text' value='' name='cognome' />";
        echo "<input type='date' value='' name='nato' />";
        echo "<input type='submit' />";
        echo "</form>";
    }
}
$show=new Visualizza ();
if (!isset($_GET['action'])) {
    $show->tetris($result);
} elseif ($_GET['action']=="edit") {
    if (!$_GET['id']) {
        exit ();
    } else {
        $show->editordati();
    }
} elseif ($_GET['action']=="new") {
    $show->nuovo();
} elseif ($_GET['action']=="submit") {
    $penna->editexisting("conoscenti", $_POST['id']);
    $show->tetris($connessione->interroga("SELECT * FROM conoscenti"));
} elseif ($_GET['action']=="submitnew") {
    $penna->addnew("conoscenti");
    $show->tetris($connessione->interroga("SELECT * FROM conoscenti"));
} elseif ($_GET['action']=="delete") {
    $penna->destroyer("conoscenti", $_POST['id']);
    $show->tetris($connessione->interroga("SELECT * FROM conoscenti"));
}
?>

controller.php

<?php
class Editor {
    public function editexisting ($table, $id) {
        if (!isset($_POST['nome']) && !isset($_POST['cognome']) && !isset($_POST['nato'])) {
            exit ();
        } else {
            $editq="UPDATE ".$table." SET Nome='".$_POST['nome']."', Cognome='".$_POST['cognome']."', Nato='".$_POST['nato']."' WHERE id='".$id."'";
            $editr=mysql_query($editq) or die (mysql_error());
            return $editr;
        }
    }
    public function destroyer ($table, $id) {
        $delq="DELETE FROM ".$table." WHERE id='".$id."'";
        $delr=mysql_query($delq) or die(mysql_error());
        return $delr;
    }
    public function addnew ($table) {
        $addq="INSERT INTO ".$table."(id, Nome, Cognome, Nato) VALUES (null, '".$_POST['nome']."', '".$_POST['cognome']."', '".$_POST['nato']."')";
        $addr=mysql_query($addq) or die (mysql_error());
        return $addr;
    }
}
$penna= new Editor ();
?>

I named the scripts so it's easy to understand who plays what role in the MVC opera. Did I write a good MVC example or should I move some code somewhere else?

\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

Classes and other code in files

Your classes should only be classes. There should be no code outside the class in the same file as a class, because when a class file is included, it is expected that nothing happens until one of it's methods is called.

You class names also seem to be wrong. Eg Collega isn't really a model of a colleague, but just a wrapper around mysql_.

Structure

  • there shouldn't be SQL statements in your view or your controller. SQL statements should only be in one place, and that's either directly in the model, or inside a DAO.
  • there shouldn't be any controlling code in your view. Your view only shows stuff, that's all it does. It doesn't deal with user input, that's what the controller does.
  • as a result, your current controller doesn't contain any code that should be in a controller. If you fix the two points above, this should be fixed.

Security

Your code is extremely unsecure!

You are completely open to SQL injection as well as XSS. Never ever echo variable data without some kind of sanitation, and never ever put variable data into an SQL query without escaping it or using prepared statements.

Also, mysql_ is deprecated since 2013(!), don't use it.

  • XSS: use htmlspecialchars to prevent it when echoing user input/all variable data. Values which have to be encoded: $_SERVER['PHP_SELF'], all $_GET/$_POST, all $row.
  • SQLi: use prepared statements with mysqli_ or PDO.

Misc

  • avoid duplication. You have your form three times. You can put it in a function which accepts the variable parts as arguments.
  • if you can (and you only work with people on your code who also can) use english variable names.
  • write function names in camelCase.
  • use more spaces, eg around = and ..
  • don't die in your model, it will make it hard to handle this in the controller.
\$\endgroup\$
1
  • \$\begingroup\$ Ok, understood. I'll rewrite the code and post what comes out for further corrections. Collega = Connect, so class name wasn't so wrong after all; and i don't think that give english names to classes and functions is a good idea around here, but i'll do this for testing only. \$\endgroup\$ Apr 11, 2015 at 12:23
0
\$\begingroup\$

@tim I'm stalling. I wrote a new model and a new view, but now i don't know what to put in the controller since it seems that model can do everything. Something's strange still.

model.php

<?php
class Model {
    private $host = "localhost";
    private $user = "root";
    private $pwd = "";

    private $active = false;

    private $mysqli;

    public function connectDatabase ($dbname) {
            if (!$this->active) {
                $this->mysqli = new mysqli($this->host, $this->user, $this->pwd, $dbname);
                if ($this->mysqli->connect_errno) {
                echo $this->mysqli->connect_errno.": ".$$this->mysqli->connect_error;
            } else {
                echo "Connected to database ".$dbname."<br />";
            }
        $this->active=true;
    }
}

private function stringifyList (array $list) {
    $n = count($list);
    $liststring = "";
    for ($i=0; $i<$n; $i++) {
        if (!($i==$n-1)) {
            $liststring = $liststring.$list[$i].", ";
        } else {
            $liststring = $liststring.$list[$i];
        }
    }
    return $liststring;
}
private function pointsOfQuestion (array $list) {
    $n = count($list);
    $what = "";
    for ($i=0; $i<$n; $i++) {
        if (!($i==$n-1)) {
            $what = $what."?, ";
        } else {
            $what = $what."?";
        }
    }
    return $what;
}
public function dataTypes (array $list) {
    $n = count($list);
    $dt = "";
    for ($i=0; $i<$n; $i++) {
        switch (gettype($list[$i])) {
            case ("string") :
            $dt = $dt."s";
            break;
            case ("double") :
            $dt = $dt."d";
            break;
            case ("integer") :
            $dt = $dt."i";
            break;
            case ("object") :
            $dt = $dt."b";
            break;
            case ("NULL") :
            $dt = $dt."i";
        }
    }
    return $dt;
}

public function selectq ($table, array $voices) {
    $mysqli=$this->mysqli;
    if (!$stmt = $mysqli->prepare("SELECT ".$this->stringifyList($voices)." FROM ".$table)) {
        echo "Errore ".$mysqli->errno.": ".$mysqli->error;
    } else {
        echo "Query eseguita correttamente<br />";
    }
    $stmt->execute ();
    $result = $stmt->get_result();
    $stmt->close();
    return $result;
}

public function insertq ($table, array $voices, array $params) {
    $mysqli=$this->mysqli;
    $voicestring = "";
    $paramsuse = array ();
    $args= array ();
    $what = "";
    $dt = "";
    if (!(count($voices) == count($params))) {
        echo "Errore: numero parametri e voci diversi. Controlla.";
    } else {
        $voicestring = $this->stringifyList($voices);
        //$paramstring = $this->stringifyList($params);
        $what = $this->pointsOfQuestion($voices);
        $dt = $this->dataTypes($params);
        $paramsuse[0] = $dt;
        $args = array_merge($paramsuse, $params);
        $refs = array ();

        foreach ($args as $k=>$arg) {
            $refs[$k]=&$args[$k];
        }
    }

    if (!$stmt = $mysqli->prepare("INSERT INTO ".$table." (".$voicestring.") VALUES (".$what.")")) {
        echo "Errore ".$mysqli->errno.": ".$mysqli->error;
    } else {
        echo "Preparazione eseguita <br />";
    }
    $bindparams = call_user_func_array(array($stmt, 'bind_param'), $refs);

    if (!$bindparams) {
        echo "Non ho bindato un cazzo. Errore ".$stmt->errno;
    } else {
        echo "Binding eseguito correttamente";
    }
    if (!$stmt->execute()) {
        echo "Non sono riuscito a eseguire :(";
    } else {
        echo "Database scritto con successo";
    }
    $stmt->close();
}
public function deleteq ($table, $voice, $param) {
    $mysqli=$this->mysqli;

    if (!$stmt = $mysqli->prepare ("DELETE from ".$table." WHERE ".$voice." = ".$param)) {
        echo "Qualcosa non va(".$stmt->errno.": ".$stmt->error;
    }
    $stmt->execute();

    if (!$mysqli->affected_rows>0) {
        echo "Non esiste!";
    } else {
        echo "Cancellato";
    }
    $stmt->close();
}

public function editq ($table, array $voices, array $params, $voice, $param) {
    $mysqli = $this->mysqli;
    $querystr = "";

    if (!(count($voices) == count($params))) {
        echo "Errore: numero parametri e voci diversi. Controlla.";
    } else {
        for ($i=0; $i<count($voices); $i++) {
            if (!($i == count($voices)-1)) {
                $querystr = $querystr.$voices[$i]." = "."'".$params[$i]."'".", ";
            } else {
                $querystr = $querystr.$voices[$i]." = "."'".$params[$i]."'";
            }
        }
    }

    if (!$stmt = $mysqli->prepare("UPDATE ".$table." SET ".$querystr." WHERE ".$voice." = ".$param)) {
        echo "Problemi: ".$mysqli->errno.": ".$mysqli->error;
    } else {
        echo "Modifica preparata <br />";
    }

    $stmt->execute ();
    $stmt->close ();
}
}
?>

view.php

<?php
class View {
    public function showTableFirstRow ($row) {
        $html = "<tr>";
        foreach ($row as $key => $value) {
            $html = $html."<td>".htmlentities($key)."</td>";
        }
        $html = $html."</tr>";
        return $html;
    }
    public function showResultTable ($row) {
        $html = "<tr>";
        foreach ($row as $key => $value) {
            $html = $html."<td>".htmlentities($value)."</td>";
        }
        $html = $html."</tr>";
        return $html;
    }
    public function tableWrap ($html) {
        echo "<table>";
        echo $html;
        echo "</table>";
    }
    public function showEditForm ($action, array $fields) {
        echo "<form action='".htmlentities($_SERVER['PHP_SELF'])."?action=".$action."' method='post'>";
        foreach ($fields as $key => $value) {
            echo "<input type='text' name='".$key."' value='".$value."' />";
        }
        echo "<button type='submit'>Invia</button>";
        echo "</form>";
    }
}
?>
\$\endgroup\$
3
  • \$\begingroup\$ I don't think you fully grasp the concept of the MVC pattern just yet, but indeed you are right. The model layer is supposed to be able to do "everything" since it's the heart of your application. Only, it cannot control itself, right? And the View layer can't either, so if u do 1+1 you'll find the sole purpose of a Controller. \$\endgroup\$ Apr 14, 2015 at 19:09
  • \$\begingroup\$ I am studying it yet in facts. I'm trying to make practice because no book i read did really explain how MVC should be; just theory, theory, theory. \$\endgroup\$ Apr 14, 2015 at 22:19
  • \$\begingroup\$ @DanieleSqualo Regarding MVC: your code currently doesn't do anything. It doesn't parse user input and it doesn't call the appropriate model and view functions based on it. That would be exactly what the controller generally does. When you add the controller code and have working code, you might want to post a follow-up question instead of an answer here (there are a lot of small-ish things that could be improved). Regarding Security: your code still doesn't seem secure, SQL injection is possible in DELETE as well as UPDATE queries. \$\endgroup\$
    – tim
    May 14, 2015 at 18:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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