4
\$\begingroup\$

I've created a working XML RSS reader. I wanted to know if it's a good one or not and if it's efficient for the server or not. I run this code every 12 hours or something give or take the amount of time that a new article will be published.

This is the code itself with all the explanation of every part of the code. Tell me if it's efficient or if it needs some changes to do an even better a job.

<?php

function GetRSSData(){
    // this is an array with all the xml data are stored
    $RSSXMLURL = array(
        'http://www.buzzfeed.com/tech.xml',
        'http://www.buzzfeed.com/category/celebrity.xml',
        'http://feeds.bbci.co.uk/news/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/front_page/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/world/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/uk/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/england/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/northern_ireland/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/scotland/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/wales/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/business/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/uk_politics/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/health/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/education/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/sci/tech/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/technology/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/entertainment/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/talking_point/rss.xml',
        'http://newsrss.bbc.co.uk/rss/newsonline_uk_edition/magazine/rss.xml',
        'http://feeds.bbci.co.uk/news/system/latest_published_content/rss.xml',
        'http://rss.cnn.com/rss/edition_space.rss', 
        'http://rss.cnn.com/rss/edition_meast.rss',
        'http://rss.cnn.com/rss/edition.rss',
        'http://rss.cnn.com/rss/edition_world.rss',
        'http://rss.cnn.com/rss/edition_africa.rss',
        'http://rss.cnn.com/rss/edition_americas.rss',
        'http://rss.cnn.com/rss/edition_asia.rss',
        'http://rss.cnn.com/rss/edition_europe.rss',
        'http://rss.cnn.com/rss/edition_us.rss',
        'http://rss.cnn.com/rss/money_news_international.rss',
        'http://rss.cnn.com/rss/edition_technology.rss',
        'http://rss.cnn.com/rss/edition_entertainment.rss',
        'http://rss.cnn.com/rss/edition_sport.rss',
        'http://rss.cnn.com/rss/edition_football.rss',
        'http://rss.cnn.com/rss/edition_golf.rss',
        'http://rss.cnn.com/rss/edition_motorsport.rss',
        'http://rss.cnn.com/rss/edition_tennis.rss',
        'http://travel.cnn.com/rss.xml',
        'http://rss.cnn.com/rss/cnn_freevideo.rss',
        'http://rss.cnn.com/rss/cnn_latest.rss',
        'http://rss.cnn.com/rss/edition_connecttheworld.rss',
        'http://rss.cnn.com/rss/edition_worldsportblog.rss'
    );

    // the following variables are for every article
    $Title; // article title
    $PublishingDate; // the publishing date
    $Description; // the article itself

    $NumSaved = 0; // i will use this later on to know how many article were saved in the database
    $NumUnSaved = 0; // how many articles weren't saved in the database where this means how many
    // dublicated article the code found

    $mysqli = new mysqli("localhost", "root", "", "rss"); // just logging into my database
    // database Name: rss

    // preparing my statment to be excuted as soon as i put the data into the variables
    $stmt = $mysqli->prepare("insert INTO `articles` (PublishDate, ArticleTitle, ArticleBody) VALUES (?, ?, ?)");
    $stmt->bind_param("sss", $PublishingDate, $Title, $Description); // making sure that all of them are string

    foreach ($RSSXMLURL as $URL) { // this for each loop will go to every xml file on the revious array
        $XML = simplexml_load_file($URL); // getting the xml file content

        foreach ($XML->channel->item as $item) { // this for each loop will get every item(article) which is 
            //how it's written in every xml file
            $Title = $item->title; // getting the title
            $String = $item->description; // getting the article body in a temp variable for now so we can remove the ads
            // later on the code
            $Link = $item->link; // getting the article link
            $PublishingDate = $item->pubDate; // getting the publishing date of the article


            // this part of code will get all the ads from the articles
            // after some reseach i found out that most of them have one thing in commen
            // clear='all' which is written in a tag
            // so i check if there was any
            if (strpos($String,"clear='all'") !== false) { // if there was any
                $Pos = strpos($String, "clear='all'"); // i get it's position
                $String = str_replace(substr($String, $Pos), "", $String);// i remove it from the string through
                // replacing it with an empty space with that i completly remove the ads
                if (strlen($String) <= 4) { // now check if the article body has any data in it after removing the ads
                    // if there wasn't any make it so it would show some meaniful data
                    $String = "There is no Description"; // <==
                }
            }
            $Description = $String; // now after we finished all that, put it in the description variable


            // clean up the publishing date varibale as it's our primary key in the database
            // there is some extra data in it that we don't want
            // like the day name Sun, Sat, Mon, etc
            // the time zone GMT, EST, etc

            $PublishingDate = substr($PublishingDate, 5); // this one removes the first five letter of the date
            // which means removeing the M-O-N-,-Space
            // this five extra letters that we don't want in the date

            // now for next thing the date has the month in alphabitcal format and we need it
            // in number format only
            // so i made the following code 
            // it removes the month name and replace it with it's number
            // then put the variable month with the same data
            if (strpos($PublishingDate, "Jan") !== false) {
                $PublishingDate = str_replace("Jan", "01", $PublishingDate);
                $Month = "01";
            } else if (strpos($PublishingDate, "Feb") !== false) {
                $PublishingDate = str_replace("Feb", "02", $PublishingDate);
                $Month = "02";
            } else if (strpos($PublishingDate, "Mar") !== false) {
                $PublishingDate = str_replace("Mar", "03", $PublishingDate);
                $Month = "03";
            } else if (strpos($PublishingDate, "Apr") !== false) {
                $PublishingDate = str_replace("Apr", "04", $PublishingDate);
                $Month = "04";
            } else if (strpos($PublishingDate, "May") !== false) {
                $PublishingDate = str_replace("May", "05", $PublishingDate);
                $Month = "05";
            } else if (strpos($PublishingDate, "Jun") !== false) {
                $PublishingDate = str_replace("Jun", "06", $PublishingDate);
                $Month = "06";
            } else if (strpos($PublishingDate, "Jul") !== false) {
                $PublishingDate = str_replace("Jul", "07", $PublishingDate);
                $Month = "07";
            } else if (strpos($PublishingDate, "Aug") !== false) {
                $PublishingDate = str_replace("Aug", "08", $PublishingDate);
                $Month = "08";
            } else if (strpos($PublishingDate, "Sep") !== false) {
                $PublishingDate = str_replace("Sep", "09", $PublishingDate);
                $Month = "09";
            } else if (strpos($PublishingDate, "Oct") !== false) {
                $PublishingDate = str_replace("Oct", "10", $PublishingDate);
                $Month = "10";
            } else if (strpos($PublishingDate, "Nov") !== false) {
                $PublishingDate = str_replace("Nov", "11", $PublishingDate);
                $Month = "11";
            } else if (strpos($PublishingDate, "Dec") !== false) {
                $PublishingDate = str_replace("Dec", "12", $PublishingDate);
                $Month = "12";
            }

            // now the publishing date is in this format
            // DD:MM:YYYY HH:MM:SS
            // all what i have to do is making sure that we have every thing on it's own for now
            // and since a string is an array of characters i begain to take what i want from it as array
            $Day = $PublishingDate[0] . $PublishingDate[1];
            $Year = $PublishingDate[6] . $PublishingDate[7] . $PublishingDate[8] . $PublishingDate[9];
            $Hour = $PublishingDate[11] . $PublishingDate[12];
            $Minute = $PublishingDate[14] . $PublishingDate[15];
            $Seconds = $PublishingDate[17] . $PublishingDate[18];

            // now let's put them in the right order and put the dashes and the ':'
            $PublishingDate =  $Day . "-" . $Month . "-" . $Year;
            $PublishingDate .= " " . $Hour . ":" . $Minute . ":" . $Seconds;

            // at last but not least i will now excute the SQL code which will return true if it was sucessful
            // which will add +1 to the numbers of articles which were saved in the database
            if ($stmt->execute()){
                $NumSaved++;
            } else { // if something went wrong that would be because of the primary key as the rest of the
                // attribute are Text data type which means that the article is already in the database
                // because what are the chances that someone publish an article in the same time someone else
                // publish another article up to the seconds accurtacy
                $NumUnSaved++;
            }
        }
    }
    // finally show me how many articles were saved and unsaved in the database
    // Note: this is just for me to know won't do something with it
    echo "Saved: $NumSaved <br>";
    echo "UnSaved: $NumUnSaved <br>";
}

