Code optimization - mysql output

Question

I think that my code can be easier, but I'm not sure. Look and tell me please some alternative if you have. This code i using to show informations about movies

$sql='SELECT DISTINCT id,title,img,description,adder,added,
GROUP_CONCAT(DISTINCT cid,"-",caty ) AS caty,
GROUP_CONCAT(DISTINCT oid,"-",obs,"-",face,"-",rola,"-",typ) AS obs
FROM film

LEFT JOIN f_o ON f_o.f_id = film.id
LEFT JOIN obs ON f_o.o_id = obs.oid

WHERE film.id ='.$fid;

$wynik=mysql_fetch_assoc(mysql_query($sql));
if(isset($wynik['id'])){
echo '<pre>';
print_r($wynik);
echo '</pre>';
////
$array  = explode(',', $wynik['obs']);

$r=array();//director - 0
$s=array();//Screenwriter - 1
$ak=array();//actors - 2
$akn=array();//actors 2 plan - 3
$np=array();//From Idea By - 4
$p=array();//producers - 5
$m=array();//music - 6

foreach ($array as $item)
{
    $a = explode('-', $item);
    if( $a[4] == 0 )
    {
        $r[] = $a[0].','.$a[1].','.$a[2].','.$a[3];
    }
    elseif($a[4] == 1 )
    {
        $s[] = $a[0].','.$a[1].','.$a[2].','.$a[3];
    }
    elseif($a[4] == 2 )
    {
        $ak[] = $a[0].','.$a[1].','.$a[2].','.$a[3];
    }
    elseif($a[4] == 3 )
    {
        $akn[] = $a[0].','.$a[1].','.$a[2].','.$a[3];
    }
    elseif($a[4] == 4 )
    {
        $np[] = $a[0].','.$a[1].','.$a[2].','.$a[3];
    }
    elseif($a[4] == 5 )
    {
        $p[] = $a[0].','.$a[1].','.$a[2].','.$a[3];
    }
    elseif($a[4] == 6 )
    {
        $m[] = $a[0].','.$a[1].','.$a[2].','.$a[3];
    }
}

function dzielperson($data){    
    $i = 0;
    $ile=count($data);
    while ($i < $ile) {
        $a  = explode(",", $data[$i]);
        $caty='<a href="/person/'.dolink($a[1]).'-'.$a[0].'" class="link1">'.$a[1].'</a>'.($i==($ile-1) ? '':', ');
        $i++;
    }
    return $caty;
}
echo '<br>Title: '.$wynik[title];
echo '<br>Desription: '.$wynik[description];
echo '<br>directors: '.dzielperson($r);
echo '<br>screenwriters: '.dzielperson($s);
echo '<br>actors: '.dzielperson($ak);
echo '<br>actors 2 plan: '.dzielperson($akn);
echo '<br>From Idea By '.dzielperson($np);
echo '<br>Producers: '.dzielperson($p);
echo '<br>Music: '.dzielperson($m);

}

Here is mysql output:

Array
(
    [id] => 1
    [title] => Pirates
    [img] => /images/1page_img1.jpg
    [description] => 
    [adder] => baambaam
    [added] => 1349387322
    [obs] => 1-aktor1-foto.jpg-shrek-3,2-aktor2-foto2.jpg-batman-0,3-aktor3-f1.png-Pirat-1,4-aktorzyna4-f2.png-Pirat 3-1
)

Tables structure:

film:id,title img,description,adder,added
obs:oid,obs,face,rola,typ
f_o:f_id,o_id

in column obs i have names of actors,directors....

It's not completly code but i wish that you understand

