SitePoint Sponsor

User Tag List

Results 1 to 18 of 18
  1. #1
    SitePoint Wizard PHPycho's Avatar
    Join Date
    Dec 2005
    Posts
    1,201
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    I want to improve my coding style, any suggestions??

    Hello forums,
    Here i am mentioning the style of my coding.
    This is for the admin section,
    Suppose i had any section(say portfolio in this case) which i had manage
    ie

    list | add | edit | delete
    For that I always make the four files for every section
    ie

    Code:
    listSection.php (for listing)
    addSection.php (for add section)
    editSection.php (for edit section)
    deleteSection.php (for delete section)

    and separate .tpl files for HTML section
    ie

    Code:
    listSection.tpl
    addSection.tpl
    editSection.tpl

    And i perform operations ie list | add | edit | delete in listSection.php
    My listSection.php (say listPortfolio.php in this case)
    PHP Code:
    <?php
    /********************************************************
    * PORTFOLIO OPERATIONS
    ********************************************************/
    /* DELETE */
    if(isset($_GET['mode']) &&
    $_GET['mode'] == "deletePortfolio" &&
    isset(
    $_GET['deleteID']))
    {
    $deleteID $_GET['deleteID'];
    $portfolioObj->delete($deleteID);
    }
    /* ADD */
    if(isset($_POST['mode']) &&
    $_POST['mode'] == "addPortfolio")
    {
    $portfolio_name $_POST['portfolio_name'];
    //print_r($_POST);
    $portfolioObj->insert($portfolio_name);
    }
    /* EDIT */
    if(isset($_POST['mode']) &&
    $_POST['mode'] == "editPortfolio" &&
    isset(
    $_GET['editID']))
    {
    $editID $_GET['editID'];
    $portfolio_name $_POST['portfolio_name'];
    $portfolioObj->update($editID,$portfolio_name);
    }
    ?>

    <?PHP
    /* FETCH ALL THE PORTFOLIOS */
    $listPortfolio $portfolioObj->selectAll();
    //echo $listPortfolio[0];
    //print_r($listPortfolio[1]);
    if($listPortfolio[0] == 0)
    {
    $smarty->assign("error","No existing portfolio !!");
    $smarty->display("errorDisplay.tpl");
    $smarty->display("addPortfolio.tpl");
    }
    else
    {
    $listPortfolio $listPortfolio[1];
    $smarty->assign("listPortfolio",$listPortfolio);
    $smarty->display("listPortfolio.tpl");
    }
    ?>
    addPortfolio.php:
    PHP Code:
    <?php
    $smarty
    ->display("addPortfolio.tpl");
    ?>
    editPortfolio.php:
    PHP Code:
    <?php
    /* FETCH THE REQUIRED PORTFOLIO */
    if(isset($_GET['editID']))
    {    
     
    $editID $_GET['editID'];
     
    $getPortfolio $portfolioObj->selectOne($editID);
     
    $smarty->assign("getPortfolio",$getPortfolio);
     
    $smarty->display("editPortfolio.tpl");
    }
    else
    {
    $smarty->assign("error","Error Occurred");
    $smarty->display("errorDisplay.tpl");
    }
    ?>
    (here i didnt mention the .tpl files to reduce bulkiness)
    ----------------------------------------------------------------------------------------------------------------
    My this style seems easy to maintain but its time consuming and doesnt look pro type coding..
    Is there any suggestion ie any style for managing sections with ease creating less file ??

    Thanks in advance to all of you !!

  2. #2
    SitePoint Addict miggl's Avatar
    Join Date
    Feb 2007
    Location
    Los Angeles, CA
    Posts
    286
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Look into MVC methods and OOP programming. Here's what I would do:
    Code:
    class.List.php (model)
     |
    index.php  (controller)
     |
     |-index.tpl (view)
    So that your index.php contains the the logic to manipulate your lists, including fetching the information for display. Note that it impliments the list class, and uses its accessors, which contain the specific logic of what to with the list itself. It also prepares the smarty tags for your view.
    index.tpl is made up of your html and smarty tags to represent a view of the list.
    The class.list.php file describes the list, its accessor methods, and also its data storage (accesses the database class you have defined elsewhere and) and makes all the necessary data calls.

    This is just a short overview (and may not be the right solution for your particular job). But its worth looking into.

  3. #3
    Maniacally depressed robot poncho's Avatar
    Join Date
    Dec 2004
    Location
    Belfast, N.Ireland
    Posts
    452
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Have a look at my post here, I used to use exactly the method you show here. Creating a class to 'model' the database table makes things a WHOLE lot easier

    Cheers;
    Poncho
    Perfecting the art of breaking stuff.
    Check 'em: CakePHP | TextMate

  4. #4
    SitePoint Evangelist superuser2's Avatar
    Join Date
    Aug 2006
    Posts
    598
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by poncho View Post
    Creating a class to 'model' the database table makes things a WHOLE lot easier
    Yes it certainly does. As to your coding style, indent your code that's inside things (ifs, loops, classes, etc). It makes it more readable. For example

    PHP Code:
    if($cond)
    {
         
    //action

    instead of

    PHP Code:
    if($cond)
    {
    //action

    It just makes your code look nicer and easier to read.

  5. #5
    SitePoint Wizard stereofrog's Avatar
    Join Date
    Apr 2004
    Location
    germany
    Posts
    4,324
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    PHPycho, your code looks very clean because your Model and View are well separated. The only thing that remains is to make the Controller properly. There are many ways to code a controller, the simplest is to use one monolithic object where the "action" ("mode" in your case) parameter is used to choose a correct method. Something along the lines of
    PHP Code:
    # file index.php

    class PortfolioController
    {
        const 
    defaultMode 'list';

        function 
    run() {
            
    $mode self::defaultMode;
            if(isset(
    $_REQUEST['mode']))
                
    $mode $_REQUEST['mode'];
            if(!
    method_exists($this"mode_$mode"))
                
    $mode self::defaultMode;
            
    $this->{"mode_$mode"}();
        }
        
    // specific "modes"
        
    function mode_list() { ... }
        function 
    mode_add { ... }
        
    // etc
    }

    // instantiate and run the controller

    $cc = new PortfolioController;
    $cc->run(); 
    Action-specific files like "listSection.php" are not needed anymore, just one single index.php.

  6. #6
    Non-Member
    Join Date
    Jan 2003
    Posts
    5,748
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Umm...

    PHP Code:
    $dispatcher = new QDispatcher();
    $dispatcher -> setPluginsController( ... );
    $dispatcher -> setApplicationDirectory( ... );
    $dispatcher -> getRouter() -> addRoute( ... );
    $dispatcher -> dispatch(); 
    Not very helpful but the point I'm trying to make is that you need to encapsulate;

  7. #7
    SitePoint Wizard PHPycho's Avatar
    Join Date
    Dec 2005
    Posts
    1,201
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Thanks Mr.stereofrog
    I would be greatful if you post some of your more code like you have mentioned here...I just got a bit idea..if i got some more of your code i would definately get more idea..
    Anyway thanks a lot Mr. stereofrog

  8. #8
    SitePoint Wizard stereofrog's Avatar
    Join Date
    Apr 2004
    Location
    germany
    Posts
    4,324
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Posting more code is no problem, however for this to be helpful, we need to know what exactly doesn't work for you. I suggest you just start rewriting your controller in this style. It's always better to work with specific problem field rather than study abstract "sample code".

  9. #9
    SitePoint Wizard PHPycho's Avatar
    Join Date
    Dec 2005
    Posts
    1,201
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Thanks again..
    As you suggested my model and view part are ok....
    I want to implement the controller part more effectively...
    as you mentioned some of your code.....what i want is a bit more code
    so that i will be more clear..
    thanks again

  10. #10
    SitePoint Wizard REMIYA's Avatar
    Join Date
    May 2005
    Posts
    1,351
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by stereofrog View Post
    PHPycho, your code looks very clean because your Model and View are well separated. The only thing that remains is to make the Controller properly. There are many ways to code a controller, the simplest is to use one monolithic object where the "action" ("mode" in your case) parameter is used to choose a correct method. Something along the lines of
    PHP Code:
    # file index.php

    class PortfolioController
    {
        const 
    defaultMode 'list';

        function 
    run() {
            
    $mode self::defaultMode;
            if(isset(
    $_REQUEST['mode']))
                
    $mode $_REQUEST['mode'];
            if(!
    method_exists($this"mode_$mode"))
                
    $mode self::defaultMode;
            
    $this->{"mode_$mode"}();
        }
        
    // specific "modes"
        
    function mode_list() { ... }
        function 
    mode_add { ... }
        
    // etc
    }

    // instantiate and run the controller

    $cc = new PortfolioController;
    $cc->run(); 
    Action-specific files like "listSection.php" are not needed anymore, just one single index.php.
    I wouldn't say that the best way to teach someone to improve code style is by setting constants in a class, that are not used externally.

    Also the run function is completely useless, keeping in mind the controller can be directly instantiated and run in a constructor.

  11. #11
    SitePoint Wizard stereofrog's Avatar
    Join Date
    Apr 2004
    Location
    germany
    Posts
    4,324
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by PHPycho View Post
    Thanks again..
    As you suggested my model and view part are ok....
    I want to implement the controller part more effectively...
    as you mentioned some of your code.....what i want is a bit more code
    so that i will be more clear..
    thanks again
    Ok, I see. Here's some random thoughts, take it with grain of salt, I am by no means an MVC professor

    Let's begin with file structure. Model, View and Controller classes go to their own files, additionaly we obviously need a DB access layer (DAO) and a Config file where we define application options (e.g. username for db connection). So, the minimal MVC index.php would look like this:

    PHP Code:
    include "config.inc.php";
    include 
    "dao.inc.php";

    include 
    "model.inc.php";
    include 
    "view.inc.php";
    include 
    "controller.inc.php";

    # instantiate db access layer
    $dao DAO::new_connection(DB_USERNAMEDB_PASSWORD);

    # model knows only about dao
    $model = new Model($dao);

    # view knows about nothing, except possibly config options
    $view = new View(TEMPLATE_PATH);

    # controller knows about Model and View
    $controller = new Controller($model$view);

    # let controller do its job
    $controller->run(); 
    index.php is the only one accessible from the web. All other php files can be freely moved to the protected directory or outside of web root.

    Now to the Controller. Controller consists of the following:

    1) Constructor.
    PHP Code:
    function __construct($model$view) {
        
    $this->model $model;
        
    $this->view $view;

    This is pretty straightforward and does nothing but save Model and View internally. For simplicity we will operate on PHP superglobals to obtain request parameters and use simple 'echo' to produce output. In real life, both request and response must be wrapped in their own objects, and Controller constuctor will receive them as well:
    PHP Code:
    function __construct($model$view$request$response)... 
    2) Dispatcher. This is basically "run()" function as shown above. run() reads an "action" (or "mode" or whatever) parameter from the request, selects one of the action methods (or "handlers") and passes control over to it.

    3) Handlers. They have an uniform naming scheme ("action_xxx") and take no arguments. A handler has full access to Controller internals (Model, View) and must either call a View ("selector" handlers) or forward() to another URL ("modificator" handlers), where forward() is just a wrapper for "location" header:

    PHP Code:
    function forward($query_string '') {
        
    header("Location: http://{$_SERVER[SERVER_NAME]}/index.php{$query_string}");

    Example of selector handler:

    PHP Code:
    function action_find_by_title() {
        
    $search trim(@$_REQUEST['search']);
        if(!
    strlen($search)) {
            
    $this->view->show("error_message");
            return;
        }
        
    $records $this->model->find_by_title($search);
        if(!
    count($records)) {
            
    $this->view->show("nothing_found");
            return;
        }
        
    $this->view->set("list_contents"$records);
        
    $this->view->show("list_view");


    Example of modificator handler:

    PHP Code:
    function action_delete() {
        
    $record_id intval(@$_REQUEST['id']);
        
    $success $this->mode->delete_record($record_id);
        if(!
    $success) {
            
    $this->view->show("error_message");
            return;
        }
        
    $this->forward("?action=show_all_records");

    Well, that's basically all about that. Sorry if code examples were too abstract... after all I have no idea on what your project is all about. Nevertheless I hope this was helpful to some extent.

  12. #12
    SitePoint Addict
    Join Date
    Sep 2006
    Posts
    219
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by REMIYA View Post
    I wouldn't say that the best way to teach someone to improve code style is by setting constants in a class, that are not used externally.

    Also the run function is completely useless, keeping in mind the controller can be directly instantiated and run in a constructor.
    I also use class constants that are only used internally - especially for SQL statements at the top of my data gateways it just means it's immutable - it's better than variables IMHO - why would you say this?

    I also don't see anything wrong with the run function - my controller is similar (i use execute). There are many instances when you don't want an object to process on construct.

    Dan
    Last edited by danh2000; Mar 6, 2007 at 21:32.

  13. #13
    SitePoint Wizard REMIYA's Avatar
    Join Date
    May 2005
    Posts
    1,351
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by danh2000 View Post
    I also use class constants that are only used internally - especially for SQL statements at the top of my data gateways it just means it's immutable - it's better than variables IMHO - why would you say this?

    I also don't see anything wrong with the run function - my controller is similar (i use execute). There are many instances when you don't want an object to process on construct.

    Dan
    Usually it is the Controller is called at the very start of the application. That is why I don't see the reason for a run/execute function. It is completely useless in a Controller.

    See this:

    # controller knows about Model and View
    $controller = new Controller($model, $view);

    # let controller do its job
    $controller->run();

    And this:

    # let controller do its job
    new Controller($model, $view);

    Which is more readable


  14. #14
    SitePoint Wizard REMIYA's Avatar
    Join Date
    May 2005
    Posts
    1,351
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by danh2000 View Post
    I also use class constants that are only used internally - especially for SQL statements at the top of my data gateways it just means it's immutable - it's better than variables IMHO - why would you say this?
    Actually I don't see the reason for the const to be used. I can't see who is going to change your variable, if it is private for the class.

    Excuse me, if I am wrong, but I feel like it turns us back to the old C days.

  15. #15
    SitePoint Addict
    Join Date
    Sep 2006
    Posts
    219
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by REMIYA View Post
    Actually I don't see the reason for the const to be used. I can't see who is going to change your variable, if it is private for the class.

    Excuse me, if I am wrong, but I feel like it turns us back to the old C days.
    It's a personal opinion I suppose, but I use reflection quite a lot and I don't want immutable/constant variables mucking things up. I sometimes loop through a class's properties or use magic methods, so don't want constants getting in the way of those operations.

  16. #16
    SitePoint Wizard REMIYA's Avatar
    Join Date
    May 2005
    Posts
    1,351
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by danh2000 View Post
    It's a personal opinion I suppose, but I use reflection quite a lot and I don't want immutable/constant variables mucking things up. I sometimes loop through a class's properties or use magic methods, so don't want constants getting in the way of those operations.
    Take a look at this post: http://bg.php.net/manual/en/language...ants.php#48224

  17. #17
    SitePoint Addict
    Join Date
    Sep 2006
    Posts
    219
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Hi,

    I think the main point is that they are immutable.

    Also consider ReflectionClass->getProperties() - and If I have a class with private properties that I get and set using magic methods (for meta data mapping for instance), then I want my class constants kept away from these operations - yes I could work around it, but why - I just use const for those immutable variables it's easier and safer.

    I'd be interested to know the general feeling from others - are constants considered good or bad and why?

    Dan

  18. #18
    SitePoint Wizard stereofrog's Avatar
    Join Date
    Apr 2004
    Location
    germany
    Posts
    4,324
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    If you're referring to the "const" keyword, I can't think of a single reason not to use it.


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •