Posts Tagged cleancode

ObservableCollection is a Leaky Abstraction

And so is BindingList. Or, rather, exposing them through your controller’s interface is a Leaky Abstraction.

Anyone who’s done any Windows desktop development has eventually come across ObservableCollection and BindingList. How often have you written a controller (ViewModel or Presenter, doesn’t matter what you call it, they’re all some variant of the Controller in MVC) that looks like this.

    public class LeakyWpfController : INotifyPropertyChanged
    {
        public ObservableCollection<Person> People { get; } = new ObservableCollection<Person>();

        public event PropertyChangedEventHandler PropertyChanged;
    }

Or this

    public class LeakyWinformsController
    {
        public BindingList<Person> People { get; } = new BindingList<Person>();
    }

What’s wrong with this code?! We need to expose a collection type that the framework understands for the databinding to work!

No. We don’t. Both Winforms and WPF don’t care what you expose. They only care that the backing field is of the proper type. All you need to expose in your public interface is an ICollection, in both cases. Databinding to these two controllers works perfectly well.

    public class WpfController : INotifyPropertyChanged
    {
        public ICollection<Person> People { get; } = new ObservableCollection<Person>();

        public event PropertyChangedEventHandler PropertyChanged;
    }

    public class WinformsController
    {
        public ICollection<Person> People { get; } = new BindingList<Person>();
    }

Ok, so big deal. It’s not like I’m ever going to use my ViewModel or Presenter for anything but the specific View they were built for…

Are you so absolutely sure of this? If you’re certain that you’ll never want to use the same presentation logic in more than one window or application, then sure. You’re probably right. Leaking this abstraction doesn’t hurt, but what if we wanted to reuse logic from our old Winforms application in our new WPF app? Or what if we wanted to set ourselves up to migrate away from that old Winforms app into a new WPF application? What do we do when WPF and ObservableCollection aren’t the newest, greatest thing anymore and we’d like to migrate to that newest framework? Wouldn’t it be nice to be able to do so with minimal effort?

Our two controllers are at this point so similar, we can combine them into one. (Thanks to the fact that WPF supports BindingList as a backing field.)


    public class Controller : INotifyPropertyChanged, IController
    {
        public ICollection<Person> People { get; } = new BindingList<Person>();
        public ICommand AddItemCommand = new AddItemCommand(this);

        public event PropertyChangedEventHandler PropertyChanged;
    }

I only recommend using BindingList as a backing field if your Controller (view model/presenter) needs to work in a Winforms environment though. There are some performance issues with BindingList. If you only need to target WPF or UWP, then ObservableCollection is the right way to go. Either way, there’s no reason to expose anything more than an ICollection to the outside world.

, , , , , , ,

Leave a comment

Be Careful with ToList()

I recently came across some code during a review that seemed perfectly fine at first glance, but upon further inspection, had potential to perform terribly. Take a look. What’s wrong with this code?

using (var context = new DbContext())
{
context.SomeTable.Where(t => t.Id == someId)
.GroupBy(t => t.Category)
.Select(tg => new { tg.Category, Profit = tg.Sum(p => p.Profit) })
.ToList()
.ForEach(SaveToDb);
}

Do you see it? It’s easy to miss.

Calling ToList() on the Enumerable materializes the query, iterating over the Enumerable in order to generate the List. Then, just a moment later, we iterate over the List. We’ve potentially doubled the run time of this method. In most cases, likely not a big deal, but what if the List contains thousands of items? We’re doing more work than necessary here.

Fixing this is a simple matter of storing the IEnumerable in a variable and using the traditional foreach syntax instead of the ForEach extension method.

using (var context = new DbContext())
{
var query = context.SomeTable.Where(t => t.Id == someId)
.GroupBy(t => t.Category)
.Select(tg => new { tg.Category, Profit = tg.Sum(p => p.Profit) });

foreach(var item in query)
{
SaveToDb(item);
}
}

It just goes to show that you really have to be paying attention when reviewing (or writing!) code. Some issues can be terribly difficult to spot at a glance.

, , , , , ,

Leave a comment

Replacing simple factories with Func<TResult> delegate

I’ve recently noticed the emergence of a recurring pattern in my code. Each project has the same class in it, and this duplication has really been bothering me. The solution to this repetition just hit me this morning while I was reading Jon Skeet’s “C# In Depth.”

First, let me illustrate the pattern I was using and why it’s a problem.

I’ve been refactoring 1-tiered legacy code into something testable. This has involved creating a data layer class, typically called DataProvider which implements an IDataProvider interface so that I can easily mock out the data access.  Now, because any kind of data access is extremely likely to have unmanaged resources, IDataProvider extends IDisposable.

Read the rest of this entry »

, , , , , , ,

Leave a comment

Comments on Doc Comments

I was reviewing some VBA code over at Code Review Stack Exchange the other day and it got me to thinking about comments. The code wasn’t too bad, but there were so many comments in the code that, well…

I just read over this again and I think the fact that I mention the comments so often really highlights the issue. I barely even looked at the code because I was too distracted by all the comments.

Read the rest of this entry »

, , , , , ,

Leave a comment