you could use a switch inside of the `foreach()` construct, no easier but in my opinion easier to read. also, why not use a `for()` loop in `dzielperson()`? — anditpainsme, Oct 08 '12 at 19:47
I'm using while becouse it's faster to write, for() will be better? — Bolek Lolek, Oct 08 '12 at 20:00
You're trying to cram too much logic into too narrow space. It would be helpful (for you as well) if your database tables had more descriptive names. Also, it would be useful to know database structure. As a quick tip, you could move `$a[0].','.$a[1].','.$a[2].','.$a[3]` into separate variable before all *if*s. — Denys Kniazhev-Support Ukraine, Oct 08 '12 at 20:02
Does nobody read the php manual nowadays, mysql_fetch and associated functions are deprecated, you should learn to use one of PDO or mysqli instead of the mysql extension. — xception, Oct 08 '12 at 20:02
Bolek Lolek, see: http://stackoverflow.com/questions/3875114/why-use-a-for-loop-instead-of-a-while-loop — anditpainsme, Oct 08 '12 at 20:04

Slava · Accepted Answer · 2012-11-01T17:32:10.300

2

$sql='SELECT DISTINCT id,title,img,description,adder,added
FROM film
WHERE film.id ='.$fid;
$wynik=mysql_fetch_assoc(mysql_query($sql));
if(isset($wynik['id'])){

// you should use constants instead of just if(type==_some_meaningless_number_):
$personTypeMap = array(
    'r'    //director - 0
    ,'s'   //Screenwriter - 1
    ,'ak'  //actors - 2
    ,'akn' //actors 2 plan - 3
    ,'np'  //From Idea By - 4
    ,'p'   //producers - 5
    ,'m'   //music - 6
);
// so above should be something like:
// define('PERSON_TYPE_DIRECTOR', 0);
// define...
// then you wouldn't need that array-map above as well as would be easier to understand who is what

// init all people subarrays:
$people = array_fill_keys($personTypeMap, array());

$sql='SELECT oid, obs, typ   #add other fields if you actually use them
FROM f_o
INNER JOIN obs ON f_o.o_id = obs.oid
WHERE f_o.f_id ='.$fid;
$peopleResult = mysql_query($sql);
while ($person=mysql_fetch_object($peopleResult)) {
    $people[$personTypeMap[$person->typ]][] = dzielperson($person);
}

function dzielperson($person){    
   return '<a href="/person/'.dolink($person->obs)."-{$person->oid}\" class=\"link1\">{$person->obs}</a>";
}

// join people in all categories through comma:
foreach ($people as &$category) {
    $category = implode(', ', $category);
}

echo '<br>Title: '.$wynik['title'];
echo '<br>Desription: '.$wynik['description'];
echo '<br>directors: '.$people['r'];
echo '<br>screenwriters: '.$people['s'];
echo '<br>actors: '.$people['ak'];
echo '<br>actors 2 plan: '.$people['akn'];
echo '<br>From Idea By '.$people['np'];
echo '<br>Producers: '.$people['p'];
echo '<br>Music: '.$people['m'];

P.S. I'm fixing your code for you for one reason: your original made me laugh for 10 minutes non-stop :) Thank you.

P.P.S. I left some of the original mess behind, but take it as an opportunity to learn what was wrong with your code and try to simplify that yourself.

P.P.P.S. Yes, multiple queries in this case is better than single monster collecting unrelated stuff in single row.

edited Nov 01 '12 at 17:32

answered Oct 08 '12 at 20:20

Slava

2,040
15
15

I disagree with your P.P.P.S., usually making round trips to the database is slow, even on localhost, also running multiple queries makes it even slower. The rest looks cool, didn't actually test it but seems ok so you still get a vote from me, the code is simpler and more readable which is I think what he desired with his question. – xception Oct 08 '12 at 20:25
but what was funny? this long foreach? – Bolek Lolek Oct 08 '12 at 20:29
@xception: It makes it slower for you because your PHP host isn't loaded much, and also don't forget you're counting ping between host<=>db too. 2 queries usually run faster in MySQL if tables are indexed properly, and it is easier to process results on PHP side. So even though overall query+transmission_time is greater than 1 query, it still takes less resources. Bolek: Привет полякам! :) – Slava Oct 08 '12 at 20:30
1

@Bolek: the overall idea of your code: getting different things as one, rearranging that multiple times into arrays with incomprehensible names... Just think how difficult it would be to read after, say, half year on unrelated tasks, what actually $a[1] was, what column was that in DB? "Aha, that's coming from $data... Data... Oh... 1 row of data is $a[0].','.$a[1].','.$a[2].','.$a[3]; Of course! That makes perfect sense!" :) Just imagine that the one who will be supporting your code later is a maniac and will hunt you down if he finds it difficult to alter your code. – Slava Oct 08 '12 at 20:41
1

