7
\$\begingroup\$

I'm arguing here with a colleague over two implementations of how to handle a processing logic:

VERSION 1:

public class SomeManager {
    private readonly Dictionary<Type, Action<IData>> dataHandlers; 

    public SomeManager() {
        dataHandlers = new Dictionary<Type, Action<IData>> {
            {typeof(DataTypeOne), HandleDataTypeOne},
            {typeof(DataTypeTwo), HandleDataTypeTwo},
            {typeof(DataTypeThree), HandleDataTypeThird}
            //... and so on 
        };
    }

    //... could be other code here          

    public void Process(IData data) {
        var dataType = data.GetType();

        if (!dataHandlers.ContainsKey(dataType )) {
            Log.Error(string.Format("Unrecognized data type: {0}", dataType ));
            return;
        }

        dataHandlers[dataType ](data);
    }

    private void HandleDataTypeOne(IDispatchData data) {
        //...specific handling here
    }

    private void HandleDataTypeTwo(IData data) {
        //...specific handling here
    }

    private void HandleDataTypeThree(IData data) {
        //...specific handling here
    }
} 

VERSION 2:

public class SomeManager {
    public SomeManager() {
    }

    //... could be other code here          

    public void Process(IData data) {
        if (data is DataTypeOne) {
            HandleDataTypeOne(data);
        } else if (data is DataTypeTwo) {
            HandleDataTypeTwo(data);                
        } else if (data is DataTypeThree) {
            HandleDataTypeThree(data);      
        //other cases here           
        } else {
           Log.Error(string.Format("Unrecognized data type: {0}", data.GetType()));
        }
    }

    private void HandleDataTypeOne(IDispatchData data) {
        //...specific handling here
    }

    private void HandleDataTypeTwo(IData data) {
        //...specific handling here
    }

    private void HandleDataTypeThree(IData data) {
        //...specific handling here
    }
} 

In our case, performance is not an issue.

We are talking here about a specific scenario of 5 data types.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ I'm afraid this question does not match what this site is about. Code Review is about improving existing, working code. The example code that you have posted is not reviewable in this form because it leaves us guessing at your intentions. Unlike Stack Overflow, Code Review needs to look at concrete code in a real context. Please see Why is hypothetical example code off-topic for CR? \$\endgroup\$
    – Dan
    Apr 4, 2016 at 15:35
  • 1
    \$\begingroup\$ @DanPantry let me know if you need more context or intent details. \$\endgroup\$ Apr 4, 2016 at 15:36
  • \$\begingroup\$ We need real excerpts of code that are working implementations. The ellipses and naming in your question indicates that you have omitted code and / or have posted code that is not actually used. This makes this more of a conceptual question. If you post the real code behind both implementations, then we can offer meaningful reviews. Otherwise we would just be giving you information on a concept, which is off-topic for code review. \$\endgroup\$
    – Dan
    Apr 4, 2016 at 15:37
  • 1
    \$\begingroup\$ I would definitely go with the Dictionary implementation. Looks much cleaner. Plus you don't have to change the Process method every time you want to add a new type. \$\endgroup\$
    – 404
    Apr 5, 2016 at 13:25
  • \$\begingroup\$ Just for the completeness of the topic there is a similar question on SO. There is as well some info about the way compiler works with dictionary vs switch. \$\endgroup\$
    – Bakudan
    Jan 14, 2019 at 9:00

1 Answer 1

10
\$\begingroup\$

Both approaches are acceptable, but the one using a Dictionary feels cleaner to me, and brings some advantages at no significant cost.

  • A Dictionary will detect a duplicate key, whereas it could be easily overlooked in an if-else construct, leading to a possibly baffling bug

  • It could be modified in run-time, eg. for debugging purposes.

  • It's more flexible: it could be injected from outside, easily refactored towards a DI-based approach etc.

This being said, it could be overengineering for simple cases. It's hard to say without knowing broader context. All info you've provided is that there's 5 cases, which is quite a few already, and it makes me guess there's a chance for more to come; if all I knew was that there's 2 or 3, I would say keep it simple.

\$\endgroup\$
5
  • 3
    \$\begingroup\$ Avoiding duplicate keys automatically is a great benefit. Great answer \$\endgroup\$
    – nmit026
    May 9, 2019 at 2:48
  • \$\begingroup\$ Another benefit of Dictionary is that value access time complexity in Dictionary is O(1), while the if else time complexity is O(n). \$\endgroup\$ Apr 20, 2021 at 8:10
  • \$\begingroup\$ Also, I do not understand what is meant by the A Dictionary will detect a duplicate key, whereas it could be easily overlooked in an if-else construct, leading to a possibly baffling bug. How will it detect it? Dictionary will just override the value by key. That is not a detection mechanism I guess. \$\endgroup\$ Apr 20, 2021 at 8:12
  • \$\begingroup\$ @manymanymore A dictionary would throw an ArgumentException if you use Add in order to add a key when it already exists unlike an assign statement: docs.microsoft.com/es-es/dotnet/api/… \$\endgroup\$
    – Noman_1
    Mar 24, 2022 at 20:23
  • \$\begingroup\$ to avoid the duplicate if else cases, one could refactor to use a switch case, and in that case it would be very similar to the Dictionary check. A benefit from this, is that there is no way of overwritting the case and in the dictionary you can (replacing the value, deleting and adding, etc). The biggest issue of the switch case (and the if else handling), is that it could become huge depending on the amount of cases, which I would prefer to avoid, but it all depends on the context (i.e. 2-3 cases is fine). \$\endgroup\$
    – c-chavez
    Jun 27, 2022 at 11:14

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