My favorite tactic for “killing” these is (to use the example from the post):
# e.g. "hello everyone" => "Hello Everyone"
def upcase_words(sentence)
sentence.split(' ').map!{|x| x = x[0..0].upcase << x[1..-1]}.join(' ')
end
In an ideal world the name is clear enough that someone reading the code at the call site understands what’s happening, and if they don’t the example alongside the definition hopefully gets them there.
# e.g. "col1\tcol2\n ^ woah" => "Col1 Col2 ^ Woah"
Naming it hurts in this case, because the function does not do what you named it (e.g. in a string of tab-separated values, or a string where multiple spaces are used for formatting). If you had to name it, it would be better named as split_on_whitespace_then_upcase_first_letter_and_join or leave it unnamed and hope that everyone on your team knows that split in Ruby doesn’t work as expected.
The best solution is one that embodies exactly what you intend for it to do, i.e. substitute the first letter of each word with the upper case version of itself. In Ruby, that would be:
If you had to name it, it would be better named as splitonwhitespacethenupcasefirstletterandjoin or leave it unnamed and hope that everyone on your team knows that split in Ruby doesn’t work as expected.
I disagree. You should name functions and methods based on what they’re supposed to do. If it does something else, then everyone can see it is a bug.
I don’t agree with your naming system. I think the name of your function should describe what it does instead of how it does it. If your function name describes how it’s implemented, you have a leaky abstraction.
Introducing a variable called ‘words’ is a solid hint about the unit we’re working with. We may not want to pollute the caller with a new variable, but in a subroutine that’s not a problem.
Naming it does help in this case, but mostly because the reader no longer has to scrutinize over what it’s actually doing. Isn’t this sort of like polishing a turd?
Any maintenance on that line will still have the same problems, whereas refactoring it to split it up into smaller segments AND giving it a name avoids that issue.
It gives the reader a good frame of reference to what the function’s doing. Context helps a lot when trying to read code, and although this isn’t as readable as it could be yet, it’s definitely a lot more readable than minus the function signature.
I agree with the general sentiment of this post, but the example they use seems pretty weak. I don’t even know ruby, but i have a solid idea of what that one-liner does from reading it (upcase the first word of the string; granted, it took more effort to do so than many lines I’ve read – but again i don’t know Ruby).
A better example of a darling one-liner (imo) would be the one-statement swaps in C
In fairness, I did mention that I don’t know ruby. That I got that close without knowing the language seems to indicate that that snippet of code isn’t actually that difficult to read (and, besides, both you and @magikid were able to read it).
These annoy me because people do them in the name of performance, as if the compiler wouldn’t know the best way to swap two integers. Seriously? It’s 2016.
Clang even optimizes the triple XOR trick into MOV instructions because it’s inane. Most of the time it can optimize the swap out completely into its normal register juggling anyway.
It took me a while longer than I’d like to figure out what that code does, but I think that’s more because it’s not very idiomatic ruby. I’d be happy to let this more readable single line into the codebase, because it’s somewhat more readable:
@sentence.split(" ").map(&:capitalize).join(" ")
Although I’d probably go for something more along the lines of @jamesjporter’s suggestion, given a gnarly enough one-liner that I don’t fancy rewriting right now (or some nasty implementation that future programmers shouldn’t have to care about at the call site.)
I’m pretty sure the programmer wrote that “darling” with some bad intentions in mind. Why doesn’t the author call it for what it is? The mixture of side-effecting and non-side-effecting operations, all willy-nilly and for no apparent reason, is telling.
On the other hand, languages like Go which are verbose, sometimes have the opposite problem. You have to wade through the boilerplate to figure out the point…
I like @jamesjporter’s suggestion, and use it often.
Why use map! (mutate original object) when you then join the returned value? Why upcase the first character instead of leaning on String#capitalize? :-(
I’ve always held the philosophy of “Write code like some one else is going to read it.” because, well, some one probably will. Darlings can be cool, and they’re fun to figure out (like regex strings) but the article is right in that it can waste a lot of time.
A more important step, to my mind, is having a standard and sticking to it. If your standard says “favor well named helper functions, and readable code, over darlings,” and you make a habit of following the standard during code reviews, you’ll probably catch these well before production.
Quick, what does the following line of Ruby code do?
I think the expectation that you are able to read code “quickly” is what prevents programmers from developing the skill of reading programs. Reading how you can see the code that is there, instead of the code that you think is there. How many times have we been led astray by a comment that seems perfectly clear, or worse, a function named “quick” or “fast” that wasn’t?
I have no problem seeing this:
@sentence = @sentence.split(' ').map!{|x| x = x[0..0].upcase << x[1..-1]}.join(' ')
as split " " ➡ F ➡ join " " then seeing F x as (uppercase first x) concat rest x and I don’t program in Ruby every day. Or at all. But that’s not reading.
In order to read this, I need to see ?=???{?=(??)??}? and transpose it to something more like ?=?{(??)??}??? and my eyes will dance around the line as I do it, putting the expression into an execution order that I can model. This is hard work, but once I have consumed it fully, I can say that I have read it.
Or does .upcase do what I think it does? Or .map!? Or .split(‘ ’) or .join(‘ ’)? What about those brackets? Hey, that says 0..0 is redundant. That’s good to know. I wonder why they did that? And .map!{|x| x= f} is the same as .map!{|x| f} when we’re the only reference holder, so I wonder why they did that?
Those questions that kick in there, that wondering, is a byproduct of reading when you know how to read. Funny language choice feels funny, and that’s often where I find the bugs.
My favorite tactic for “killing” these is (to use the example from the post):
In an ideal world the name is clear enough that someone reading the code at the call site understands what’s happening, and if they don’t the example alongside the definition hopefully gets them there.
You mean
Naming it hurts in this case, because the function does not do what you named it (e.g. in a string of tab-separated values, or a string where multiple spaces are used for formatting). If you had to name it, it would be better named as
split_on_whitespace_then_upcase_first_letter_and_joinor leave it unnamed and hope that everyone on your team knows that split in Ruby doesn’t work as expected.The best solution is one that embodies exactly what you intend for it to do, i.e. substitute the first letter of each word with the upper case version of itself. In Ruby, that would be:
I disagree. You should name functions and methods based on what they’re supposed to do. If it does something else, then everyone can see it is a bug.
I don’t agree with your naming system. I think the name of your function should describe what it does instead of how it does it. If your function name describes how it’s implemented, you have a leaky abstraction.
Among other benefits, giving it a name means we can explode the code without worrying about a few extra lines in the middle of the caller.
Introducing a variable called ‘words’ is a solid hint about the unit we’re working with. We may not want to pollute the caller with a new variable, but in a subroutine that’s not a problem.
Naming it does help in this case, but mostly because the reader no longer has to scrutinize over what it’s actually doing. Isn’t this sort of like polishing a turd?
That only masks the issue.
Any maintenance on that line will still have the same problems, whereas refactoring it to split it up into smaller segments AND giving it a name avoids that issue.
It gives the reader a good frame of reference to what the function’s doing. Context helps a lot when trying to read code, and although this isn’t as readable as it could be yet, it’s definitely a lot more readable than minus the function signature.
A kind of offtopic question based on this comment.
Would I use Coq to prove this function?
I agree with the general sentiment of this post, but the example they use seems pretty weak. I don’t even know ruby, but i have a solid idea of what that one-liner does from reading it (upcase the first word of the string; granted, it took more effort to do so than many lines I’ve read – but again i don’t know Ruby).
A better example of a darling one-liner (imo) would be the one-statement swaps in C
I actually think this would put the sentence in title case, not upcase the first word of the string.
Yes, it does put the sentence in title case.
I agreed with emallson—it did seem pretty easy to read—up until his interpretation of what it did, ironically proving it wasn’t easy to read. ;)
In fairness, I did mention that I don’t know ruby. That I got that close without knowing the language seems to indicate that that snippet of code isn’t actually that difficult to read (and, besides, both you and @magikid were able to read it).
These annoy me because people do them in the name of performance, as if the compiler wouldn’t know the best way to swap two integers. Seriously? It’s 2016.
Clang even optimizes the triple XOR trick into MOV instructions because it’s inane. Most of the time it can optimize the swap out completely into its normal register juggling anyway.
It took me a while longer than I’d like to figure out what that code does, but I think that’s more because it’s not very idiomatic ruby. I’d be happy to let this more readable single line into the codebase, because it’s somewhat more readable:
Although I’d probably go for something more along the lines of @jamesjporter’s suggestion, given a gnarly enough one-liner that I don’t fancy rewriting right now (or some nasty implementation that future programmers shouldn’t have to care about at the call site.)
This is a good point about a not-so-great example. If I read this I wouldn’t even bat an eye.
Splitting on whitespace is the default behavior so you can just do this:
Furthermore, there is some handy shorthand for that join, but I feel like the join is more readable:
I’m pretty sure the programmer wrote that “darling” with some bad intentions in mind. Why doesn’t the author call it for what it is? The mixture of side-effecting and non-side-effecting operations, all willy-nilly and for no apparent reason, is telling.
On the other hand, languages like Go which are verbose, sometimes have the opposite problem. You have to wade through the boilerplate to figure out the point…
I like @jamesjporter’s suggestion, and use it often.
@sentence.gsub! /\b\w/, &:upcaseWhy not just
x[0]instead ofx[0..0]?Probably old code, pre-unicode days, dating back to when x[0] was a character not a string.
Why use
map!(mutate original object) when you then join the returned value? Why upcase the first character instead of leaning onString#capitalize? :-(Not the point but here’s the code in Haskell lens:
I’ve always held the philosophy of “Write code like some one else is going to read it.” because, well, some one probably will. Darlings can be cool, and they’re fun to figure out (like regex strings) but the article is right in that it can waste a lot of time.
nested list comprehensions in python….
Definitely agree with the sentiment: favor clear, concise code over code golf. Steps to make sure this doesn’t get committed:
Review your own code at least twice before submitting it for review.
Have a code review process that ensures catching this kind of code before it goes into production.
I see this as more of a process problem than a mindset problem.
A more important step, to my mind, is having a standard and sticking to it. If your standard says “favor well named helper functions, and readable code, over darlings,” and you make a habit of following the standard during code reviews, you’ll probably catch these well before production.
I think the expectation that you are able to read code “quickly” is what prevents programmers from developing the skill of reading programs. Reading how you can see the code that is there, instead of the code that you think is there. How many times have we been led astray by a comment that seems perfectly clear, or worse, a function named “quick” or “fast” that wasn’t?
I have no problem seeing this:
as
split " " ➡ F ➡ join " "then seeingF xas(uppercase first x) concat rest xand I don’t program in Ruby every day. Or at all. But that’s not reading.In order to read this, I need to see ?=???{?=(??)??}? and transpose it to something more like ?=?{(??)??}??? and my eyes will dance around the line as I do it, putting the expression into an execution order that I can model. This is hard work, but once I have consumed it fully, I can say that I have read it.
Or does .upcase do what I think it does? Or .map!? Or .split(‘ ’) or .join(‘ ’)? What about those brackets? Hey, that says
0..0is redundant. That’s good to know. I wonder why they did that? And.map!{|x| x= f}is the same as.map!{|x| f}when we’re the only reference holder, so I wonder why they did that?Those questions that kick in there, that wondering, is a byproduct of reading when you know how to read. Funny language choice feels funny, and that’s often where I find the bugs.
And there’s more good news: When you practice your reading, you will get good at reading, and this will make you faster at other languages when you have to use them.