Cleaning up my code.. a little advice?

[BLiZZaRD]

Okay, so it is no secret that me and php don't get along. I wrote a few things and it works. It works well.

But looking at it I think I have more in there than I need. So I decided to clean it up. Stream line it if you will.

The problem is that I don't know enough about php to do it without breaking the code.

So I ask.. what is the best method of doing this? Is there something I can read about that will help me prune my coding for this script?

I just don't know what I am doing, but I hate how long it is, and how much is there that I don't actually need.

[djr33]

Hard to say.

First, use proper tabs and line breaks, if you haven't already.

Second, see if you can simplify any operations with simpler/faster functions.

Commenting your code can also be a help (though you could strip comments for the final script to maximize efficiency), as you can see quickly what stuff does. Commenting might be annoying, but it's very helpful.

You can test how long a repeated series of actions takes by looping through it 100+ times then comparing it, and you can see which takes less time from start of execution to the end.

Regex is certainly something to look into, if you have the time to learn it. I haven't yet, and I wish I knew it, but actually starting on it is daunting-- seems I always have something else to do first.

Quote styles are important too-- double quotes allow parsing of the string inside, so they are slower. Use single quotes unless you need that parsing.

Nothing else I can really say without seeing the code itself.

[BLiZZaRD]

Well... because of the nature of the code I can't post it here (google indexing and all) but if you want to see it I can email or PM it to you.

Cause, what you just said.. umm.. yeah.

[djr33]

Post chunks if you want, or you could send it to me if you need some insight. I don't personally "want" to see it; it's fine if you don't send it; but if you do need some help, I could take a look.
I can't claim I'm the best at efficiency, though. I'm good at using what I know (which is a fairly limited amount in some sense) well, to do a lot, but that doesn't mean it's always the easiest/best way.

Any particular questions? Type of code? (Databases? Math? etc.)

[BLiZZaRD]

No, I will send it, it's not a "private" code so to speak. It is a type of password thing I have been working on for my game site, and due to google indexing I don't really want my players seeing my password code ;)

I have commented it with where things go, what changes I need to make as I put it on line and I also commented my questions below each section. It works, and it works pretty good. I just think I have redundant things and unnecessary things in it.

I will email the file, it's a single page PHP file.

Thanks for looking!

[tech_support]

Replace the password with random words in it. :p

[Twey]

Attach it in a ZIP file. Google only caches files it reads, as far as I'm aware. We can remove it later. You can also change the actual password.

For clean code:avoid repetition: if you find yourself with a lot of lines of code that look the same, chances are you should've used a loop. Use object orientation where appropriate: PHP's OO support is a bit pants, but still better than nothing. If you're working with databases, see Propel (http://propel.phpdb.org/). Separate your code from your HTML. See Smarty (http://smarty.php.net/). Won't make your code cleaner in itself, but it's a good idea to have one entrance page include all the others (if done properly). This enables you to have clean URLs based on $_SERVER['PATH_INFO'], especially if you combine it with mod_rewrite. It also allows you to include what you need without doing it explicitly in each page.

[BLiZZaRD]

I suppose it is better to see it, I do have repetitive lines, but they are in different files. (read: go on different pages) It is pretty clean I just think there is extra crap in there that I don't really need.

I have zipped up the file I sent to Daniel, note it is not a log in script, so the entrance page include is kind of out. I do have a piece in there that gets placed on other pages (checks sessions) and it is total PHP, my host says they don't support SQL_Auth, and I don't feel like testing them on that one (although I proved them wrong with PHP_Auth, LOL)

I don't mind using what I have, I just think it is good practice to use clean, small, efficient code, and I have never dealt with this much php to know how to clean it up properly.

notes included. :D

[djr33]

Placing the same code on different pages is a very bad idea.

It's much better to write functions and base it on that.

include('main.php');

Then you can just have all of that processing scripted in one page, and then you can just call each chunk as a function, like "processtop()", etc.

That will allow you to not worry if it's perfect, either, because you can update the main page any time you want.

Your script didn't seem to really have any specific problems.

It was mainly if and else statements. Might be ways to get it simpler, using functions.

Switch statements can compress it further, too, at times.

if and else aren't very complex for the processor, either, though it seems more advanced programmers have less long lists of if/else, so there are some methods to look into.

A couple things could use regex, and there may be some more efficient string functions if not. strchr(), which I recently discovered (thanks to tech_support), seems much better than the constant use of substr(...,strpos(...)) that always shows up in string parsing.
If that doesn't work perfectly, I would suggest writing your own better substring function for simpler code, and less repetition.

Look through your code; if you write anything 3 or more times, make it a function; if you write it twice, you can, though that doesn't help so much (by the time you write the function and call it twice, you haven't saved much work).

[BLiZZaRD]

Okay, well the part that is going on all the pages is going to be an include, LOL. SO that isn't a problem.

Like I said the code works well, I was just assuming there was something I didn't need in there, little extra crap, you know?

Thanks for looking! I appreciate it!

[djr33]

Sure.

It's pretty basic-- not much you can do to simplify if/else statements, unless there's another method (like switches).

But looking into those better string functions is a good idea.

If you're using an include, then should work well.

Good luck.

Are there any specific parts of the code you were wondering about?

[BLiZZaRD]

Yes, there was one in particular. Lines 35 to 47. As stated in the //comment tags there I am not sure all that is needed, since the page that section goes on doesn't have a password input box. It needs to check the session, and load the include if there is no session. It then needs to show the HTML content (the last else{..)

But it only checks for the posted password, so I don't think it needs the array and the post ID sections (exactly lines 38 to 41 I think can just be deleted, but I am not sure).

Also, ALL the pages here that they go to are .html, and I have this part:



header('Location: http://' . $_SERVER['HTTP_HOST'] . $p[$_POST['id']][1] . '?pass=' . $_POST['pass']);


on both parts, and I would like to NOT have the "?pass=password" in the URL as it is distracting. If I could get rid of that, that would be awesome!

[djr33]

Why don't you set a session variable of "pass" to "[password]"? That would eliminate any need for the link and make it more secure.

[BLiZZaRD]

How would that effect the rest of it though? I guess I don't understand what you mean exactly.

[djr33]

The only point in sending "password" in the url is to get it to the next page, right? You can do this:

//first page, above header:
$_SESSION['pass'] = $passval;
//next page:
if isset($_SESSION['pass'])....

[BLiZZaRD]

hmm Okay... I will think about that. Thanks!