GetRSSData(); // run the code above ^_^
?>
Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
6
\$\begingroup\$

I'm not very familiar with PHP so this will only cover some basic observations.

Readability.
On the plus side, your code is full of comments explaining what everything does.
On the minus side. It also suggests you're not separating things out. Really good code should never need to explain what it is doing. It should only ever need to explain why something was done the way it was. The rest of it should be procedures and subprocedures which read remarkably close to plain english along the lines of:

Begin Procedure
Create Foo
Determine size of Foo
For one to size of Foo
Get basic info
Remove ABC from Foo
Reformat XYZ To GHI
Check DEF
Output BAR
Next
End Procedure

Which makes your procedure incredibly easy to follow, change and maintain.

To quote people much more experienced than me: "Refactor Mercilessly".
You may find this useful.
Generally, refactoring just means taking parts of your code that do specific things and breaking them down into smaller and smaller procedures / subprocedures / classes / etc.

As an example, this:

 $RSSXMLURL = array(
        'http://www.buzzfeed.com/tech.xml',
        'http://www.buzzfeed.com/category/celebrity.xml',
        'http://feeds.bbci.co.uk/news/rss.xml',  
        ....

needs to be a separate procedure that you call. Probably named something like FillArrayWithUrls.

And this:

        // this part of code will get all the ads from the articles
        // after some reseach i found out that most of them have one thing in commen
        // clear='all' which is written in a tag
        // so i check if there was any
        if (strpos($String,"clear='all'") !== false) { // if there was any
            $Pos = strpos($String, "clear='all'"); // i get it's position
            $String = str_replace(substr($String, $Pos), "", $String);// i remove it from the string through
            // replacing it with an empty space with that i completly remove the ads
            if (strlen($String) <= 4) { // now check if the article body has any data in it after removing the ads
                // if there wasn't any make it so it would show some meaniful data
                $String = "There is no Description"; // <==
            }
        }
        $Description = $String; // now after we finished all that, put it in the description variable

Can be a separate procedure called something like RemoveAdsFromString.

Nested if statements (especially beyond 2 or 3) always smell very fishy.
This segment:

if (strpos($PublishingDate, "Jan") !== false) {
            $PublishingDate = str_replace("Jan", "01", $PublishingDate);
            $Month = "01";
        } else if (strpos($PublishingDate, "Feb") !== false) {
            $PublishingDate = str_replace("Feb", "02", $PublishingDate);
            $Month = "02";
        } else if (strpos($PublishingDate, "Mar") !== false) {
            $PublishingDate = str_replace("Mar", "03", $PublishingDate);
            $Month = "03";
        } 
        .........

Should really be something like:

Switch (strpos($PublishingDate)) {

    case "Jan":
        doFoo('jan');
        break;

    case "Feb":
        doFoo('feb');
        break;
    ...

    default:
        errorHandler();
}

Plus, it should really be its own procedure too.

And suddenly, you've gone from 'wall of text' code to:

<?php

function GetRSSData(){

    FillArrayWithUrls();

    //Declare Key variables
    ..
    ..

    foreach ($RSSXMLURL as $URL) { // this for each loop will go to every xml file on the revious array
        $XML = simplexml_load_file($URL); // getting the xml file content
        foreach ($XML->channel->item as $item) {
            $title = $X;
            $link = $y;
            $publishingDate = $z;
            ..
            ..

            RemoveAdsFromString();

            ConvertStringMonthToNumerical();

            ReformatPublishingDate();

            CheckForSuccess();
        }

    }
}

Which is much easier to follow and understand and it doesn't need 6 lines of comments every few steps to document what everything is doing, because the procedure names are self-explanatory.

Improve this answer
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thank you very much and I will do so as you suggested it makes more sense and might even enhance the performance \$\endgroup\$
    – hamada147
    Aug 31, 2015 at 3:38
  • \$\begingroup\$ Feel free to post your revised code as a follow-up question \$\endgroup\$
    – Kaz
    Aug 31, 2015 at 16:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.