good code ?

[poison]

I just hacked together a RSS feed parser...it's a non-critical app, as such lacking even the most simple error handling...
besides that I'm curious what you guys think could have been done better...or what you would have done in a different way ...since I began with PHP about a month ago...
thanks

<?php
class NewsItem {
    var $_news_data;

/**
* @return NewsItem
* @param string[optional] $title
* @param string[optional] $link
* @param string[optional] $description
* @desc represents a news item
*/
function NewsItem ($title=null, $link=null, $description=null) {
    $this->setTitle($title);
    $this->setLink($link);
    $this->setDescription($description);
}

/**
* @return void
* @param string $title
* @desc set title
*/
function setTitle ($title) {
    $this->_news_data['title']=$title;
}

/**
* @return string
* @desc get title
*/
function getTitle () {
    return $this->_news_data['title'];
}

/**
* @return void
* @param string $link
* @desc set URL
*/
function setLink ($link) {
    $this->_news_data['link']=$link;
}

/**
* @return string
* @desc get URL
*/
function getLink () {
    return $this->_news_data['link'];
}

/**
* @return void
* @param string $description
* @desc set item description
*/
function setDescription ($description) {
    $this->_news_data['description']=$description;
}

/**
* @return string
* @desc get item description
*/
function getDescription () {
    return $this->_news_data['description'];
}

/**
* @return string
* @desc generate html code for this news element
*/
function getHtml () {
    $html='<table border="0" cellspacing="0" cellpadding="1" width="100%" class="newsbox"><tr><td>';
    $html.='<div class="title"><a href="' . $this->getLink() . '" target="_blank">' . htmlentities($this->getTitle()) . '</a></div>';
    $desc=$this->getDescription();
    if (!is_null($desc)) {
        $html.='<div class="description">' . htmlentities($desc) . '</div>';
    }
    $html.='</td></tr></table>';
    return $html;
}

}


class Vector {
    var $_vector_data;

/**
* @return void
* @desc simple vector with type-checking
*/
function Vektor () {
    $this->_vector_data=array();
}

/**
* @return void
* @param mixed $data
* @desc append an element
*/
function add ($data) {
    if ($this->_checkType_($data) && $this->_checkAddable_($data, null)) {
        $this->set($this->elements(), $data);
    }
}

/**
* @return mixed
* @param int $index
* @desc get element at index
*/
function get ($index) {
    return $this->_vector_data[$index];
}

/**
* @return void
* @param int $index
* @param mixed $data
* @desc set element at index
*/
function set ($index, $data) {
    if ($this->_checkType_($data) && $this->_checkAddable_($data, $index)) {
        $this->_vector_data[$index]=$data;
    }
}

/**
* @return void
* @param int $index
* @desc remove element at index
*/
function remove ($index) {
    $tmp=array();
    for ($i=0; $i<$this->elements(); $i++) {
        if ($i!=$index) {
            $tmp[]=$this->_vector_data[$i];
        }
    }
    $this->_vector_data=$tmp;
}

/**
* @return void
* @param int $one
* @param int $two
* @desc swap two elements
*/
function swap ($one, $two) {
    if ($one!=$two) {
        $tmp=$this->get($one);
        $this->set($one, $this->get($two));
        $this->set($two, $tmp);
    }
}

/**
* @return void
* @desc reset vector
*/
function reset () {
    $this->_vector_data=array();
}

/**
* @return int
* @desc get number of contained elements
*/
function elements () {
    return count($this->_vector_data);
}

/**
* @return bool
* @param mixed $data
* @desc check if element can be contained in this vector
*/
function _checkType_ ($data) {
    return true;
}

/**
* @return bool
* @param mixed $data
* @param int $index
* @desc check if element can be added/set at specified vector
*/
function _checkAddable_ ($data, $index) {
    return true;
}

}


