last day (16 days later) » 

03:53
100
Q: My boss asks me to stop writing small functions and do everything in the same loop

TivBrocI have read a book called Clean Code by Robert C. Martin. In this book I've seen many methods to clean up code like writing small functions, choosing names carefully, etc. It seems by far the most interesting book about clean code I've read. However, today my boss didn't like the way I wrote code...

Self-documenting code may be good, but notice that your version uses 6× more code! “Keep it simple” should be kept in mind. And neither version addresses the actual source of complexity in that piece of code: the conditional. If you extract a function, it should be phoneNumer = getPhoneNumber(headers), thus simplifying the main code, and only increasing the amount of code by 3×.
When reading most of the books targeted at this industry, never drink too much of the authors' Kool Aid.
If the one line of code is clear and easy to understand - why do you need to split it into N lines? I would agree with you boss regarding the code in your question
if isApplicationInProduction isn't used anywhere else, then it's not worth making it into a function as isn't complicated by itself.
The side discussion about our policy on duplicates has been moved to Meta: When is it appropriate to vote to close as a duplicate?
03:53
Frankly, having logic in your production code that is used only for testing is a massively huge design issue. How you structure ifs and whether you use a function is irrelevant in the face of such a bad practice. Forget about this and go learn about dependency injection.
I'd say use short functions and let the compiler inline them so you get speed of 1 function with the readability of many, but I don't think that applies to JS... :(
The Peter Principle says that people get promoted to their level of incompetence. It seems to apply to best practices as well - they tend to get implemented to the level they actually harm the codebase...
Sorry, I don't take people serious who recommend writing functions with >300 lines.
Argh, why do so many developers have their bosses telling them how to write code? Here is my take. Who will be supporting/maintaining the code in production? He who maintains it should have the most control over how it gets written. Write it once (or twice, or thrice), maintain it forever. Optimize for maintainability.
"Any fool can write code that a computer can understand. Good programmers write code that humans can understand." --Martin Fowler
03:53
Irrelevant to the question, but in JavaScript phoneNumber = headers.resourceId ? headers.resourceId : DEV_PHONE_NUMBER; can be written as just phoneNumber = headers.resourceId || DEV_PHONE_NUMBER;.
@Valery "why do you need to split it into N lines?" - If your "productivity" (and salary increase) is measured by counting the number of lines of code you write, that's a good enough reason ;)
If the boss is passionate about code style, you could try and lend him your copy of the book.
_.has(headers, 'resourceId'); looks to me like a very unreliable test for whether the application is running in production!
Until you said "Javascript", I was mostly with your boss, but for completely different reasons. Coming from the embedded side, making super-responsive "apps" work on a 20MHz clock, maybe 1k of RAM, and 4k for instructions, the use of a function is a bit different compared to desktop/web development. For example, the function call itself might be unaffordable overhead for a task that has to finish in X-number of clock cycles. Or I might combine some seemingly unrelated things into one function that is never used in its entirety, but it saves me a few bytes of code space to make it fit.
Sure, it'd be a lot easier to use a bigger/faster chip and not worry about it, but when you're designing for mass production, there's a pretty good motivation to cram all you can into the smallest/cheapest fleck of sand that can be made to do the job, even if only barely.
You should never write code in a particular code style because a book said so. You should always write code in a particular code style because it's better. However, books may give you inspiration of possible styles and reasons you hadn't thought of.
03:53
Great video on exactly this topic: youtube.com/watch?v=Bks59AaHe1c
@AaronD: The other side of this is when you're doing compute-intensive stuff, and you're already using the biggest, fastest chip they make - or maybe 64K of fairly fast chips tuned for low power consumption, so the machine doesn't melt. Sometimes it's clearer to put stuff in functions, and overhead is minimal. Other times it's not. Bottom line here is who signs the OP's paycheck?
For a project large enough for anything to matter; you can expect to waste 30+ seconds finding the function just to confirm that it actually does do what its name suggests it might do. In this case, for your code, it'd be faster to delete it and rewrite it than it would be to read it. That is a huge red flag.
@jamesqf: Yep, there's that too. Fortunately, the only DSP work that I've had to do so far is to vastly oversample some analog inputs (~4kHz for a 50Hz PID controller), IIR lowpass them, and offer the results as global variables for the rest of the code to pick up as it needs. With a bit of analog noise, this produces several bits better resolution than the ADC can provide directly. I say so far because there's at least one project that's going to need some bandpasses in the near future, and they're not as trivial as a first-order lowpass.
(This is on a non-DSP chip, by the way. Integers only, hardware multiply, no divide instruction. Fixed-point is wonderful once you understand it!)
@Brendan: Sometimes, if I can spare the overhead, I'll make a short function like that because I don't know all the details yet. I make a guess, call it everywhere, and in the likely event that my guess was wrong, there's only one place to fix it.
I wouldn't write a function if that function was only called once from one place.
usr
usr
No need for a function if you just want to name an expression. bool isApplicationInProduction = headers.resourceId; or similar is more light-weight.
03:53
I have read that book as well, and generally agree with its ideas. However, when it comes to small functions, the author significantly goes way beyond the limit of what is good IMHO: A function needs a bit of meat to be a good abstraction (= there must be something from which it can abstract), otherwise you just replace spaghetti code with rice code. Personally, I like functions of something like ten lines best, but that is a matter of taste.
@cmaster For me this book was more like an utopia. Something you must try to reach and I do agree, a good function should have around 10 lines.
Kos
Kos
Great story on exactly this topic: thecodelesscode.com/case/189 We have a printout of this sticked to a wall.
@AaronD: Sure, if the same code is used in multiple places then it makes some sense. That's not what the OP is describing though. To me, the OP is describing an excessive number of tiny functions that are not split logically (to improve readability) but are instead split to fit an arbitrary line count (to destroy readability).
@TivBroc I agreed with Brendan. You gave just one example (maybe not the best one) but we don't know how many of them have you been implementing all over the project. Might have you been abusing a little of so atomic functions? Be a code reviwer for a moment and try to read your code. Too many jumps from function-to-function and back to the previous one? Are these functions one line functions? Try first to look at it from your boss' point of view.
If it goes against everything you've read, you haven't read widely enough.
03:53
Skimming the comments/answers, it looks like no one has pointed out that this code is doing different things. Your version would take resourceId if it's defined, whatever the value is. His version would take resourceId if it's defined and a non-falsy value. Solely on technical meaning, I'd say typically his version is what you would want, so setting it to null or false or the value undefined doesn't have unexpected results further down the line (this of course assumes 0 isn't a valid resourceId). Though if that is the case, the idiomatic || form would better.

  last day (16 days later) »