@BolekLolek I didn't take that much time with optimizations as Slava but I still shortened a hell of a lot of that foreach, yeah, it was funny, made me laugh too. – xception Oct 08 '12 at 20:41
1

@Slava you make an interesting argument, I guess you are right in some cases, it all depends on how well you write your queries and how well indexed your database is, however from my observations it's usually faster if you can write a smart query than to use multiple queries. Regarding your next statement to Bolek, did anyone hunt you down so far, do you give that advice about maniacs from experience? – xception Oct 08 '12 at 20:43
@xception: I understand your point, I used to think exactly that way some year ago. :) Then I joined a project with over 500 hits/minute which makes for living by aggregating loads of data in DB. Latest dump was ~600Gb; we've got lots of trouble with it and now plan to replace MySQL with something more enduring... – Slava Oct 08 '12 at 20:52
1

@Slava try postgresql, but you really have to tune it's configuration if you use such huge tables, it's quite fast and has this really cool extension, running `explain analyze QUERY` will run the query and output it's estimates as well as real timing information on each step of the chosen plan, and you can adjust estimation factors so it always chooses the best (most efficient) plan. And you can also use that information to tune your queries quite well, helped me a lot in optimization. – xception Oct 08 '12 at 20:56
@xception: ...But yes, there is no magical solution for everything. Every situation has own vision of "ideal algorithm". So you're right as well. About Postgre: that's already one of the candidates, but thanks for the tip. :) – Slava Oct 08 '12 at 20:56
I suspect the other's oracle or some really funky NOSQL stuff. – xception Oct 08 '12 at 21:00

xception · Answer 2 · 2012-10-08T20:20:42.540

Shorter version of your code starting with your foreach

$result = array(
    array(),
    array(),
    array(),
    array(),
    array(),
    array(),
    array()
);

foreach ($array as $item)
{
    $a = explode('-', $item);
    $result[ $a[4] ][] = $a[0].','.$a[1].','.$a[2].','.$a[3];
}

function dzielperson($data){
    $i = 0;
    $ile=count($data);
    while ($i < $ile) {
        $a  = explode(",", $data[$i]);
        $caty='<a href="/person/'.dolink($a[1]).'-'.$a[0].'" class="link1">'.$a[1].'</a>'.($i==($ile-1) ? '':', ');
        $i++;
    }
    return $caty;
}
echo '<br>Title: '.$wynik[title];
echo '<br>Desription: '.$wynik[description];
echo '<br>directors: '.dzielperson($result[0]);
echo '<br>screenwriters: '.dzielperson($result[1]);
echo '<br>actors: '.dzielperson($result[2]);
echo '<br>actors 2 plan: '.dzielperson($result[3]);
echo '<br>From Idea By '.dzielperson($result[4]);
echo '<br>Producers: '.dzielperson($result[5]);
echo '<br>Music: '.dzielperson($result[6]);

dzielperson function was not modified at all.

simplified syntax for initialization of $result:

$result = json_decode('[[],[],[],[],[],[],[]]');

in case you want the old names for readability:

$names = array_flip(array('r', 's', 'ak', 'akn', 'np', 'p', 'm'));
echo '<br>directors: '.dzielperson($result[$names['r']]);
echo '<br>screenwriters: '.dzielperson($result[$names['s']]);
echo '<br>actors: '.dzielperson($result[$names['ak']]);
echo '<br>actors 2 plan: '.dzielperson($result[$names['akn']]);
echo '<br>From Idea By '.dzielperson($result[$names['np']]);
echo '<br>Producers: '.dzielperson($result[$names['p']]);
echo '<br>Music: '.dzielperson($result[$names['m']]);

Code optimization - mysql output

2 Answers2