3
\$\begingroup\$

I am trying to return a value (a.tnAddress) from a custom class based on a lookup (foreach loop). Depending on the type of transaction, I will need to do the foreach loop based on different properties (sExecID, iMsgSeqNum, orsClOrderID). I prefer to not have 3 differentforeach` loops just but I am not sure how else to re-write this.

Keep in mind that the code is working fine; I just want to simplify it.

private TreeNode GetNodeAddress(cls_Transactions trPassedInTransaction)
{  
    switch (trPassedInTransaction.sMessageType)
    {
        case "Q":
        case "8b":
            foreach (cls_Transactions a in cls_GlobalVariables.transList)
            {
                if (trPassedInTransaction.sExecID == a.sExecID)
                {
                    return a.tnAddress;
                }
            }
            break;
        case "3":
            foreach (cls_Transactions a in cls_GlobalVariables.transList)
            {
                if (trPassedInTransaction.iMsgSeqNum == a.iMsgSeqNum)
                {
                    return a.tnAddress;
                }
            }
            break;
        default:
            foreach (cls_Transactions a in cls_GlobalVariables.transList)
            {
                if (trPassedInTransaction.sClOrderID == a.sClOrderID)
                {
                    return a.tnAddress;
                }
            }                    
            break;
    }

    return null;
}
\$\endgroup\$
  • \$\begingroup\$ Do you think the performance is poor? Your code seems good. \$\endgroup\$ – Chibueze Opata Jul 18 '13 at 0:39
  • \$\begingroup\$ I just didn't want to replicate the foreach loop 3x. Plus if I wanted to add messages types in the future, i thought there would be an easier way. \$\endgroup\$ – user2437909 Jul 18 '13 at 0:49
  • \$\begingroup\$ Switch statements are good here. It could be simplified using LINQ but won't be of much help. \$\endgroup\$ – Chibueze Opata Jul 18 '13 at 0:51
  • \$\begingroup\$ I would avoid switch statements in favour of polymorphism where possible, and in this case it looks easily possible... \$\endgroup\$ – Gayot Fow Jul 18 '13 at 13:45
10
\$\begingroup\$

This will add the use of LINQ to clean up the code the way you want:

private TreeNode GetNodeAddress(cls_Transactions trPassedInTransaction)
{
    //a predicate to pass to the FirstOrDefault method
    Func<cls_Transactions,Boolean> filter = null;
    switch (trPassedInTransaction.sMessageType)
    {

        case "Q":
        case "8b":
                 filter = x => trPassedInTransaction.sExecID == x.sExecID;
            break;
        case "3":
                 filter = x => trPassedInTransaction.iMsgSeqNum == x.iMsgSeqNum;
            break;
        default:
                 filter = x => trPassedInTransaction.sClOrderID == x.sClOrderID;                 
            break;
    }

    cls_Transactions result = cls_GlobalVariables.transList.FirstOrDefault(filter);
    return result != null ? result.tnAddress : null;
}

As an explanation, the switch statement has just been converted to use a predicate, which is a function type where it passes one parameter (in this case a cls_Transactions) and returns true/false.

The FirstOrDefault method is shorthand for the foreach loop and return, foreach-ing through the elements and using the predicate to determine if it meets the required condition, if none meet the required condition it will return a default value (in the case null).

You can use the First method also, which will throw an exception if nothing is found :)

\$\endgroup\$
  • \$\begingroup\$ Ok....I think I will have to study my LINQ before I implement this, but Thank You. \$\endgroup\$ – user2437909 Jul 18 '13 at 1:00
  • \$\begingroup\$ Func<cls_Transactions,Boolean> filter = null; --> in case of Q it will throw an ArgumentNullException so change null to: () => true \$\endgroup\$ – Peter Kiss Jul 18 '13 at 5:48
  • \$\begingroup\$ In case of 'Q' (as there is no 'break;') it will fall through to '8b' and use it's filter. There should be no condition here that filter is null, as there is a default on the switch statement. \$\endgroup\$ – m.t.bennett Jul 18 '13 at 8:04
  • \$\begingroup\$ @m.t.bennett: I think you should change Func<cls_Transactions,Boolean> filter = null; to Func<cls_Transactions,Boolean> filter; . This makes it more obvious that filter can never be null. The C# compiler does not allow possibly unassigned variables to be used, but in this case filter is always assigned. When I see you assign filter to null, my first inclination is to wonder if there is a code path where that value is used (because otherwise, why assign it at all?). \$\endgroup\$ – Brian Jul 18 '13 at 13:17
  • 1
    \$\begingroup\$ @m.t.bennett : If you don't assign the variable at all, the compiler will enforce that it needs to be handled at a later date. Any new code which introduces new switch elements will fail to compile unless those elements assign filter (since filter must be definitively assigned before it can be used). \$\endgroup\$ – Brian Jul 18 '13 at 23:17
2
\$\begingroup\$

I like using a dictionary because to me it looks like tighter cleaner code and you can easily add to the dictionary or if you want later pull the logic from another source and put into the dictionary.

        var dictionary = new Dictionary<string, Predicate<cls_Transactions>>();
        dictionary.Add("Q", x => x.sExecID == trPassedInTransaction.sExecID);
        dictionary.Add("8b", x => x.sExecID == trPassedInTransaction.sExecID);
        dictionary.Add("3", x => x.iMsgSeqNum == trPassedInTransaction.iMsgSeqNum);
        dictionary.Add("default", x => x.sClOrderID == trPassedInTransaction.sClOrderID);

        var switchValue = dictionary[trPassedInTransaction.sMessageType] != null
                              ? trPassedInTransaction.sMessageType
                              : "default";
        var result = cls_GlobalVariables.transList.FirstOrDefault(t => dictionary[switchValue](t));
        return result != null ? result.tnAddress : null;
\$\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.