Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am in two minds about function calls as parameters for other functions. The doctrine that readability is king makes me want to write like this:

br = mechanize.Browser()
raw_html = br.open(__URL)
soup = BeautifulSoup(raw_html)

But in the back of my mind I feel childish doing so, which makes me want to write this:

br = mechanize.Browser()
soup = BeautifulSoup(br.open(__URL))

Would it actually look unprofessional to do it the first way? Is there any serious reason to choose one method over the other?

share|improve this question

3 Answers 3

up vote 5 down vote accepted

I'm typically an advocate of liberal use of space, but in this situation, I'd go with the third option:

soup = BeautifulSoup(mechanize.Browser().open(__URL))

(I'm not actually sure if that's valid syntax or not. I'm not very familiar with Python [I think it's Python?].)

I find that just as readable. There are of course situations where you must separate it though. The first thing that comes to mind is error handling. I suspect that open throws exceptions, but if it were to return boolean false on failure rather than a string, then I would be a fan of option two. Option two would still be brief, but would allow for properly checking for the false return.


I think this really comes down to personal taste. There's no magical rule book for this type of thing (though I'm sure there are convoluted, borderline-dogmatic metrics somewhere that support one way or the other).

I tend to just eyeball things like this and see if they make me visually uncomfortable or not.

For example:

superLongVariableNameHere(someParamWithLongName, someFunctionLong(param1, param2), someFunc3(param, func4(param1, param2, func5(param)))

Makes me cry a little whereas:

func(g(x), y(z))

Seems perfectly fine. I just have some weird mental barrier of what length/nesting level becomes excessive.


While I'm at it, I'd also disagree with Juann Strauss' dislike of JavaScript's practically idiomatic use of anonymous and inline-defined functions. Yes, people go overboard sometimes with a 30 line callback, but for the most part, there's nothing wrong with a 1-10 line anonymous function as long as it's used in one and only one place and it doesn't overly clutter its context.

In fact, to have a closure, you often have to define functions in the middle of scopes (well, by definition you do). Often in that situation you might as well inline it within another function call rather than defining it separately (once again, provided the body is brief).

If he means this though, then yeah, I'd agree 95% of the time:

f((function(x) { return x * x; }(5))

That gets ugly fast.

share|improve this answer
1  
+0.5 for pointing out that sometimes variables are necessary for error checking; +0.5 for disagreement with Juann's personal feelings. –  Ekkehard.Horner Feb 4 '13 at 8:11
    
+1 This is the reverse of Introduce Explaining Variable and IMHO a good example of when the refactoring isn't needed. The raw HTML is used just once right away, and it's pretty clear that opening a URL would produce HTML. The one-line statement is clear and concise. Introducing temporary variables in this case could mislead the reader to think there's more going on here than there is. –  David Harkness Jul 2 at 0:03

"readable" is a relational predicate. It does not depend on the text alone, but also on the person looking at it. So learning a language means to get familiar with statements like

soup = BeautifulSoup(mechanize.Browser().open(__URL))

that only use standard names to refer to often used components. The more you use this style, the more 'natural' it gets.

Using unnecessary variables

brow = mechanize.Browser()
...
br = "<br /"
...
raw_html = br.open(__URL)
soup = BeautifulSoup(rwa_html)

forces you to invent non-standard (good for the specific context) names and increases the risk of typos or confusion - mistakes that happen randomly even to a pro.

share|improve this answer

If it's only one parameter, then sure, why not.

I personally hate the way JQuery syntax (and recently C# and Scala) is usually written with lots of inline function calls (and even inline functions complete with bodies!).

Readability is key. But if you can't make your code readable, then you should at least put in a comment explaining what it is you're trying to do.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.