RSS feed class in C#

How can I make this more readable?

public class RssFeed
{

    string url = "http://www.pwop.com/feed.aspx?show=dotnetrocks&filetype=master";

    public string responseBody;

    private string DATE_FORMAT = "ddd, dd MMM yyyy HH:mm:ss \\E\\D\\T";

    public Story[] Getstories()
    {
        getContent();
        var stories = Parse(responseBody);
        return GetMostRecent5(stories, new StoryComparer());
    }

    public void getContent()
    {
        HttpWebRequest req = WebRequest.Create(url) as HttpWebRequest;
        WebResponse response = req.GetResponse();
        StreamReader sr = new StreamReader(response.GetResponseStream());
        responseBody = sr.ReadToEnd();
    }

    static Story[] Parse(string content)
    {
        var items = new List();
        int start = 0;
        while (true)
        {

            var nextItemStart = content.IndexOf("", start);
            var nextItemEnd = content.IndexOf("", nextItemStart);
            if (nextItemStart ();
        for (byte i = 0; i ).*(?=)").Value,
                link = Regex.Match(items[i], "(?).*(?=)").Value,
                date = Regex.Match(items[i], "(?).*(?=)").Value
            });                
        }

        return stories.ToArray();
    }

    private static T[] GetMostRecent5(T[] stories, IComparer comparer)
    {
        List recentStories = stories.Take(5).ToList();

        recentStories.Sort(comparer);

        var recentStoriesArray = new T[5];
        for (int i = 0; i 
    {
        public int Compare(Story x, Story y)
        {
            if (x.Equals(y))
                return 0;
            //redundant else
            else
                return x.date.CompareTo(y.date);
        }
    }
}

Not that ugly. I've seen ugly code, and trust me, this is not ugly.

However, this is kind of a "God Class" that has more than a single responsibility, hence it violates SRP principle.

How to improve

Separate the class to:

ContentFetcher, which is responsible for fetching only

StoryParser, which parses the content and creates instances of your object model (Story[])

Both of the above should implement the Try.. pattern, so that the single method in the ContentFetcher has the following signature:

bool TryFetch(out string content){ /* code */ }

The same goes for StoryParser.

As for the fetcher itself

-
You can choose if the URL is either injected in the constructor, or is part of the method signature. Modern OOP coding is biased towards the constructor technique.

-
The code should implement the using (...) { ... } pattern for classes that implement the IDisposable interface, and be wrapped in a try-catch what so ever.

As for the parser itself

-
since rss is a format on top of xml, I don't think it's a good idea to parse it using regular expressions. Let the framework do the parsing by using either XmlDocument or XDocument. Or use an external library to parse the string for you (hopefully it's implemented by the above techniques, and not by using Regexes).

-
a try-catch might be needed here, too

Putting it all together

So finally, the class you have should use the above two classes (a fetcher and a parser), and then, getting the stories from the feed should look like this:

public Story[] Getstories()
{
    string content;
    if (!this.fetcher.TryGetContent(out content)) return null;
    Story[] stories;
    if (!this.parser.TryParse(out stories)) return null;        
    return GetMostRecent5(stories, new StoryComparer());
}

As you can see, null is a special value for failure. You can change that to TryGetStories which returns a boolean and and out parameter if you want to avoid the special value of null.

The actual instances of fetcher and parser can be created by the class itself (in the constructor) and/or be injected to the class by the constructor. Let's see the injection pattern:

public class RssFeed
{       
    private readonly ContentFetcher fetcher;
    private readonly StoryParser parser;

    public RssFeed(ContentFetcher fetcher, StoryParser parser)
    {
        if (null == fetcher) throw new ArgumentNullException("fetcher");
        if (null == parser) throw new ArgumentNullException("parser");
        this.fetcher = fetcher;
        this.parser = parser;
    }
}

As a final step of refactoring, you can extract the preparing code to its own method, moving the parsed stories to be part of the class' state:

private Story[] stories = null;

private bool Prepare()
{
    if (this.stories != null) return true;
    string content;
    if (!this.fetcher.TryGetContent(out content)) return false;
    Story[] temp;
    if (!this.parser.TryParse(out temp)) return false;
    this.stories = temp;
    return true;
}

public Story[] Getstories()
{
    if (!Prepare()) return null;
    return GetMostRecent5(this.stories, new StoryComparer());
}

Good luck!

StackExchange Code Review Q#15942, answer score: 12