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
– anditpainsme, Commented Oct 8, 2012 at 19:47
I'm using while becouse it's faster to write, for() will be better? — Bolek Lolek
– Bolek Lolek, Commented Oct 8, 2012 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 ifs. — Denys Kniazhev-Support Ukraine
– Denys Kniazhev-Support Ukraine, Commented Oct 8, 2012 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
– xception, Commented Oct 8, 2012 at 20:02

Slava · Accepted Answer · 2012-11-01 17:32:10Z

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 1, 2012 at 17:32

answered Oct 8, 2012 at 20:20

Slava

2,05015 silver badges15 bronze badges

Sign up to request clarification or add additional context in comments.

10 Comments

xception Over a year ago

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.

Slava Over a year ago

@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.

xception Over a year ago

@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 Over a year ago

@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 Over a year ago

@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 · Accepted Answer · 2012-10-08 20:20:42Z

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']]);

Collectives™ on Stack Overflow

Code optimization - mysql output

2 Answers 2

10 Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

10 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related