18 ms·

I spend 90% of my "code time" (meaning time interacting with actual source code in some way) reading it rather than writing it. At the moment I'm reading https

I spend 90% of my "code time" (meaning time interacting with actual source code in some way) reading it rather than writing it. At the moment I'm reading https://github.com/golang/go/blob/master/src/encoding/json/encode.go https://github.com/golang/go/blob/master/src/encoding/json/e... to diagnose a problem in my code. Overall, the readability is good. My only complaint is the terse variable names, which makes the more complex methods more time consuming to follow (because you have to keep going back to refer to their definitions to remember what they are). For example, https://github.com/golang/go/blob/master/src/encoding/json/encode.go#L704 https://github.com/golang/go/blob/master/src/encoding/json/e...

seanmcdirmid 8y ago
The variable naming scheme in that code is incredibly consistent with respect to type. That should alleviate some of the problems, and I think the code would be more annoying to read if the variable names weren’t terse (like Simonyi Hungarian).
- fouc 8y ago
  The consistency/types help a lot, but I would rather read 'destination' instead of 'dst', 'value' instead of 'v' etc in methods or blocks that are more than 2-3 lines. I don't want to have to guess or think about what the variable is supposed to be about.
  - seanmcdirmid 8y ago
    I actually feel like this would hinder readability, since we should be trying to read it like code rather than English. Destination is just as vague to me as dst, ditto for value and v, and I don’t have to move my eyes as much horizontally to read a line of code, not to mention that more verbose makes would sacrifice readability of the algorithmic structure for more context that quickly becomes redundant.
- 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 ago
    Type 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 ago

seanmcdirmid 8y ago

The variable naming scheme in that code is incredibly consistent with respect to type. That should alleviate some of the problems, and I think the code would be more annoying to read if the variable names weren’t terse (like Simonyi Hungarian).

fouc 8y ago
The consistency/types help a lot, but I would rather read 'destination' instead of 'dst', 'value' instead of 'v' etc in methods or blocks that are more than 2-3 lines. I don't want to have to guess or think about what the variable is supposed to be about.
- seanmcdirmid 8y ago
  I actually feel like this would hinder readability, since we should be trying to read it like code rather than English. Destination is just as vague to me as dst, ditto for value and v, and I don’t have to move my eyes as much horizontally to read a line of code, not to mention that more verbose makes would sacrifice readability of the algorithmic structure for more context that quickly becomes redundant.
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 ago
  Type 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 ago