Refactoring C#, Javascript style.
-
I was going through some C# code I had recently written, when I spotted a cool way to refactor it. See if you agree: BEFORE:
public IList<Something> GetSomethings() { var list = new List<Something>(); AddIfAllowed(list, SomeId.Table); AddIfAllowed(list, SomeId.Chair); AddIfAllowed(list, SomeId.House); AddIfAllowed(list, SomeId.Car); return list; } private void AddIfAllowed(ICollection<Something> list, SomeId id) { if (IsAllowed(id)) list.Add(new Something {Id = id}); }AFTER:
public IList<Something> GetSomethings() { var list = new List<Something>(); Action<SomeId> addIfAllowed = id => { if (IsAllowed(id)) list.Add(new Something {Id = id}); }; addIfAllowed(SomeId.Table); addIfAllowed(SomeId.Chair); addIfAllowed(SomeId.House); addIfAllowed(SomeId.Car); return list; }:cool:
ShamWow
I concur that this simply makes the code less readable for no reason. This also raises the question of what refactoring means; by literal definition you refactored the code. But the philosophy of refactoring suggests that the code must actually to have been improved by speed, reliability, size or drastic improvements in maintainability. Otherwise, you are simply changing code for the sake of changing it, which greatly introduces the chances for bugs and/or decreases in performance.
-
If you like that sort of coding style, you might want to use Linq and do something like :
public IList<Something> GetSomethings()
{
return new[] { SomeId.Table, SomeId.Chair, SomeId.House, SomeId.Car }
.Where(id => IsAllowed(id)).Select(id => new Something(id)).ToList();
}Regards, Nish
Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
My latest book : C++/CLI in Action / Amazon.com linkAnd if you're using query methods, you might as well use the LINQ syntax. :-)
public IList GetSomethings()
{
var things = new[] { SomeId.Table, SomeId.Chair, SomeId.House, SomeId.Car };return ( from id in things where IsAllowed(id) select new Something(id) ) .ToList();}
-
I was going through some C# code I had recently written, when I spotted a cool way to refactor it. See if you agree: BEFORE:
public IList<Something> GetSomethings() { var list = new List<Something>(); AddIfAllowed(list, SomeId.Table); AddIfAllowed(list, SomeId.Chair); AddIfAllowed(list, SomeId.House); AddIfAllowed(list, SomeId.Car); return list; } private void AddIfAllowed(ICollection<Something> list, SomeId id) { if (IsAllowed(id)) list.Add(new Something {Id = id}); }AFTER:
public IList<Something> GetSomethings() { var list = new List<Something>(); Action<SomeId> addIfAllowed = id => { if (IsAllowed(id)) list.Add(new Something {Id = id}); }; addIfAllowed(SomeId.Table); addIfAllowed(SomeId.Chair); addIfAllowed(SomeId.House); addIfAllowed(SomeId.Car); return list; }:cool:
ShamWow
I prefer the original.
Cheers, Vikram. (Proud to have finally cracked a CCC!)
Recent activities: TV series: Friends, season 10 Books: Fooled by Randomness, by Nassim Nicholas Taleb.
Carpe Diem.
-
If you like that sort of coding style, you might want to use Linq and do something like :
public IList<Something> GetSomethings()
{
return new[] { SomeId.Table, SomeId.Chair, SomeId.House, SomeId.Car }
.Where(id => IsAllowed(id)).Select(id => new Something(id)).ToList();
}Regards, Nish
Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
My latest book : C++/CLI in Action / Amazon.com link -
Prefer the original personally, more readable and I don't think the lambda is really adding any value. But each to their own I guess, my co-worker loves using List.ForEach everywhere rather than using the foreach keyword.
Dave Parker wrote:
Prefer the original personally, more readable
I agree -- I'm still not used to reading C# code with methods declared inside methods.
Dave Parker wrote:
I don't think the lambda is really adding any value.
The value I see is that it avoids having to repeatedly pass the collection into the method. That's my biggest reason for favoring this approach.
ShamWow
-
I don't see how the re factored version adds any value. If anything it removes the possibility of re-using that bit of code in the private method elsewhere in the class.
Simon
Simon Stevens wrote:
I don't see how the re factored version adds any value.
The list does not need to be passed into the method each time. That's the value I see.
Simon Stevens wrote:
If anything it removes the possibility of re-using that bit of code in the private method elsewhere in the class.
Sure, but this is a case where it won't be used anywhere else.
ShamWow
-
Refactoring to add cleverness? I don't see the value here, the change does not add clarity, readability, function or maintainability.
Rob Graham wrote:
Refactoring to add cleverness?
Nothing wrong with more cleverness. That's what makes LINQ so appealing.
Rob Graham wrote:
I don't see the value here
The list is not passed into the sub-method each time. That's where I see the value.
ShamWow
-
And if you're using query methods, you might as well use the LINQ syntax. :-)
public IList GetSomethings()
{
var things = new[] { SomeId.Table, SomeId.Chair, SomeId.House, SomeId.Car };return ( from id in things where IsAllowed(id) select new Something(id) ) .ToList();}
-
I was going through some C# code I had recently written, when I spotted a cool way to refactor it. See if you agree: BEFORE:
public IList<Something> GetSomethings() { var list = new List<Something>(); AddIfAllowed(list, SomeId.Table); AddIfAllowed(list, SomeId.Chair); AddIfAllowed(list, SomeId.House); AddIfAllowed(list, SomeId.Car); return list; } private void AddIfAllowed(ICollection<Something> list, SomeId id) { if (IsAllowed(id)) list.Add(new Something {Id = id}); }AFTER:
public IList<Something> GetSomethings() { var list = new List<Something>(); Action<SomeId> addIfAllowed = id => { if (IsAllowed(id)) list.Add(new Something {Id = id}); }; addIfAllowed(SomeId.Table); addIfAllowed(SomeId.Chair); addIfAllowed(SomeId.House); addIfAllowed(SomeId.Car); return list; }:cool:
ShamWow
I don't mind seeing one-off function definitions brought inside the scope where they're used, but in this case i've gotta wonder if there's any point to defining a function at all - you really just want to filter a list. Nish and J Dunlap demonstrate two ways to accomplish this...
-
If you like that sort of coding style, you might want to use Linq and do something like :
public IList<Something> GetSomethings()
{
return new[] { SomeId.Table, SomeId.Chair, SomeId.House, SomeId.Car }
.Where(id => IsAllowed(id)).Select(id => new Something(id)).ToList();
}Regards, Nish
Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
My latest book : C++/CLI in Action / Amazon.com link -
Rob Graham wrote:
Refactoring to add cleverness?
Nothing wrong with more cleverness. That's what makes LINQ so appealing.
Rob Graham wrote:
I don't see the value here
The list is not passed into the sub-method each time. That's where I see the value.
ShamWow
Al Beback wrote:
Nothing wrong with more cleverness. That's what makes LINQ so appealing.
Almost a guarantee that some maintainer will later screw it up. Cleverness for cleverness sake is bad, particularly when it obscures function or intent.
Al Beback wrote:
The list is not passed into the sub-method each time. That's where I see the value.
No, the list appears not to be passed into the sub-method. You would have to inspect the IL to see if there was a substantial difference in the way the compiler implemented the code, or to determine which was more efficient.
-
And if you're using query methods, you might as well use the LINQ syntax. :-)
public IList GetSomethings()
{
var things = new[] { SomeId.Table, SomeId.Chair, SomeId.House, SomeId.Car };return ( from id in things where IsAllowed(id) select new Something(id) ) .ToList();}
J. Dunlap wrote:
you might as well use the LINQ syntax.
I don't like LINQ syntax. It's like they shoehorned SQL syntax into C#, unnecessarily. I prefer this a lot more:
return things.Where(id => IsAllowed(id))
.Select(id => new Something(id))
.ToList();ShamWow
-
I was going through some C# code I had recently written, when I spotted a cool way to refactor it. See if you agree: BEFORE:
public IList<Something> GetSomethings() { var list = new List<Something>(); AddIfAllowed(list, SomeId.Table); AddIfAllowed(list, SomeId.Chair); AddIfAllowed(list, SomeId.House); AddIfAllowed(list, SomeId.Car); return list; } private void AddIfAllowed(ICollection<Something> list, SomeId id) { if (IsAllowed(id)) list.Add(new Something {Id = id}); }AFTER:
public IList<Something> GetSomethings() { var list = new List<Something>(); Action<SomeId> addIfAllowed = id => { if (IsAllowed(id)) list.Add(new Something {Id = id}); }; addIfAllowed(SomeId.Table); addIfAllowed(SomeId.Chair); addIfAllowed(SomeId.House); addIfAllowed(SomeId.Car); return list; }:cool:
ShamWow
I like lambdas and all - but sometimes the language gets in the way.... Haskell-esque:
GetSomethings = (map Something.filter IsAllowed) [SomeId.Table, SomeId.Chair, SomeId.House, SomeId.Car]
or, using list comprehensions,
GetSomethings = [Something x | x <- [1..10], isAllowed x]
Woo - polymorphism rules! If you want/need a type signature, it'll be something like
GetSomethings :: [Id] -> [Something], presuming Something and isAllowed are free. Does C# (or rather, the CLR) have the equivalent of filter and map functions? I know that Linq is the closest to a list comprehension that you'll get in C#...Java, Basic, who cares - it's all a bunch of tree-hugging hippy cr*p
-
And if you're using query methods, you might as well use the LINQ syntax. :-)
public IList GetSomethings()
{
var things = new[] { SomeId.Table, SomeId.Chair, SomeId.House, SomeId.Car };return ( from id in things where IsAllowed(id) select new Something(id) ) .ToList();}
Not my personal choice. AFAIK, that takes away the on-demand nature of LINQ when it converts to a List. I would personally prefer to return the query straight away - IList<T> inherits from IEnumerable<T> anyway
Between the idea And the reality Between the motion And the act Falls the Shadow
-
If you like that sort of coding style, you might want to use Linq and do something like :
public IList<Something> GetSomethings()
{
return new[] { SomeId.Table, SomeId.Chair, SomeId.House, SomeId.Car }
.Where(id => IsAllowed(id)).Select(id => new Something(id)).ToList();
}Regards, Nish
Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
My latest book : C++/CLI in Action / Amazon.com linkThe horrifying thing is in a few years you are going to run across this sort of code everywhere and have to support it. Lambda does NOT make your code more readable, I see a support nightmare in the making
Never underestimate the power of human stupidity RAH
-
Not my personal choice. AFAIK, that takes away the on-demand nature of LINQ when it converts to a List. I would personally prefer to return the query straight away - IList<T> inherits from IEnumerable<T> anyway
Between the idea And the reality Between the motion And the act Falls the Shadow
-
J. Dunlap wrote:
you might as well use the LINQ syntax.
I don't like LINQ syntax. It's like they shoehorned SQL syntax into C#, unnecessarily. I prefer this a lot more:
return things.Where(id => IsAllowed(id))
.Select(id => new Something(id))
.ToList();ShamWow
I will often use the methods directly like that for small queries, but I like the LINQ syntax best for more complex ones. Also, I tend to think they took the good parts of SQL and discarded most of the bad ones (for example they moved the
selectportion after thefromso that intellisense works) - but LINQ vs direct methods is likely just a matter of personal opinion. -
I like lambdas and all - but sometimes the language gets in the way.... Haskell-esque:
GetSomethings = (map Something.filter IsAllowed) [SomeId.Table, SomeId.Chair, SomeId.House, SomeId.Car]
or, using list comprehensions,
GetSomethings = [Something x | x <- [1..10], isAllowed x]
Woo - polymorphism rules! If you want/need a type signature, it'll be something like
GetSomethings :: [Id] -> [Something], presuming Something and isAllowed are free. Does C# (or rather, the CLR) have the equivalent of filter and map functions? I know that Linq is the closest to a list comprehension that you'll get in C#...Java, Basic, who cares - it's all a bunch of tree-hugging hippy cr*p
map= Select() filter= Where() Method as lambda functions:
//plain method calls
getSomethings= (ts)=> ts.Where(t=>IsAllowed(t)).Select(new Something {Id = id});//LINQ
getSomethings= (ts)=> from t in ts where IsAllowed(t) select new Something {Id = id};Unfortunately the
getSomethingsvariable has to be defined and would look like this:Func<IEnumerable<SomeId>,IList<Something>> getSomethings;
...the downside of explicit strong typing.
-
And if you're using query methods, you might as well use the LINQ syntax. :-)
public IList GetSomethings()
{
var things = new[] { SomeId.Table, SomeId.Chair, SomeId.House, SomeId.Car };return ( from id in things where IsAllowed(id) select new Something(id) ) .ToList();}
J. Dunlap wrote:
And if you're using query methods, you might as well use the LINQ syntax.
For some reason I can't stand the C# linq syntax - I always use the method calls directly.
Regards, Nish
Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
My latest book : C++/CLI in Action / Amazon.com link -
Shog9 wrote:
I like it, but why cram it all on two lines?
Yeah, normally I'd have new-lined on each of the dots. This was just a quick reply.
Regards, Nish
Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
My latest book : C++/CLI in Action / Amazon.com link