Checking URL validity

Question

This is sort of a follow up to a previous question (Do I have any SQL leaks?).

I have changed some things around and am working on a solution where I use Active Directory to gather information about the user rather than use the user table from the application that is run on these App Servers (the thing I am trying to keep track of).

I think that this method might be better utilized if it were written differently:

public bool isValid(string url)
{
    HttpWebRequest urlReq;
    HttpWebResponse urlRes;
    try
    {
        urlReq = (HttpWebRequest)WebRequest.Create(url);
        urlReq.Method = "HEAD";
        urlReq.Timeout = 200000;
        urlRes = (HttpWebResponse)urlReq.GetResponse();
        urlRes.Close();
        return true;
    }
    catch (Exception ex)
    {
        //Url not valid
        strErrorMSG = "Exception From isValid Method. Exception to follow: " + ex.ToString();
        return false;
    }
}

And this is how I am currently using it:

if (isValid(strInput))
{
    try
    {
        XmlReader xmlReader = XmlReader.Create(strInput);
        using (xmlReader)
        {
            while (xmlReader.Read())
            {
                if (xmlReader.NodeType == XmlNodeType.Text)
                {
                    strServer = xmlReader.Value.ToString();
                    strServer = strServer.Replace("\r", "");
                    strServer = strServer.Replace("\n", "");
                    strServer = strServer.Replace(" ", "");
                }
            }
            xmlReader.Close();
        }
    }
    catch (Exception e)
    {
        strServer = "Error";
        strErrorMSG = "Error with the xmlReader Exception as follows: " + e.ToString();
    }
}
else
{
    strServer = "XML-Missing";
}

I take this information and then I store it in an SQL table where I pull the information from it to populate a data grid on a website so I can see how many users are on each app server (using the App Server indirectly by running the application).

The variables strServer and strErrorMSG (are horrible names I know, I may refactor that as well while I am working on the version) get inserted into an SQL table so that I can see that there is an error with the Windows Service and have an error message that I can grab.

One of the things on my to-do list is to add a function that will email me with errors should the SQL Server not be accessible from the client machine.

I only use 1 SQL table at the moment for all the information that is gathered with this Windows service, another reason why I am working on the e-mail functionality.

Note: This is code from a Windows service that runs on almost 1000 computers.

How does my boolean method look, and can I tweak this somehow to make it work with the XmlReader more efficiently (or even at all)?

---

I created this Windows service to track which AppServer a user's application is connected to via a load balancer.

I install an XML on each AppServer with their own unique "server" node(which is pretty much the only thing there), the windows service needs to query that XML and return the value in that node giving it the server that the application is connected to. After that the Windows service sends the information to a SQL table so that it can be viewed via a website.

The reason for the weird logging is because this service is running on somewhere around 1000 computers in the state, and I am already sending information to the SQL Table, where the information is being displayed on the website(what we monitor frequently) so this is the fastest way at the moment for seeing the error messages. I am looking into a way where I can have the exceptions E-mailed to me instead of only sending to the Table so that I don't have to stare at the site all the time.

I agree that I don't like the isValid method and that it creates an extra request. What I am looking for is a way to merge it with the XmlReader and some of you have given me really good ideas how to accomplish this and I appreciate it greatly.

Answer 1

It appears that you're using a class-level variable (strErrorMSG) to retain your error information.

That is a style of programming that I highly discourage, where values are essentially "passed" between methods inside of a class using class-level variables instead of actual parameters.

The reason that I discourage this style of programming is that I have had many experiences where I've had to maintain some existing code written this way. Generally problems are much more difficult to diagnose, because when I read through the code I have to keep in mind all of these nebulous class-level variables: where those variables are being set, where those variables are being used, what the call structure of the methods inside of the class is, etc. The result is a very large mental map of the entire class that I have to reconstruct every time that I have to work on that class, which makes diagnosing problems slow and tedious. Additionally, the hidden interdependencies between methods caused by these class-level variables usually makes the code difficult to refactor without significant risk of breaking something.

In your case, the code you've presented is fairly simple and does not appear to be difficult to read. Even so, I think the practice that I've described above should be avoided whenever possible. I guess I can't say for sure if that's what you're doing, but it appears to be.

Please note, I'm not saying that class-level variables shouldn't be used. What I'm saying is that sometimes people use class-level variables as a shortcut to pass data from one method to another internally within a class, as a way to avoid complicating their method signatures, and that is what I am against. Variables used in that way are not truly part of the state of the object; their values only have meaning at certain times during execution of the class's methods.

So, the question that I have is, what are you doing with that strErrorMSG variable? Are you just logging it? Assuming that is the case, here are a few alternatives:

