I'm going to pre-empt anyone who was thinking of the third option that is even worse than number 2 (but probably what I would have done a year ago :).

Replacing List<Address> with a custom collection that ignores nulls when added.

This is bad. Don't do it.

You could use a derived AddressList : List<Address> that didn't ignore nulls on add, but did have a "RemoveEmpties" method that hid the delegeate.

Or had a method "AddNonNull(params Address[] items)" that did the null check internally.

Or you could define a static Collections.RemoveEmpties() class+method to hide the delegate. (Or do the same in C#3 with extension methods.)

The first one is awkward, the second a bit repetitive. Using a list that doesn't add nulls is not that bad and not really invisible if you name the class ListThatIgnoresNullItems. Lastly I would use customer.AddYourAddresses(adresses) and remove the properties to improve encapsulation...

How about having a method like AddIf? which takes a delegate & adds an item only if it fits the criteria specified.

I don't think there's anything wrong with the former. I would use it, and I wouldn't be suprised if you did too. The second is clearly more clunky and not as easy on the eyes.

This relates to your recent post (http://ayende.com/Blog/archive/2007/03/05/Great-Code-and-Average-Programmers.aspx) in that while the first looks cleaner, I think a lot of "average programmers" aren't particularly comfortable with delegates. From my experience, many of them feel its something to be used in supporting frameworks, but not so much in everyday application development. I certainly don't agree with that statement, but I think it's a common misperception.

second, but it would have to be refactored, I hate repetitive code

@Eber,

Just curious.. how would you suggest refactoring the second example? IMHO, the first example is just that.

the first would raise a happy smile if i found it in someone elses' code, but the second does have the virtue of being idiot proof when it comes to maintainence.

normally i reckon it's easy to recognise code that's overly terse/smart-arsed, or code that's overly explicit/mindless, but these two examples are both sort of on the fence for me.

The first code is cute and clever, but not so efficient (what about adding 10 items, 100 items ?) and with obfuscated intention (akin to batch insert everything into some database then deleting items that should not have been there in the first place). The intent is "do not add addresses that are not defined", not "add every address, oh wait, no, get the list and delete non defined addresses". I think the first solution is plain awful.

The second code does not look so spiffy, but it does the job and conveys the intent. You could refactor it with the helper method suggested by Adam Vandenberg and it would be just fine and still be readable.

I use often this approach:

List<Address> addresses = new List<Address>();

foreach(Address a in new Address[] {customer.HomeAddress, customer.BusinessAddress, customer.ShippingAddress})

{

if(a != null) addresses.Add(a);

}

List<T> is based on an object[] .RemoveAll packs the array if any items are removed. So if the list gets large there's a lot of re pointing and in the end the Array is truncated.

Maybe Customer should implement a custom iterator which returns non null addresses. Then use List<T>.AddRange

Well I would choose the second. It's self describing, simple to maintain, has the least object creations and probably best performance wise.

The question that I had was why does the customer class has 3 address properties/datamembers and why doesn't the address object has some kind of address type.

Are you really going to add a new property for each new address type that gets added?

I don't think that performance is an issue here, the context is that he's adding 3 items, not 1000 (we shouldn't always generalize). The discussion is interesting in terms of brevity, cleverness and clarity of intention. The only problem I find with the first snippet is that it's intention its not completely clear (/a nice comment would solve it/). I like olegz solution too. I thing deriving List<T> is a bit overkill just to add a non null check.

Well, Bill Pierce pre-empted me. I still like the idea of moving the filter, and I was thinking of an addresses.AddValid(...) or .AddNonNull(...).

I don't think the first approach is all that cool, especially if there is some way that the inserted null might be seen before it is removed or if addresses.Add would actually fail or throw an exception -- why doesn't it throw an exception, now that I think of it?

Oh, hmph, I should have paid more attention to the first line before asking about throwing an exception and all of the other wrinkles that came to mind.

Since this is all about building an internal list that you don't want null entries in, why not just overload it to drop null additions?

Just in terms of maintaining an invariant, approach #3 does strike me as applicable here.

As to your question. The title suggests you went for #1, but I can't believe that you did that. The mixing of levels of abstraction and some other things (though I don't know about testability in this case) has me think that if you did go for #1, you're not happy about it.

1) There is bug in both - customer can be null :)

2) More important - Customer definition is wrong it should be:

