4
\$\begingroup\$

Here is my controller class for my MVC pattern with all the includes removed. From what I know of the MVC pattern it is good to have a small controller class. Mine is just a collection of public static functions which accept requests and send data to the client.

Regarding send(). I'm just wrapping the echo command for sending text from an Ajax call. This is for consistency and to have all text exiting the same point in code.

I have a ControlEntry (Snippet 2) which help decouple Super Globals and direct entry into the Control, so the two kind of work in Tandem.

Pull_Pic just pulls a path and inserts it into HTML for display.

Snippet 1

<?php

    class Control
    {
        public static function ajax($ajax_type)
        {
            Session::start();
            switch($ajax_type) 
            {
                case 'ControlBookmark_add': 
                    $control_bookmark_instance = new ControlBookmark('remove');
                    $control_bookmark_instance->add();
                    break; 
                case 'ControlBookmark_delete': 
                    $control_bookmark_instance = new ControlBookmark('remove');
                    $control_bookmark_instance->delete();
                    break; 
                case 'ControlTweet_add': 
                    $control_tweet_instance = new ControlTweet('remove');
                    $control_tweet_instance->add();
                    break; 
                case 'ControlSignIn':
                    $control_signin_instance = new ControlSignIn('remove');
                    $control_signin_instance->invoke();
                    break; 
                case 'ControlSignUp':
                    new ControlSignUp();
                    break;
                case 'ControlTryIt':
                    new ControlTryIt();
                    break; 
                case 'ControlSignOut': 
                    Session::finish();
                    new ControlSignOut();
                    break;
                default:
                    throw new Exception('Invalid ajax_type');
            }
        }
        public static function reload() // Because reload may be triggered by File Upload (second control path)
        {
            if (session_id() == "")
            {
                Session::start();
            }
            if((Session::status())==='bookmarks')
            {
                include 'class.ViewBookmarks.php';
            }
            else
            {
                include 'class.ViewMain.php';
            }
        }
        public static function upload($fileType, $fileName)
        {
            Session::start();
            $fileInstance = new UploadedFile($fileType, $fileName);
            $fileInstance->createImages();
            Session::updatePic();
            self::reload();
        }
        public static function send($textStream)
        {
            echo $textStream;
        }
    }

**Snippet 2**

    new ControlEntry();

    class ControlEntry
    {
        public function __construct()  
        {        
            if(isset( $_FILES['ufile']['tmp_name']) )
            {
                if( ( $_FILES['ufile']['tmp_name']) != '' )
                    {
                    Control::upload( $_FILES['ufile']['type'], $_FILES['ufile']['tmp_name'] );
                    }
                else
                    {
                    Control::reload();
                    }
            }
            else if( isset($_POST['ajax_type']) )
            {
                Control::ajax( $_POST['ajax_type'] );
            }
            else
            {
                Control::reload();
            }
        }
    }
\$\endgroup\$
8
\$\begingroup\$

It looks like you are trying to write object oriented code, however this is completely procedural. Remove static everywhere and use dependency injection if you want to make this OO.

Static code is hard to test. It is the equivalent of a namespaced function.

Even your new objects that you create are the equivalent of calling a function.

        case 'ControlBookmark_add': 
            new ControlBookmark('add');
            break;

I'd recommend one of the following:

  • Spend a long while learning how to write real object oriented code.
  • Give up and convert to procedural code. Skipping all of the wasted classes and new's

Some people may disagree that I would suggest moving away from object oriented code, but your code is so far away from object oriented that it makes sense to me.

Response to the Edit

It does look a bit better now. It would be a good experience for you to unit test this with PHPUnit. I think you would find it very hard with the static and new. That would give you a motivation to change it further. However, I'm guessing the testing this code will receive will be based on seeing the page on the browser, which is okay for a lot of people.

One comment on the edited version:

// Do you really need the 'remove' parameter to your constructor?
// Especially when you call the add method on it?
// Also, objects aren't that special I wouldn't bother with _instance.
$control_bookmark = new ControlBookmark();
\$\endgroup\$
  • \$\begingroup\$ Thinking about things as if they were hardware is the exact reason that you should write procedural code. Remove all of the classes, they are only getting in your way. If you wanted to write OO you would need to break your system into real world objects and your design would require a lot of change. \$\endgroup\$ – Paul Jan 12 '12 at 2:57
  • 1
    \$\begingroup\$ Whoever gave my answer a -1, please give your own answer to the OP. I love OO so much that I created the most OO PHP framework in existence. I didn't make my suggestions without thinking about the history of the OP and the code they are working with. Maybe your answer could make me see the error of my ways. \$\endgroup\$ – Paul Jan 12 '12 at 3:06
  • 1
    \$\begingroup\$ I disagree. State is not a major part of OO. I would say it was creating real objects that are able to interact while hiding much of their implementation from each other. This allows a program to be split into chunks for each object. Whether an object has a lot of state depends on what the object is. This page nefariousdesigns.co.uk/archive/2006/05/object-oriented-concepts is a good read and it also hints that PHP is not likely to have many statefull objects. However, I think it depends on each object. \$\endgroup\$ – Paul Jan 12 '12 at 17:17

Your Answer

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