Method for returning valid URLs from a sitemap URL I need a method which fetches all of the text from a URL (generally a sitemap URL), and returns an IEnumerable of all valid URLs contained in the text returned from the initial address. What I have so far is:

public IEnumerable&lt;Uri&gt; GetSitemapUrls(Uri sitemapUrl)
{
    var sitemapText = GetSitemapText(sitemapUrl);

    if (string.IsNullOrWhiteSpace(sitemapText))
        yield break;

    var urls = new List&lt;string&gt;();

    var urlRegex = new Regex(@"\b(?:https?://|www\.)[^ \f\n\r\t\v\]]+\b", RegexOptions.Compiled | RegexOptions.IgnoreCase);

    foreach (Match m in urlRegex.Matches(sitemapText))
        urls.Add(CleanUriString(m.Value));

    foreach (var url in urls)
    {
        var cleanedUriString = CleanUriString(url);

        if (Uri.IsWellFormedUriString(cleanedUriString, UriKind.RelativeOrAbsolute))
            yield return new Uri(cleanedUriString);
    }
}

string GetSitemapText(Uri sitemapUri)
{
    var wc = new WebClient
    {
        Encoding = System.Text.Encoding.UTF8
    };

    return wc.DownloadString(sitemapUri);
}

string CleanUriString(string dirtyUriString)
{
    var legalCharacters = @"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~:/?#[]@!$&amp;'()*+,;=".ToCharArray();

    var cleanedString = dirtyUriString;

    foreach (var character in dirtyUriString)
    {
        var matchIndex = dirtyUriString.IndexOf(character);

        if (!legalCharacters.Any(x =&gt; x.Equals(character)) &amp;&amp; matchIndex &gt; 0)
            cleanedString = dirtyUriString.Substring(0, matchIndex);
    }

    return cleanedString;
}


It seems to work as intended for actual sitemaps, null/empty responses from the URL it receives, and URLs with URL-illegal characters in them. I have a feeling I'm missing out on potential issues or letting bad URLs through anyway, not to mention I haven't thought about spidering through URLs that the initial sitemap (if it is a sitemap) returns.

Is there anything I can do to improve it?
Just a couple of thougths...




if (string.IsNullOrWhiteSpace(sitemapText))
  yield break;



This is a very unusual handling of null/empty argument values and should be documented. A more meaniningful name like GetSitemapUrlsOrDefault would also help. Without it the expected way is to throw an ArgumentException.




foreach (Match m in urlRegex.Matches(sitemapText))
  urls.Add(CleanUriString(m.Value));

foreach (var url in urls)
{
  var cleanedUriString = CleanUriString(url);

  if (Uri.IsWellFormedUriString(cleanedUriString, UriKind.RelativeOrAbsolute))
      yield return new Uri(cleanedUriString);
}



You don't need all this. Instead you can build a nice query that does everything in one go and in a single run:

return 
    urlRegex
        .Matches(sitemapText)
        .Cast&lt;Match&gt;()
        .Select(m =&gt; CleanUriString(m.Value))
        .Where(cleanedUriString =&gt; Uri.IsWellFormedUriString(cleanedUriString, UriKind.RelativeOrAbsolute))
        .Select(cleanedUriString =&gt; new Uri(cleanedUriString));


or alternatively:

return
    from m in urlRegex.Matches(sitemapText).Cast&lt;Match&gt;()
    let cleanedUriString = CleanUriString(m.Value)
    where Uri.IsWellFormedUriString(cleanedUriString, UriKind.RelativeOrAbsolute)
    select new Uri(cleanedUriString);




You don't need the .ToCharArray(); on the legalCharacters string. A string is already a char-array.



Oh, and of course the WebClient needs to be disposed. Wrapping it with a using would be good.