Criticize My Class Diagram/Structure

[Roadie]

I want everybody to criticise the crap out of my class diagram for this app I'm building. Here's the link to my class diagram:

http://www.visible-form.com/blog/pos...es/k2_uml.html

The layout may look a little funky becasue i was trying to squeeze everything into the screenshot, but here's the rundown:

I have a SiteManager class that acts as a psuedo-controller, all the requests go through that.

I have two main superclasses, DynamicOutputter and RecordHandler. Each section of the web site has a corresponding "outputter" class of its own...so for news it would be News_Outputter, and that inherits from DynamicOutputter. The outputters return the formatted output for the page. Each outputter is composed with its corresponding RecordHandler(same deal for the RecordHandlers, each site section has its corresponding record handler that handles communication with the database, and inherits from the RecordHandler superclass). Each RecordHandler's sublcass overrides one method, setValueArray(), which sets its values, which are the names of the rows in the table it will be dealing with.

When the SiteManager recieves a request, it uses a factory (either DynamicContentFactory or RecordHandlerFactory) to determine which sections class to instantiate, bases on a given id parameter.

If anything else isnt clear, just ask, otherwise, fire away, as long as its constructive criticism.

[armandoxxx]

I use the same Idea .. and it work perfectly .. the only think I did different is naming them differently .. hehe .. I use MODULES (for handling logic) and PLUGINS (to handle outputs) and I added COLLECTIONS (for data ) .. I thought that sometimes it happens that MODULES use different types of data .. for example three different types of content .. so thats why I created collections .. module assembles them and plugins formats the output ...

and your Site Manager class I turned into a little framework I use to handle everything i need .. but btw .. I guess its working for both of us !

bye

Armando

[Roadie]

armondo, how has your setup been as far as reusability? Have you been able to use this framework on multiple projects? Have you run acoss any major downsides?

[armandoxxx]

not really . I'm only upgrading it .. from project to project .. usualy projects have different demands but the idea stands .. down side is maybe only that design is used in a classes not in a XSLT stylesheet .. since I write it really fast .. and these moduels can be actually copy pasted from one project to another .. and it will still work with minor modifications .. I dont say its the best .. I wont even say its good .. but it solves the case without big overhead of clases and stuff

bye

armando

[ozone990]

WOW! I can't believe I stumbled across this post! I was going to post a question asking for names of programs to create that image you just created! How lucky! So what did you make that in? I read in the title that it's UML. After some googling that looked like a viable option, but somewhat hard to learn. But now after seeing it applied to PHP, I know that's what I need. So what UML program did you make that in? Also, do you have recomendations of other UML programs do perform the same task other than what you used?

Thanks!

[lastcraft]

Hi...

I want everybody to criticise the crap out of my class diagram for this app I'm building.

Game on...

Here's the link to my class diagram:

Ok, I think I know where you are going with this. I have a few suggestions.

Firstly, let's try to ditch some of the accessors. Create a Writer interface with a few simple methods, say writeRow(), writeHeaderRow(), writeText() say. The writer just executes print statements in the simplest case. Your output methods should accept the writer in the call. Take the getAllFromTable() method which probably causes you all sorts of data munging hassle...

$outputter = new NewsOutputter();
$outputter->writeNewsSummary(new HtmlWriter()); 

The outpuuter can do all of the looping, etc, to dump the table: startTable(), writeRowHeader(), writeRow(), ..., stopTable(). The benefit? How do you know it's a table? It could be a list? The outputter decides, but the writer handles the formatting. You could have a textWriter() for e-mail news, or a PdfWriter().

In addition your code will get a lot more cleaner and alot more flexible regarding the data structure. You are encapsulating not just the data, but the format of the data as well with this trick.

Next problem is naming. What you usually call an Outputter is more often called a View. What you call a RecordHandler is more usually called a RecordSet. When you are instatiating RecordSet objects and their wrappers, the factories are more usually called Finders or Repositories. So ViewFactory makes sense (you don't know it's DB data), but RecordHandlerFactory could just be DocumentRepository or DocumentFinder or even Librarian. These are just naming conventions, but it allows people to look at your diagram and immediately understand the design.

The final trick is to decouple the views and the SiteManger (ghastly name) with a dash of dependency injection. If you have more smaller views, say NewsSummaryView, rather than cutting the views into artificial structural blocks, you can standardise their constructors into just accepting the record sets they need.

Say we define the interface like so...

class NewsSummaryView {
    function __construct(NewsSummaryRecordSet $records) {
        ...
    }

