Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 a SQL table where I pull the information from 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 a 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.

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

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

share|improve this question
5  
EEEEK!! Hungarian!!! –  Mat's Mug Jun 13 at 15:51
    
IKR @Mat'sMug I am working on that too. it was one of the first projects I worked on here. lol –  Malachi Jun 13 at 15:52
    
Are the two snippets in the same class? –  Mat's Mug Jun 13 at 17:38
    
@Mat'sMug yes, it is a small windows service. I am creating an Email sender method right now, it didn't seem reasonable to make a mailer class just for this small service, although I wish I had time to. –  Malachi Jun 13 at 17:43
    
if you need any more clarity on this I can be pinged (use @malachi) in The Second Monitor –  Malachi Jun 16 at 15:15

4 Answers 4

up vote 8 down vote accepted

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.

share|improve this answer
    
I will clarify more when I am back at work, but this is a Windows Service that runs on every machine in the network that uses a specific Network based application. I should have explained what this code does a lot better. –  Malachi Jun 13 at 23:49

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;
}
share|improve this answer
    
the big reason why I do a lot of the weird stuff is that I don't want this Service to alert the user that it has failed. the users probably don't know this service is on their machines, so I want a way that it will send me the exception information and then when it runs again it may clear up, most of what I have seen so far is network lag. does that make sense in a bad sort of way? –  Malachi Jun 13 at 19:46
1  
That makes sense, but you should at least move it out of the core methods and leave the "protect the users" code to do the exception handling at the very top level. That way anything in between can properly handle relevant exceptions when they occur. –  Ocelot20 Jun 13 at 19:49
    
instead of catching exceptions I should code for conditions. the ones that I know will happen, like where I have "missing XML" do more stuff like that, and then let the real Exceptions bubble up and catch them once where they can be reported to the Table or to a log. –  Malachi Jun 13 at 20:12

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.

share|improve this answer
    
this is a windows service that resides on each computer in the network that uses a certain application, so I have an interface that displays the information gathered from all these windows services on all the computers, so when one of them isn't getting a response from the URL I want to know about it and it displays on the interface. all of the answers have been giving me good advice and ideas of things that I can change about this service. –  Malachi Jun 13 at 23:45
    
The XML only holds one small piece of information, a server name. it only has that one node. the data is a less than 10 character string. so if there is no XML there is something wrong with either the Load Balancer or the windows service. –  Malachi Jun 13 at 23:47
    
"XML-Missing" is technically a valid hostname. –  200_success Jun 13 at 23:53
    
I am trying to see if the XML file Exists on the Server... don't know if I said that already or not. –  Malachi Jun 16 at 15:32

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.

share|improve this answer
    
it goes through a Load balancer to grab a specific app server in the group of balanced servers –  Malachi Jun 14 at 23:19

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.