I'm a pretty terrible coder, but am trying to get better. I recently coded something that works, hooray! But I feel like there must be a cleaner way. Object is:

If GET variable exists use it as the cookie value. If the Variable doesn't exist use a default value. If the Cookie already exists use that value instead of a new one.

if (!isset($_COOKIE['id'])) {
  if (!isset($_GET[id])) {
    $cookid="115";
  } else {
    $cookid= $_GET["id"];
  } 
  setcookie("id", $cookid , time() + 31536000);
} else {
  $cookid= $_COOKIE['id'];
}

Comments

Your code looks like it should do what you want it to do? Is it not behaving as expected? If you were looking for a cleaner method, I think you have a pretty simple method in place already. Use "Occam's Razor" whenever you are in doubt and go with simple solutions to complex problems. Good luck!

Written by hypervisor666

Accepted Answer

Here's how I'd have written it:

if(isset($_COOKIE['id'])) {
  $cookid = $_COOKIE['id'];
} else {
  $cookid = isset($_GET["id"]) ? $_GET["id"] : "115";

  setcookie("id", $cookid, time() + 31536000);
}

First of all, according to some conventions you should test for the positive case (isset(...)), not the negative (!isset()). I think it makes for more readable code, so I've switched it around.

Secondly, I've used the ternary operator (condition ? expr1 : expr2) to eliminate an if/else block, which is acceptable because you were just using the if/else block to decide which of two values to assign to the variable. The ternary operator should be used with caution, though, because if overused it can make code less readable.

Thirdly, it's my preference to use curly braces even for one-line if/else blocks, but at the very least I think you should use curly braces for the else if you use them for the if.

And lastly, just a readability note: Try to be consistent with your whitespace. On line four you don't have any spaces around the =, but on lines six and ten you have a space after it, but not before. For readability it's almost always preferable to have whitespace on both sides of an operator.

Oh, and $_GET["id"] is correct; $_GET[id] is not. It (correctly) throws a warning if you have your error reporting level high enough (and you should usually develop with error_reporting(E_ALL); so you see them).

Written by Jordan
This page was build to provide you fast access to the question and the direct accepted answer.
The content is written by members of the stackoverflow.com community.
It is licensed under cc-wiki