Optimization of some code!
 

Hellow dear fellow PHP developers.

I have made a simple portfolio, but also "too simple". It's basically only if() statements, and then it does something - which results in high MySQL traffic - and lot's of useless code (I think at least!)

I've now looked around for a couple of hours in search for a function which I could use, to make this script a lot smaller. But, beeing unsure what to do - and everything I tried went very very wrong, I decided to search for help from more experinced people.

My code looks like this..:

I'm super emberassed with this code, but as I said - also very unsure how I would do this with a while, do, for, foreach function? Can anyone show me? That would lead me a long way!

I've read somewhere that using SELECT * FROM causes too much CPU usage, so even if it is a pain, I suppose you can make a SELECT call on each column, this way mysql doesn't have to think.

You can probably get away with using mysql_fetch_assoc($result) instead of array, I don't know if it's true, but I heard it was a little faster too...

Also, to replace the if's, you can use switch, it is faster.

Any chance you can show me a small bunch of code with what you exactly mean? :)

Sure, here's a snippet, you should be able to figure out the rest based on the pattern..


I'll take a look at your IF statements, a lot is redundant...

Indeed! You should always be careful when using SELECT *.

You need to ask yourself: Do I need all of the columns? more than likely not.

The reason you need to be wary is that getting MySQL to retrieve data it doesn't need is a waste of resources better used elswhere. This waste adds disk I/O, memory and CPU overheads where they are simply not needed, not to mention network overhead and also potentially b0rking up covering indexes (if there are any).

This may be true (haven't tested it) because mysql_fetch_array by default will return both numeric AND associative arrays for the result (unless you pass in the 2nd param), where as mysql_fetch_assoc will only return an associative array.

Whilst I do believe this to be true (it also adds a heap of readability to long conditional statements), this post suggests otherwise: http://www.fluffycat.com/PHP-Design-...-if-VS-switch/

N.B read the comments, its quite intriguing, I'll have to run my own benchmarks.

This brings me nicely to my next point, if a script needs so many if/else's there must be an easier way to do it.

Agreed. Looking over his IF conditionals, I thought it might be easier to consolidate them but in each one he echo's out a $p1 variable as well as the variable that is related to the IF test itself.

It kinda looks like a pagination script....

I'm also curious what all the p1, p2,p3 is all about...pages?

Cool.

To clear up, this is for a mini-portfolio - so it adds more pictures (the $p1 = picture one url, $p1d = picture one description) - if I choose so from my Admin panel. Which is why I have all the if()'s, so that if those rows are empty, it should not add an empty picture, but just skip that line of code and do NOTHING. :D - but if the row is filled with an url, it should print out the picture to the gallery.

It's in action here.

Sorry if the explanation is messy, just ask if I need to clear something up. :) Cheers

I could not find where you were using $id, $name and $website.

You should change the Selet * to Select col1, col2... however for most smaller sites it really does not matter unless you have a ton of extra columns. There is no need for the extra variables, and a switch would probably be better but it really does not matter on that either. You only need 1 echo to print all the junk instead of the multiples. so something like this would be a bit better.

I see, I don't think that is the correct DB structure then.

You are basically creating a unique column for each unique type of picture. You should only have 2 columns.

I mean, if you have 1000 pictures, you can't seriously think about having 1000 columns?

There's a limit for columns for mysql tables that in it itself!

How would I do that then Allwork? I suppose their gonna be seperated by a space or something, but how am I gonna sort those out for good? :)
That really sounds like the stuff I'm looking for!

CoryMathews, thanks for your clearing up! Let's see how this ends up :))

I'll use Corey's example.

$id = $row['id'];
$name = $row['name'];
$website = $row['website'];

So your table now has just 3 columns.

Each portfolio/picture gets a new row. Not new column.

That way as you loop through the rows, you can base your conditionals on the values you get from the rows, not going through each column and doing IF conditionals..

Back to what Sketch said, "if you're using so many if conditionals, there's gotta be a better way to do it" or something like that...

In this case, it's redesigning your DB structure......

Alright, I get the idea of this!
I suppose I'm gonna do somehting like 5 fields:

1. ID
2. Thumb [0 if this is not the thumb, 1 if this is the thumb]
3. Picture [The picture url, left blank if Thumb == 0]
4. Picture Description [Will be what's in the <a title='' /> in the Thumb]
5. Bound To [The gallery the pictures are bound to, the ID of the thumb]

In the PHP I will only have 2 IF's

Yup, you can make the thumb column of bool type since it's just going to be either 0 or 1.

Yep, its a rule I personally stick too. If something turns out fabulously long winded, there is obviously a problem in the design. In those cases, a rethink and a re-factor are in order.

Do you need the thumb in the DB? I think we can leave PHP out of the processing of this (i.e. serving thumbs or full pictures and all the logic the accompanies it).

Easiest solution (bearing in mind, i quickly hack this together):

Have 2 pictures (I'm assuming you have a full and a thumb of each), name them <picturename>.gif and thumb_<picturename>.gif (or whatever).

Now all we do is prefix 'thumb' to the image to get the thumb:
And for the full size:
Now there is no need for more DB columns.


With a DB structure like this:

Test data:

We could crudely output it thus:
That what you were after?

Sketch, cool idea! I'm gonna work on it in a few minutes. :) I'm very happy for everyones help!

Alright, here is how it all ended up..

I felt like I needed to create a Thumb BOOL, as I have > 1 pictures in one gallery, which are invisible on the page before the thumb is opened, and you can browse them in the Thumbail.


After that, I began the coding part. I realised the hard part was to tell the image which "hidden gallery" it was in, so I decided to use functions, to check that - my first use of functions effectively!

Here's how it all ended up:
I got one question though..!
How do I just make the echo's one long line, with the functions in? I couldn't manage to do that! Like this:

Edit: Fuck, for some reason it bugs when there's > 1 image bound to the thumb.
Edit2: It seems like that the HTML just stops after this PHP, even though theres around 100 lines more HTML. Anyone got an idea of what's going on?! :O
Edit3: It gotta be in the else{ ], since it only runs one of those, and is supposed to do 3 with my current database.
Edit4:
It looks like this when I view the HTML source on my page:
So we can see it does only run the else { } one time, but it should run it two times:

Edit5:
It's fixed! I just cleaned up the code on the couch watching a movie! Macbook ftw.
It seems to work fine! Haven't tested it with more thumbs yet though. :(
I'm just gonna clean up the rest of the code first.
Edit6: Worked great.

You should really try and use sprintf when you have such complex lines of echoes! Otherwise you do often find you create a pretty mess.

As I'm currently learning about classes, it'd be super cool if someone had somne time on their hands to make my code to a class, and how to use that class so I get the same output!

I'm referring to this code..: