Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm creating a CRUD page. My first approach was use the same class editCategory.php for doing these actions:

  • If this file is called via GET and categoryId parameter doesn't exist -> shall show a empty form
  • If this file is called via GET and categoryId parameter is provided -> shall show a form with the data of this category.
  • If this file is called via POST and there is no categoryId parameter -> shall create a new category.
  • If this file is called via POST and categoryId parameter is provided -> shall update this category.

The code works ok, but I have the sense that the code is cluttered and that it could be organized better.

    <?php
        require_once "../include_dao.php";
        $action = isset($_POST["action"]);

        $category = new Category();
        $categoryDao = new CategoryMySqlDAO();

        $categoryName = "";
        if ($action == "save") {                                        // DO_POST
            $categoryName = $_POST["name"];
            $category->name=$categoryName;

            if (isset($_POST["categoryId"])) {                              // update
                $categoryId = (int) $_POST["categoryId"];
                $category->categoryId=$categoryId;
                $categoryDao->update($category);
            } else {                                                        // insert
                $categoryDao->insert($category);
            }
            $messageSuccess = "Category saved";
        } else {                                                        // DO_GET
            if (isset($_GET["categoryId"])) {
                $categoryId = (int) $_GET["categoryId"];

                $category = $categoryDao->load($categoryId);
                $categoryName = $category->name;
            } 

        ?>
<html>
 HTML code here showing the form
</html>
share|improve this question
1  
I'm not quite sure everything is wrong here. I'd expect $action = isset($_POST["action"]); to assign a boolean value to $action as per php.net/manual/fr/function.isset.php but then you compare it to a string : if ($action == "save"). If anyone has an explanation, I'm pretty intested. –  Josay Feb 18 '13 at 21:24
1  
@Josay I suspect you know this since you're already questioning it, but for the OP's sake: The string will be casted to a boolean. It's essentially equivalent to: if ($action === ((bool) "save")) which is in turn equivalent to if ($action === true) which is of course loosely equal to if ($action). In other words, if $_POST['action'] is set, despite the value, that branch will be followed. Interestingly, isset('') === true meaning if the field is provided at all that branch will be followed. –  Corbin Feb 18 '13 at 22:20
1  
@Corbin Thanks for the explanation. I wasn't too sure about what was happening because I was expecting this not to work but OP was saying that everything was fine. I understand now. Thanks again. –  Josay Feb 18 '13 at 22:55
    
OMG, it's working by chance! When submit the form, I have an input hidden with name="action" and value="save". So, when this script is called by POST, $_POST["action"] is set to "save" and when this script is called by GET, $_POST["action"] is not set. I don't know if there is a better way to know if script is called by GET or by POST –  user674887 Feb 19 '13 at 8:34
1  
If you want to check for POST requests, use $_SERVER['REQUEST_METHOD']. When comparing strings, get in the habit of strict comparisons, e. g. $_POST['action'] === 'save'. –  Peter Feb 24 '13 at 19:50
show 7 more comments

2 Answers 2

up vote 1 down vote accepted

I am not a php developer, but I think that your objects should encapsulate a more logic, I made a small simple example:

$category = new Category();
$categoryDao = new CategoryMySqlDAO();

if ($_SERVER['REQUEST_METHOD'] === 'POST') 
{
        if (isset($_POST["action"])
        {
            //$category has logic with get varialbles in method fill 
            $category->fill($_POST);

            //$categoryDao has logic insert or update $category
            $result = $categoryDao->process($category);

            //.. do something with $result
        }
} else if (isset($_GET["categoryId"]))
{
    $categoryId = (int) $_GET["categoryId"];
    $category = $categoryDao->load($categoryId);
}
share|improve this answer
add comment

Your concept using the HTTP verbs is pretty much the idea behind REST. You could go one step further and use the less known verbs like PUT, DELETE, UPDATE.

Also u could use the $_SERVER['REQUEST_METHOD'] to switch between the HTTP modes you would like to process.

I would suggest to write a "class Request" to be reused by all your CRUD pages or use / get inspired by (for example) symfony2

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Request.php

u are using classes like CategoryMySqlDAO why do not u use a CRUD class? u could implement an abstract class CRUD with "create, update, delete" methodes and then use subclasses for each crud page. Then you would inject the DAO and stuff in the constructor and use autoload to get them instead of a smelly required statement in each file.

also having HTML and logic in the same file never is a good idea.

i hope i could give you some inspiration how to improve your current state

share|improve this answer
    
+1 Thank you for the ideas. –  user674887 Feb 21 '13 at 10:01
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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