5
\$\begingroup\$

Is there a better way to write this procedure?

    public List<IDenomination> GetInventory(int[] denominations)
    {
        var inventory = new List<IDenomination>();

        foreach (var denomination in denominations)
        {
            foreach (var item in Account)
            {
                if (item.Currency == denomination)
                {
                    inventory.Add(item);
                }
            }
        }

        return inventory;
    }
\$\endgroup\$
  • \$\begingroup\$ Account is a property contained within this class: public List<IDenomination> Account { get; set; } \$\endgroup\$ – TrevorBrooks Feb 20 '15 at 16:17
7
\$\begingroup\$

You could use some linq to do this fairly easily. Converting the array to a HashSet will allow Contains to operate faster

public List<IDenomination> GetInventory(int[] denominations)
{
    var denoms = new HashSet<int>(denominations);
    return Account.Where( a => denoms.Contains(a.Currency) ).ToList();
}

This assumes that Account : IDenomination though, as does your original code. If that were not the case, you would use a .Select() prior to .ToList() in order to select the IDenomination from Account.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ When I try this I get: 'System.Array' does not contain a definition for 'ToHashSet' and no extension method 'ToHashSet' accepting a first argument of type 'System.Array' could be found (are you missing a using directive or an assembly reference?) \$\endgroup\$ – TrevorBrooks Feb 20 '15 at 16:20
  • 1
    \$\begingroup\$ @TrevorBrooks - Ah yes, that was a custom extension. Please see my edit, it contains a native approach. \$\endgroup\$ – Travis J Feb 20 '15 at 16:22
  • \$\begingroup\$ Also, if Account is changed much rarely than GetInventory method is called, it might be better to maintain HashSet over IDenomination.Currency values, and change the GetInventory method appropriately \$\endgroup\$ – DixonD Feb 28 '15 at 9:41
2
\$\begingroup\$

It's really not a great idea to return a List from your method if you can help it. You've effectively bound your code to always using a list. What if your client wants to use a FancyCustomList<IDenomination>? You'd have to do some messy casting.

By all means, use the Linq that @TravisJ provider, but you should change the signature to return a IList<IDenomination> instead.

| improve this answer | |
\$\endgroup\$

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.