[SQL/PHP] Genres toevoegen aan DB, kan dit mooier?

[monnick]

Ik heb twee tabellen met bijbehorende kolommen:

genres
genre_id, name

movie_genres
movie_id, genre_id

De gebruiker kan films toevoegen met 3 genres, deze genres kunnen al bestaan in de tabel 'genres'. Als het genre al bestaat dan moet het genre_id aan de film gelinkt worden in 'movie_genres'. Als het genre nog niet bestaat moet dit nieuwe genre geinsert worden in 'genres' en moet vervolgens het nieuwe genre_id voor de film weer gelinkt worden in 'movie_genres'.

Nu heb ik dit als volgt in PHP geïmplementeerd. Alleen volgens mij zou dit een stuk netter (minder regels) kunnen door wat geavanceerde SQL te gebruiken. Ik heb alleen geen idee hoe??

Iemand die een suggestie heeft?

PHP:

// Inserting the genres
foreach ($_POST['genre'] as $genre) {
    $getGenreIDQuery = "SELECT `genre_id` FROM `genres` WHERE `name` = '{$genre}'";
    $execQuery = mysql_query($getGenreIDQuery) or die("MySQL error: could not retrieve genre ID from database.");
            
    $numResults = mysql_num_rows($execQuery);
            
    if ($numResults === 0) {
        // Genre does not exist
        $newGenre = mysql_real_escape_string($genre);
                
        $newGenreQuery = "INSERT INTO `genres` (`name`) VALUES ('{$newGenre}')";
        $execQuery = mysql_query($newGenreQuery) or die("MySQL error: could not insert new genre");
                
        $genreID = mysql_insert_id();
        $insertGenreQuery = "INSERT INTO `movie_genres` (`movie_id`, `genre_id`) VALUES ('{$movieID}', '{$genreID}')";
        $execQuery = mysql_query($insertGenreQuery) or die("MySQL error: could not insert genre");
                
    } elseif ($numResults === 1) {
        // Genre already exists
        $genreID = mysql_result($execQuery, 0);
            
        $insertGenreQuery = "INSERT INTO `movie_genres` (`movie_id`, `genre_id`) VALUES ('{$movieID}', '{$genreID}')";
        $execQuery = mysql_query($insertGenreQuery) or die("MySQL error: could not insert genre");
    }
}

[NMe]

Ik zie twee keer zelfde query in je code.

[mulder]

Misschien is het interessanter om naar OOP te kijken. Dat levert je meer overzichtelijke en herbruikbare code op.

[monnick]

Ja, die insertGenreQuery kan sowieso buiten het if-statement. Maar ik doelde meer op wat diepere SQL, die alleen iets insert als het genre nog niet bestaat. Is dat mogelijk?

EDIT: Aha! Hoe zou OOP mij hierbij kunnen helpen? Zou je een heel kort voorbeeld kunnen geven of een linkje? Ik heb altijd zoiets gehad van: OOP en PHP doe ik niet, want ik zie niet in hoe het me zou kunnen helpen. Ik heb alleen een beetje OOP ervaring met Java, dus heel goed de krachten begrijpen van OOP doe ik niet.

[ Voor 48% gewijzigd door monnick op 04-09-2010 20:59 ]

[mulder]

Naar aanleiding van je code in de TS kwam ik bij herbruikbare code; stel je wilt ergens anders in je webapplicatie een gerne aan een movie toevoegen, moet je daar weer een sql query doen. In het geval van OOP heb je een object Movie, dan heeft deze bv een functie Add(Gerne). 1 keer query schrijven dus, en je houdt het op 1 plek. Bovendien worden je scripts zelf leesbaarder; je 'verstopt' bepaalde code in je class.

Verder is er heel veel over OOP en OOP + PHP om internet te vinden. Ik zou simpel beginnen met een classje Movie te maken die Gernes aan zichzelf kan toevoegen.

[krvabo]

http://dev.mysql.com/doc/.../insert-on-duplicate.html

Maar of het daarmee betere sql oplevert.. Ja, het scheelt een externe query als je een goede primary key zou gebruiken, maar dat doe je niet (Niet voor dit doel.. want je gebruikt een auto increment ID)

Verder moet je eens kijken naar het veilig invoeren (keyword: sql injection) van data, want dat doe je zo op deze manier ook niet. Verder kan ik het je aanraden om je variabelen buiten je query te plaatsen:

PHP:

$sSql = "INSERT INTO genre (id, name) VALUES (".(int)$id.", '".mysql_real_escape_string($sGenre)."')";

