HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

MVC Improvement - The View Module - 1

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
themvcmoduleviewimprovement

Problem

```
/****
VIEW MODULE
view, view_database, view_message
view_arche_1 (external), view_arche_2 (external) - these are
primarily html files
****/

/view/

class view extends database
{
}

/view_db/

class view_db extends view
{
function __construct($type)
{
parent::__construct();
$this->invoke($type);
}
private function invoke($type)
{
switch ($type)
{
case "bookmarks":
$this->html_bookmarks();
break;
case "tweets":
$this->html_tweets();
break;
default:
throw new Exception('Invalid View Type');
break;
}
}
private function html_bookmarks()
{
$email = $_SESSION['email'];
$query_return = database::query("SELECT * FROM bo WHERE email='$email' ORDER BY name ASC");
$html_string='';
while ($ass_array = mysqli_fetch_assoc($query_return))
{
$fav=$this->favicon($ass_array['url']);
$html_string = $html_string . "$ass_array[name]";
}
echo $html_string;
}
private function html_tweets()
{
$query_return = database::query("SELECT * FROM tw ORDER BY time DESC LIMIT 7");
$time = time();
$html_string='';
while ($a = mysqli_fetch_assoc($query_return))
{
$html_string = $html_string . "$a[fname] posted document.write(v0($time,$a[time]))$a[message]";
}
echo $html_string;
}
private function favicon($url)
{
$pieces = parse_url($url);
$domain = isset($pieces['host']) ? $pieces['host'] : '';
if(preg_match('/(?P[a-z0-9][a-z0-9\-]{1,63}\.[a-z\.]{2,6})$/i', $domain, $regs))
{
return $pieces['scheme'] . '://www.' . $regs['domain'] . '/favicon.ico';
}
return false;
}
}

class view_message extends view
{
function __construct($type)
{
$this->invoke($t

Solution

Some random observations:

  • What does your base class database do? I hope not the stuff it sounds like. Your aim is to separate storage and view logic, not add view logic to your database.



  • Having both static and non-static functions in a class should make you suspicious (as this is an indicator this class has one then more objective or is inconsistently written).



  • Some functions do echo, some return and some call other functions. Decide for one way ;).



  • invoke for parameter to function resolving? Have a look at the much cooler magic function __call.



Some suggestions:

  • It is probably a good idea to move the single view operations into it's own classes. This becomes more important when you add even more operations.



  • Unify the way your class is accessed and the return values.



  • Views don't extend a database.



Easy to implement way:

  • Let every view operation be it's own class.



  • Define a common interface for all view operations (e.g. ViewOperation with one required function run()).



  • Create one main class View that is responsible to resolve view operations to subclasses.



Example implementation:

interface ViewOperation
{
    public function run();
}

public class ControllAdd implements ViewOperation
{
    private $_types = array('pass' => '', 'fail' => '');

    public function run()
    {
        $params = func_get_arg(1);
        if(isset($params) && !empty($params) && array_key_exists($this->_types, $params))
        {
            return $this->_types[$params];
        }
    }
}

public class View
{
    public function __call($name, $arguments)
    {
        // More security required here of course
        require_once './ViewOperartopns/' . $name . '.php';

        $instance = new $name;
        return $instance->run($arguments);
    }
}

Code Snippets

interface ViewOperation
{
    public function run();
}

public class ControllAdd implements ViewOperation
{
    private $_types = array('pass' => '<xx_p>', 'fail' => '<xx_f>');

    public function run()
    {
        $params = func_get_arg(1);
        if(isset($params) && !empty($params) && array_key_exists($this->_types, $params))
        {
            return $this->_types[$params];
        }
    }
}

public class View
{
    public function __call($name, $arguments)
    {
        // More security required here of course
        require_once './ViewOperartopns/' . $name . '.php';

        $instance = new $name;
        return $instance->run($arguments);
    }
}

Context

StackExchange Code Review Q#5173, answer score: 2

Revisions (0)

No revisions yet.