Code critique

[PaulRyan]

I was pretty bored and wanted to code some, so I made a flat file shout box.

I'm gunna develop a few other scripts over the next few days, to refresh my basic knowledge and hopefully pick up some new things along the way.

This is the first in line of about 6 or 7 things I want to put together.

I just want get some critique on the code I've written below:
 

<?PHP

  //### Set up variables
  $messageFile = 'file.txt';
  $maxMessages = 15;

  //### Open the file and put the messages in an array
  $messageArray = file($messageFile); 

  //### Assign the current page name
  $pageName = basename($_SERVER['PHP_SELF']);    

  //### Check to see if the form has been posted
  if($_SERVER['REQUEST_METHOD'] == 'POST') {    

    //### Check incoming data and assign it
    $name    = isset($_POST['name']) ? htmlentities(trim($_POST['name'])) : FALSE ;
    $message = isset($_POST['message']) ? htmlentities(trim($_POST['message'])) : FALSE ;    

    //### Do some variable validation
    if(empty($name)) {
      $error = 'Please enter your name.';
    } else if(strlen($name) > 20) {
      $error = 'You name cannot exceed 20 characters in length.';
    } else if(empty($message)) {
      $error = 'Please enter a message.';
    } else {     

      //### Count the messages in the array
      $messageCount = count($messageArray);      

      //### If there is $maxMessages or more messages, remove the last one(s) to add the new one
      if($messageCount >= $maxMessages){        

        //### Calculate the message difference
        $messageDifference = $messageCount-$maxMessages;        

        //### Remove messages that are over the max message limit
        while($messageDifference >= 0) {
          $messageDifference--;
          array_pop($messageArray); 
        }
      }     

      //### Add the posted message to the array
      array_unshift($messageArray, "{$name};{$message};{$_SERVER['REQUEST_TIME']};{$_SERVER['REMOTE_ADDR']} \r\n");

      //### Implode the messages back to a long string for the file
      $newMessages = implode($messageArray);    

      //### Put the messages back into the message file
      file_put_contents($messageFile, $newMessages);    

      //### Redirect to the message page
      header('Location: '.$pageName);
      exit;
    }//### End of variable check
  }//### End of form post check 

  //### Start the output
  $output = '<strong><u>Shoutbox</u></strong> <br>';

  //### Check to see if an error exists, if so, apply it
  if(isset($error)) {
    $output .= '<span style="color:red;"> Error: '. $error .' </span> <br>';
  } 

  //### Iterate over the messages and add them to the output
  foreach($messageArray AS $messageData) {
    list($name, $message, $timestamp, $ip) = explode(';', $messageData);

    $output .= ucfirst($name) .": {$message} \r\n";
  }

  //### Display the messages
  echo '<pre>';
  echo $output;
  echo '</pre>';
?>

  <form method="POST" action="<?PHP echo $pageName;?>" style="margin:10px 0 0; padding:0;">
    <table cellpadding="0" cellspacing="2" border="0">
      <tr>
        <td> Name: </td>
        <td> <input type="text" name="name" value="<?PHP if(isSet($_POST['name'])) { echo htmlentities(trim($_POST['name']), ENT_QUOTES); } ?>"> </td>
      </tr>
      <tr>
        <td> Message: </td>
        <td> <input type="text" name="message" value="<?PHP if(isSet($_POST['message'])) { echo htmlentities(trim($_POST['message']), ENT_QUOTES); } ?>"> </td>
      </tr>
      <tr>
        <td colspan="2"> <input type="submit" value="Send Shout"> </td>
      </tr>
    </table>
  </form>

Edited March 21, 2013 by PaulRyan

[Hall of Famer]

Well what I can say...

 

First of all, congratulations on what you have accomplished so far, I suppose the code is working properly? I like how you pay attention to security and not to trust user input data, thats a very important step.

 

Second, try separating your business logic from presentation layer. A true professional script requires this separation of concerns, you can have a controller file handling business logic/commands, and a view file displaying HTML results.

 

On the other hand, use OOP. Procedural programming is for amateurs and only good for newbie's learning process, use objects and object oriented design can improve your code dramatically, especially for future extensibility/reusability.

Edited March 29, 2013 by Hall of Famer

[KevinM1]

On the other hand, use OOP. Procedural programming is for amateurs and only good for newbie's learning process

 

Do we really need to have this discussion again?

[Hall of Famer]

 

Do we really need to have this discussion again?

 