    function showFullListing($writer) {
        ...
    } 

Note that type hint. The ViewFactory can now do something clever. It can use reflection to read those type hints and then ask the LIbrarian for an object of that type. Say the data in the record set has not been fetched yet. The constructor of NewsSummaryView can then set criterai on the record set and then call fetch() on it.

The point? You don't need SiteManager. This is a god class anyway and knows way to much about constructor parameters. This is too constraining and unnecessary for a dynamic language like PHP. beware on classes called Manager...or Controller .

I would call Viewfactory something like ViewPopulator now, because it has a much more specific job to do.

Note that I have also subtly moved more resposibility into the RecordSet classes. This is because tabular applications are likely to have all sorts of special queries that are probably quite tied to the data they are representing. An external finder that does more than instantiate classes will have to build queries that can be passed in. A query interface is a large and complicated thing and probably data specific. If we can keep that in the RecordSets then your top level design is less fussy. Your milleage may vary on this one though.

If anything else isnt clear, just ask, otherwise, fire away, as long as its constructive criticism.

Well, you can criticise my ideas back .

yours, Marcus

[Dr Livingston]

beware on classes called Manager...or Controller

Or just about anything that ends with an er I'd think? There is a reference somewhere (lost the bookmark) that gives a number of reasons as to why not to use a name which ends with er, but Marcus is right (again )

+2 Marcus

[Roadie]

ozone - the app is called gModeler, it a free online tool. Check it out, i think its great.

Marcus - great post! thanks, that was exactly the kind of stuff I was looking for. Some questions:

I'm just a litte bit unclear about the how the View(what I called "outputter") works with the Writer interface. Would it be too much to ask for a more flushed out explanation of that part, so I can get a better idea? I think what are saying is that the View is still composed with a Recordset object (by the way, i used the term "RecordHandler" becasue I think of a RecordSet as a result from a query...the RecordHandler handled different queries on a db), and then hands that Recordset's data off to the Writer for formatting. So, there would be a different Writer for each section then?:

$outputter->writeNewsSummary(new NewsHtmlWriter()); 

Note that type hint. The ViewFactory can now do something clever. It can use reflection to read those type hints and then ask the LIbrarian for an object of that type.

Hmm...well, I am using php4, and I don't think that has type hinting. But, since I am instansiating my classes based on a id that I passing to the factory(like "News"), couldn't I just have the factory get the RecordSet first, based on that id, and then pass that that RecordSet into the new View?

Take the getAllFromTable() method which probably causes you all sorts of data munging hassle...

no, not really, because each section of the site had its own table...to each outputter I created just asked it's record handler to return all the rows from the associated table. Each outputter then knew what to do with the records returned.

[lastcraft]

Hi...

Would it be too much to ask for a more flushed out explanation of that part, so I can get a better idea?

It really is as simple as I have painted it. Basically think of the writer as accepting generic paint operations. You have a HtmlWriter say (just one, not one for every view) taht can handle the typical atoms of presentation, such as text elements...

class HtmlWriter {
    ...
    function writeText($text) {
        print htmlentities($text);
    }

    function startTable() {
        print '<table>';
    }
    ...
} 

Just accumulate them as you need them. this follows a 'tell don't ask' policy. The View just tells the writer what element to paint. The top level app. just tells the view to paint itself. No need to keep unpicking accessors and if the format changes, say a single table becomes two tables, this does not affect anything other than the View.

I think what are saying is that the View is still composed with a Recordset object (by the way, i used the term "RecordHandler" becasue I think of a RecordSet as a result from a query...the RecordHandler handled different queries on a db), and then hands that Recordset's data off to the Writer for formatting.

Could the RecordSet just do all of this itself? By having a handler you are exposing complications to the upper levels. Push detail down, push decisions up is a good motto when you want to split a class.

So, there would be a different Writer for each section then?:

No. Which writer to pass in (probably into the constructor if more than one View method is called) is the decision of the application. The View should not know it is writing to a web page or an e-mail or whatever. Also if you have four formats (HTML, PDF, RTF, RSS) and four views (NewsSummaryView, NewsArticleView, ReportView, StopPressView) you will end up with 16 writer classes. This is a class explosion and definitely something you don't want.

Hmm...well, I am using php4, and I don't think that has type hinting.

You are correct. In that case pass the Librarian into the newly created view, using say a load method...

class ArticleView {
    ...
    function load($librarian) {
        $query = new Query('articles');
        $query->setField('id', $this->_article_id);
        $this->_article_data = &$librarian->find($query);
    }
} 

But, since I am instansiating my classes based on a id that I passing to the factory(like "News"), couldn't I just have the factory get the RecordSet first, based on that id, and then pass that that RecordSet into the new View?

The your manager class has to have this capability upgraded for every new view and record set. There is nothing to stop it kicking off this negotiation, but it's best to let the view decide what objects to load and let the higher levels supply the resources to do so.

Each outputter then knew what to do with the records returned.

Who handles unpacking the data and wrapping it in HTML tags? Is that part not on your diagram?

yours, Marcus

[Roadie]

ok, i think it's best if i answer your post in reverse order.

Who handles unpacking the data and wrapping it in HTML tags? Is that part not on your diagram?

that is taken care of in the outputter. Basically, flow would be like this:

-the Manager would take in id variable (this id is set in the nav on the html page)

-the Manager then calls the either the Outputter factory, or the RecordHandlerFactory, and instansiates the correct object based on the id that was passed in.

Here is how the SiteManager looks. i took out some of hte methods, but the other ones look very similar to these:

<?php


class SiteManager{
    
