Return to Answer

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;
}

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;
}