5
\$\begingroup\$

So I decided to expand a little one-off C# console program I wrote awhile ago to download the free e-book offered by Syncfusion, and decided to also do it for other free e-book publishers... like O'Reilly.

But I now want to make it a generic e-book downloader, and make it easily expandable and maintainable. I plan to open source it on Github, and accept pull requests for any additional free e-book sites.

So currently, there's a main program that prints a menu, and depending on the chosen site, uses a specific "downloader class" to download the html of the page that lists the free e-books, and then uses regex (Yes I know now thanks to this answer that this is not the proper tool, I'm open to advise on C# html parsers) to create a list of direct links to those ebooks, attempt download them, and log any download errors.

I would like input on how to best follow and implement SOLID principles for the downloader classes.

So far I have: An interface to define the most basic behavior/functionality

public interface IEbookDownloader
{
    void Run();
    void OutputLogs();
}

A base downloader class that implements it

public class BaseDownloader : IEbookDownloader
{
    public Uri EbooksUrl { get; private set; }

    public Uri DownloadFolder { get; private set; }

    public bool OvewriteExisting { get; private set; }

    public List<string> EbookDownloadUrls { get; set; }

    public List<string> BadEbookDownloadUrls { get; set; }

    /// <summary>
    /// Instantiates the BaseDownloader class
    /// </summary>
    /// <param name="downloadFolder">The string path where the file should be downloaded to.</param>
    /// <param name="overwrite">Flag that if true forces the downloader ignore existing/found downloaded files in the download folder, and re-download them.</param>
    public BaseDownloader(string ebooksurl, string downloadFolder, bool overwrite = false)
    {
        this.DownloadFolder = Uri.IsWellFormedUriString(downloadFolder, UriKind.RelativeOrAbsolute) ? new Uri(downloadFolder) : null;
        this.OvewriteExisting = overwrite;

        this.EbooksUrl = Uri.IsWellFormedUriString(ebooksurl, UriKind.Absolute) ? new Uri(ebooksurl) : null;

    }

    public void OutputLogs()
    {
        File.WriteAllLines(Path.Combine(this.DownloadFolder.ToString(), "links.txt"), this.EbookDownloadUrls);
        File.WriteAllLines(Path.Combine(this.DownloadFolder.ToString(), "404s.txt"), this.BadEbookDownloadUrls);
    }

    protected void PrepareToDownload()
    {
        // check data make sure we have proper dl destination and dl source url
        if (this.DownloadFolder == null || !Directory.CreateDirectory(this.DownloadFolder.ToString()).Exists)
        {
            throw new DirectoryNotFoundException("There was a problem with the downloads destination folder.");
        }

        if (this.EbooksUrl == null)
        {
            throw new Exception("The url passed is malformed.");
        }

        // Download html page with list of e-books
        var ebookspagehtmlpath = Path.Combine(this.DownloadFolder.ToString(), "ebookspage.html");
        if (!File.Exists(ebookspagehtmlpath) || this.OvewriteExisting)
        {
            using (var wclient = new WebClient())
            {
                wclient.DownloadFile(this.EbooksUrl, ebookspagehtmlpath);
            }
        }

        var pagehtml = File.ReadAllText(ebookspagehtmlpath).Replace("\"", "'");

        // TODO: For this generic one just web-scrape parse/regex html for urls that have: href='anything.[epub|pdf|mobi|zip]'
        // and fill in the BookDownloadUrls list, and the BadBookDownloadUrls
        var matches = Regex.Matches(pagehtml, @"href='(.+\.[epub|pdf|mobi|zip])'", RegexOptions.IgnoreCase);
        if (matches.Count > 0)
        {
            foreach (Match matchgroup in matches)
            {
                string titleslug = string.Empty;
                var cg = matchgroup.Groups[1].Value;
                if (Uri.IsWellFormedUriString(cg, UriKind.Absolute))
                {
                    Console.WriteLine("Will download {0}.", cg);
                    this.EbookDownloadUrls.Add(cg);
                }
            }
        }
    }

    public void Run()
    {
        this.PrepareToDownload();

        // thingy needed for TitleCasing Ebook folder names
        var textinfo = new CultureInfo("en-US", false).TextInfo;

        using (var wclient = new WebClient())
        {
            var ok200count = 0;
            foreach (var link in this.EbookDownloadUrls)
            {
                var urilink = new Uri(link);
                var filename = urilink.Segments.Last();

                var dotindex = filename.LastIndexOf(".");
                var booktitle = textinfo.ToTitleCase(filename.Substring(0, dotindex)).Replace(" ", "_");

                var ebookdir = Path.Combine(this.DownloadFolder.ToString(), booktitle);
                if (!Directory.Exists(ebookdir))
                {
                    Directory.CreateDirectory(ebookdir);
                }

                try // download of ebook
                {
                    var ebookfilepath = Path.Combine(ebookdir, filename);
                    if (!File.Exists(ebookfilepath) || this.OvewriteExisting)
                    {
                        wclient.DownloadFile(link, ebookfilepath);
                    }
                    else
                    {
                        Console.WriteLine("Skipped: File already in {0}", ebookfilepath);
                    }

                    Console.WriteLine(++ok200count + " files Downloaded");
                }
                catch (WebException e)
                {
                    Console.WriteLine("Error! Could not find {0} at {1}", booktitle, link);
                    Console.Write("Client error was: {0}", e.Message);
                    this.BadEbookDownloadUrls.Add(link);
                    continue;
                }
            }

            File.WriteAllLines(Path.Combine(this.DownloadFolder.ToString(), "links.txt"), this.EbookDownloadUrls);
            File.WriteAllLines(Path.Combine(this.DownloadFolder.ToString(), "notfound.txt"), this.BadEbookDownloadUrls);
        }
    }
}