of
http://php.net/manual/en/function.sprintf.php
(Eigen voorkeur)

[edit]

Aangezien je aan het loopen bent:
Bouw het 'values' gedeelte van je query op in de loop en voer hem pas daarna uit.

SQL:

INSERT INTO
tabel
VALUES 
(1, 'drama'), 
(2, 'comedy'),
(3, 'oorlog')

of beter:

SQL:

INSERT INTO
tabel
(genre)
VALUES 
('drama'), 
('comedy'),
('oorlog')

[ Voor 20% gewijzigd door krvabo op 04-09-2010 21:16 ]

[monnick]

@mulder: bedankt! Ik ga me zeker op korte termijn even verdiepen in OOP voor PHP

@krvabo: bedankt voor je reactie. Ik heb één van de queries nu buiten de loop gehaald, een betere optie inderdaad.

Wat ik nog niet helemaal begrijp is waarom de bovenstaande code SQL injection gevoelig is. Ik had alleen het $genre in de eerste query door mysql_real_escape_string() moeten halen, maar verder zie ik niet wat er onveilig aan is. De manier van variabelen in een (query)string zoals ik dat doe is toch niet verkeerd? Dat wil zeggen:

"blablabla = {$var} en blablabla"

Voor zo ver ik weet is de { } methode een goede manier om variabelen in een string te zetten.

Ik heb in de onderstaande nog even twee keer extra (int) toegevoegd, maar dat zou qua veiligheid niet echt uit moeten maken aangezien deze variabelen al terug komen uit de database en sowieso integer zijn.

PHP:

    // Inserting the genres
    foreach ($_POST['genre'] as $genre) {
    if (!empty($genre)) {
        $sGenre = mysql_real_escape_string($genre);

        $getGenreIDQuery = "SELECT `genre_id` FROM `genres` WHERE `name` = '{$sGenre}'";
        $execQuery = mysql_query($getGenreIDQuery) or die("MySQL error: could not retrieve genre ID from database.");
        $numResults = mysql_num_rows($execQuery);
                
        if ($numResults === 0) {
            // Genre does not exist
            $newGenreQuery = "INSERT INTO `genres` (`name`) VALUES ('{$sGenre}')";
            $execQuery = mysql_query($newGenreQuery) or die("MySQL error: could not insert new genre");
                    
            $genreID = (int)mysql_insert_id();
        } elseif ($numResults === 1) {
            // Genre already exists
            $genreID = (int)mysql_result($execQuery, 0);
        }
                
        $movieGenres[] = "('".$movieID."', '".$genreID."')";
    }
}
        
if (count($movieGenres) > 0) {
    $queryString = implode(", ", $movieGenres);
    $insertGenreQuery = "INSERT INTO `movie_genres` (`movie_id`, `genre_id`) VALUES " . $queryString;
    $execQuery = mysql_query($insertGenreQuery) or die("MySQL error: could not insert genres");
}

[ Voor 3% gewijzigd door monnick op 04-09-2010 23:24 ]

[NMe]

Lees dit eens door. http://bogdan.org.ua/2007...if-not-exists-syntax.html

[RobIII]

mulder schreef op zaterdag 04 september 2010 @ 20:57:
Misschien is het interessanter om naar OOP te kijken. Dat levert je meer overzichtelijke en herbruikbare code op.

Maar dat is voor zo'n klein simpel stukje helemaal niet relevant en prima procedureel op te lossen. Gebruik functies et voila. Sure, OOP is gewoon iets dat je bij voorkeur het liefst gebruikt, maar het is echt geen silver bullet. Daarbij is OO, IMHO, binnen PHP < 6 ook nog niet je-van-het (en PHP6 kan ik nog niets over zeggen want ik heb er nog niet naar gekeken _{en wou dat ook graag zou houden} ).

[ Voor 16% gewijzigd door RobIII op 05-09-2010 13:14 ]

[Voutloos]

offtopic:
Inderdaad allemaal offtopic, maar met PHP 5.3 zijn er toch echt weinig goede excuses over om jezelf niet een OO style aan te meten. Wmb klets je maar wat lukraak over PHP 6 Rob

edit:
Roep dan niet meteen wat over versies

[ Voor 29% gewijzigd door Voutloos op 05-09-2010 13:50 ]

[RobIII]

Voutloos schreef op zondag 05 september 2010 @ 13:38:

