3
\$\begingroup\$

To understand the basic workings of PHP I wrote a test page where one can create, load and edit entries (name-info-pairs) in a list.

It would be cool if you could give me some advice on that to improve.

<?php
class User {
  public $name;
  public $info;
  Public function __construct($name, $info){
    $this->name = $name;
    $this->info = $info;
  }
}

function show_user_radio_button($user) {
  return '<input type="radio" name="load" value="' .
  $user->name .'">' . $user->name . "<br>\n";
}

function read_user_from_text_line($line) {
  $cells = explode(",", $line);
  return new User($cells[0], $cells[1]);
}

function user_to_text_line($user) {
  return $user->name . ',' . $user->info;
}

function load_all_users() {
  $contents = file_get_contents('user_database.csv');
  $lines = array_filter(explode("\n", $contents));
  $allUsers = array();
  foreach ($lines as $line)
    $allUsers[] = read_user_from_text_line($line);
  return $allUsers;
}

function save_all_users($allUsers) {
  $contents = '';
  foreach ($allUsers as $user)
    $contents .= user_to_text_line($user) . "\n";
  file_put_contents('user_database.csv', $contents);
}
function save_user($newUser) {
  $allUsers = load_all_users();
  $replaced = false;
  for ($i = 0; $i < count($allUsers); $i++) {
    $user = $allUsers[$i];
    if ($newUser->name === $user->name) {
      $allUsers[$i] = $newUser;
      $replaced = true;
    }
  }
  if (!$replaced) {
    $allUsers[] = $newUser;
  }
  save_all_users($allUsers);
}
function loadUser($name) {
  $allUsers = load_all_users();
  foreach ($allUsers as $user)
    if ($user->name === $name) return $user;
  return new User('', '');
}
function sanitize($str) {
  return filter_var($str, FILTER_SANITIZE_EMAIL);
}

$newName = sanitize($_POST['name']);
$newInfo = sanitize($_POST['info']);
if ($newName && $newInfo)
  save_user(new User($newName, $newInfo));
?>

<!DOCTYPE html>
<html>
<head>
  <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  <title>basic form to create and edit entries in a text file</title>
</head>
<body>
  from database:<br>
  <form name="load" action="index.php" method="get">
    <?php
    $allUsers = load_all_users();
    foreach ($allUsers as $user)
      echo show_user_radio_button($user);
    echo '<input type="submit" value="load">';
    ?>
  </form>
  <br>
  <br>
  -------
  <br>
  <br>
  edit or create<br>
  <form name="save" action="index.php" method="post">
    <?php
    $user = loadUser($_GET['load']);
    echo 'name: <input type="text" name="name" value="' .
    $user->name . '"><br>';
    echo 'info: <input type="text" name="info" value="' .
    $user->info . '"><br>';
    ?>
    <input type="submit" value="save">
  </form>
</body>
</html>
Improve this question
\$\endgroup\$
4
\$\begingroup\$

First of all, welcome to the world of PHP. It seems you are on the right track. I have a few pointers I can give you though:

  • I don't know if you pasted this all together for the question, but all that code should not be in a single file. While it is fine for this 'test' page, it will very quickly become a big mess if you start building real sites or applications. There are some nice patterns you can use to structure your code. MVC is the most popular in PHP, and I would recommend looking into that.

  • You start of very well with your User class, but then you resort to 'old school' functions. I would recommend you go OOP all the way. You will have to use built in functions when working with php unfortunately, but all the code you write should be in classes! Better to learn it the right way from the start.

  • I am not sure why you wrote that show_user_radio_button function. It could just as well be inside your view (the HTML bit at the bottom). Whenever you write a function with only a single line of code, you should wonder whether it is worth putting it inside a function.

  • All those functions that handle reading and writing Users to and from your text file should be in a class. I would call it my UserMapper, and it would have an API that looks something like this:

    class UserMapper {
    private $_users = array();

    public function getById($id) { ... }
    public function getByName($name) { ... }
    public function getAll() { ... }
    public function save(User $user) { ... }

    private function _textLineToUser($line) { ... }
    ...
}

If you ever decide to store your Users in a different way, you could create a class that implements this same interface and just replace it in your application, and all should keep working.

  • The way you load and save users right now is not very performant. You have to go over your entire array of users when you save or look for a specific user. It would be much better to use your id (the name in your example) as a key in your array of users. That way you can easily:
    • check if a user exists with array_key_exists
    • load a specific user with $users[$name]
    • update a user by just setting $users[$name] = new User($name, $info). No need to check if it exists as it will overwrite the array value automatically if that is the case.

There is probably a lot more that could be said about your code, but I leave that up to others. This should already give you some food for thought and point you in the right direction. Do feel free to ask if anything I wrote isn't clear, and happy coding!

Improve this answer
\$\endgroup\$
  • \$\begingroup\$ My current version. * I'm not that big a fan of the Java's OOP-everywhere philosophy, but I did what you suggested, since I also see that it is good to be able to swap the UserMapper with a another class implementing the same interface using a SQL DB or something. * show_user_radio_button is still a function, because I think it makes the view code more readable (self-commenting). * The hint to use the array not just as a simple list but as a dictionary with the user name as index simplified the addUser function a lot. \$\endgroup\$ – Tobias Hermann Aug 3 '14 at 20:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.