And the original syncfusion downloader class:

using System;

public class SyncfusionDownloader : BaseDownloader
{
    public SyncfusionDownloader(string ebooksurl, string downloadFolder, bool overwrite = false)
        : base(ebooksurl, downloadFolder, overwrite)
    {
    }

    public override void Run()
    {
        // specific steps for Syncfusion's site
    }

}
\$\endgroup\$
  • 1
    \$\begingroup\$ Parse HTML with regex... this answer is also useful ;-) \$\endgroup\$ – Phrancis Feb 27 '17 at 5:25
  • \$\begingroup\$ haha yeah thanks... I've realized every question with regex + html leads to that answer! Though I'm trying to focus more how to properly structure this program... Do you recommend any good C# html parser libs? Is there any built-in ones? I've run across TidyNet and a few others but I'm not sure which to choose \$\endgroup\$ – unknownprotocol Feb 27 '17 at 7:33
  • 1
    \$\begingroup\$ I don't really do much C#, so I wouldn't know. I've read that XML parsers work good for [X]HTML, so you could use the built-in XmlReader perhaps. \$\endgroup\$ – Phrancis Feb 28 '17 at 0:34
6
\$\begingroup\$

Downloader

When I think of an ebook-downloader I see something like this:

var downloader = new SyncfusionDownloader();
downloader.Download(ebookUrls, downloadDirectoryName);

This means I'd like to create the downloader first and then tell it what to download and where to save it. I don't want to create a new downloader for each url or each download directory so the actual interface should be simply:

public interface IEbookDownloader
{
    void Download(IEnumerable<string> ebookUrls, string downloadDirectoryName);
}

It doesn't need to do anything else. Logging should be an internal matter of the downloader.


Next, if you want to provide a base implementation for such a downloader then it should be an abstract class and the name should be like the interface but without the I.

public abstract class EbookDownloader : IEbookDownloader
{
    ...
}

It makes no sense to instantiate it and use it on its own because it cannot download anything yet or rather it shouldn't. The derived SyncfusionDownloader overrides the base's implementation so why is it implemented at all? If the base-downloader is able to download and can be used on its own then give it some concrete name and don't use it as a base class for other downloaders that most likely will override its methods anyway.


SRP

A downloader should only be able to download. If you need to search for urls or parse some websites then you need other classes that specialize in such tasks. Don't put all these responsibilities insinde a downloader. It's not its job.

It might even turn out that you will have only one downloader but mutliple website-parsers or link finders etc.


Logging

The user of the downloader should not need to call the log methods. Logging should be invisible and ideally you should use dependency injection to pass the logger to the downloader. Currently it's not only tightly coupled to the console but also to the file system and even hardcoded file names. This is all bad for testing.

\$\endgroup\$
  • \$\begingroup\$ Got it, thanks a lot. Yeah I think I need to break up the "search for ebook urls from this specific site" from the downloading of them. That way the downloader can be generic and I can have downloader.Download(ebookUrls, downloadDirectoryName); where ebookUrls is just a list of links. \$\endgroup\$ – unknownprotocol Feb 27 '17 at 20:03
4
\$\begingroup\$

I have some issues with your validation.

public class BaseDownloader : IEbookDownloader
{
    public Uri EbooksUrl { get; private set; }
    public Uri DownloadFolder { get; private set; }
    public bool OvewriteExisting { get; private set; }
    public List<string> EbookDownloadUrls { get; set; }
    public List<string> BadEbookDownloadUrls { get; set; }

The properties with private set are only set in the constructor, but PrepareToDownload() is the method that actually throws an exception if they're null. Even though the constructor can intentionally set them to null. That makes no sense. The constructor should set them to valid values, and if it cannot (due to problematic input), it should throw an exception right there and then, telling you what the problem is. Once that's done, there's no longer any need for validation in PrepareToDownload() because the object can only be constructed with valid state.

Meanwhile for the publicly settable properties, there are no null checks - but these are the ones that actually require them since any external code can set them to null! Personally I would make them privately settable.

\$\endgroup\$
  • \$\begingroup\$ Got it yea, I'll fix that. I meant to do some checking in PrepareToDownload() but in my haste to get the base class done I forgot that! \$\endgroup\$ – unknownprotocol Feb 27 '17 at 20:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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