offtopic:
Inderdaad allemaal offtopic, maar met PHP 5.3 zijn er toch echt weinig goede excuses over om jezelf niet een OO style aan te meten. Wmb klets je maar wat lukraak over PHP 6 Rob

offtopic:
Er zijn sowieso geen excuses om jezelf geen OOP aan te leren; ik zeg alleen dat 't voor zoiets simpels niet meteen nodig is (en dat ik de opmerking dus weinig vond toevoegen). En ik beweer ook niet dat PHP niet OO is, alleen dat er nogal wat aan schort (last I looked). Of dat met 5.3 of 6 of 21 opgelost is weet ik niet (precies).

[krvabo]

monnick schreef op zaterdag 04 september 2010 @ 23:19:

Wat ik nog niet helemaal begrijp is waarom de bovenstaande code SQL injection gevoelig is. Ik had alleen het $genre in de eerste query door mysql_real_escape_string() moeten halen, maar verder zie ik niet wat er onveilig aan is.

Dat was ook precies het onveilige
Alle strings (user input strings) die je naar de database gooit, tenzij je ze op een andere manier valideerd (regex bijvoorbeeld), moet je met mysql_real_escape_string() behandelen.

De manier van variabelen in een (query)string zoals ik dat doe is toch niet verkeerd? Dat wil zeggen:

"blablabla = {$var} en blablabla"

Voor zo ver ik weet is de { } methode een goede manier om variabelen in een string te zetten.

Hoewel het een legitieme manier is, blijft het (zeker voor mij) lelijk en onoverzichtelijk.

Een string met single quotes ' pakt de variabelen niet uit de string, dat wil zeggen;

PHP:

$sSql = 'hallo $naam';

Output gewoon $naam en niet de waarde van de variabele.
Het is best practice om strings zoveel mogelijk single quotes te houden, om twee redenen:

1. Strings met dollartekens erin laten ook het dollarteken zien
2. Het is marginaal sneller

(Bij strings met SQL-code erin worden wel vaak dubbelquotes gebruikt om het feit dat je dan makkelijker single quotes om waardes kunt zetten: "where naam = ' ".$sNaam." ' " )

Als je de variabelen buiten de strings houdt dan zal ook in elke IDE/editor de syntax highlighting goed werken, maar dit is maar een klein voordeel boven dit:

Stel je hebt twee stukken code:

PHP:

$sSql = "select 1 from tabel where id=$id";

en

PHP:

$sSql = "select 1 from tabel where id=".$id;

Stel je nou voor dat je het wil veranderen naar een IN():

PHP:

$sSql = "select 1 from tabel where id IN (".implode(',',$id).")";

Het eerste stukje code kan dan al niet meer.
Als je bij de wat grotere projecten uitkomt zul je al snel zien dat het makkelijker en overzichtelijker is om variabelen niet in een string te zetten.

Ik heb in de onderstaande nog even twee keer extra (int) toegevoegd, maar dat zou qua veiligheid niet echt uit moeten maken aangezien deze variabelen al terug komen uit de database en sowieso integer zijn.

Als ze uit de database komen zal het inderdaad weinig uitmaken.
Ik heb er echt voor mezelf ( ) een best practice van gemaakt om altijd te typecasten naar een int voor ik het naar de database gooi. Op deze manier weet ik zeker dat er geen malafide data in mijn database komt, maar hooguit niet-kloppende. (een 0)

[ReenL]

Ik vind de "or die" niet echt subtiel. Als 1 genre niet toegevoegd of gevonden kan worden betekend dat niet dat dat voor alle genres het geval is. Daarnaast wordt je layout in het midden afgebroken.

Om race conditions te voorkomen zou je eigenlijk dit moeten doen:
- Unieke key op genre.name;
- Insert
- Als de insert faalt een select
Wanneer je dit niet doet kan het voorkomen dat een genre 2 keer toegevoegd wordt als je (per ongeluk) tegelijk submit.

Hoe het mooier kan? Naar mijn mening:

PHP:

