Sure, I'll try to cover the basic points where you can improve - in particular on coding standards:
- Keep data types in mind when defining constant and variables, there's for an example no need to use a string to represent an int/number:
This:
<?php
//define a maxim size for the uploaded images in Kb
define ("MAX_SIZE","1024");
?>
Is better written as:
<?php
// Define a maxim size for the uploaded images in Kb
define('MAX_SIZE', 1024);
?>
- Your indentation is varying greatly, some places you use as much as 9 spaces for a single indentation. Keeping this consistent and at a reasonable number is important for code readability and future maintenance. I would recommend using two spaces, which is pretty much the standard on major projects these days.
That means that this:
<?php
function getExtension($str) {
$i = strrpos($str,".");
if (!$i) { return ""; }
$l = strlen($str) - $i;
$ext = substr($str,$i+1,$l);
return $ext;
}
?>
Becomes this:
<?php
function getExtension($str){
$i = strrpos($str,".");
if (!$i) { return ""; }
$l = strlen($str) - $i;
$ext = substr($str,$i+1,$l);
return $ext;
}
?>
(On a side note, I can also mention that this way of getting the file extension seems rather unnecessary. getimagesize() will provide you with imagetype and mimetype which can be used for validation. (In additon, you also get image dimensions if you would like to enforce a maximum there.))
- You should make sure to space out parameters for function calls properly, with a single space after each separator.
In other words, so that this:
<?php
substr($str,$i+1,$l);
?>
Becomes this:
<?php
substr($str, $i+1, $l);
?>
- Control structures such as if, for, while and so on should always have a space between the control keyword (e.g. if) and the opening parenthesis to distinguish them from function calls.
For an example..
<?php
if(isset($_POST['Submit']))
?>
Should be written as this:
<?php
if (isset($_POST['Submit']))
?>
- Binary operators such as =, !=, == and so on should have a space before and after, so that..
<?php
$errors=0;
?>
Becomes:
<?php
$errors = 0;
?>
- When using string concatenation (i.e. linking string together), you should use a space before and after the dot/period, so that..
<?php
$newname="./forums/images/avatars/gallery/".$avatar."/".$image_name;
?>
Becomes..
<?php
$newname = './forums/images/avatars/gallery/' . $avatar . '/' . $image_name;
?>
- For multi-line comments, the /** style commenting is more suitable:
<?php
//This variable is used as a flag. The value is initialized with 0 (meaning no error found)
//and it will be changed to 1 if an errro occures.
//If the error occures the file will not be uploaded.
?>
Becomes:
<?php
/**
* This variable is used as a flag. The value is initialized with 0 (meaning no error found)
* and it will be changed to 1 if an errro occures. If the error occures the file will not be uploaded.
*/
?>
- You should be consistent with what kind of block style you use. The two alternatives are:
<?php
if ($foo == $bar)
{
print 'meh!';
}
?>
Or..
<?php
if ($foo == $bar){
print 'meh!';
}
?>
I would recommend the latter (last one) because it generally gives better flow/readability in the code, and is the more common style to use these days. Also the standard for most major open source projects. Whatever you do, do not use both / mix them. It will be a mess.
- In some cases it can be sensible to use parenthesis to enclose/separate parts of complex mathematical expressions and similar. In this case however, it is rather unnecessary:
<?php
if (($extension != "jpg") && ($extension != "jpeg") && ($extension != "png") && ($extension != "gif"))
?>
- When you end a hierarchy of blocks enclosed in brackets, do not end them all at the same line like this..
<?php
(...)
if (!$copied)
{
echo '<center>Upload unsuccessfull!</center>';
$errors=1;
}}}}
?>
Instead, you should have one closing bracket at a time - at new lines and a negative indentation that corresponds to the opening bracket of the block you are closing. Like this:
<?php
// Checks if the form has been submitted
if (isset($_POST['Submit'])){
(...)
// If it is not empty
if ($image){
(...)
if ($extension != 'jpg' && $extension != 'jpeg' && $extension != 'png' && $extension != 'gif'){
(...)
}
else {
(...)
// Compare the size with the maxim size we defined and print error if bigger
if ($size > MAX_SIZE * 1024){
(...)
}
(...)
// Check if target file exists, copy the file if not.
if (!file_exists($newname)){
if (!copy($_FILES['image']['tmp_name'], $newname)){
(...)
}
}
else {
(...)
}
}
}
}
?>
This allows you to trace a closing bracket up to the corresponding control keyword / start of the block, and greatly helps in keeping the stricture clean and simple. Important in regards to avoiding bugs, and less trouble debugging issues.
- Instead of include, you should use
require_once() - because this will avoid including the file if it has already been included elsewhere in the code, thus avoiding potential collisions in cases where for an example multiple files/scripts depends on the same function declarations defined within the same include.
- A handy trick for multi-line strings such as blocks of HTML markup, is to use
heredoc delimiting. This is done using the operator <<< followed by an identifier, a new line, then the text, and finally the same identifier to close. Example using your markup:
<?php
echo <<<EOL
<center>Your avatar has been uploaded!<br /><br />
<img src="../../$newname"><br>
To view the Avatar Gallery go to "<a href="/forums/profile.php?mode=editprofile">Edit Profile</a>" go to the Avatar section and click "Show Gallery"!
<a href="/avatar/upload/">Upload</a> another avatar.
</center>
EOL;
?>
This will behave just like a double-quoted string, meaning you have variable substitution. Thus $newname in the above example will be replaced with the appropriate value. It will also preserve any indentation you add within the text, so that for an example indented HTML is actually outputted the way you wrote it. Another advantage in relation to markup is that there's no need for escaping either quote types, as the string/text is delimited by the identifiers (in my example EOL) instead.
--
I might have missed a few, but it's 5 am where I am in the world.. so time to get some sleep.
Hopefully this should help a little on your journey towards learning PHP anyway. Apart from the coding standard, you seem to be doing reasonably well so far. If you run into any issues, or would like to discuss anything related to web development, you know where to find us.
- S.
Hi there! If you found this post useful, or used this information to help others, we would greatly appreciate a link back to our forum from your website/blog. Thanks! =)
0
Thanks again.
