Refactoring C#, Javascript style.

Al Beback

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

Dave Parker

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.

Simon P Stevens

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

Rob Graham

Refactoring to add cleverness? I don't see the value here, the change does not add clarity, readability, function or maintainability.

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