Refactoring C#, Javascript style.

0x3c0

Not what I'd personally do. I'd derive a class from List, and give it a constructor which accepts a 'validator delegate', which returns a bool. If it returns true, then add the item to the List. Then, I'd simply pass the function AddIfAllowed (or the lambda expression) to that List derived class as the validation delegate, and replace all of the AddIfAllowed calls with ordinary list.Adds. That way, the code looks neater and the List derived class is reusable if you keep the generics. Something like this:

class ValidatingList<T> : List<T>
{
public Func<bool, T> Validator { get; set; }

public ValidatingList(Func<bool, T> validator)
{
    this.Validator = validator;
}

//Obviously, you'd have to shadow AddRange, Insert, etc. Not that difficult, but I'm lazy
public new void Add(T item)
{
    if(this.Validator == null || this.Validator(item))
        base.Add(item);
}

}

public IList<Something> GetSomethings()
{
var list = new ValidatingList<Something>(IsAllowed);

list.Add(new Something() {Id = SomeId.Table});
list.Add(new Something() {Id = SomeId.Chair});
list.Add(new Something() {Id = SomeId.House});
list.Add(new Something() {Id = SomeId.Car});

return list;

}

Between the idea And the reality Between the motion And the act Falls the Shadow

Nish Nishant

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

Marc Clifton

Nope. Reduces readability and usability and doesn't really improve anything. If you want to go nuts, do what Nishant suggested. Marc

Will work for food. Interacx

I'm not overthinking the problem, I just felt like I needed a small, unimportant, uninteresting rant! - Martin Hart Turner

Chris Losinger

wow... now that's some ugly code.

image processing toolkits | batch image processing

Simon P Stevens

Cool, but ugly. I wouldn't want that in any serious project. It's very unreadable.

Simon

Nish Nishant

Chris Losinger wrote:

now that's some ugly code.

Precisely my point :-)

Regards, Nish

Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
My latest book : C++/CLI in Action / Amazon.com link

Nish Nishant

Simon Stevens wrote:

I wouldn't want that in any serious project. It's very unreadable.

Yep, that's what I was trying to establish.

Regards, Nish

Nish’s thoughts on MFC, C++/CLI and .NET (my blog)
My latest book : C++/CLI in Action / Amazon.com link

Joe Woodbury

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.

J Dunlap

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();

}

Vikram A Punathambekar

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.

Al Beback

I love it! :laugh: Actually, it does go too far -- more than anything because it wastes memory... but I can't help admiring the power of LINQ.

ShamWow

Al Beback

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

Al Beback

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

Al Beback

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

Shog9 0

Nice. :cool:

Shog9 0

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...

Shog9 0

I like it, but why cram it all on two lines?

Rob Graham

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.

Al Beback

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

Stuart Dootson

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