Nope, not really. I was just posting my advices/feedback to the OP, it was not meant for a thorough discussion. It is his thread anyway, and wont help him much if we discuss something unrelated to the main topic.

Edited March 29, 2013 by Hall of Famer

[Psycho]

A flat-file shout-box is probably not a good idea as there is too much of an opportunity for multiple read/writes to be attempted concurrently.

 

Some other notes:

 

1. You have a line of error checking that ends with an else statement to retrieve the new content. I would just have the error code and then close the if/else chain. You can then check if there were any error before getting/setting the messages. On that same note, I would define $errors = false at the top of the page and check for that instead of isset(). Or, better yet - set $error as an empty string. Then you can always display $error in the output without the need for an if condition. Also, I don't like stopping at the first error and instead prefer to find all errors to display to the user. But, what you have will work.

 

2. The logic for max messages is way over complicated. Never do something in loops that doesn't need to be. In this case, just add the message to the beginning of the array and then use array_slice() to get the max messages. Do this regardless of whether the messages exceeds the max or not.

 

3. There is no error handling for the reading/writing of the file

 

4. When there is POST data, at the end of writing the file you appear to do a redirect back to the same page. Why not continue the script and just display the data? I would simply process the post data - if it exists. Then, completely separately, display the data.

 

5. Your data is stored using a semi-colon as the deliminator. If a user puts a semi-colon in their data it will break the output. Use a character that can not be in the data. Since you are using htmlentities() you can use any character that would be transformed by that process, use a non-printable character (e.g. tab "\t") or something else to prevent that problem.

 

 

6. You use ucfirst() on the $name AFTER you saved the data using htmlentities(). This could result in some unexpected results if the character the user entered is transformed by htmlentities(). Either, save the data without htmlentities() and use that during the output or use ucfirst() before you run htmlentities. Personally, I try to store data in it's original state whenever possible and only transform when needed based upon the context.

[PaulRyan]

@Hall of Famer, thanks for your input, I will take into consideration what you have said. I'm not to keen on OOP if I'm completely honest, either I haven't found a good enough tutorial to explain it, or I understand and just don't find it useful.

 

@Psycho, thanks for your input. I will make said changes to the script, I totally forgot about array_slice so thanks for that. The error handling will be added too. Regarding the redirect, this is something I've always done with processing data plus it stops the refresh Pop-Up with the post data.

I totally agree with the deliminator, I didn't really think it though.

 

Thanks for your critique, I will work with what you have given me.

[Christian F.]

First of all I'd just like to congratulate you too, you seem to be progressing nicely and taking heed of the advice given by others.

 

Secondly: There is no point, what so ever, in writing this using OOP principles. Contrary to what Famer thinks, procedural (or OOP) is no indication what so ever on the skill level of the developer. It's the quality of the code itself, and whether or not the proper design principles for the given task has been followed.

Using OOP only for the sake of OOP, is actually a sign of an inexperienced programmer.

 

Then, let's get cracking with the critique:

• basename is not adequate protection for $_SERVER['PHP_SELF'], as evidenced by this snippet:

  php > echo basename ("test/index.php/test2/forall?test=jalla&true#bleh");
  forall?test=jalla&true#bleh

  Notice that the URL is valid, but the result is far from what you'd want. Not only does it strip out the path of the file, but it also incorrectly determines what the path is. To properly secure PHP_SELF, please see this thread.
• As Famer already mentioned, your input validation isn't much of an actual validation.

  Not only do you use output functions in it, which should only be used when sending content to the relevant third party system, but you're also allowing the users to post just about anything they like. Including content which would mess up the structure of your message file, and as such break your "database".

  You're also failing to validate that the REMOTE_ADDR is actually an IP-address. Seeing as this value is populated from the value that the client sends, it can be subject to injection attacks.

   

  What you should have done, is to actually validate the content that the user sends. To make sure that it conforms to a set of rules, that is (generally) universally true for the content you expect. Names, for instance, does not (normally) contain hash tags, colons or any other non-alphanum character (except certain punctuation characters).

  This is an abbreviated version of the function I use for validating names:

  function validate_name ($name, $maxLength) {
  	if (!preg_match ('/^[a-zA-Z\\pL][\\w\\pL\' \\.\\-]{1,'.$maxLength.'}\\z/u', $name)) {
  		return false
  	}
  
  	return true;
  }

  The message content is a bit special, as you are expecting your users to be able to use all (or just about all) printable characters. Still, that leaves some non-printable characters out in the cold, which you should test for. Looking at an ASCII table should tell you what those are.