class NewsCollection extends Vector {

/**
* @return NewsCollection
* @desc container for NewsItem's
*/
function NewsCollection () {
    $this->Vektor();
}

/**
* @return bool
* @param mixed $data
* @desc check if $data is a NewsItem
*/
function _checkType_ ($data) {
    return is_a($data, 'NewsItem');
}

/**
* @return void
* @param NewsCollection $newscollection
* @desc append collection items
*/
function appendCollection ($newscollection) {
    if (is_a($newscollection, 'NewsCollection')) {
        /* @var $newscollection NewsCollection */
        for ($i=0; $i<$newscollection->elements(); ++$i) {
            $this->add($newscollection->get($i));
        }
    }
}

/**
* @return void
* @desc shuffle the NewsItem's
*/
function shuffle () {
    for ($i=0; $i<$this->elements(); ++$i) {
        $this->swap(rand(0, $this->elements()-1), rand(0, $this->elements()-1));
    }
}

/**
* @return string
* @desc generate HTML code for the news collection
*/
function getHtml () {
    $html='';
    for ($i=0; $i<$this->elements(); ++$i) {
        if ($this->_checkType_($this->_vector_data[$i])) {
            $html.=$this->_vector_data[$i]->getHtml();
        }
    }
    return $html;
}
}


// static class...
class NewsProvider {

/**
* @return string
* @param string $host
* @param string $page
* @desc get $page from $host inclusive HTTP headers
*/
function _getPage ($host, $page='/') {
    $data=null;
    // open socket
    $handle=fsockopen ($host, 80, $err, $errdesc, 30);
    if (!$handle) {
        echo '<b>' . $err . '</b>' . $errdesc . '<br>';
    } else {
        // send get request
        fputs ($handle, "GET " . $page ." HTTP/1.0\r\n\r\n");
        // read entire data in 1K blocks
        while (!feof($handle)) {
            $data.=fgets($handle, 1024);
        }
        // close socket
        fclose($handle);
    }
    return $data;
}

/**
* @return string
* @param string $url
* @desc get $url inclusive HTTP headers
*/
function getPage ($url) {
    preg_match('/http:\\/\\/([^\\/]+?)(\\/[^s]+)/', $url, $matches);
    $data=NewsProvider::_getPage($matches[1], $matches[2]);
    return $data;
}

/**
* @return NewsCollection
* @param string $url
* @param NewsCollection[optional] $newscollection
* @param int $amount[optional]
* @desc returns the News from the specified RSS document in a NewsCollection
*/
function getNews ($url, $newscollection=null, $amount=null) {
    // if no NewsCollection was passed...instantiate one
    if (!is_a($newscollection, 'NewsCollection')) {
        $newscollection=new NewsCollection();
    }

    // get data from URL
    $news_data=NewsProvider::getPage($url);

    // grep all <item></item> elemnts in RSS into array
    preg_match_all('/<item[^>]*?>(.*?)<\\/item>/sm', $news_data, $news_items, PREG_PATTERN_ORDER);

    for ($i=0; $i<count($news_items[1]); ++$i) {
        if (!is_null($amount)) {
            if ($amount>=$i) {
                break;
            }
        }
        // extract titel, link and description
        preg_match('/<title>(.*?)<\\/title>/smi', $news_items[1][$i], $tmp_title);
        preg_match('/<link>(.*?)<\\/link>/smi', $news_items[1][$i], $tmp_link);
        preg_match('/<description>(.*?)<\\/description>/smi', $news_items[1][$i], $tmp_description);
        // add new NewsItem to collection
        $newscollection->add(new NewsItem($tmp_title[1], $tmp_link[1], $tmp_description[1]));
    }
    return $newscollection;
}

}

?>

usage:

<?php
$news=NewsProvider::getNews('http://kuro5hin.org/backend.rdf');
$news=NewsProvider::getNews('http://freshmeat.net/backend/fm.rdf', $news);
$news=NewsProvider::getNews('http://www.kde.org/dot/kde-apps-content.rdf', $news);
$news->shuffle();
echo ($news->getHtml());
?>

[goldbug]

Since RSS is XML.... why didn't you use the XML functions in PHP? Seems like you reinvented the wheel a bit with your regular expressions.

Even better:
RSS is XML...
XSL transforms XML...
PHP can do XSL transformations...

