ResourceReaderRefactor
Raul post in his blog (eXtreme Programming. Net) a few lines of code demonstrating how to read embedded resources from one assembly.
https://blogs.clearscreen.com/ragc/archive/2004/06/09/316.aspx
Today we have had a lunch (at 16:300 we are in Spain ;-) and we discussed about this post, because I replied looking for a better way to get the resource without the loop, he thought my comment was about performance, and we will never have a big number of resources so the loop is not a performance problem.
What I said is about “Clean Code that Works”, not performance, in the code I see too many statements:
1. looking for the resource name with a loop though all the resources
2. compare the resource name
3. if found try to load as stream
4. if not found throw an exception
And here is the original code
public string GetEmbeddedResourceContent(string resourceFileName)
{
Assembly myAsembly = Assembly.GetExecutingAssembly();
string resName = String.Empty;
foreach(string resourceName in
myAsembly.GetManifestResourceNames())
{
if (resourceName.IndexOf(resourceFileName)>=0)
{
resName = resourceName;
break;
}
}
if (resName.Length > 0)
{
Stream s = myAsembly.GetManifestResourceStream(resName);
StreamReader sr = new StreamReader(s,
System.Text.Encoding.GetEncoding(1252));
return sr.ReadToEnd();
}
else
{
throw new Exception("The resource can't be found");
}
}
Before to start refactor this method I need a test:
[TestFixture]
public class ResourceTextReaderTest
{
ResourceTextReader reader;
[SetUp]
public void SetUp()
{
reader = new ResourceTextReader();
}
[Test]
public void GetEmbeddedResourceContent()
{
Assert.AreEqual("Hello",
reader.GetEmbeddedResourceContent("File.txt"));
}
[Test, ExpectedException(typeof(Exception))]
public void GetEmbeddedResourceContent_NotFound()
{
Assert.AreEqual("Hello",
reader.GetEmbeddedResourceContent("FileNotFound.txt"));
}
}
The fist thing I would change is the method name, and because this is a sample, I added two new tests instead of change the old ones, before writing the new code:
[Test]
public void ReadResourceFromAssembly()
{
Assert.AreEqual("Hello",
reader.ReadResourceFromAssembly("File.txt"));
}
[Test, ExpectedException(typeof(Exception))]
public void ReadResourceFromAssembly_NotFound()
{
Assert.AreEqual("Hello",
reader.ReadResourceFromAssembly("FileNotFound.txt"));
}
Now I have red bar, so I can start to implement my method.
public string ReadResourceFromAssembly(string resourceFileName)
{
Assembly a = Assembly.GetExecutingAssembly();
string result = string.Empty;
Stream s = a.GetManifestResourceStream("ResReader." +
resourceFileName);
using (StreamReader sr = new StreamReader(s))
{
result = sr.ReadToEnd();
}
return result;
}
Note the use of the “using” clause to avoid close the stream manually.
But I still have a red bar because the test ReadResourceFromAssembly_NotFound expects an exception of type “Exception” and I’m getting a ArgumentNullException because the stream returned is null.
For users of my method this exception is not very informative, so I’m going to check if the stream is null, and throw a more explanatory exception.
Here it is the final version code (with green bar of couse ;-)
public string ReadResourceFromAssembly(string resourceFileName)
{
Assembly a = Assembly.GetExecutingAssembly();
string result = string.Empty;
Stream s = a.GetManifestResourceStream("ResReader." +
resourceFileName);
if (null==s)
{
throw new Exception("Resource " + resourceFileName + " not found
in assembly " + a.FullName);
}
using (StreamReader sr = new StreamReader(s))
{
result = sr.ReadToEnd();
}
return result;
}
Final Conclusion.
Readers probably are thinking if all this work was really needed, I have reduced the lines of code from 16 to 12 not a big reduction, however this code works in the same way, but the readability has been improved and the complexity has been reduced from 8 to 4 (devMetrics report).
Finally the main advantage is that I don’t need to loop to throw the exception, if the returned stream is null is enough to throw.