HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Free general e-book downloader

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
generaldownloaderbookfree

Problem

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 EbookDownloadUrls { get; set; }

public List BadEbookDownloadUrls { get; set; }

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

this.

Solution

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 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.

Code Snippets

var downloader = new SyncfusionDownloader();
downloader.Download(ebookUrls, downloadDirectoryName);
public interface IEbookDownloader
{
    void Download(IEnumerable<string> ebookUrls, string downloadDirectoryName);
}
public abstract class EbookDownloader : IEbookDownloader
{
    ...
}

Context

StackExchange Code Review Q#156420, answer score: 6

Revisions (0)

No revisions yet.