How can I improve this code?

var maxDateValue = list.Where(x => x.ID == 1).Max(c => c.LastUpdDt);
list.RemoveAll(x => x.ID == 1 & x.LastUpdDt!= maxDateValue);
  • 2
    \$\begingroup\$ Could you add a bit of conedt for this code? There is only so much that can be said about 2 lines. Maybe include the method and class where this code is used? \$\endgroup\$ – Phrancis Dec 14 '15 at 18:28
  • \$\begingroup\$ Could you please add more context how you want this code to be improved (e.g. performance, readability, usage, etc.). As is it seems too broad to give you concise answers. \$\endgroup\$ – πάντα ῥεῖ Dec 14 '15 at 18:39
  • \$\begingroup\$ Do not abbreviate unnecessarily: LastUpdDt is no improvement on LastUpdateDate. \$\endgroup\$ – BCdotWEB Dec 15 '15 at 8:19

I think that Heslacher's answer skips some important issue because of the wrong arguments for list.RemoveAll:

List<T>.RemoveAll(Predicate<T>) - it takes a predicate, not an IEnumerable. Thus creating a filtered list doesn't make that much sense.

I'll give a bit more verbose code answer.

I'll assume that you operate on class Item.

// Add some code to the Item class
class Item
    // ...

    // Add this method to this class, because it's a simple question asked about this object
    public bool UpdatedBefore(DateTime time)
        return LastUpdDt < time;

// If 1 is widely used in context of Item objects, MagicId and IsMagic should be somewhere else (eg in class Item)
private const int MagicId = 1;

private bool IsMagic(Item item)
    return item.ID == MagicId;

private void RemoveMagicItemsExceptOfMostRecentlyUpdated(List<Item> items)
    var latestMagicUpdate = items.Where(IsMagic).Max(it => it.LastUpdDt);
    Predicate<Item> isNotLatest = item => item.UpdatedBefore(latestMagicUpdate);
    items.RemoveAll(it => IsMagic(it) && isNotLatest(it));

You may think it is inefficient, because I just generated much more lines of code.

But the benefits are:

  • every element I added is well named and gives you its exact meaning
  • when you read "RemoveMagicItemsExceptOfMostRecentlyUpdated" and you trust it's fair and does what you say, you don't have to look inside
  • Code reuse, consistency, less bugs - next time You would want to write "item.ID == 1", you'll use already existing, well named, describing method IsMagic
  • \$\begingroup\$ Hi! Welcome to Code Review. Good first answer! \$\endgroup\$ – TheCoffeeCup Dec 14 '15 at 22:21

By calculating the entries of the list which satisfy the condition that the ID == 1 which by the way is a magic number which should be avoided, and storing the result in a variable the recalculating isn't needed.

As @JSparrow correctly pointed out in his answer the previous shown code was wrong. Not only takes the RemoveAll() method a predicate as an argument but the used IList<T> interface doesn't have that said method.

So this will do the same

IList<yourType> filteredList = new List<yourType>(list.Where(x => x.ID == 1));
var maxDateValue = filteredList.Max(c => c.LastUpdDt);
list = list.Except(filteredList.Where(x=> x.LastUpdDt != maxDateValue)).ToList(); 

this eleminates your bug with x.ID == 1 & x.LastUpdDt!= as well. <- single &

Coming back to the magic number/string issue. Why is this 1 and not 88 ? Thats just not clear from the code. If you extract this 1 to a meaningful constant it will be much better.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.