    function SiteManager(){}
    
    function doConnect($host, $user, $pass, $db){
        $conn = mysql_pconnect ($host, $user, $pass);
    
        if (!$conn){
            echo 'Error connecting to db'.'<br>';
        }
        
        //select the right database to use
        $db = mysql_select_db($db);
        
        if (!$db){
            echo 'Error selecting database'.'<br>';
        }
    }
    
    
    function getDynamicContent($cat, $sortBy, $sortDirection, $limit){
        require_once("outputters/DynamicContentFactory.php");
        $outputter = DynamicContentFactory::getDynamicOutputter($cat);
        $content = $outputter->getContent($sortBy, $sortDirection, $limit);
        return $content;
    }
    
    
    
    function insertNewRecord($section, $withFile){
        //the $withFile parameter is letting us know there is a file to upload as well
        require_once("record_handlers/RecordHandlerFactory.php");
        $recordHandler = RecordHandlerFactory::getRecordHandler($section, $withFile) or die('Error Getting Record Handler');
        $res = $recordHandler->insertRecord();
        
        if ($res){
            $message = 'Record inserted successfully.';
        }else{
            $message = 'Error inserting record.';
        }
        $this->onRecordResult($message);
    }

}

Now, the outputter gets created in the getDynamicContent() method autmatically gets composed with its corresponding RecordHandler...so for the News_Ouputter object, it gets composed with a News_RecordHandler on creation. I have a different *_Outputter and *_RecordHandler for each section.

So, when I call $outputter->getContent(), the $outputter calls a method on it's RecordHandler. The record handler retireves the records from it's assigned table(each table in the database also has the corresponding id for each section...so there is a site_news table, site_about table, etc.) and returns the results to the outputter. The outputter then formats the data and outputs that formatted data.. Here is how it looks in the News_Outputter:

function getContent($sortBy, $sortDirection, $limit){
        
        $rh = $this->recHandler;
        $res = $rh->getAllFromTable($sortBy, $sortDirection, $limit);
        
        $this->outString = "";
        
        while($row = mysql_fetch_array($res) ){
        
            $newsDate = $row['date'];
            $blurb = $row['blurb'];
            
            $this->outString .= '<p><div class="date">'. $newsDate . '</div>';
            $this->outString .= $blurb;
            $this->outString .= "<br /><br />";
        }
        return $this->outString;
    } 

The your manager class has to have this capability upgraded for every new view and record set.

Hmm..how so? All the names of my classes correspond to the id being passed in...it just takes the id and asks the factory for the right object. Here's what the entire factory looks like:

<?php 

require_once("DynamicContentOutputter.php");

class DynamicContentFactory{
    
    function DynamicContentFactory(){}
    
    function getDynamicOutputter($id){
    
        $id = strtoupper($id);
        
        $classFile = "{$id}_Outputter.php";
        $class = $id.'_Outputter';
        
        require_once( $classFile );
        return new $class($withImage);
            
    }

}

The View should not know it is writing to a web page or an e-mail or whatever.

ok, i thought i was following along but now i'm really confused. How could the View NOT know what it's writing to? I would think it would need to call very different methods for printing to an html page than printing to an rss feed?

[Dr Livingston]

I would think it would need to call very different methods for printing to an html page than printing to an rss feed?

The implementation would be different in each format, but the interface remains the same all the same. The client script that requires the use of the underlying classes would expect to use the same API for HTML, as it would for RSS.

As Marcus has just said, the client shouldn't know about the output method,...

The View should not know it is writing to a web page or an e-mail or whatever.

[REMIYA]

Looks like everything is fine within the UML

[sid egg]

For the HTML writer class, were you thinking something like

<?
class HTMLWriter {
    var $mode[];    
    var $tags[];
    
