Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I've written the following code to determine if an Internet connection is available in mind of this article at Microsoft's technet.

In short, there are two methods of prove:

//using System;
//using System.Linq;
//using System.Net;

public static bool IsInternetAvailable()
{
    try
    {
        string ncsi = new WebClient().DownloadString("http://www.msftncsi.com/ncsi.txt");
        if (ncsi == "Microsoft NCSI")
        {
            return true;
        }
    }
    catch { }
    try
    {
        IPHostEntry iphost = Dns.GetHostEntry("dns.msftncsi.com");
        if (iphost != null)
        {
            foreach (var item in iphost.AddressList)
            {
                if (item.ToString() == "131.107.255.255")
                {
                    return true;
                }
            }
        }
    }
    catch { }
    return false;
}

Is there anything I can improve or is it good as it stands now?

share|improve this question
    
Everything that does I/O should be in an async function and return a Task for the result. Also use the new HttpClient API and not the old WebClient one. – Benjamin Gruenbaum Jan 2 at 23:42
5  
Also, the whole premise of "is internet available" is prone to race conditions - what if the internet becomes unavailable after you test for it? Or is not available when you test but is later? You should ask for forgiveness and not permission and just try to make the request and handle it failing due to no internet. – Benjamin Gruenbaum Jan 2 at 23:44
7  
It is a flawed assumption that the question "is internet available" can be answered with yes or no. There are so many possible scenarios in which you will have partial access to the internet. And you'll never experience a scenario where you have access to all of the internet. A much more relevant question is "do I have access to the service I need", which is best answered by trying to use the service and handle failures. Additionally your code is flawed in depending on a specific return value from a DNS lookup, which could change at any moment. – kasperd Jan 3 at 1:38
    
@BenjaminGruenbaum I can think of a couple of examples where it would be beneficial to check whether internet is available. For example, I'm running a server which has connection problems every once in a while which can only be fixed with a reboot. Asking for forgiveness there is done by detecting the problem and forcing to reboot if it isn't fixed within a timely manner. – Mast Jan 3 at 16:34
up vote 9 down vote accepted

The class WebClient implements IDisposable so it should be disposed after we are finished with it. An excellent way to do that is to use the using statement:

string ncsi;
using (var client = new WebClient())
{
    ncsi = client.DownloadString("http://www.msftncsi.com/ncsi.txt");
}
share|improve this answer
    
Very good approach! I've written this, should I now add my new code as an edit or how would I do that on CodeReview? – cramopy Jan 2 at 19:32
1  
Please check the guideline here What should I do when someone answers my question? – Martin Jan 2 at 19:37

preface: This answer is more of a design/architecture related meta-answer about this type of check and might be seen as missing the point of a code review by some. QUESTIONS about design and architecture are off-topic, but I can't find any meta discussions about ANSWERS that address these.

I think a generic IsInternetAvailable check misses the point. 99% of software that requires internet connectivity realistically only requires connectivity to one server (or one group of servers), i.e. your own servers. Checking that msftncsi is available does not mean that your own backend servers are available. Your own servers might have an outage, or msftncsi might have an outage while your servers are still online.

Moving beyond "Are my servers available", your servers being available doesn't mean that your webservices are functional. Your webservices might not be available while your servers themselves are happily reporting their functional status. Or (less likely) your servers heartbeat monitor is reporting broken, but your webservice still works.

Finally, since webservices are inherently time sensitive and the internet is a fickle mistress precariously balanced network of interconnected and interdependent fragilities, service status can change between the time that you check availability and the time that you need availability.

Because of these remarks, here is my opinion:

  1. A check for IsInternetAvailable that checks 3rd party servers but doesn't check your own servers is like a check for IsMyToiletClogged that checks that the sink drains but doesn't try to flush the toilet.
  2. Check for the availability of the services you need, not the server you need.
  3. Checking the services you need before you actually need them does not guarantee that they will be available when you need them.