Coded properly:
RSS + XSL = XHTML 😃

[poison]

thanks for your reply

Since RSS is XML.... why didn't you use the XML functions in PHP? Seems like you reinvented the wheel a bit with your regular expressions.

mainly because I have crappy webspace -> PHP with no XML support..
2nd -> the RSS DTD isn't really huge and I only care about 3 elements.
3rd -> pretty much the same reason why I wrote NewsProvider::_getPage() ... because it's supposed to work on an very basic and restricted PHP compile...
regards

[goldbug]

Originally posted by poison
thanks for your reply
mainly because I have crappy webspace -> PHP with no XML support..
2nd -> the RSS DTD isn't really huge and I only care about 3 elements.
3rd -> pretty much the same reason why I wrote NewsProvider::_getPage() ... because it's supposed to work on an very basic and restricted PHP compile...
regards

Ok, that makes sense. 🙂

BTW... you rock for using structured comments. It seems nobody bothers these days, despite the fact it goes a long way for readable code (not to mention documentation generation) 😃

[ednark]

well it looks like you are stuck in OO land... im assuming you are coming from a java background. I would decalre most of your code unnecessary to complete the task at hand.

i reduced this to what I think is needed for the task. You can forget about get() set() you can forget about vectors, you can foget about needless classing and subclassing..

try this:

beware the stripping of slashed from regular expressions by this bb system.

<?php

// static class...
class NewsProvider 
{


    function getPage ( $url ) 
    {
        preg_match('/http:\/\/([^\/]+?)(\/[^\s]+)/', $url, $matches);
        $host = $matches[1];
        $page = $matches[2];
        $data = null;
        // open socket
        $handle=fsockopen ($host, 80, $err, $errdesc, 30);
        if (!$handle) {
            echo "<b>{$err}</b>{$errdesc}<br>";
        } else {
            // send get request
            fputs ($handle, "GET " . $page ." HTTP/1.0\r\n\r\n");
            // read entire data in 1K blocks
            while (!feof($handle)) {
                $data .= fgets($handle, 1024);
            }
            // close socket
            fclose($handle);
        }
        return $data;
    }


    function getNews ( $url, $newscollection=array(), $limit=null ) 
    {

        // get data from URL
        $news_data = NewsProvider::getPage($url);

        // grep all <item></item> elemnts in RSS into array
        preg_match_all('/<item[^>]*?>(.*?)<\/item>/sm', $news_data, $news_items, PREG_PATTERN_ORDER);

        for ($i=0; $i<count($news_items[1]) && ($limit==null || $limit>$i); ++$i) {
            // extract title, link and description
            preg_match('/<title>(.*?)<\/title>/smi', $news_items[1][$i], $tmp_title);
            preg_match('/<link>(.*?)<\/link>/smi', $news_items[1][$i], $tmp_link);
            preg_match('/<description>(.*?)<\/description>/smi', $news_items[1][$i], $tmp_description);
            // add new NewsItem to collection
            $newscollection[] = array( 'title'       => $tmp_title[1], 
                                       'link'        => $tmp_link[1], 
                                       'description' => $tmp_description[1], 
                                       'source'      => $url );
        }
        return $newscollection;
    }


    function getHtml ( $newscollection )
    {
        $html = '';
    foreach ( $newscollection as $news_item ) {
      $html .= <<<END_TEMPLATE
<table border="0" cellspacing="0" cellpadding="1" width="100%" class="newsbox">
  <tr>
    <td><div class="title"><a href="{$news_item['link']}" target="_blank">{$news_item['title']}</a> <span class="source"> :: {$news_item['source']}</span></div>
        <div class="description">{$news_item['description']}</div></td>
  </tr>
</table>
END_TEMPLATE;
	    }
	    return $html; 
        }

}
$news = array();

$news=NewsProvider::getNews('http://kuro5hin.org/backend.rdf');
$news=NewsProvider::getNews('http://freshmeat.net/backend/fm.rdf', $news);
$news=NewsProvider::getNews('http://www.kde.org/dot/kde-apps-content.rdf', $news);

