11
\$\begingroup\$

I decided to rewrite a previous program of mine from scratch. The result is a lot better than the previous one but now that it seems like it is working I want to optimize it (hopefully it is not too early..). Provided a UrlList.txt, it read line by line the urls of a manga page, e.g. http://www.readmanga.today/naruto and downloads everything in parallel. Only the list is gone through synchronous.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Net;
using System.Threading.Tasks;

namespace MangaRipper
{
    internal class Manga
    {
        public string mainUrl { get; private set; }

        public List<string> chapterList { get; private set; }

        public Dictionary<string, List<string>> chapterImages { get; private set; }

        public Manga(string url)
        {
            this.mainUrl = url;
            this.chapterList = new List<string>();
            this.chapterImages = new Dictionary<string, List<string>>();
        }
    }

    internal class Program
    {
        private static string currentWorkingDirectory = Directory.GetCurrentDirectory();
        private static string outputDirectory = "";
        private static int degreeOfParallelism = Environment.ProcessorCount;

        private static void Main(string[] args)
        {
            Stopwatch sw = Stopwatch.StartNew();
            Init();
            List<string> Urls = ReadUrlListFromFile();
            List<Manga> mangaList = new List<Manga>();

            if (Urls != null)
            {
                foreach (var url in Urls)
                {
                    Console.WriteLine(url);
                    mangaList.Add(new Manga(url));
                }

                mangaList = PopulateChapterList(mangaList);
                mangaList = PopulateChapterImages(mangaList);
            }

            // Download
            foreach (var manga in mangaList)
            {
                var mangaName = Directory.CreateDirectory(outputDirectory + @"\" + GetStringUntilSlashFromBack(manga.mainUrl)).Name;
                Parallel.ForEach(manga.chapterList, new ParallelOptions { MaxDegreeOfParallelism = degreeOfParallelism },
                chapter =>
                {
                    var chapterName = Directory.CreateDirectory(outputDirectory + @"\" + mangaName + @"\" + GetStringUntilSlashFromBack(chapter)).Name;

                    Console.WriteLine("starting to download manga {0} - chapter {1}", mangaName, chapterName);
                    Parallel.ForEach(manga.chapterImages[chapter], new ParallelOptions { MaxDegreeOfParallelism = degreeOfParallelism },
                    s =>
                    {
                        using (var client = new WebClient())
                        {
                            if (!File.Exists(outputDirectory + @"\" + mangaName + @"\" + chapterName + @"\" + GetStringUntilSlashFromBack(s)))
                            {
                                //Console.WriteLine("starting to download {0}", s);
                                client.DownloadFile(s, outputDirectory + @"\" + mangaName + @"\" + chapterName + @"\" + GetStringUntilSlashFromBack(s));
                                //Console.WriteLine("finished downloading {0}", s);
                            }
                        }
                    });
                    Console.WriteLine("finished downloading manga {0} - chapter {1}", mangaName, chapterName);
                });
            }

            sw.Stop();
            Console.WriteLine("Time taken: {0}s", sw.Elapsed.Seconds);
            Console.ReadKey();
        }

        #region Helper

        /// <summary>
        /// Returns string from back until first forward slash
        /// </summary>
        /// <param name="str">String containing forward slashes</param>
        /// <returns>String from last forward slash until end</returns>
        private static string GetStringUntilSlashFromBack(string str)
        {
            int index = str.LastIndexOf('/');
            str = str.Substring(index + 1);
            return str;
        }

        /// <summary>
        /// Create an output directory if none exists yet and else set output directory.
        /// </summary>
        private static void Init()
        {
            const string outputDirectoryName = @"\Output";
            if (!Directory.Exists(currentWorkingDirectory + outputDirectoryName))
            {
                outputDirectory = Directory.CreateDirectory(currentWorkingDirectory + outputDirectoryName).Name;
            }
            else
            {
                outputDirectory = currentWorkingDirectory + outputDirectoryName;
            }
        }

        /// <summary>
        /// Read line by line all URLs from inside a txt file.
        /// </summary>
        /// <returns></returns>
        private static List<string> ReadUrlListFromFile()
        {
            string path = currentWorkingDirectory;
            string fileName = @"\UrlList.txt";
            string filePath = path + fileName;

            try
            {
                if (File.Exists(filePath))
                {
                    return File.ReadLines(filePath).ToList();
                }
                else
                {
                    Console.WriteLine("No UrlList.txt found.");
                    return null;
                }
            }
            catch (Exception e)
            {
                Console.WriteLine(e.Message);
            }

            return null;
        }

        /// <summary>
        /// Get all chapters of a manga.
        /// </summary>
        /// <param name="mangas"></param>
        /// <returns></returns>
        private static List<Manga> PopulateChapterList(List<Manga> mangas)
        {
            foreach (var manga in mangas)
            {
                WebClient webClient = new WebClient();
                string source = webClient.DownloadString(manga.mainUrl);
                HtmlAgilityPack.HtmlDocument document = new HtmlAgilityPack.HtmlDocument();
                document.LoadHtml(source);

                // Return URL for each chapter
                var chapterLink = document.DocumentNode.SelectNodes(@"//ul[@class='chp_lst']//li//a/@href").Reverse().ToList();
                Parallel.ForEach(chapterLink, new ParallelOptions { MaxDegreeOfParallelism = degreeOfParallelism },
                s =>
                {
                    manga.chapterList.Add(s.GetAttributeValue("href", "not found"));
                });
            }

            return mangas;
        }

        /// <summary>
        /// Get all pages of a chapter.
        /// </summary>
        /// <param name="mangas"></param>
        /// <returns></returns>
        private static List<Manga> PopulateChapterImages(List<Manga> mangas)
        {
            foreach (var manga in mangas)
            {
                Parallel.For(0, manga.chapterList.Count,
                s =>
                {
                    List<string> imageList = GetAllPagesOfChapter(manga.chapterList[s]);
                    manga.chapterImages.Add(manga.chapterList[s], imageList);
                });
            }

            return mangas;
        }

        /// <summary>
        /// Get url of each page
        /// </summary>
        /// <param name="chapter"></param>
        /// <returns></returns>
        private static List<string> GetAllPagesOfChapter(string chapter)
        {
            chapter = chapter + @"/all-pages";
            List<string> urls = new List<string>();
            WebClient webClient = new WebClient();
            string source = webClient.DownloadString(chapter);
            HtmlAgilityPack.HtmlDocument document = new HtmlAgilityPack.HtmlDocument();
            document.LoadHtml(source);
            var imagesUrl = document.DocumentNode.SelectNodes("//div[@class='content-list col-md-12 page_chapter']//img[@class='img-responsive-2']");
            if (imagesUrl != null)
            {
                Parallel.ForEach(imagesUrl, new ParallelOptions { MaxDegreeOfParallelism = degreeOfParallelism },
                s =>
                {
                    urls.Add(s.GetAttributeValue("src", "not found"));
                });
                return urls;
            }
            return null;
        }

        #endregion Helper
    }
}
\$\endgroup\$
12
\$\begingroup\$

Three programmers are ordering Christmas presents online.

The first programmer orders a present, and goes to wait by the mailbox. He waits there for 24 hours until the first present arrives. He takes the present inside, orders a second present, and heads out to wait by the mailbox again. This goes on for several days until all his presents have arrived.

The second programmer sees this and thinks to himself, "How inefficient." He invites some friends around, and they each order one present online. He and his friends go outside together to wait by the mailbox. After 24 hours all the presents have arrived, and the programmer thanks his friends for saving him so much time.

The third programmer orders all his presents, puts on a movie, tidies the house, and makes some eggnog. The next day, when he has a break between chores, he goes outside to check his mailbox and finds that all his presents have arrived.

It's not the perfect analogy, but I hope it gets the idea across.


PopulateChapterList is like the first programmer -- ordering gifts one-at-a-time, and waiting by the mailbox for each one to arrive.

In Main, we have the second programmer, waiting by the mailbox with his friends.

WebClient provides asynchronous methods for downloading files, so you can send off all your orders at once, and then wait for them all to arrive. If you want, you can even do other work in the meantime.

Eric Lippert has a series of blog posts about asynchrony in C#, if you're not familiar with it. I would also recommend reading Stephen Cleary's There is no thread.

\$\endgroup\$
2
  • \$\begingroup\$ thanks for the resources. I will read them. I'm just concerned about the amount of requests if I were not to synchronously do the chapter populating. The server could block my requests or IP address as it could look like a DDoS attack. What do you suggest to do with this background? \$\endgroup\$ – Takeru Dec 16 '15 at 9:21
  • \$\begingroup\$ @Takeru I guess that would depend on whether you want to limit the number of concurrent requests, the number of requests per time period, or both. I'm afraid it's more than I can address in a comment. \$\endgroup\$ – mjolka Dec 16 '15 at 10:17
6
\$\begingroup\$

Classes are internal by default then you do not need to explicitly declare them internal, moreover until you do not need to add a derived class you can mark them sealed.

sealed class Manga { }

Public properties are, usually, in title case then, for example: mainUrl should be MainUrl. Also note that you may simply use Url.

In my opinion Dictionary<string, List<string>> and List<string> are good types for private fields or local variables but class public interface should be much more self-describing. For example Dictionary<string, List<string>> may have its own type:

sealed class ChapterImageCollection
{
    public ChapterImageCollection()
    {
         _items = new Dictionary<string, List<string>>();
    }

    public void Add(string chapterName, string url)
    {
        List<string> list = null;
        if (_items.ContainsKey(chapterName))
        {
            list = _items[chapterName];
        }
        else
        {
            list = new List<string>();
            _items.Add(list);
        }

        list.Add(url);
    }

    private readonly Dictionary<string, List<string>> _items;
}

In ReadUrlListFromFile() you return null in case of error, simply return an empty list. Also you do not need List<string> and you may simply use IEnumerable<string>. You will also drop null-check for its return value. Also you do not need to name a variable xyzList, the fact it is a list is an implementation detail that may change (array? dictionary?), its purpose won't.

var mangas = ReadUrlListFromFile()
    .Select(url => new Manga(url));

PopulateChapterList() and PopulateChapterImages() accepts a list and return that list. It's useful for a fluent interface but you're not doing that then you can drop it (also note that they should be changed to accept an IEnumerable<Manga> as input, in general you should have in input less restrictive interface/base class you need to perform your task).

PopulateChapterList(mangas);
PopulateChapterImages(mangas);

Code block for foreach (var manga in mangas) is way too big and it does too many things. There is no reason you don't split it across multiple functions.

You do not need to set MaxDegreeOfParallelism unless you really know what you're doing. Let framework decides proper value unless you want to set a limit for multiple connections. Some servers do not allow many concurrent connections from same client then you may want to set this value to 2 or 4 (use a const) but for sure it's unrelated to Environment.ProcessorCount.

Do not build paths using string concatenation, you can use Path.Combine().

Do not parse URL using GetStringUntilSlashFromBack(), you can build an Uri object and use its properties to retrieve parts you need (note that you may have an URL like http://www.example.com/naruto/images?width=256&height=128&name=front).

One for all in PopulateChapterList() you use ToList() but there is no reason to do it. Avoid such unnecessary copy and keep IEnumerable<string>.

Also in GetAllPagesOfChapter() you return null in case of error but you may simply return Enumerable<string>.Empty(). Less code (no null-checks) and less unhandled errors (NullReferenceException).

Everywhere you use Parallel.ForEach you're updating your collections in...parallel but you don't have any lock and you're not using any concurrent collection then it will fail. For example using lock (sugar for Monitor):

Parallel.ForEach(imagesUrl, s =>
{
    lock (urls) urls.Add(s.GetAttributeValue("src", "not found"));
});

Also consider to use a collection from System.Collections.Concurrent. Note parallel execution in this case will degrade performance (because code to execute is less than overhead), then code may be simplified to:

urls.AddRange(imageUrls.Select(x => x.GetAttributeValue("src", "not found")));

In general you should carefully check what you want to make parallel. Parallelization comes with a cost and it's not a magic tool to speed-up everything. For example outer foreach loop in PopulateChapterList() makes sense to be parallel but inner loop (where you simply parse some HTML) won't speed-up anything (also here remember to synchronize access to shared resources).

Next step should be to refactor your code little bit to extract classes, now you have a stupid entity Manga and a God class Program (!!!) that does everything else. I can imagine at least a Downloader, an HtmlParser and a InputFileParser classes.

One note about logging. If you delegate to an abstract object Logger (with, so far, one derived class ConsoleLogger) you may update to a UI version of your program without changing a single line of existing code (just adding a - let's say - new ListViewLogger class).

Last final note: error checking. Network operations fail. I/O operations fail. You're not checking for any error but in real world you must do it. Errors (especially network errors) may be temporary. Also read System.Net.WebException... and Delete files older than a date for few examples.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you. I just skimmed through for the moment because of the overwhelming input. I have one question. Is it really needed to use a concurrent collection because in the end the order of how my collections are populated does not matter because they are processed in parallel thus making any order they have before meaningless. Oh, and is the use of IEnumerable<string> really needed because as an interface I would have to declare it with new List<string> anyway later to use it right? \$\endgroup\$ – Takeru Dec 15 '15 at 16:39
  • 1
    \$\begingroup\$ Concurrent collections (or locks) are mandatory when you update them in parallel (for example with Parallel.ForEach). If you don't then simply you will have random run-time exceptions or random missing/duplicated elements. IEnumerable<string> is not mandatory but if you stick with List<string> you have to use ToList() (making copies) and it costs time and memory then...well do it only if (and where) really required keeping your code as much generic as possible. \$\endgroup\$ – Adriano Repetti Dec 15 '15 at 16:42
5
\$\begingroup\$

Paths should be created only by using the System.IO.Path.Combine() method to make sure any slash \ is at the proper place and makes sure that any invalid characters will throw an ArgumentException.


    private static void Init()
    {
        const string outputDirectoryName = @"\Output";
        if (!Directory.Exists(currentWorkingDirectory + outputDirectoryName))
        {
            outputDirectory = Directory.CreateDirectory(currentWorkingDirectory + outputDirectoryName).Name;
        }
        else
        {
            outputDirectory = currentWorkingDirectory + outputDirectoryName;
        }
    }

You don't need to check if the directory exists, just call CreateDirectory() which checks if the directory exists and won't throw an exception.


You are using the result of ReadUrlListFromFile() to enumerate over the entries so an IEnumerable<string> would be the better return type. As a rule of thumb you should always try to code against interfaces and not against concrete implementations.

The method could look like this

    private static IEnumerable<string> ReadUrlListFromFile()
    {
        string fileName = @"\UrlList.txt";
        string filePath = Path.Combine(currentWorkingDirectory, fileName);

        try
        {
            if (File.Exists(filePath))
            {
                return File.ReadLines(filePath).ToList();
            }
            else
            {
                Console.WriteLine("No UrlList.txt found.");
            }
        }
        catch (Exception e)
        {
            Console.WriteLine(e.Message);
        }

        return Enumerable.Empty<string>();
    }  

PopulateChapterList()

Because WebClient implements IDisposable it should be enclosed in a using statement to proper dispose it like so

    private static List<Manga> PopulateChapterList(List<Manga> mangas)
    {
        using (WebClient webClient = new WebClient())
        {
            foreach (var manga in mangas)
            {
                WebClient webClient = new WebClient();
                string source = webClient.DownloadString(manga.mainUrl);

                HtmlAgilityPack.HtmlDocument document = new HtmlAgilityPack.HtmlDocument();
                document.LoadHtml(source);

                // Return URL for each chapter
                var chapterLink = document.DocumentNode.SelectNodes(@"//ul[@class='chp_lst']//li//a/@href").Reverse().ToList();
                Parallel.ForEach(chapterLink, new ParallelOptions { MaxDegreeOfParallelism = degreeOfParallelism },
                s =>
                {
                    manga.chapterList.Add(s.GetAttributeValue("href", "not found"));
                });
            }
        }

        return mangas;
    }

The same is true for GetAllPagesOfChapter() as well.


Commented out code like //Console.WriteLine("finished downloading {0}", s); should be deleted because this dead code only adds noise. If you want to keep track of any code changes you should use a code revision control system like git or subversion.

\$\endgroup\$

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.