Looking over a project I found the following code:
public class AzureLocalStorageTraceListener : XmlWriterTraceListener
{
public AzureLocalStorageTraceListener()
: base(Path.Combine(AzureLocalStorageTraceListener.GetLogDirectory().Path, "WebService.svclog"))
{
}
public static DirectoryConfiguration GetLogDirectory()
{
DirectoryConfiguration directory = new DirectoryConfiguration();
directory.Container = "wad-tracefiles";
directory.DirectoryQuotaInMB = 10;
directory.Path = RoleEnvironment.GetLocalResource("WebService.svclog").RootPath;
return directory;
}
}
My attention was attracted by two things. First, was that we had a method called GetLogDirectory that returns the directory configuration for the logs. This method don’t has any custom logic there, only read a configuration from a specific location and retrieves an object populated with specific information. In this case we could have a property called LogDirectoryConfigurationpublic class AzureLocalStorageTraceListener : XmlWriterTraceListener
{
private const string WebServiceConfigurationKey = "WebService.svclog";
public AzureLocalStorageTraceListener()
: base(Path.Combine(LogDirectoryConfiguration.Path, WebServiceConfigurationKey))
{
}
private static DirectoryConfiguration LogDirectoryConfiguration
{
get
{
DirectoryConfiguration directory = new DirectoryConfiguration
{
Container = "wad-tracefiles",
DirectoryQuotaInMB = 10,
Path = RoleEnvironment.GetLocalResource(WebServiceConfigurationKey).RootPath
};
return directory;
}
}
}
Next things is that we return a DirectoryConfiguration and we end up using only the path. Well, why we don’t return directly the string path of the directory.public class AzureLocalStorageTraceListener : XmlWriterTraceListener
{
private const string WebServiceConfigurationKey = "WebService.svclog";
public AzureLocalStorageTraceListener()
: base(Path.Combine(LogDirectoryPath, WebServiceConfigurationKey))
{
}
private static string LogDirectoryPath
{
get
{
DirectoryConfiguration directory = new DirectoryConfiguration
{
Container = "wad-tracefiles",
DirectoryQuotaInMB = 10,
Path = RoleEnvironment.GetLocalResource(WebServiceConfigurationKey).RootPath
};
return directory.Path;
}
}
}
Now, if we log in our new property will notify that even if we create a directory configuration, we only use the path from it, which is read from configuration. Because of this we don’t need to create an instance of DirectoryConfiguration, we can return directly the RootPath.public class AzureLocalStorageTraceListener : XmlWriterTraceListener
{
private const string WebServiceConfigurationKey = "WebService.svclog";
public AzureLocalStorageTraceListener()
: base(Path.Combine(LogDirectoryPath, WebServiceConfigurationKey))
{
}
private static string LogDirectoryPath
{
get
{
return RoleEnvironment.GetLocalResource(WebServiceConfigurationKey).RootPath
}
}
}
Now, we could even delete the property and make directly the call from the constructor, but personally I prefer to keep the call in a separate place. I easier to read (personal opinion).
Next refactoring :) - if the property is accessed only from within it's own class, why keep it public?
ReplyDeleteThank you Tudor. This is happening when you rewrite the sample code in Notepad++. I wanted to write private, I already made the update.
DeleteThe important thing here is what GetLocalResource does. If it is an expensive (or potentially expensive) operation, exposing it like a property is not a great idea.
ReplyDeleteWhen I read a property, I don't expect it to have any performance impact. I also don't expect it to throw an exception.
If there is some resource access (reading from the DB, file system, calculating something expensive) then I would choose to expose it as GetSomething().
100% True. I didn't thought about properties from this perspective. :-)
Delete