...

string firstName

...

List<Address> customerAddresses

so Address should have type or something.

So the whole thing will be extensible :)

The first option actually reads easier but when you think about what it's doing, I don't like it. I'm the type that prefers to not do something if it's just going to be reverted/backed out/etc. anyways. If the intent is to only have non-null addresses in your list, I would go with a third option. Build a method to filter and only add if non-null (as others have mentioned). Basically what you did in #2 but a little prettier. The thought of adding a bunch of items then removing them based on a condition (when that condition could mean removing all items) just seems like a bad approach to a simple problem. Fail first rather than cleanup later.

No point in considering the inefficiency with adding 3 items and then potentially removing 3 items, unless its being called in a tight loop and the loop is slow when running.

The first one is cleaner to read, and if I had the option to code in C# at work Id use that (no anonymous delegates in VB.Net). The second one could be just as easy to read if you changed them into one liners and used tabs to align up the addresses.Add though.

Id personally go with the following, although that might be because I'm not used to being able to use anonymous delegates:

List<Address> addresses = new List<Address>();

ListExtensions.IfNotNullAdd(addresses, customer.HomeAddress, customer.BusinessAddress, customer.ShippingAddress);

public static void IfNotNullAdd<T>(/* this */ ICollection<T> collection, params T[] items) where T : class

{

foreach (T item in items)

{

    if (item != null)

        collection.Add(item);

}

}

Using the first method is bad programming - you add something empty to the list only to be deleted shorty after.

Someone may take this approach and use it in a frequently-called section of the code, impacting performance.

So if there are only 3 'add' statements, I would have used the second, otherwise this:

List<Address> addresses = new List<Address>();

IfNotNullAdd(customer.HomeAddress);

IfNotNullAdd(customer.BusinessAddress);

IfNotNullAdd(customer.ShippingAddress);

private void IfNotNullAdd(Address address, List<Address> addresses)

{

if (address != null)

addresses.Add(address);

}

So you did not tell us what solution you favored...

Not to be nagging or anything, but if you chose #1, it would shed an interesting light on some of your previous posts.

Like this one

http://www.ayende.com/Blog/archive/2007/03/05/Great-Code-and-Average-Programmers.aspx#feedback

and this one about the Morts

http://www.ayende.com/Blog/archive/2007/03/05/Great-Code-and-Average-Programmers.aspx#feedback

Honestly, ditching clarity of intention, performance, readability just to play cute with brand new language features is not really on par with

"Using smarter tools and working with higher level of abstraction would allow to deliver a higher quality system within a shorter period of time."

or

"Coding to the Mort's level will get you bad code, period. I deal with big applications, which means that I have zero use of demo-ware features."

Or do you only count wizards and GUI stuff as demo ware and not brand new ways of obfuscating a simple user requirement by abusing new language features ?

But I'm sure you chose solution #2, so my point is moot.

On a side note : can someone remember when the KISS principle was officially taken out of the agile dogma ?

Yes I'm not playing fair.

I really enjoy reading your blog, and I find many of your posts insightful (and I appreciate your work on rhinomocks) but I sometimes lose my temper when I perceive superiority complex and astronaut agilism.

No harm meant, really.

IMHO the winner is #2, not #1 or other possibilities. #1 doesn't obey KISS and is reverting the work done (it's also strange). #2 is perfect. Somebody argue to refactor #2, and many ways to do it have been commented, but hey! we are talking about 3 adresses, 3 lines of code. You take much more time to refactor that than to obtaining any benefit of it. #2 obeys KISS and is the fastest portion of code to write.

And another way to make #2 usefully readable is coming: extension methods. http://weblogs.asp.net/scottgu/archive/2007/03/13/new-orcas-language-feature-extension-methods.aspx

It has very nice maintenance and testing properties, as well as providing greater understandability at low cost.

Sugar, sugar, ...