3 ms·
Type isn't the important information; PURPOSE is. When you dive into the code, especially at the point I chose, you are presented with: e.string(kv.s, opt
by kstenerud 8y ago
Type isn't the important information; PURPOSE is.
When you dive into the code, especially at the point I chose, you are presented with:
e.string(kv.s, opts.escapeHTML)
And so, you have the following questions: What is e? What is kv? What is s in kv? To answer these, you have to scan upwards. e is encodeState. OK, not too bad. kv is maybe key-value of something? it comes from sv (string value maybe?), which comes from:
sv := make([]reflectWithString, len(keys))
So a string value? Or a list of string keys? A list of structs, OK. s might be a string value inside, which has ... some meaning I guess? Digging around, I see it defined as:
type reflectWithString struct {
v reflect.Value
s string
}
So it is a string, but I still don't know what its purpose is. After a bunch of digging around to see how it's used, it looks like s is a string representation of the value, but I'd need to look over more code to be 100% sure.
Now, contrast this with:
encoderState.string(keyValue.asString, opts.escapeHTML)
Now I know what this calling string() on the encoder state, using the string representation of the key value. "string" is still a bit cryptic. It could be renamed. Looking inside "string" I see that it's writing things, so maybe a better name is:
encoderState.writeString(keyValue.asString, opts.escapeHTML)
With this version, I know without having to look at any other lines that this code is supposed to write the string representation of a key value to the encoder state, doing HTML escapes. I don't need to look inside the guts of anything to figure this out. I don't even have to leave this line.
This is code U/X.
- seanmcdirmid 8y agoType is purpose. And anyways, the English alternatives are still vague and aren’t really improvements. KV is incredibly standard, even in English conversations (KV store is said more often than key value store). E is not descriptive, but neither is encodeState (WTF does that even mean?). S is a perfect abbreviation for a generic string, calling it string instead isn’t much better, really! It’s obvious in this case that it is only meant to be a generic string (what else could be meant by reflectWithString?). I do prefer method names to be descriptive, so verbs are nice. Well, unless we are dealing with very generic lambda code, then there might not be better options beyond f, g, and p.
- seanmcdirmid 8y agoAlso, as a rule of thumb: the more rarely something is used, the more descriptive its name needs to be. If something is used often in some code, terseness is more forgivable and even desirable in many cases. A great example of this is "i" as a standard indexer, or "x" and "y" as horizontal and vertical coordinates. They are used often therefore terseness can be applied and is useful. Also why "KV" is often used to be mean key-value, the concept is just very pervasive. The terseness for variable names in the linked to code seems more acceptable because those variables of consistency and repeated usage.