foreach ($_POST['genre'] as $genre) { 
    if (empty($genre)) { 
        continue;
    }
    $sGenre = mysql_real_escape_string($genre); 

    $getGenreIDQuery = "SELECT `genre_id` FROM `genres` WHERE `name` = '{$sGenre}'"; 
    $execQuery = mysql_query($getGenreIDQuery) or die("MySQL error: could not retrieve genre ID from database."); 
    (...etc...)

Op deze manier kun je meteen zien dat wanneer genre leeg is er verder niets gebeurd. Daarnaast zorg je ervoor dat je code niet aan de rechterkant van je scherm eindigd omdat er allemaal spaties/tabs voor staan en je if block is lekker klein.

[ Voor 44% gewijzigd door ReenL op 07-09-2010 10:00 . Reden: Belangrijk boven ]

[NMe]

Of je nou if (conditie) continue; gebruikt of if (!conditie) als voorwaarde stelt voor een compleet codeblok is puur persoonlijk dus dat soort adviezen zou ik niet geven. Beiden methodes zijn correct en beide hebben voor- en nadelen. Eenmaal gecompileerd zal een compiler beiden ruwweg op dezelfde manier uitpoepen.

[Killemov]

NMe schreef op zondag 05 september 2010 @ 23:36:
Of je nou if (conditie) continue; gebruikt of if (!conditie) als voorwaarde stelt voor een compleet codeblok is puur persoonlijk dus dat soort adviezen zou ik niet geven. Beiden methodes zijn correct en beide hebben voor- en nadelen. Eenmaal gecompileerd zal een compiler beiden ruwweg op dezelfde manier uitpoepen.

Het is natuurlijk wel enigzins off-topic maar ik vindt dat ReenL een punt heeft. Netto is het code-oppervlak minder en dus een verbetering.

[NMe]

Je zegt al "ik vind". Dat is dus precies wat ik zeg, advies geven op basis van persoonlijke meningen. Als je niet verder nest dan twee of drie tabs maakt het geen fluit uit welke variant je kiest.

[Killemov]

NMe schreef op maandag 06 september 2010 @ 11:17:
Je zegt al "ik vind". Dat is dus precies wat ik zeg, advies geven op basis van persoonlijke meningen. Als je niet verder nest dan twee of drie tabs maakt het geen fluit uit welke variant je kiest.

Ik vind slaat dus alleen op dat hij een punt heeft niet op het vervolgstatement dat het een verbetering betreft.

[ReenL]

Beter zo?

[monnick]

Ja, zeker beter! Jouw lijntjes vond ik de netste oplossing

Code is inmiddels aanpast in de file. Nu binnenkort nog verdiepen in OOP. Mijn projecten zijn inmiddels dusdanig groot dat OOP mij hier kan gaan helpen.

[ Voor 3% gewijzigd door monnick op 07-09-2010 10:12 ]

[Michali]

Ik zou het netjes verdelen over functies die mooi beschrijven wat ieder stukje code doet. Dat helpt al een hoop om code overzichtelijker te maken. Het wordt dan wel meerdere niveaus diep, maar je kunt dan per functie verder kijken om achter meer details te komen.

Even snel uit de losse hand zou je bijvoorbeeld dit kunnen doen:

PHP:

$genreIDs = getIDsForGenreNames($_POST['genre']);
if (!empty($genreIDs)) {
    linkMovieToGenres($movieID, $genreIDs);
}

function getIDsForGenreNames($genreNames) {
    $genreIDs = array();
    foreach ($genreNames as $genreName) {
        if (empty($genreName)) {
            continue;
        }
        
        $id = getGenreIDByName($genreName);
        if (empty($id)) {
            $id = addNewGenre($genreName);
        }
        $genreIDs[] = $id;
    }
    return $genreIDs;
}

function getGenreIDByName($genreName) {
    $getGenreIDQuery = "SELECT `genre_id` FROM `genres` WHERE `name` = '" . mysql_real_escape_string($genreName) . "'";
    $execQuery = mysql_query($getGenreIDQuery) or die("MySQL error: could not retrieve genre ID from database.");
    $numResults = mysql_num_rows($execQuery);
    return $numResults ? (int)mysql_result($execQuery, 0) : null;
}

function addNewGenre($genreName) {
    $newGenreQuery = "INSERT INTO `genres` (`name`) VALUES ('" . mysql_real_escape_string($genreName) . "')";
    $execQuery = mysql_query($newGenreQuery) or die("MySQL error: could not insert new genre");
    return (int)mysql_insert_id();
}

function linkMovieToGenres($movieID, $genreIDs) {
    $movieGenres = array();
    foreach ($genreIDs as $genreID) {
        $movieGenres[] = "('".intval($movieID)."', '".intval($genreID)."')";
    }
    $insertGenreQuery = "INSERT INTO `movie_genres` (`movie_id`, `genre_id`) VALUES " . implode(", ", $movieGenres);
    $execQuery = mysql_query($insertGenreQuery) or die("MySQL error: could not insert genres");
}

Ik vind dat wel al een aardige verbetering.