7
\$\begingroup\$

I made a LinkChecker program which checks URLs from a text file and outputs status into another file like this:

http://valid-url.com;OK
http://invalid-url.com;The remote server returned an error: (404) Not Found.

I have about 1400 URLs in the input file and it takes about 12 minutes to run.

I made it work. In what ways can I achieve rightness and fastness?

public class LinkChecker
{
    static void Main()
    {
        string line = string.Empty;
        string inputFile = @"D:\tmp\links.txt";
        StreamReader file = new StreamReader(inputFile);
        string outputFile = @"D:\tmp\out.txt";

        ClearOutputFile(outputFile);

        while(!string.IsNullOrEmpty(line = file.ReadLine()))
        {
            string currentUrlAndStatus = CheckURL(line);
            if(!string.IsNullOrEmpty(currentUrlAndStatus))
            {
                WriteToFile(currentUrlStatus, outputFile);
            }
        }
    }

    private static string CheckURL(string url)
    {
        string status = string.Empty;

        HttpWebRequest request = (HttpWebRequest)WebRequest.Create(url);
        request.Method = "HEAD";
        request.Proxy = null;

        try
        {
            using(HttpWebResponse response = (HttpWebResponse)request.GetResponse())
            {
                status = response.StatusCode.ToString();
            }
        }
        catch (WebException ex)
        {
            status = ex.Status;
        }

        return url + ";" + status;
    }

    private static void ClearOutputFile(string file)
    {
        using (Filestream fileStream = File.Open(file, FileMode.Open)
        {
            fileStream.SetLength(0);
        }
    }

    private static void WriteToFile(string message, string filename)
    {
        using(var writer = new StreamWriter(filename, true))
        {
            writer.WriteLine(message);
        }
    }
}
\$\endgroup\$
5
\$\begingroup\$

You forgot using using in your StreamReader

    using(StreamReader file = new StreamReader(inputFile)){
    string outputFile = @"D:\tmp\out.txt";

    ClearOutputFile(outputFile);

    while(!string.IsNullOrEmpty(line = file.ReadLine()))
    {
        string currentUrlAndStatus = CheckURL(line);
        if(!string.IsNullOrEmpty(currentUrlAndStatus))
        {
            WriteToFile(currentUrlStatus, outputFile);
        }
    }
  }

Use the var keyword for local variables, let the compiler figure out the type for you. Type inference helps in refactoring.

 var line = string.Empty;
 var inputFile = @"D:\tmp\links.txt";
 using(var file = new StreamReader(inputFile)){
 //...
 }
 var outputFile = @"D:\tmp\out.txt";
 // etc..
\$\endgroup\$
  • 2
    \$\begingroup\$ Microsoft has conflicting recommendations about implicit typing. Could you explain how it helps to refactor? \$\endgroup\$ – RubberDuck Jul 1 '14 at 0:55
  • \$\begingroup\$ I've found that using var everywhere makes the code less readable. Sure, the compiler is good at inferring the proper type, but I'm not. And don't most refactoring tools update the type anyway? \$\endgroup\$ – Snowbody Jul 1 '14 at 14:20
  • \$\begingroup\$ @Snowbody Yes, refactoring tools do update the type, but this requires changing the text in the file, which means a merge to your source control and I don't think you are a massive fan. \$\endgroup\$ – Sleiman Jneidi Jul 1 '14 at 14:24
  • \$\begingroup\$ @SleimanJneidi but the variable did change, so what's the problem of getting a line change in source control? I agree with Snowbody, that var is best used only sometimes, and that most times the type should be explicit. \$\endgroup\$ – ANeves Jul 3 '14 at 9:36
  • \$\begingroup\$ @ANeves var x = GetX(); if we decide to change the Type of X, then nothing will change in the file.However, object x = GetX(); requires change, if the type is not object anymore, does this make sense??? \$\endgroup\$ – Sleiman Jneidi Jul 3 '14 at 9:43
5
\$\begingroup\$

It takes 12 minutes because it has to wait for a request response on every URL, one by one. If you want it to go faster, it's better to load all URLs from your file into an array of strings first, then run a number of threads so you can check multiple URLs simultaneously. Wait for all threads to finish before writing it back to a file.

More on threading in C# here: http://msdn.microsoft.com/en-us/library/aa645740%28v=vs.71%29.aspx

\$\endgroup\$
  • 7
    \$\begingroup\$ Actually, you should almost never use raw Threads. Use Tasks or other parts of TPL instead, they are easier to work with and are also generally more efficient. \$\endgroup\$ – svick Jun 29 '14 at 18:05
  • 4
    \$\begingroup\$ Instead of threads, it might be better to use async web requests, as described here and here. \$\endgroup\$ – Ilmari Karonen Jun 29 '14 at 18:06
  • \$\begingroup\$ Async web requests are definitely the superior option here, but I feel knowledge on the subject of threading and how to work with it is overall pretty useful. \$\endgroup\$ – lvk Jun 29 '14 at 18:12
  • 1
    \$\begingroup\$ @lvk: the knowledge being useful does not make it the right approach to use; knowledge on recursion is also useful, would you use it here? ;) \$\endgroup\$ – ANeves Jul 1 '14 at 9:45
  • \$\begingroup\$ @IlmariKaronen you really ought to have posted that as an answer; it'd be quite useful. \$\endgroup\$ – ANeves Jul 1 '14 at 9:54
