Below is the almost complete code (stripped a couple DAL calls and anonymized company-specifics) for one of those functionalities. I'm not going to supply DAL and Presentation layer code here, since my concerns are mainly around the Business Logic layer - but for the record I have DAL implemented with Linq to SQL and Presentation taken care of with WPF (which I just started learning). So here it goes:
- If I wanted to write unit tests for this functionality, what would I need to do in order to make the code unit-testable? I have no issues making methods public, as they wouldn't be exposed to COM anyway (because only
IFunctionalityis accessible through COM). Or perhaps I should make them protected, and derive from this class so I could test it (the test wrapper class would expose public methods that call into the protected ones; tests would call those exposed public methods and it could even have dependencies injected into its constructor - am I thinking the right way about this?). - If I wanted to write unit tests for this functionality, I couldn't write a test for the
Executemethod, right? I'd have to test the more specialized code and figure out how to mock the DAL calls? And forCanExecute, I'd need a way to get rid of theCurrentUserdependency and inject some dummy provider that doesn't hit the database nor Active Directory, right? - Is this code "clean"? (easy to read, easy to understand, easy to maintain) I guess the sole fact that I'm asking this, answers for itself... How could it be made cleaner then?
- Are static classes and methods hindering anything? (like
CurrentUser.IsAuthorized,FileDialogHelper.GetOpenFileName, andExcel8OleDbHelper.ImportExcelFile) -MsgBoxlives in the Presentation layer and it aims at replacing the ugly default message box, I think the static class is warranted in that particular case. - Sharp eyes will have noticed I initiate a db transaction, do some work while showing progress (the process can take anywhere between a minute and 2 hours, depending on inputs) and then committing or rolling back the transaction only after the ProgressMsgBox has closed. This leaves the affected tables unnecessarily locked, longer than they need to be. How can I work around this? (hmm now that I'm thinking about it, I could have the ViewModel raise an event when progress completes and handle it in this class to commit or rollback as needed... but I'd have to make data a property of the class... makes sense? Then I guess the functionality class would need to implement
IDisposableand rely on the client code to properly dispose its resources, no?)
EDIT
Following up with the answer I got, here's the refactored class - there's still a parameterless constructor for easy COM-client instantiation, but there's also one that allows unit tests to inject mock-up dependencies:
[ComVisible(true)]
[ClassInterface(ClassInterfaceType.None)]
[ComDefaultInterface(typeof(IFunctionality))]
public class ImportFunctionality : FunctionalityBase
{
private IImporter _importer;
/// <summary>
/// Instantiates a new <see cref="ImportFunctionality"/> with default dependencies.
/// </summary>
public ImportFunctionality()
: this(new CurrentUser(), new Importer(
new ExcelSourceProvider(new FileDialogHelper(), new Excel8OleDbHelper()),
new ImportSettingsProvider(new DataModel(Settings.Default.DefaultDb)))
) { }
/// <summary>
/// Instantiates a new <see cref="ImportFunctionality"/> with specified dependencies.
/// </summary>
/// <param name="user"></param>
/// <param name="importer"></param>
public ImportFunctionality(ICurrentUser user, IImporter importer)
: base(user)
{
_importer = importer;
}
public override void Execute()
{
_importer.DoImport();
}
}
And the new base functionality class, bound to be massively reused:
public abstract class FunctionalityBase : IFunctionality
{
protected ICurrentUser CurrentUser { get; private set; }
public FunctionalityBase(ICurrentUser user)
{
CurrentUser = user;
}
public abstract void Execute();
public virtual bool CanExecute()
{
return CurrentUser.IsAuthorized((this as IFunctionality).AuthId);
}
void IFunctionality.Execute()
{
if ((this as IFunctionality).CanExecute())
{
Execute();
}
else
{
throw new NotAuthorizedException(resx.Functionality_Execute_NotAuthorised);
}
}
bool IFunctionality.CanExecute()
{
return CanExecute();
}
string IFunctionality.AuthId
{
get { return GetType().ToString(); }
}
}
The default implementation for IImporter is where the UI-dependent code now resides:
/// <summary>
/// Encapsulates business logic for the importing functionality.
/// </summary>
public class Importer : IImporter
{
private IExcelSourceProvider _excelDataSourceProvider;
private IImportSettingsProvider _importSettingsProvider;
public Importer(IExcelSourceProvider dataSourceProvider, IImportSettingsProvider settingsProvider)
{
_excelDataSourceProvider = dataSourceProvider;
_importSettingsProvider = settingsProvider;
}
public void DoImport()
{
var xlData = _excelDataSourceProvider.GetExcelSourceData();
if (xlData == null) return;
var viewModel = _importSettingsProvider.GetImportSettings(xlData);
if (viewModel == null) return;
if (!GetUserConfirmation(viewModel)) return;
ImportGridContent(viewModel);
}
...
There's still a little bit of refactoring left to be done in the default IImporter implementation (some more dependencies to be injected rather than instantiated), but overall I think this is exactly where I need to be, and continuing down this road will lead to easily readable, maintainable and testable code. Thanks!
- If I wanted to write unit tests for this functionality, what would I need to do in order to make the code unit-testable? I have no issues making methods public, as they wouldn't be exposed to COM anyway (because only
IFunctionalityis accessible through COM). Or perhaps I should make them protected, and derive from this class so I could test it (the test wrapper class would expose public methods that call into the protected ones; tests would call those exposed public methods and it could even have dependencies injected into its constructor - am I thinking the right way about this?). - If I wanted to write unit tests for this functionality, I couldn't write a test for the
Executemethod, right? I'd have to test the more specialized code and figure out how to mock the DAL calls? And forCanExecute, I'd need a way to get rid of theCurrentUserdependency and inject some dummy provider that doesn't hit the database nor Active Directory, right? - Is this code "clean"? (easy to read, easy to understand, easy to maintain) I guess the sole fact that I'm asking this, answers for itself... How could it be made cleaner then?
- Are static classes and methods hindering anything? (like
CurrentUser.IsAuthorized,FileDialogHelper.GetOpenFileName, andExcel8OleDbHelper.ImportExcelFile) -MsgBoxlives in the Presentation layer and it aims at replacing the ugly default message box, I think the static class is warranted in that particular case. - Sharp eyes will have noticed I initiate a db transaction, do some work while showing progress (the process can take anywhere between a minute and 2 hours, depending on inputs) and then committing or rolling back the transaction only after the ProgressMsgBox has closed. This leaves the affected tables unnecessarily locked, longer than they need to be. How can I work around this? (hmm now that I'm thinking about it, I could have the ViewModel raise an event when progress completes and handle it in this class to commit or rollback as needed... but I'd have to make data a property of the class... makes sense? Then I guess the functionality class would need to implement
IDisposableand rely on the client code to properly dispose its resources, no?)