Is it "bad form" to perform map lookup and type assertion in one statement?

Question

I just realized that it's possible to do a map lookup and a type/interface-assertion in one statement.

m := map[string]interface{}{
    "key": "the value",
}
if value, ok := m["key"].(string); ok {
    fmt.Printf("value exists and is a string: %s\n", value)
} else {
    fmt.Println("value does not exist or is not a string")
}

Is this considered bad? I've not seen any official documentation commenting this.

edit: I know this code can not distinguish between "key doesn't exist" and "value is of incorrect type".

edit2: ahem, the print in the else-clause was incorrect :(

Yanyanaton

Answer 1

What you do actually works, because you use the special comma-ok idiom of the type assertion which does not panic if the assertion does not hold, and because maps can be indexed with keys which are not in it (which will result in the zero value of the value type of the map).

It's true that with this you can't tell if the key is in the map or not, or it is but its value is nil, but you already suspected this as if the assertion does not hold, you print "value does not exist or is not a string".

To test all "corner" cases, see this example:

m := map[string]interface{}{
    "key":  "the value",
    "key2": 2,
    "key3": nil,
    // "key4":"", // Uncommented on purpose
}

for _, k := range []string{"key", "key2", "key3", "key4"} {
    if value, ok := m[k].(string); ok {
        fmt.Printf("[key: %s] value exists and is a string: %s\n", k, value)
    } else {
        fmt.Printf("[key: %s] value does not exist or is not a string: %s\n",
            k, value)
    }
}

Output (try it on the Go Playground):

[key: key] value exists and is a string: the value
[key: key2] value does not exist or is not a string: 
[key: key3] value does not exist or is not a string: 
[key: key4] value does not exist or is not a string: 

So basically you can use this, nothing bad will happen from it (e.g. panic or memory leak), just know its limits (e.g. you couldn't get the value associated for "key2" as it's not of string type).

If your intention is to get the value for a key if it exists and is of type string, then this is what your code exactly does. Although you should avoid data structures and constructs where you need this, as it's harder to understand and maintain in larger projects.

What I mean by this is for example if at some point in your code you expect the key "somekey" to have a string value associated and it does not, you won't immediately know why it is so; is it because the map does not contain that key, or is it because it does but with a value of wrong type (or the value may even be nil)? Further testing / debugging is needed to track down the root cause.

Shiller

Yeah, I did a small test in the playground to test all combinations and it does indeed work (i.e. not panic). I was mostly curious whether this was an unsupported quirk or something actually intended. – Yanyanaton

I was wondering the same, it works but the language spec doesn't say anything about it. I forgot the fact the map lookup returns nil if there was noting added with that key... – Anacardiaceous

@Anacardiaceous Map indexing returns the zero value of the map's value type, which is nil for interface{}, but for example it's 0 for int. E.g. map[string]int{}["a"] will result in 0, not nil. – Shiller

Yes, I implicitly meant map[string]interface{}. It wouldn't be possible to do type assertion if it wouldn't ;). Thanks tho. – Anacardiaceous

Answer 2

Is this considered bad?

No, this is plain wrong. You cannot combine map lookup and type assertion like this.

What you code does is:

1. It looks up "key" in the map unconditional. If "key" is not in the map it will yield nil (as this is the zero value of interface{}).

2. The value retrieved from the map (possible nil if not in the map is type asserted to a string.

Your code is incapable to determine whether "key" is in the map or not.

Rustice

"You cannot combine map lookup and type assertion like this." - Obviously you can if this code compiles. ... "Your code is incapable to determine whether "key" is in the map or not." - The OP never stated wanting to determine that. – Dayledaylight

Perhaps I should've stated that more clear (I assumed it was obvious by the messages I print); I can not distinguish between missing key and wrong value type using this "shortcut". – Yanyanaton

Exactly you cannot. Which makes your code unclear. This is wrong. You do a plain simple comm-okay type assertion, and map-lookup has nothing to do with this. It's like being astonished that (2*3)+4 is multiply-and-add in one step. No it's not. It's plain multiply and plain add. With one difference: It's clear. – Rustice

@Rustice it clearly isn't wrong. It is both correct and behaves as expected by the OP. It might instead be unclear for readers but neither wrong nor bad. See icza's answer. – Tellurium

I'll easily concede that this is somewhat unclear. I never said it was the preferred way to do it. It's ok (at least in my specific case) that the code only acts on the case where the key exists and the value is of the correct type, and nothing else. That is, its behavior is the same for non-existance and incorrect type. If the desire is to weed out all existing-but-bad-data cases, then yes, this will not cut it. – Yanyanaton