    function HTMLWriter($type="XHTML") {
        $mode['pageType'] = $type;
        $mode['pageStatus'] = "NOPAGE";
    }
    function startPage() {
        if($mode['pageType'] == "XHTML") {
            print '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">';
        } else {
            print '<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">';
        }
        print "<html>";
        $mode['pageStatus'] = "PAGE";
    }
    
    function br() {
        if($mode['pageType'] == "XHTML") {
            print "<br />";
        } else {
            print "<br>";
        }
    }
    
    function openTag($tag, $parameters) {
        $tags[] = $tag;
        print "<$tag $parameters>";
    }
    
    function closeTag($tag) {
        //hmm, how to clear the array... whatever, this is just an example anyways, I
        //think I'll make each tag an instance of class HTMLTag later... maybe
        //remove $tag from $tags[]
        print "</$tag>";
    }
    
    function endPage($closeAll) {
        if($mode['pageStatus'] != "PAGE") {
            trigger_error("Error [HTMLWriter::endPage]: No page to close.");
        }
        if($closeAll) {
            for($i=0;$i<count($tags);$i++) {
                print "</" . $tags[$i] . ">";
            }
        }
        print "</html>";
    }
    function writeText($text) {
        if($mode['pageStatus'] != "PAGE") { 
            trigger_error("Error [HTMLWriter::writeText]: Text should not be written outside of a page. HTMLWriter::startPage() should be called first!");
        }
        print htmlentities($text);
    }
    
    function startTable($padding, $spacing, $title="", ) {
        $tags[] = "table";
        print "<table title='$title' cellpadding='$padding' cellspacing='$spacing'>";
        
    }
}
?>

Edit, oooooh! Found a Javadoc: http://java.sun.com/j2se/1.3/docs/ap...TMLWriter.html

[lastcraft]

Hi...

that is taken care of in the outputter.

But something has to loop through the result set, right? If the result set is a DB cursor, then that means goinbg through an iterator interface to build each row, right? Wher is thet loop? I agree it should be in the View (Outputter), but I am trying to get teh actual HTML into a writer so that the two don't get munged together.

Here is how the SiteManager looks. i took out some of hte methods, but the other ones look very similar to these:

It's got a lot of responsibilities. I don't have a direct criticism here, but it makes me uneasy. For example the site manager knows that a relational DB is being used and which one. Surely you don't want to be rewriting this complex class on each project. You just want to write views I think.

Now, the outputter gets created in the getDynamicContent() method autmatically gets composed with its corresponding RecordHandler...so for the News_Ouputter object, it gets composed with a News_RecordHandler on creation.

What if a view has multiple result sets to display?

As an aside, how are the articles edited? Do you have a NewsEditView or some such?

Hmm..how so? All the names of my classes correspond to the id being passed in...it just takes the id and asks the factory for the right object.

It actually looks like you have a name mangling solution, which is similar to DI, but without type handling.It does strike me taht you are doing the same step twice though. Firstly to get the view, and then to get the data. Why not just have the View as for it's data from some kind of repository. You already have all of the information in the view, so don't need any kind of autowiring at this point.

Or have I misunderstood?

ok, i thought i was following along but now i'm really confused. How could the View NOT know what it's writing to? I would think it would need to call very different methods for printing to an html page than printing to an rss feed?

Not necessarily, if you describe the output in very general terms, such as tables and rows. For example a web script...

<?php
    $views = new ViewBuilder();
    $view = $views->createView('LatestArticles', new HtmlWriter());
?><html>
    <?php $view->showTitles(); ?>
</html>

...or when sending a mail...

class Newsletter {
    function mailHeadlines($contacts) {
        $views = new ViewBuilder();
        $view = $views->createView('LatestArticles', new TextWriter());
        foreach ($contacts as $contact) {
            $this->mail($contact->getEmail, "Hi " . $contact->getName() . "\n" .
                    "Here are todays headlines...\n" . $view->showTitles());
        }
    }
} 

The View doesn't care whether it's show() method is writing to HTML or Text. It could just be this...

class LatestArticles {
    ...
    function showTitles() {
        $this->_writer->startList();
        $iterator = $this->_results->getCursor();
        while ($row = $iterator->next()) {
            $this->writer->writeItem($row['title']);
        }
        $this->_writer->endlist();
    }
} 

The output format is chosen by the top level app. (as it should be), but the details of this process are in the View and the Writer choices. That's what I mean about promoting decisions (up to the top level script in this case), but pushing down details (was it a list or a table? who cares).

It's up to you of course.

yours, Marcus

[Tryst]

Are there any books or online articles which discuss and explain View and Writer Classes?

Tryst