Refactoring C#, Javascript style.
-
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();}
-
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 -
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...
-
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 -
The 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
Mycroft Holmes wrote:
Lambda does NOT make your code more readable, I see a support nightmare in the making
If we can charge 300/hr fixing that sorta code, I am all for it :rolleyes:
Regards, Nish
Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
My latest book : C++/CLI in Action / Amazon.com link -
Mycroft Holmes wrote:
Lambda does NOT make your code more readable, I see a support nightmare in the making
If we can charge 300/hr fixing that sorta code, I am all for it :rolleyes:
Regards, Nish
Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
My latest book : C++/CLI in Action / Amazon.com linkIf I could charge $300/hr I would fix cobol code.
Never underestimate the power of human stupidity RAH
-
If I could charge $300/hr I would fix cobol code.
Never underestimate the power of human stupidity RAH
Mycroft Holmes wrote:
If I could charge $300/hr I would fix cobol code.
Yeah me too, in fact I'd port VB6 to Cobol if that! :)
Regards, Nish
Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
My latest book : C++/CLI in Action / Amazon.com link