2
\$\begingroup\$

I am currently doing a code review for a coworker.

I have encountered something like this:

switch (type)
{
    case DatabaseType.Type1:
        switch (domain)
        {
            case DOMAIN_1:
                return DatabaseConfigType.Type1Domain1;
            case DOMAIN_2:
                return DatabaseConfigType.Type1Domain2;
            case DOMAIN_3:
                return DatabaseConfigType.Type1Domain3;
            case DOMAIN_4:
                return DatabaseConfigType.Type1Domain4;
            default:
                Trace.Error("Undefined Type1 domain :" + domain);
                throw new ArgumentOutOfRangeException("domain", domain, null);
        }
    case DatabaseType.Type2:
        switch (domain)
        {
            case DOMAIN_1:
                return DatabaseConfigType.Type2Domain1;
            case DOMAIN_2:
                return DatabaseConfigType.Type2Domain2;
            case DOMAIN_3:
                return DatabaseConfigType.Type2Domain3;
            case DOMAIN_4:
                return DatabaseConfigType.Type2Domain4;
            default:
                Trace.Error("Undefined Type2 domain :" + domain);
                throw new ArgumentOutOfRangeException("domain", domain, null);
        }
    case DatabaseType.Type3:
        switch (domain)
        {
            case DOMAIN_1:
                return DatabaseConfigType.Type3Domain1;
            case DOMAIN_2:
                return DatabaseConfigType.Type3Domain2;
            case DOMAIN_3:
                return DatabaseConfigType.Type3Domain3;
            case DOMAIN_4:
                return DatabaseConfigType.Type3Domain4;
            default:
                Trace.Error("Undefined Type3 domain :" + domain);
                throw new ArgumentOutOfRangeException("domain", domain, null);
        }
    default:
        Trace.Error("Undefined type :" + type);
        throw new ArgumentOutOfRangeException("type", type, null);
}

Obviously, Types and Domains have actual wordy names, I just changed them for the question because they are irrelevant and probably sensitive information. On the other hand, the actual names for the DatabaseConfigType Enum are literally of the format TypeDomain, with the actual names of the type and domain instead of my numbered version.

I see this code, and it just feels wrong. There is some obvious redundancy by having three times (possibly more in the future) the exact same switch conditions inside each "outer" switch case, but since they return an Enum with different values, I just can't figure out a way to actually make this better.

The enum is in no particular order, and can't be expected to stay the same, so I can't do some math-ish unreadable but fast solution, not that I think it would be better anyway.

The code is probably as fast as it can be while still being readable, I'm mostly looking for a way to reduce the copy-paste aspect, therefore the lines of code, because I can't shake the feeling that there has to be a better way. If there isn't, feel free to tell me that.

\$\endgroup\$
  • \$\begingroup\$ I am currently doing a code review for a coworker. + I am not the one who made the original code = Questions seeking an explanation of someone else's code are also off-topic. \$\endgroup\$ – t3chb0t Jul 21 '17 at 13:42
  • 3
    \$\begingroup\$ @t3chb0t - it is fine to post code that you maintain. If it was written by a coworker, we can assume OP has to maintain the code too. Also doesn't seem to be seeking an explanation. The code is a smell to them and they wanted a review from other people. All within site remit as far as I can tell. \$\endgroup\$ – RobH Jul 21 '17 at 14:30
  • \$\begingroup\$ Use a dictionary (they map values to values)! \$\endgroup\$ – D. Ben Knoble Jul 21 '17 at 14:33
  • \$\begingroup\$ @RobH ok, makes sense to me. \$\endgroup\$ – t3chb0t Jul 21 '17 at 14:34
7
\$\begingroup\$

Just declare a (static readonly) dictionary:

var values = new Dictionary<(DatabaseType, DomainType), DatabaseConfigType>()
{
    { (DatabaseType.Type1, DomainType.Domain1), DatabaseConfigType.Type1Domain1 }
};

Filled with all possible combinations. Then your function will be reduced to:

DatabaseConfigType GetDatabaseConfiguration(DatabaseType database, DomainType domain)
    => values[(type, domain)];

You may want to use TryGetValue() to customize error message or to throw a different, more informative, exception. I'm using C# 7 named tuples but you can do it without them, it's just little bit more verbose because you need to declare your own Tuple because we need it to be a struct unless you use a custom comparer.


Edit: what if you need custom error messages (saying which part is unknown)? You need to nest dictionaries:

var values = new Dictionary<DatabaseType, Dictionary<DomainType, DatabaseConfigType>>()
{
    {
        DatabaseType.Type1, Dictionary<DomainType, DatabaseConfigType>
        {
            { DomainType.Domain1, DatabaseConfigType.Type1Domain1 }
        }
    }
};

In this case to get a value:

DatabaseConfigType GetDatabaseConfiguration(DatabaseType database, DomainType domain)
{
    if (values.TryGetValue(database, out var configTypes))
    {
        if (configTypes.TryGetValue(domain, out var configType))
            return configType;

        throw new ArgumentException($"Unknown domain {domain} for {database}");
    }

    throw new ArgumentException($"Unknown database {database}");
}

