Thanks for the review. > Please prefer let, here and elsewhere. As you wish. I'd be curious to hear your rationale for not using const though. If a programmer doesn't intend for a variable to be re-assigned then why wouldn't they use const to express that? Preferring const over let makes code easier to read and understand. (See https://eslint.org/docs/rules/prefer-const ) It's why languages like Rust default to immutability. When do you think I should use const instead of let? > But, I don't think you need this temp, you can just tag on the .reduce Yes, but I wanted to have the temp because it makes for a clearer separation between the two steps of getting the input from the form and then processing it. Anyway, on second look I found a way to simplify things and avoid the reduce. > better to use .test() when not using the results. But it could be easier to > just do value => value.trim(). It wouldn't ever be null Ah, makes sense, good call. (The new patch avoids the need to filter.) > Please just pass addressInput as the only param, the rest is not needed. Done. > Seems there is old junk around for a few other callers of this too. Can you > take care of those too? I'll do a separate patch for that.
Bug 1584211 Comment 20 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
Thanks for the review. > Please prefer let, here and elsewhere. As you wish. I'd be curious to hear your rationale for not using const though. If a programmer doesn't intend for a variable to be re-assigned then why wouldn't they use const to express that? Preferring const over let makes code easier to read and understand. (See https://eslint.org/docs/rules/prefer-const ) It's one reason languages like Rust default to immutable variables. When do you think I should use const instead of let? > But, I don't think you need this temp, you can just tag on the .reduce Yes, but I wanted to have the temp because it makes for a clearer separation between the two steps of getting the input from the form and then processing it. Anyway, on second look I found a way to simplify things and avoid the reduce. > better to use .test() when not using the results. But it could be easier to > just do value => value.trim(). It wouldn't ever be null Ah, makes sense, good call. (The new patch avoids the need to filter.) > Please just pass addressInput as the only param, the rest is not needed. Done. > Seems there is old junk around for a few other callers of this too. Can you > take care of those too? I'll do a separate patch for that.