MVC Improvement - The View Module - 0 *

Question

This simply pulls data from mysql and creates the html to be sent to the page.

class view_html extends database
  {
  function __construct($type)
    {
    parent::__construct();    
    switch ($type) 
      {
      case "bookmraks":
        $this->bookmarks();
        break;
      case "tweets":
        $this->tweets();
        break;
      default:
        echo "Invalid View Type";
        break;
      }
    }
  private function bookmarks()
    {
    $email = $_SESSION['email'];
    $query_return = database::query("SELECT * FROM bo  WHERE email='$email' ORDER BY name ASC");
    while ($ass_array = mysqli_fetch_assoc($query_return))
      {
      $fav=$this->fav($ass_array['url']);
      echo "<img name=\"bo_im\" class=\"c\" src=\"$fav\"/ onerror=\"i_bm_err(this)\"><a target=\"_blank\" name = \"a1\" class = \"b\" href = \"$ass_array[url]\">$ass_array[name]</a>";
      }      
    }
  private function tweets() 
    {
    $query_return = database::query("SELECT * FROM tw ORDER BY time DESC LIMIT 7");
    $time = time();
    while ($a = mysqli_fetch_assoc($query_return))
      {
      echo  "<div class=\"Bb2b\"><img class=\"a\" src=\"p/$a[email].jpg\" alt=\"\"/><a class=\"a\" href=\"javascript:void(0)\">$a[fname] posted <script type=\"text/javascript\">document.write(v0($a[time],$time))</script></a><br/><p class=\"c\">$a[message]</p></div>";
      }
    }
  private function fav($url)     
    {    
    $pieces = parse_url($url);    
    $domain = isset($pieces['host']) ? $pieces['host'] : '';    
    if(preg_match('/(?P<domain>[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;
    }
  }

Answer 1

Replace all usages of direct Values.
Try assiging values first to a named variable and then concating it, so its sustainable in the future like:

return $pieces['scheme'] . '://www.' . $regs['domain'] . '/favicon.ico';  

could be:

$protocol = $pieces['scheme'];
$domainName = $regs['domain'];
$faviconName = . '/favicon.ico';  
$favIconLink = $protocol. '://www.' .  $domainName . $faviconName
return  $favIconLink;

This would make it reusable and readable for other developers, and it reduces comments ;-) Currently you have many of them

$fav=$this->fav($ass_array['url']);

I love regex, but once written nobody understands it:

if(preg_match('/(?P<domain>[a-z0-9][a-z0-9\-]{1,63}\.[a-z\.]{2,6})$/i', $domain, $regs))  

i would add a comment which explaines detailed what this regex is suppose to do. So if there is an Problem with the Regex everybody can check if the regex is doing it's job or if there is a problem in the expression.

Throw an exception in your constructor, so the errormessage can be cought. Now the error Message is displayed in the frontcode.

function __construct($type)
{
parent::__construct();    
switch ($type) 
  {
  case "bookmraks":
    $this->bookmarks();
    break;
  case "tweets":
    $this->tweets();
    break;
  default:
    throw new Exception('Invalid View Type');
  }
}

Last but not least:

try using "'" for strings, so its more readable and you make less errors by escaping not escaping closing, whatever

 '<div class="Bb2b"><img class="a" src="p/$a[email].jpg" alt=""/><a class="a" href="javascript:void(0)">$a[fname] posted <script type="text/javascript">document.write(v0($a[time],$time))</script></a><br/><p class="c">$a[message]</p></div>'

instead of :

 "<div class=\"Bb2b\"><img class=\"a\" src=\"p/$a[email].jpg\" alt=\"\"/><a class=\"a\" href=\"javascript:void(0)\">$a[fname] posted <script type=\"text/javascript\">document.write(v0($a[time],$time))</script></a><br/><p class=\"c\">$a[message]</p></div>"

Answer 2

I'd refactor this class into subclasses for bookmarks, favorites, and tweets. Then in each subclass I'd have a similiar API, maybe called render_HTML. Then I'd use the original class as a proxy for these subclasses. I'd also avoid sending the html directly from each class, instead return the html and either use a controller to set a variable equal to the html for your view page, or if you aren't using a mvc framework output the value of return_html directly in your view.

Now we have OK separation of concerns (you could take this even farther and abstracting out your database access but we won't go that far here). Now you can also add new views to render rather easily without having to worry about breaking your view class, and you can do other things with those factored out classes down the road.

Heres some psudo code of what i'm talking about:

class view_html extends database
  {
  function __construct($type){
    parent::__construct();    

    this->type = type;
    }

    function render_html(){
        case "bookmraks":
        $this->bookmarks->render_html();
        break;
      case "tweets":
        $this->tweets->render_html();
        break;
      default:
        echo "Invalid View Type";
        break;
      }
    }
 }

class bookmarks{
    public function render_html(){
        //query
        //generate html in loop

        return html;
    }
}  

class tweets{
    public function render_html(){
        //query
        //generate html in loop

        return html;
    }
}

class fav{
    public function render_html(){
        public function render_html(){
            //query
            //generate html in loop

            return html;
        }
    }
  }