0

How to make the following code shorter, perhaps using anonymous method or extensions and LINQ.

Since I have to repeat this code several times and I want to make it as succinct as possible.

var imagesToUnlock = App.ImageListVM.Items.Where(img => img.Category == key);

foreach (var image in imagesToUnlock)
{
    image.IsLocked = false;
}
PutraKg
  • 2,226
  • 3
  • 31
  • 60
  • It changes depends on the condition – PutraKg May 16 '14 at 01:14
  • I think what you have there is fine - maybe move it into a generic method/extension. On a side note, I've noticed people are getting too obsessed with condensing the hell out of code and going LINQ crazy, even if it means going against best practice. – mnsr May 16 '14 at 01:27
  • 2
    You might also consider the speed factor. I've done a few tests with a few of [Euler's Project's problems](https://projecteuler.net/problems), comparing [LINQ and non-LINQ solutions](https://github.com/joce/EulerProject/), and the `foreach` solution always out performs the LINQ one, frequently by an order of magnitude or more. – joce May 16 '14 at 01:32

4 Answers4

7

The other solutions here feel dirty because they mutate objects in a collection via the use of LINQ.

I would instead, put the code and the filter condition into an extension method and call that:

public static IEnumerable<Item> UnlockWhere(this IEnumerable<Item> list, Func<Item, bool> condition) {
    foreach (var image in list)
        if (condition(image)) {
            image.IsLocked = false;
            yield return image;
        }
}

The keeps the immutability-concerns of LINQ intact and still produces the expected result.

The call becomes:

var unlockedItems = App.ImageListVM.Items.UnlockWhere(img => img.Category == key);

EDIT

Re-written to completely remove LINQ. Instead, this new method iterates only once and returns a new, mutated collection.

Simon Whitehead
  • 63,300
  • 9
  • 114
  • 138
  • I accepted this as an answer but after further testing. `UnlockWhere` isn't being called for some reason. May be I am missing something – PutraKg May 16 '14 at 02:24
  • It is an extension method.. so you'll need to put it in a static class and include the namespace it resides in with a `using` directive. – Simon Whitehead May 16 '14 at 02:25
  • Yes I did that. Just to add that `App.ImageListVM.Items` is an `ObservableCollection` – PutraKg May 16 '14 at 02:28
  • 1
    ..well feel free to modify the method to suit your needs.. such as changing it to handle an `ObservableCollection` instead. Note also that I made it handle type `Item` because I didn't know what your actual type was called.. – Simon Whitehead May 16 '14 at 02:30
  • Correct. But when I change to `ObservableCollection` visual studio complains that the body of `UnlockWhere` cannot be an iterator block because it's not an iterator interface type? – PutraKg May 16 '14 at 02:32
  • The return type will have to remain an `IEnumerable` due to my use of `yield return`. Feel free to remove that also. – Simon Whitehead May 16 '14 at 02:32
  • What does "is still not being called" mean? Doesn't compile? Fails at runtime? Doesn't crash but produces unexpected results? ... – Simon Whitehead May 16 '14 at 02:36
  • No error, no warning, it does not fail nor crash. It compiles normally. Placed a break inside the body of `UnlockWhere` and execution does not reach there. – PutraKg May 16 '14 at 02:38
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/52802/discussion-between-simon-whitehead-and-putrakg) – Simon Whitehead May 16 '14 at 02:39
3

Not the most efficient way to do it, but I believe you can do

var imagesToUnlock = App.ImageListVM.Items.Where(img => img.Category == key).ToList().Foreach(f => f.IsLocked = false);

Check out the Foreach method on List<T> for more info.

I would also like to note (as some have pointed out in the comments) that this is not considered best practice by some people. You should take a look at this article by Eric Lippert, who explains the issue in better detail.

cost
  • 4,420
  • 8
  • 48
  • 80
  • yep ... sounds about right ... I'll probably break it into new lines on `.`, but same same ... – Noctis May 16 '14 at 01:16
  • 1
    As @EricJ said on another answer: "While you can do this, Eric Lippert considers this very bad practice http://blogs.msdn.com/b/ericlippert/archive/2009/05/18/foreach-vs-foreach.aspx . I would consider his opinion carefully. – Eric J." – joce May 16 '14 at 01:18
  • @Joce I'm not arguing that this is the proper way to do this, I was giving them a more succinct way to write out the code, which is what they asked. Perhaps I should modify my answer slightly to note that – cost May 16 '14 at 01:22
  • Good idea. Although `Foreach` does not seem to be available for Windows Phone apps. – PutraKg May 16 '14 at 01:47
1

Here's a stab as an extension method

Code

         public static IEnumerable<T> SetPropertyValues<T>(this IEnumerable<T> items, Action<T> action)
        {
            foreach (var item in items)
            {
                action(item);
                yield return item;
            }
        }

Usage

        private class Foo
        {
            public string Bar { get; set; } 
        }

        [TestMethod]
        public void SetPropertyValuesForMiscTests()
        {
            var foos = new[] { new Foo { Bar = "hi" }, new Foo { Bar = "hello" } };
            var newList = foos.SetPropertyValues(f => f.Bar = "bye");

            Assert.AreEqual("bye", newList.ElementAt(0).Bar);
            Assert.AreEqual("bye", newList.ElementAt(1).Bar);
        }

I tested it and it works fine.

BlackjacketMack
  • 5,472
  • 28
  • 32
  • Ha...Just read Lippert's first couple of lines here:http://blogs.msdn.com/b/ericlippert/archive/2009/05/18/foreach-vs-foreach.aspx and his extension method is pretty similar. – BlackjacketMack May 16 '14 at 01:37
  • I did...I think his objection is really more about adding it to the core library rather than just going ahead and doing it yourself. I personally, would rather just have explicit methods "void setToLocked(IEnumerable items)" using a foreach statement, but the extension works fine, so it is a viable answer. – BlackjacketMack May 16 '14 at 01:54
-1

Yeah you can do this. Adapted from this answer.

imagesToUnlock.Select(i => {i.IsLocked = false; return i;}).ToList();

Edit: A lot of people are saying this is bad practice. I agree with dasblinkenlight here.. Exploring the limits of LINQ and C# is our duty as programmers. It isn't unreasonable to change the objects type from the DTO to the view model or domain object, I know its not the best, but if encapsulated and commented it isn't the end of the world to use select to do this. But please be conscious of the best practices explained by Eric.

Community
  • 1
  • 1
TheNorthWes
  • 2,661
  • 19
  • 35
  • 1
    While you can do this, Eric Lippert considers this very bad practice http://blogs.msdn.com/b/ericlippert/archive/2009/05/18/foreach-vs-foreach.aspx. I would consider his opinion carefully. – Eric J. May 16 '14 at 01:15
  • 3
    This feels so very very dirty. It is bypassing the immutable behaviour of LINQ. – Simon Whitehead May 16 '14 at 01:15
  • 1
    Yup. But @PutraKg wanted it ¯\_(ツ)_/¯. I by no means endorse this, but for such a small case, or a small project I can understand abusing linq. (But I've done it) – TheNorthWes May 16 '14 at 01:18
  • 2
    Please don't do this. `Select` should be used to *answer a question*, not *produce a change*. – Eric Lippert May 16 '14 at 13:57