1. Create a Log method inside of your class. Inside of your catch blocks call the Log method. Inside of the Log method, do whatever logging it is that you're doing now with that strErrorMSG variable.

2. Use Dependency Injection techniques to give your class an ILogger object, which it can then use to log messages. The advantage of this is that your class doesn't have to care about the details of how to log something; that's the responsibility of the ILogger object. Inside of your catch blocks call whatever "Log" method is available on the ILogger object.

3. Add an errorMessage output parameter to your isValid method. Keep in mind that most people are not fond of output parameters, so this option is not likely to be popular with other developers.

4. Return an object from your isValid method that can contain both the result (your current true/false return value) and optionally an error message, in the case where an error occurred. This option is roughly equivalent to #3, but is probably less unpopular with other developers just because it doesn't use an output parameter. Arguably, it's just as cumbersome, though, if not more so.

You also seem to have a strServer variable, which adds a bit of complication. In the code that you have written, is there a reason why it made more sense to assign strServer the value of "XML-Missing" where it is now instead of doing that inside of the isValid method? If you can move that into the isValid method, then that might simplify refactoring the code to one of the options I've mentioned above.

Answer 2

It can be tough, but pretend you're not the developer who wrote this code and try to understand what it does by reading through it. At a quick glance some questions you might ask could be:

• What does isValid actually check the validity of?
• What is strInput? The line isValid(strInput) just tells me that someone has deemed some input string valid for doing something. Not very useful, right?
• When there is an exception in the XMLReader portion, what does it mean to just set the exception text to a random set of instance variables and continue on like nothing happened?

Some quick fixes might be to just use better variable/method names such as: isResolvableUrl(url) and inputXmlUrl.

On to your actual code...it's a very bad practice to use try/catch blocks for flow control. Exceptions should be exceptional (see: When to throw an exception)! Right now, you're just brushing exceptions under the carpet, which makes the rest of your application unable to properly handle them.

Think about the actual flow of your application in an ideal case. First, you make a request to verify if the file is there, which has network cost. Then, you use the same url to create an XmlReader which has to read from there again! Since you're catching all exceptions, you're better off not doing a check to see if it'll fail and just letting it fail without doing the check in the first place.

If each of your methods had a single identifiable functionality, you'll find you don't need all the try/catch blocks:

public XmlReader GetXmlFromRemoteLocation(string xmlLocation)
{ }

public void ReplaceWhitespace(XmlReader xml)
{ }

public void WhateverYourMainMethodIs()
{
    // Let it throw Exceptions that can be caught!
}

// Alternatively (if you must):
public bool TryWhateverYourMainMethodIs()
{
    try { }
    catch ( ExpectedExceptionTypes )
    { }

    return success;
}

Answer 3

It's not clear what "isValid" means. Does it mean "valid" in the XML sense of the word? You didn't provide enough context for us to tell (e.g. what class or package this method appears in).

It's also unclear what the point of isValid() is. You make an HTTP HEAD request, but never check the urlRes.StatusCode. Was there a server-side error? Did the request require authentication that you didn't provide? Was there an invalid SSL certificate? My guess is that you will be able to detect network problems, and you will be able to tell if the server is down, but you throw away a lot of other information.

But why bother calling isValid() at all? It just makes a request for the sake of possibly catching an exception. You might as well go ahead with XmlReader.Create() and handle any exception there. Even if the HTTP request of isValid() succeeds, there's no guarantee that it will succeed again when the XmlReader makes its request.

Finally, I find your exception handler odd. It appears that you are using the strServer to store the data you extract from the XML document, but also to store an error message instead on failure. Depending on your application, you may want to display an error dialog, log the error for diagnostics, explicitly rethrow the exception, or just let it propagate. Corrupting your data with an error message is just weird.

Answer 4

HttpWebResponse implements IDisposable, so ideally it should be inside a using.

---

There is a const for the Method property: System.Net.WebRequestMethods.Http contains fields like Head and Post etc.

---

Why do you do XmlReader xmlReader = XmlReader.Create(strInput); and then using (xmlReader) on the next line? Do it properly and combine the two:

using (var xmlReader = XmlReader.Create(strInput))

---

There is no reason to do xmlReader.Close();, the using does that for you.

Answer 5

I would vote to get rid of the whole IsValid() function, simply because it is not required. Its only purpose in your above code is to make a costly web check on if the URL can be accessed. This can also be accomplished where you do an XmlReader.Create() and then attempt to read off it -- if the URL cannot be accessed your XmlReader calls will fail, and that you are handling using your strange try-catch block. By doing this, you not only gain a few milliseconds of performance in your Windows service, but also reduce that much of load on your webserver by avoiding these extra calls.

By the way, if all you wanted to know was a server name, why do all the above stuff? There are much easier ways to get a target server name.