views:

170

answers:

6

I've got a bunch of functions that I want to move into a class. They're currently split into a couple of fairly long files. I'd prefer not to have one 2500 line file, but as far as I can tell, you can't use include to split a class up into multiple files. In theory, I could group the functions in different classes, but they're closely related enough that I feel like they belong together, and splitting them will reduce some of the utility that I'm hoping to get from moving away from a procedural approach (with shared properties, rather than a bunch of parameters that are in nearly every function).

I know this is a bit vague, but any suggestions/pointers? If it matters, this is for a prototype, so ease of code management takes precedence over security and performance.

UPDATE: Let me see if I can remove some of the vagueness:

This class/set of functions outputs the html for a complex form. There are many different sections and variations within each section, depending on about 5 or 6 parameters, which are currently passed into the functions. I was hoping to define the parameters once as properties of the class and then have access to them from within all of the section-creation methods. If I use sub-classes, the values of those properties won't be initialized properly, hence the desire for one class. (Hmm... unless I define them as static. I may have just answered my own question. I'll have to look to see if there's any reason that wouldn't work.)

I've currently got a mess of functions like:

get_section_A ($type='foo', $mode='bar', $read_only=false, $values_array=array()) {
    if ($this->type == 'foo') { }
    else ($this->type == 'foo') { }
}

So I was initially imagining something like:

class MyForm {
    public $type;          // or maybe they'd be private or 
    public $mode;          // I'd use getters and setters 
    public $read_only;     // let's not get distracted by that :)
    public $values_array;
    // etc.

    function __constructor ($type='foo', $mode='bar', $read_only=false, $values_array=array()) {
        $this->type = $type;
        // etc.
    }

    function get_sections () {
        $result = $this->get_section_A();
        $result .= $this->get_section_B();
        $result .= $this->get_section_C();        
    }      

    function get_section_A() { 
        if ($this->type == 'foo') { }
        else { }
    }
    function get_section_B() {}
    function get_section_C() {}
    // etc. for 2500 lines
}

Now I'm thinking something like:

// container class file
class MyForm {
    static $type 
    static $mode
    static $read_only
    static $values_array
    // etc.

    function __constructor ($type='foo', $mode='bar', $read_only=false, $values_array=array()) {
        MyForm::$type = $type;
        // etc.
    }

    function get_sections () {
        $result = new SectionA();
        $result .= new SectionB();
        $result .= new SectionC();
    }      
}

// section A file
class SectionA extends MyForm {
    function __constructor() { 
        if (MyForm::$type == 'foo') { }
        else { }
    }

    function __toString() {
        // return string representation of section
    }
}

// etc.

Or probably I need an abstract class of FormSection where the properties live.

Any other ideas/approaches?

A: 

They are all in different files, which means that they were different enough to group by file. Just take the same logic when building them into classes. I have a Page object that deals with building the page. Technically the HTML for my page header is part of the page, but I separate it into a Page_HTML class for maintaining my sanity and not creating gigantic classes.

Also, I tend to make the sub_classes, like Page_HTML in this case, static, instead of instantiating it. That way I can access the $this variables in the Page class, but still group it into another class.

class Page
{
    function buildPage($content)
    {
        $content = Page_HTML::getHeader() . $content;
    }
}

class Page_HTML
{
    function getHeader()
    {
    }
}
Chacha102
actually, they're currently in two files, and there's only a soupcon of logic for why.
sprugman
+2  A: 

Usually I do something like this:

class one
{
    public function __get($key)
    {
     // require __DIR__ / $key . php
     // instanciate the sub class
    }

    public function mainMethod()
    {
    }
}

class one_subOne extends one
{
    public function otherMethod()
    {
    }
}

class one_subTwo extends one
{
    public function anotherMethod()
    {
    }
}

$one->mainMethod();
$one->subOne->otherMethod();
$one->subTwo->anotherMethod();
Alix Axel
This is the way I doo it, too. No other way to split a class in PHP.
Pekka
I don't get why you would need to extends the first class unless the sub classes needed the functions of the first class.
Chacha102
He wants to have one huge class.
Pekka
This smells like it violates the Liskov Substitution Principle. If these three classes need to access the same data, perhaps the datasource should be its own object.
oops
+3  A: 

I'd split them up into as many classes as you want (or as many that make sense) and then define an autoloader to obviate inclusion headaches.

EDIT

Ok, after seeing more of your code - I think you're approaching subclasses wrong. You have lots of if statements against $type, which signals to me that that is what the polymorphism should be based on.

abstract class MyForm
{
  protected
      $mode
    , $read_only
    , $values
  ;

  public function __construct( $mode, $read_only=false, array $values = array() )
  {
    $this->mode      = $mode;
    $this->read_only = (boolean)$read_only;
    $this->values    = $values;
  }

  abstract function get_section_A();

  abstract function get_section_B();

  abstract function get_section_C();