4
\$\begingroup\$

Read entire file at first then start async web requests, it shouldn't take more than a minute or two. Time depends on file size, your and server's bandwidth etc.

Why Async? There are servers slower/faster than others, or even some servers may return a timeout which will take "much time", to avoid them you should start web requests async. and handle their completed event to write results to file. IMFO writing line by line or first in-memory and flush will effect performance

\$\endgroup\$
3
\$\begingroup\$

Generally IO is the slowest bottlenecks in programming, reading the file from the harddisk and making the web requests.

For the file itself, I would recommend reading the entire file as one operation, then splitting it into an array of lines that you can then check each.

For the web-requests, I would do what the others have suggested, make threads/tasks so you can call several async requests at the same time. I feel this will have the biggest impact on speed of your program. Unfortunately it is not as simple as creating as many tasks as you can and let them all run at once. Each core in your cpu can handle a single task, and it will cycle through them based on their priority and other tasks waiting to be worked on, giving each a bit of cpu cycles before moving onto the next. I would suggest starting with 2-4 threads/tasks and see if increasing or decreasing them helps performance. Using the Task Parallel Library (suggested by Snowbody) can be a useful way of automating this.

As ANeves points out, Web requests will likely be your main culprit for speed issues since they can take seconds to come back with an answer (or 30+ seconds if waiting for them to timeout).

A good way to test this is to use the StopWatch class and make a test project that calls your class numerous times and logs the average results.

Write the sum of all the results to the file at the same time when completed.

Those will likely have the biggest impact (I would focus on the web requests first, then file IO operations next). Other optimizations may help as well, but likely very small impacts or none at all, such as using StringBuilder vs string for the string concatenations. Also other improvements depend on the data. If likely to have duplicate URL's in the data, it might be worth checking against a list of already processed ones to save you time. Another possibility is if they could have badly formed url's, IO will still likely be slower than checking against an in-memory list. Hope it helps!

\$\endgroup\$
  • \$\begingroup\$ -1 Your answer seems to imply that reading 1400 lines from a file and writing 1400 lines to a file is taking a good part of 12 minutes... which is not correct! Otherwise it is a helpful answer, and I would upvote it if you fixed this. \$\endgroup\$ – ANeves Jul 1 '14 at 10:05
  • \$\begingroup\$ The Task Parallel Library does a good job of determining how many threads to use and how many tasks to be running simultaneously. Don't reinvent the wheel. \$\endgroup\$ – Snowbody Jul 1 '14 at 14:21
  • \$\begingroup\$ @ANeves Sorry I should have been more clear, the web requests will by far take more time. In general IO operations are the slowest, but anything over the web will be much slower than reading from a disk (which is much slower than reading from RAM). I was stating the the combination of the two is likely the biggest gains in performance, but I did not mean to imply that reading/writing to a harddrive is comparable to a web request. I'll update the answer, thanks :) \$\endgroup\$ – Steven Kohus Jul 2 '14 at 19:45
  • \$\begingroup\$ @Snowbody Good suggestion =). It is a nice library I haven't had much opportunity to work with yet. \$\endgroup\$ – Steven Kohus Jul 2 '14 at 19:56
2
\$\begingroup\$

With the parallelization I suggest below I expect the process to get fast enough that async does not matter much.
But @IlmariKaronen's suggestion is very good too:

Instead of threads, it might be better to use async web requests, as described here and here.


My suggestions follow.

Parallelization

You can do some parallelization with Parallel.ForEach. As simple as it gets.

string[] urls = File.ReadAllLines(inputFile);
Parallel.ForEach(urls, url =>
{
    // Probe.
});

Output

Although IO is usually the heaviest, writing 1400 lines to file is not what is taking 12 minutes. Still, you don't need to keep the file open and write as you go.

Instead, you can write to a StringBuilder sb, and in the end simply use File.WriteAllText(outputFile, sb.ToString());.

This method creates or overwrites the file, so it will have the additional improvement that you can remove the method ClearOutputFile.

Misc

  • This if is never false because your method always returns something (at least :<status>), so you can remove it: if(!string.IsNullOrEmpty(currentUrlAndStatus))
  • You can make the variable status a HttpStatusCode instead.
  • When you use streams you should dispose them... using using is the correct and idiomatic way to do it.
\$\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.