I think this is a better, more descriptive name. I think it's good to include the fact that the messageInfos are ordered in the name so it's more evident at the callsite.
This is an "opinionated" change, so open to abandoning if others disagree.
Details
Details
flow
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
In both usages, the variable is named threadsToMessageIDs, with no reference to the ordered messageInfos. Plus, the function is concise and fits the mold for a standard "populate a map" function that most programmers would be able to comprehend without even knowing the context of the function. Not sure why we need to make its title this verbose.
I'm okay with this revision as it improves code comprehension, but neither threadsToMessageIDsFromMessageInfos nor mapThreadsToMessageIDsFromOrderedMessageInfos are readable. That is, once read, it makes sense, but names shouldn't need to be that long to begin with.
Why can't we just name the function mapThreadsToMessageIDs? The only two usages of this function are:
In both usages, the variable is named threadsToMessageIDs, with no reference to the ordered messageInfos. Plus, the function is concise and fits the mold for a standard "populate a map" function that most programmers would be able to comprehend without even knowing the context of the function. Not sure why we need to make its title this verbose.
| lib/reducers/message-reducer.js | ||
|---|---|---|
| 99 ↗ | (On Diff #13689) | Does this comment still need to be here? The input to the function is orderedMessageInfos, implying that it should be ordered. |
| lib/reducers/message-reducer.js | ||
|---|---|---|
| 103 ↗ | (On Diff #13689) | Maybe this should be renamed. If we're so explicit about the return value of this function being a map of threads to messageIDs, we should probably make the variable return name more descriptive. You can title it mapThreadsToMessageIDs, or threadsToMessageIDs (which is also the name of the variable this function's return value is assigned to), or something else to describe that it is the map being returned. |
Comment Actions
Not sure why we need to make its title this verbose.
I agree that the function name is long, but I think it makes sense in this case. We want to make the "contract" of the function clear... the contract being that we must supply the function with ordered messageInfos. We can often enforce the "contract" of a function with the type system, but enforcing the "ordered" condition isn't something we can easily do.
The fact that this function requires the input to be sorted is critical and should be made exceedingly explicit at the callsite, so the verbosity is worth it.
| lib/reducers/message-reducer.js | ||
|---|---|---|
| 103 ↗ | (On Diff #13689) |
We're explicit about this with the type signature of the function. Doesn't hurt to change the name, but the internal variable names aren't as crucial as the "API" that's exposed externally. |
Comment Actions
The fact that this function requires the input to be sorted is critical and should be made exceedingly explicit at the callsite, so the verbosity is worth it.
This makes sense, but if it's so critical why doesn't the function order the input to guarantee that its sorted? The verbosity of this function would be resolved, and the caller wouldn't have to worry about fulfilling the preconditions of the function since the function guarantees them itself. Surely this would be safer than just renaming the function name, right?
Although I'm not sure the actual process in which the input gets sorted beforehand, so this probably isn't a viable solution if the input is sorted elsewhere. I'm only trying to avoid long variable names if we can.
| lib/reducers/message-reducer.js | ||
|---|---|---|
| 103 ↗ | (On Diff #13689) |
Normally, I'd agree with you for primitive values, i.e., if the type signature states something is an Integer or Boolean, for example, the name of the variable does not need to be explicit that it's of that type. But this is a mapping, and not only that, we are using a normal object to represent this map, not the Map object. So, the reader needs to look at the type signature to determine what threads means, parse it (since it doesn't just say Map or something), and then can figure out that it's the map itself. But this can easily be resolved by renaming the variable to any of the suggestions I made. We shouldn't rely on a reader to have to figure out that the type signature represents a mapping if we could just rename the variable to better dictate that it's a mapping. |
Comment Actions
I do agree we should rename threads to something like threadsToMessageIDs, but I'm not sure I agree with the reasoning, nor that we need to include "map" in the name.
The verbosity of this function would be resolved, and the caller wouldn't have to worry about fulfilling the preconditions of the function since the function guarantees them itself. Surely this would be safer than just renaming the function name, right?
That would require a much more substantial refactor of message-reducer... which I'm not opposed to, but is outside the scope of this diff. The goal of this diff is to rename the function to make the current expectation of sorted input clear at the callsite.
Normally, I'd agree with you for primitive values, i.e., if the type signature states something is an Integer or Boolean, for example, the name of the variable does not need to be explicit that it's of that type.
Are you suggesting that we should include the names of non-primitive types in variable names? Something like: https://en.wikipedia.org/wiki/Hungarian_notation? Should we suffix every Set with Set?
So, the reader needs to look at the type signature to determine what threads means, parse it (since it doesn't just say Map or something), and then can figure out that it's the map itself.
The type annotation is right beside the declaration:
const threads: { [threadID: string]: string[] } = {};which makes the type exceedingly clear to the developer when they encounter threads. Yes, they do need to look at the type signature, but I think that's better than attempting to transliterate the type into English (eg mapOfThreadIDsToArrayOfMessageIDs).
Comment Actions
Are you suggesting that we should include the names of non-primitive types in variable names?
No, I clarified this later in my comment:
not only that, we are using a normal object to represent this map, not the Map object
If we were using a Set explicitly, then we wouldn't need to write Set in the variable name. Same goes for Maps, and any other standard built-in objects. You said the "type signature of the function" is what should be explicit, and if the type signature says Map or Set or any other built-in object, it makes sense to the reader. However, in this case, we are using a normal object to represent the map, which has a type signature of [threadID: string]: string[]. That's definitely more confusing than just writing Map, which is why I suggested we should rename the variable to make it clearer that it's a mapping of some sort. Since I've always used the built-in Map object in JavaScript, that is probably why I was confused by the type signature, but it's definitely less readable.
You're right, we don't need to include map in the name, we just have to make it clear that it's a mapping of some sort. So what you renamed it to is perfect, because now the variable name indicates that. But in its original state, the variable name was confusing and the type definition was also confusing to me.
That would require a much more substantial refactor of message-reducer... which I'm not opposed to, but is outside the scope of this diff.
Makes sense 👍