  //  final only if you don't want subclasses to override
  final public function get_sections()
  {
    return $this->get_section_A()
         . $this->get_section_B()
         . $this->get_section_C()
    ;
  }
}

class FooForm extends MyForm
{
  public function get_section_A()
  {
    // whatever
  }

  public function get_section_B()
  {
    // whatever
  }

  public function get_section_C()
  {
    // whatever
  }
}
Peter Bailey
It's not about loading, it's about splitting logic across classes.
sprugman
This is the only answer which actually takes good practices and proper OO model into account. Thus it deserves some more upvotes. +1
Jani Hartikainen
"You have lots of if statements against $type, which signals to me that that is what the polymorphism should be based on."Not really. There are at least four parameters that change things in different ways, on different scales. Sometimes the changes are quite subtle, so I think I'd wind up creating a whole bunch of teeny, tiny functions to use polymorphism to accomplish the differences. Sounds like waay more trouble than it's worth to me.
sprugman
I can base my answers only on what you provide - not what I can't see.
Peter Bailey
A: 

This class/set of functions outputs the html for a complex form.

Why not remove PHP from the equation? It seems you're using PHP to organize views which can be done easily with the filesystem. Just write the views in HTML with as little PHP as possible. Then use PHP to map requests to views. I'm assuming you're processing forms with PHP, and you could continue to do so. However, your classes will become much smaller because they're only accepting, verifying and presumably saving input.

simeonwillbanks
Maybe I don't understand what you're suggesting, but this approach makes no sense to me -- there are dozens, perhaps hundreds of views. Building them in html would create a ton of redundant code.
sprugman
EDIT: Don't duplicate the repeated pieces of the views. You could consider these pieces like partials in Ruby on Rails. For each route, you can build a view by combining unique HTML with reusable partials. Is this more clear?
simeonwillbanks
A: 

If you want to do OOP, separate the concerns and encapsulate them into appropriate classes. Combine them either by extending them or by composition or better aggregation. Remove any duplicate code. Dont repeat yourself.

In your case, separate the stuff that is about any Form from the stuff that is about your specific form. The code that can be used for any Form is the code you want to place into a generic Form class. You can reuse this in later projects. For an example of a very complex Form class, check out Zend_Form.

Anything in your code related to the/a specific form gets into a class of it's own that extends the generic form. Assuming from the type property given in your code, you might end up with multiple special purpose form classes (instead of one-type-fits-all-form), which will likely eliminate the complexity from the getSection methods and make your code a lot easier to maintain because you can concentrate on what a specific type of form is supposed to look like and do.

Lastly, if you got code in there that fetches data for the form from within the form or is otherwise not directly related to form building, remove it and make it into a separate class. Remember, you want to separate concerns and your form classes' concern is to build a form, not get it's data or something. Data is something you will want to pass to the form through the constructor or a dedicated setter.

Gordon
+2  A: 

As far as building the view is concerned, you might like to try the CompositeView pattern.

Here's a small example of how it could look in PHP. Pretend, for the sake of this example, that View::$html is encapsulated in a Template class that can load html from disk and allows you to inject variables, handles output escaping, etc.

interface IView {
    public function display();
}

class View implements IView {
    public $html = ''; 

    public function display() {
        echo $this->html;
    }   
}

class CompositeView implements IView {
    private $views;
    public function addPartial(IView $view) {
        $this->views[] = $view;
    }   
    public function display() {
        foreach ($this->views as $view) {
            $view->display();
        }   
    }   
}

The reason for the IView interface is to allow you to build composite views with other composite views.

So now consider a form with three parts: header, body and footer.

class HeaderView extends View {
    public function __construct() {
        $this->html .= "<h1>Hi</h1>\n";
    }   
}

class BodyView extends View {
    public function __construct() {
        $this->html .= "<p>Hi there.</p>\n";
    }   
}

class FooterView extends View {
    public function __construct() {
        $this->html .= "<h3>&copy; 2012</h3>\n";
    }   
}

(Again, you wouldn't just write HTML into that public variable and handle output escaping yourself. You'd likely reference a template filename and register your data via the template's interface.)

Then, to put it all together you would go:

$view = new CompositeView();
// here you would make decisions about which pieces to include, based
// on your business logic.  see note below.
$view->addPartial(new HeaderView());
$view->addPartial(new BodyView());
$view->addPartial(new FooterView());
$view->display();

So now your views can be composed and the fragments reused, but you can easily make a mess with the code that builds them, especially if you have a lot of conditions and many different possible outcomes (which it sounds like you do.) In that case, the Strategy pattern will probably be of some help.

If you haven't already read UncleBob's SOLID article, do it before anything else! At least the Single Responsibility Principle. I would also recommend reading Refactoring to Patterns by Joshua Kerievsky at some point.

oops
ummm... *thanks* for this example! It's between purist OOP and pragmatic HTML-generating code. I guess, constructing pages with dozens of dynamic parts needs good design and good code. This seems to be the most intuitive pattern for newbies.
namespaceform