Recently I was browsing through a certain code base and I saw a code similar to this:
GetRecipientsAddresses method draw my attention.
Someone extracted quite complicated lambda expression
to a local function.
At first I thought that this is indeed a very nice usage for local
LINQ query is much more readable
with expressions like
instead of long lambdas.
It took me a while to realize that the local function
in the code above, was used to hide design problems.
Let’s take a closer look at the condition encapsulated by
We should deal with the simplest to fix problems first:
Bad naming: We should always format predicates in “a positive way”.
For examples we should prefer
IsDisabled should be named
As a first step in refactoring we may add
Unreadable condition: If a user has optional email then we may expect
that our codebase is littered with little
user.EmailAddres != null checks.
To increase readability we should encapsulate this check into a separate property:
Missing entity attributes: When I looked closely at the condition
user.IsRegistered && !user.IsDisabled I found out that it occurs
in many places in that codebase. For some reason the system was creating
users before they actually registered. A user that not registered yet was basically
a stub not a real user. Users could also be disabled by admins (registered or not),
this is what the second part of the condition was responsible for.
User entity is missing an attribute that could tell us whatever
a user is active, so let’s add one:
After all these refactorings we may finally rewrite our LINQ query:
This version is as readable as version with the local function, but does not attempt to hide code smells.
Conclusion: Every time when you have a too long or too complicated lambda expression, that you what to extract to a local function, think if you can simplify that lambda by extracting conditions and checks into new methods and properties on processed objects.