Which exception should be thrown? Assuming you can change this part of the code contract I feel ArgumentOutOfRangeException is not the appropriate one in this case (POLA?). Some combinations aren't allowed, an enum may have an invalid value (GetDatabaseConfiguration((DatabaseType)333, (DomainType)444)) or you may simply forgot to update this function after an update to the enum. In my opinion a generic ArgumentException may be enough (unless you want to differentiate the invalid combination case). A specific exception exists (InvalidEnumArgumentException) but I seldom saw it in real-world code (probably because it doesn't add much value over ArgumentException).


Assuming you changed names but not style then DOMAIN_1 isn't a good name because it doesn't follow common C# naming conventions: it's all uppercase and it uses underscore as separator, it should be Domain1.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I am not the one who made the original code, but I'm pretty sure the different error messages (and thrown exception) depending on if type is not found, or if domaine is not found for each type, are important. Your solution wouldn't allow for knowing which one was unknown, therefore I can't have the trace and exception as they should be \$\endgroup\$ – Kaito Kid Jul 21 '17 at 13:01
  • 1
    \$\begingroup\$ @KaitoKid think this idea is the answer to your question, and I would propose a very similar idea. You might need to adjust the details some, but this is the direction you want to move in. I don't do C# so I'm not sure exactly what you need, but surely there is some way to create a "multi-dimensional dictionary" that you can check key existence for? In many languages it would be something like (pseudo-code) if ( !(type in values) ){ throw Exception; } if ( !(domain in values[type]){ throw Exception } \$\endgroup\$ – Conor Mancone Jul 21 '17 at 13:10
  • \$\begingroup\$ @KaitoKid You just need to use the same principle but with a different structure, there are workarounds (for example inspecting key list) but also simply a nested dictionary (see updated example). Note that best advantage of using a dictionary isn't just code clarity/brevity (and updates simplicity) but the fact that you may move these associations to an external configuration file (in case you need it). \$\endgroup\$ – Adriano Repetti Jul 21 '17 at 13:20
  • \$\begingroup\$ It worked perfectly, and looks so much better. Thank you very much \$\endgroup\$ – Kaito Kid Jul 21 '17 at 13:46
  • \$\begingroup\$ @Kaito You welcome! \$\endgroup\$ – Adriano Repetti Jul 21 '17 at 13:49
0
\$\begingroup\$

I would solve this using flags, reserve a few bits for the type and the others for the domain.

Typing this on mobile, but I’d do it something like this:

// This can use the first 16 bits
enum Type {
    Type1 = 1 >> 0,
    Type2 = 1 >> 1
}

// This can use the last 16 bits
enum Domain {
    Domain1 = 1 >> 16,
    Domain2 = 1 >> 17,
    Domain3 = 1 >> 18,
    Domain4 = 1 >> 19
}

// This combines both
enum DatabaseConfigType {
    Type1Domain1 = Type.Type1 | Domain.Domain1,
    Type1Domain2 = Type.Type1 | Domain.Domain2,
    Type1Domain3 = Type.Type1 | Domain.Domain3,
    Type1Domain4 = Type.Type1 | Domain.Domain4,
    Type2Domain1 = Type.Type2 | Domain.Domain1,
    Type2Domain2 = Type.Type2 | Domain.Domain2,
    Type2Domain3 = Type.Type2 | Domain.Domain3,
    Type2Domain4 = Type.Type2 | Domain.Domain4,
}

Then to get the correct DatabaseConfigType, you just do (DatabaseConfigType)(type | domain).

Obviously you can allot more bits for types or domains, but that is depending on your personal situation.

| improve this answer | |
\$\endgroup\$
  • 4
    \$\begingroup\$ Takes me "a lot" of effort to read this code. Using bit-shifting to solve a logical problem seems inappropriate to me \$\endgroup\$ – Etsitpab Nioliv Jul 21 '17 at 14:30
  • \$\begingroup\$ @EtsitpabNioliv agreed; the problem here is the relationship of data, which is why a map that expresses that relationship makes the most sense. \$\endgroup\$ – D. Ben Knoble Jul 21 '17 at 14:35
  • \$\begingroup\$ @vrwim my +1, at first sight it's little bit hacky but (assuming it's possible to change types declaration) I don't dislike the idea (but I'd move the all the bit shifting into DatabaseConfigType, no reason to make all the other types more complex). Not sure if I'd use it in real-world but it might be a viable solution. \$\endgroup\$ – Adriano Repetti Jul 21 '17 at 14:37
  • \$\begingroup\$ @AdrianoRepetti what do you mean move all bit shifting to DatabaseConfigType? I set up 2 flags enums and then just ORed them together. You can’t move all bit shifting to the DatabaseConfigType as that would introduce possible errors and remove the ability to extract the type or domain from the resulting value. \$\endgroup\$ – vrwim Jul 23 '17 at 14:01

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.