Stop mutating input parameters #22
Conversation
Mutating input parameters is bad practice.
|
BTW, thumbs up for the great spec coverage in the project! |
olefriis
changed the title
| @@ -328,17 +319,16 @@ def format_event(title, text, opts={}) | |||
|
|
|||
| private | |||
|
|
|||
| def escape_event_content(msg) | |||
There was a problem hiding this comment.
Can we keep imperative verbs for method names, i.e do_something rather than something_done ?
There was a problem hiding this comment.
Sure thing. I wouldn't prefer this myself, but I'll respect your coding style.
The reason I don't think it should be escape_event_content is that the method does not escape anything in the passed String (it used to, but not anymore), but simply returns a new string with the event content escaped.
I'll change the method name back.
|
I think, indeed, mutating the input parameters was a bad idea. Thanks a lot for taking care of the fix @olefriis ! Besides the nitpick, your changes looks great. Let's merge it this week and release a new version. |
|
Hi @yannmh! Thanks a lot for your response. Nitpicking is appreciated! I'll implement your changes right away. |
| end | ||
|
|
||
| def rm_pipes(msg) | ||
| msg.gsub! "|", "" | ||
| def remove_pipes(msg) |
There was a problem hiding this comment.
I had renamed this method to without_pipes, since it doesn't remove anything from the passed String, but since @yannmh prefers imperative verbs, I decided to rename it to remove_pipes. (I know this is not the original method name, but personally I consequently avoid abbreviations in method names. @yannmh: If you disagree with this change, just tell me and I'll change it back to the original method name.)
There was a problem hiding this comment.
Totally agree, I like remove_pipes a lot more than its abbreviated version.
|
Thanks again for the changes @olefriis ! Let's release a new gem this week 🚢 |
Mutating input parameters is bad practice.
Background info: I was trying to send an event where I was passing strings from ENV. That gave me the error
can't modify frozen String (RuntimeError), as environment variables are frozen. I was a little bit shocked to see thatdogstatsd-rubyactually mutates the input parameters.