2
\$\begingroup\$

I have this method that I feel has code repetition that can be improved but since I am new to Go, I am not quite sure how to improve it. I would appreciate if anyone can give me their opinion.

func (c *Configuration) loadFromEnvVars(key string, target interface{}, optional bool, defaultValue interface{}) error {
    err := c.configReader.BindEnv(key)
    if err != nil {
        if optional {
            switch target := target.(type) {
            case *string:
                *target = defaultValue.(string)
            case *bool:
                *target = defaultValue.(bool)
            default:
                return fmt.Errorf("default value type provided is not supported")
            }
            return nil
        }
        return fmt.Errorf("%s could not be loaded: %v", key, err)
    }
    switch target := target.(type) {
    case *string:
        *target = c.configReader.GetString(key)
    case *bool:
        *target = c.configReader.GetBool(key)
    default:
        return fmt.Errorf("configuration value type provided is not supported")
    }
    return nil
}

The two switch statements seems like code repetition but I couldn't figure out how to refactor it.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

You won't get rid of entirely, you could trade the switch cases for if statements of course:

func (c *Configuration) loadFromEnvVars(key string, target interface{}, optional bool, defaultValue interface{}) error {
    err := c.configReader.BindEnv(key)
    if err != nil && !optional {
        return fmt.Errorf("%s could not be loaded: %v", key, err)
    }
    switch target := target.(type) {
    case *string:
        if err == nil {
            *target = defaultValue.(string)
        } else {
            *target = c.configReader.GetString(key)
        }
    case *bool:
        if err == nil {
            *target = defaultValue.(bool)
        } else {
            *target = c.configReader.GetBool(key)
        }
    default:
        return fmt.Errorf("configuration value type provided is not supported")
    }
    return nil
}

The upside would be that "similar" code is now closer together.

Or, if you really wanted to, you could use reflection to get rid of the whole thing. That has an additional runtime cost, though for configuration it likely wouldn't matter.

Something like this:

func (c *Configuration) loadFromEnvVars(key string, target interface{}, optional bool, defaultValue interface{}) error {
    refTarget := reflect.ValueOf(target).Elem()
    targetType := refTarget.Type()

    var value reflect.Value

    err := c.configReader.BindEnv(key)
    if err == nil {
        value = reflect.ValueOf(c.configReader.Get(key))
    } else {
        if !optional {
            return fmt.Errorf("%s could not be loaded: %v", key, err)
        }
        value = reflect.ValueOf(defaultValue)
    }

    valueType := value.Type()

    if !valueType.AssignableTo(targetType) {
        return fmt.Errorf("incompatible types %v and %v", targetType, valueType)
    }
    
    refTarget.Set(value)

    return nil
}

Note that you have to check the types here too to make sure that the assignment call (Set) doesn't panic.

\$\endgroup\$
1
  • \$\begingroup\$ I ended up using the if/else approach....I'm not a bog fan of reflection because besides the performance hit, it feels like magic \$\endgroup\$
    – Fouad
    Aug 4, 2021 at 16:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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