?>
<html>
<head>
<style type="text/css">

  table.newsbox { padding: 10px; }

  div.title { padding: 5px 0px 5px 20px; background-color: #DDDDDD; }

  div.title a { color: #0000AA; text-decoration: none; }
  div.title a:hover { text-decoration: underline; color: #0000FF; }
  div.title a:visited { text-decoration: none; }

  div.description { padding: 5px 0px 5px 20px; }
  div.description:hover { color: #444444; }

  span.source { font-style: italic; font-size: 10pt; color: #666666; }
</style>
</head>
<body>
<?

shuffle($news);
$html = NewsProvider::getHtml($news);
echo $html;

?>
</body>
</html>

[poison]

you rock for using structured comments

thx If you ever wrote a 50k line app and searched a stupid little bug half a year later and needed 4 weeks because you only had about 10 comments...you would probably do the same 😉

well it looks like you are stuck in OO land

yes...and I like it there =P

im assuming you are coming from a java background

no

You can forget about get() set() you can forget about vectors, you can foget about needless classing and subclassing

what was the largest app you ever wrote ? sorry...but time made me religious about design and coding style. Of course I could have done the whole thing in 1 line perl code 😉

beware the stripping of slashed from regular expressions by this bb system.

thx, I already noticed
thanks for your reply =D

[ednark]

sorry I didn't mean for my tone to be so negative. ..

The correct style of coding is the one you are most familiar with so my intent was not to say "your stupid," and I was assuming this was for your own amusement and not part of a larger project which would require defining and following standards or doing any more than "get it done as simple as possible"

I have been told that when critiquing a piece of writing you are supposed to do this. So maybe this would be a good idea for this forum.

1) praise of positive things in the work

2) ask questions about the work for things reader doesn't udnerstand or wants to challenge

3) respond to question by author about what reader did and did not see or understand

3) positive criticism of the things reader would change or doesn't like

so...

1)

I will asssuming, as you said, that you wanted to keep everything object oriented, and like to organize that way, and require for sanity sake a standard coding practice. You broke things up into very logical pieces, your code is conceptually very easy to understand, and is written in a very straightforward and clear manner. The class names are meaningful as are the function names which define clear responsibility. The comments for auto code generation would also be very meaningful if looked at w/o the code present. You keep responsibility broken up so as not to create one or two functions with 5000 lines of business logic each.

Your usage example is not complicated and shows how easy your code is to use.

2)
Why write the usage around the :: singleton? Why not generate a NewsProvider object, and have the getNews() function just keep updating the instances internal array? Is there some reason why singleton is preferred?

Why bother using an internal array to store all variable in the class? Why not simply use object attributes? Even if you wanted to use the function interface to your class and not directly access attributes it seems redundant.
Why bother with the Vecktor class? Is it because all your other code is already written with it as an interface? You don't seem to be implementing any functionality other than is already available in an array. You do have a index to index swap() function but you seem to only use that for randomizing, which is already implemented in array() itself.

3)
Please respond with any specific questions you have of me/the forum.

4)
Since you have to get into php5 before you get actual protected variables, I wouldn't bother with the get() set() interface to your functions since its really non enforcable by php.
$obj->var_a = 'foo';
is almost the same typing as
$obj->setVarA('foo');
but requires less work before you can start using your code. A set() function is good for type checking and such. But since php is usually wrapped around Html Forms, most of the validation is done at that level before the info gets into your object, or so I have seen.

The use of Vecktor and NewsCollection is invisible to the user since you made your user interface of ::getNews() so simple, so since NewsProvider is manageing the whole thing in the first place I wouldn't bother getting any more complicated than you need behind that. Your code is nice and clean but noone but you will ever see it if another developer were to use your code. However I grant that if you are someone else wanted to extend this, the common use of Vektor subclasses would be desirable.

While I like the getHtml() of each. You are not seperating content and logic, since you spent a lot of trouble making nice OO code I would look into passing the getHtml() a template of some sort or somehow seperating the html from your code.