• The next item on the list is how you do these tests, and the current way of doing it is sub-optimal. What you've done means that the user has to submit the form multiple times, to get all error messages about any invalid/missing content. Instead of wrapping all of the tests in an else-if block, do each test individually. Then, after all validation tests have been completed, check if any errors were raised. If they were, then you know that you should abort parsing the input and show the form anew. Preferably with the previous data filled out (do not forget output escaping), and all of the errors that caused the submission to be declined.

   

  In short, something like this (pseudo code):

  if (submit) {
      if (!val_name ()) {
          $error[] = 'name';
      }
  
      if (!val_message ()) {
          $error[] = 'message';
      }
  
      if (!val_email ()) {
          $error[] = 'email';
      }
  
      if (!empty ($error)) {
          show_errors ();
          add_escape_output ($name, $email, $message);
          show_form ();
          return;
      }
  
      save_data ();
      redirect ();
  }

• Also, never ever manipulate the user-data outside of purely escaping. If the content violates some of the rules then you need to alert the user about it, and let him/her fix it. Not just silently drop something, as that is extremely annoying.

   

  Imagine spending one hour on a very thought out thread, where you're asking for help on a very elusive, but critical, problem. Only to discover that the forum-software decided to silently drop half of your post, forcing you to rewrite all of that from memory. If that half contained only code, it wouldn't be much of a problem, but if it contained 1000 lines of explanations on the problem and what you've tried... I think you see my point.

• As for the actual saving of the data, there you've missed out on one crucial piece. Namely, output escaping. Seeing as you've opted to go for a semicolon delimited values file, you need to take care to ensure that any semicolons that the user adds to his message are escaped. If not, they will wreak havoc on your internal structure, as your parser will treat them as content separators instead of actual content.

  For this I'd advice you to use the built in CSV functions in PHP, as they should handle the escaping automatically.

• Your error listing needs to be updated to handle multiple errors, otherwise it's fine.
• You need to use the CSV functions for reading the message content as well, otherwise it'll treat semicolons improperly even with escaping.
• You really shouldn't use tables for formatting, as this is not tabular data. Instead use fieldsets, labels and CSS, as is proper and semantic HTML.
• Lastly, you really should consider using a proper database for this. Files doesn't lend themselves to dealing with concurrent writes, and you have a hell of a race-condition build into your script here. A race conditions, which will lead to lost messages if you get two people that chats at the same time. (Yes, only two people are enough.)

Also, contrary to what Famer wrote, I see that you have indeed separated business logic and presentation logic. Only within the same file. Preferably speaking, though, this should have been in two files. With the HTML part being in a file of its own, which is included at the very end of all of the business logic. However, seeing as this is a code review request, I'm going to assume that this is what you've done already.

 

PS: No need to use the ### in the comments, they're actually more of a distraction than help. Especially if you use an editor which uses syntax highlighting, which you should do.

Edited March 29, 2013 by Christian F.

[Christian F.]

Psycho: Regarding the redirect, it's there to prevent F5/refresh-resubmitting. At least, that's why I'd add it.

[PaulRyan]

@Christian F, thank you for your in-depth analysis of my code. I'll reply in a manner that's easily readable:

1 - Thanks for the advice on the PHP_SELF, I'll look into a better method for that, it did it's job and I didn't rigorously test it , so I assumed it was fine.

 

2 - I'll heed your advice on the validation, it should have indeed been more thorough and thought out before hand.

 

3 - Regarding the output of error messages, I've always preferred the singular message to multiple messages, I don't have a particular reason for it, just seems "right" to me. I should really do it the way you have mentioned, I'll have to get myself into the habit of doing it with grouped errors instead of singular errors.

 

4 - I agree, will look into improving that.

 

5 - I only recently realised the CSV functions within PHP, I've come across them and used them before, they just never crossed my mind. Thanks for this.

 

6, 7 - Agreed.

 

8 - I used a table for quickness, I usually use <ul> and <li> with forms, I quite like the way it works.

 

9 - This is was test for myself to use a flat file, of course a database would be 100% better, just wanted to see what I could think of without resorting to a database.

 

Regarding the ### with the comments, this is some I've really grown accustomed to. My eyes instantly spot these so I know to read the comment on the line.

 

Thanks for you critique, it has opened my eyes somewhat and I will take on board what you've said and implement the points listed above in future.

 

P.S. - This is just a test for myself, I have built more complex scripts. I wanted to get back to basics and get a new perspective on how I used to code, to how I should be coding.