Combining these 3, I'd say that an IsInternetAvailable function should be used as a purely academical exercise, especially a generic mutation like this one. If your software requires a certain webservice, just call that webservice when you need it and degrade gracefully if you get an unexpected result. I cannot determine from this question whether this is an academic exercise or intended to be added to a project in development. If this check is intended for actual development, I urge to you reconsider adding the check. If this check is written purely as an academic perspective, I still think that this answer is worth considering.


To give feedback on your implementation:

Catching EVERYTHING in a generic catch{} block is generally discouraged. In your code, there are only a handful of exceptions that could occur, related to the WebClient and Dns classes and the functions within that you call*. At the very least, you should catch Exception so you can log the type and details in case it matters.


*: If we leave out malicious intent, that is. Technically, someone could Man in the Middle your request since it's requesting an http resource and return a string long enough to crash your program. But that's easily mitigated by connecting to an https resource.

share|improve this answer
1  
I anticipated that this might be a controversial answer. Because of this, I have also created a question on the meta (meta.codereview.stackexchange.com/questions/6416/…) to determine what our policy on answers like this is, so we can discuss whether the "no design/architecture" rule only applies to questions or also to answers. – Nzall Jan 2 at 22:36
    
This is the correct answer though - OP's question's premise is inherently wrong. This is valuable code review advice. – Benjamin Gruenbaum Jan 2 at 23:45
2  
I am giving you a downvote because I think you are making a whole lot of assumptions about why this code was written. It seems to me like the reason this code was written was to replicate the built-in check in Microsoft Windows, not to check whether or not a specific server is up. – Simon Forsberg Jan 3 at 0:00
1  
@SimonForsbergMcFeely As I said, it is entirely possible this question was made from an academic perspective. In that case, a good teacher would still explain what I just did: there is a difference between "is this 3rd party site available" versus "is the internet available" versus "is MY site available" versus "does my site WORK". If this question was not written from an academic perspective, my points remain valid. – Nzall Jan 3 at 17:18
    
no need for a man in the middle. a dhcp that gives you a dns that resolves that host to your server that serves that very long string is sufficient. (i.e. no need to manipulate the request, you can go to the root more easily) – njzk2 Jan 4 at 2:33

One thing that I am missing in your code is extensibility. You currently have to modify that method if there would be any changes to the configuration or you wanted to add a third step.

What I would do is to extract an interface called IAvailabilityCheck (or something) with one method:

bool IsAvailable();

Then I would make two classes that implements this interface,

One with the following implementation, for checking an URL: (url and expectedContent are class fields, passed to the constructor)

public bool IsAvailable()
{
    try
    {
        string ncsi = new WebClient().DownloadString(url);
        if (ncsi == expectedContent)
        {
            return true;
        }
    }
    catch { }
    return false;
}

One implementation for DNS: (pass hostname and ip to the constructor and use them as fields)

public bool IsAvailable()
{
    try
    {
        IPHostEntry iphost = Dns.GetHostEntry(hostname);
        if (iphost != null)
        {
            foreach (var item in iphost.AddressList)
            {
                if (item.ToString() == ip)
                {
                    return true;
                }
            }
        }
    }
    catch { }
    return false;
}

Then you could have a List<IAvailabilityCheck>, add a URLChecker and DNSChecker to your list with the appropriate settings, and then loop through the list to see which ones are available.

This approach would also make it easy to build an application that lets you know if for example http://codereview.stackexchange.com is offline but the rest of your internet is online.

share|improve this answer
1  
These empty catches are bugging me; what about moving the final return false into the catch block and adding returning false if the if(s) in the try block fail(s)? – 11684 Jan 2 at 22:37
1  
@11684 I'll be honest with you, those empty catches are bugging me a bit too, but they do seem quite reasonable still (this review also wasn't meant to focus on them). But what you are suggesting will have no real effect whatsoever. – Simon Forsberg Jan 2 at 23:05
    
I can't agree with catch { }. that's way to evil. (and never reasonnable. your future self hates you already) – njzk2 Jan 4 at 2:34
    
also, if you go down this road, the null test is not useful, since the catch will catch it. – njzk2 Jan 4 at 2:35

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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