Comment by tayistay | Hacker News Reader

Comment by tayistay | Hacker News Reader

Parent Full thread

Not terribly impressed. This wouldn't pass my code review: https://github.com/TTimo/doom3.gpl/blob/aaa855815ab484d5bd09...

Function is 200 lines long, with 8 or so levels of nesting. Also "goto breakout"

To address the objections below, the function reads from a file into a pixel buffer. It's not some tricky in-place update. That's a great candidate for a more functional style.

Here's more ick:

https://github.com/TTimo/doom3.gpl/blob/aaa855815ab484d5bd09...

Surely that could be factored better.

Down-voters go read Carmack's own article:

https://gamasutra.com/view/news/169296/Indepth_Functional_pr...

I think it's funny that people disagree here. This is exactly the stuff a modern linter would flag in an automated code review.

Sure enough, here's a linter. I think this is essentially the same codebase:

https://lgtm.com/projects/g/Edgarins29/Doom3/context:cpp

"Code quality: D" (on an A-F scale)

And by the way, I have the utmost respect for Carmack. I just wouldn't hold up this codebase as great.

It's funny that your drive-by swipe uses an example that I might use to show why this code base is so great!

The code is readable and self documenting. The file format is practically documented by reading this code. The function has a single obvious purpose.

The function length and nesting is more a result of the file format itself. Seems like a waste of time breaking this function up. It would serve no other purpose than delaying the ship date and making this function less readable, a function that may likely never need to be visited again.

Well, this is about nice code to look at, not avoiding delaying the ship date.

Moreover, here's approximately how I'd write that TGA loading function:

1. Write a function to load just a header. 2. Unit test the function with a header. 3. Write a function to do the RLE decoding. 4. Unit test the function with some data. 5. etc. 6. Start assembling the pieces. 7. Unit test the whole thing.

Meanwhile, you tried to write it all in one monolithic function, and so now you're testing the whole thing (hopefully with a unit test) and you're staring at the debugger (or worse, some printf output) wondering what little mistake you made. Maybe if you're brilliant, like Carmack, you beat me to the finish line. But most of us are mere mortals.

I've always encouraged people to check out Jonathan Blow's talk on this topic ( https://youtu.be/JjDsP5n2kSM?t=817 ). He points out many reasons why this type of code would actually be the optimal version when weighing other factors.

Simple code is easy to write, maintain, and optimize later. Despite the code looking messy, I find it easy to understand and navigate.

A counter example of truly confusing code, which kind of does similar things, would be something like this: https://github.com/ImageMagick/dcraw/blob/master/dcraw.c

I think I've watched that talk before. The starting point you linked to talks about when to do linear searches. Anyone with some experience knows that stuff. It's a good thing to tell undergrads in case they missed it, but Blow always acts like he's some rebel and conventional wisdom has it all wrong, which isn't really the case. Conventional wisdom says there's a place for linear searches (after all, lsearch()).

Anyway, he's just wrong about long functions being a-ok. I suspect he doesn't write many unit tests. This is the main benefit of separating the code into smaller functions. You can test each. Even if it's just parsing an image file header.

That some of the folks here think smaller functions are useless because they would only be called from from one function just shows they don't write enough tests.

That kind of function would be tested with couple of correct files and several, wrong ones.

Splitting everything up and testing separately, make sense if you are building a library or general user program. For program where you control whole toolchain its overkill.

You need to be able to read correct image and hopefully not segfault on bad one. And this function does that.

Counter argument: http://number-none.com/blow/john_carmack_on_inlined_code.htm...

I hope you read his 2014 update.

The whole function is operating on a giant shared buffer representing a texture, though. So you can't break it into pure functions without introducing unacceptable copies of the buffer, right?

It depends on what you're doing. If parts of the giant function are only reading the buffer, than you can just pass it as a const T& to your smaller inner function. If the inner function is modifying it, then i'd still pass it in, just as a T&. Or maybe I'd define an immediately invoked lambda that captures the buffer by ref. Functions are a tool for defining interfaces to pieces of logic - maybe I'm not as smart as Carmack and I don't have such a huge working memory, but I just can't keep more than 3-4 concepts in my head at the same time. Functions let you isolate those concepts and only think about their interface, rather than their internals.

I think the principle behind Carmack's letter is that factoring into multiple functions that all touch the same state is an illusion of reducing the burden on your memory. They still interact with each other via that state.

Pure functions, or your const T& situation, actually create smaller arenas of state. Splitting out multiple functions operating on the same object does not.

I think about that email a lot at work (where we have some very long functions). I particularly like this excerpt, which is applicable outside of performance-critical situations:

> Besides awareness of the actual code being executed, inlining functions also has the benefit of not making it possible to call the function from other places. That sounds ridiculous, but there is a point to it. As a codebase grows over years of use, there will be lots of opportunities to take a shortcut and just call a function that does only the work you think needs to be done. There might be a FullUpdate() function that calls PartialUpdateA(), and PartialUpdateB(), but in some particular case you may realize (or think) that you only need to do PartialUpdateB(), and you are being efficient by avoiding the other work. Lots and lots of bugs stem from this. Most bugs are a result of the execution state not being exactly what you think it is.

in general I don't think it's worthwhile to split a long function into several static helpers just to get under an arbitrary maximum function length target. I don't think it leads to a net improvement in readability, since I now have to go back and forth between the helpers and the main function to see in what order the helpers get called (what if someone swaps the order of helperA and helperB but not the order of their definitions?). imo this is only worth doing if you're also willing to think long and hard about what happens if someone uses your helpers in a different context.

This exposes one of the weaknesses of C and C++. Nesting functions inside functions is actually a very useful thing to avoid precisely what's described, while still allowing code to be more readable. The inner functions can always be extracted later if deemed appropriate. So if C permitted it you could do something like:

  void FullUpdate(...) {
    void PartialUpdateA(...) {
      ...
    };
    ...
  }

So that function is only visible in this one scope, but where it's used it can have the effect of greatly improving readability (especially if it's called multiple times).

Now, C++ can halfway get there with private methods in classes. So anyone outside the class has to really try to break that encapsulation and access the function. C and C++ can get there by not exposing the functions in headers, so they remain file local.

But that doesn't prevent something like (within a file):

  // should only be called from FullUpdate
  void PartialUpdateA() {...}

  void AnotherFunc() {
    ...
    PartialUpdateA();
    ...
  }

You can do that fairly easily with C++ lambdas. I think the primary deficiency, though, is that the functions aren't exposed for unit testing.

C++ lambdas do solve the problem, I'm not sure why I didn't include that.

However, unit testing them is not the only concern. There are reasons for using nested functions, class methods, file global, or program global functions.

If you make them global or methods, you lose control of how and when they're called. This can break invariants. So some functions can be hoisted up, but others oughtn't be (in particular, any pure function can be made global without any concern other than occupying a name, side effecting functions should be more carefully considered).

The interface to the functions may change if they capture any variables. If they capture nothing in the local scope, then hoisting them doesn't impact their interface. If they do capture something, hoisting them means adding parameters (complicating the interface) or making them observe variables either in the class (complicating the class) or file/program globals (bad practice).

Regarding unit testing. Nesting functions (or lambdas) are essentially a wash. You were, hopefully, testing the host function to begin with so nothing is changed if you use nesting functions as your first pass refactoring approach. You can then examine those functions and consider which should be moved out and why, and then add tests to any that have been pulled out of the host function.

C++ gives you enough access control so you can expose things just for testing if you want (make the test a friend, encapsulate the functions into another class). You can do this without breaking encapsulation.

If the functions are capturing a lot of variables, then I would try to reconsider the design. Usually things aren't irreducibly complex.

I don't exactly follow your last paragraph, but tests aren't just something you throw away when they pass. So if I were to write tests for the helper functions, I wouldn't delete those tests on a refactoring pass in order to use nested functions.

> C++ gives you enough access control so you can expose things just for testing if you want (make the test a friend, encapsulate the functions into another class). You can do this without breaking encapsulation.

If tests can do this, then so can anyone else. Consequently encapsulation is broken and your invariants aren't invariant anymore.

> I don't exactly follow your last paragraph, but tests aren't just something you throw away when they pass. So if I were to write tests for the helper functions, I wouldn't delete those tests on a refactoring pass in order to use nested functions.

I didn't write clearly because I didn't re-present the context of that paragraph.

I'm not talking about throwing away tests after they're run. Keep in mind my original post's context: manually inlined functions for access control to that functionality. You already can't test those separately because they aren't exposed. By moving to nested functions you regain some semblance of reasonability (versus 1k+ line functions with who knows how many levels of nested blocks) and the compiler can do the inlining (for performance). But it has zero net effect on testing, it's a wash. Because the public interface is the same (only the primary function interface is accessible to a tester).

If nested functions are available (and with C++ they are with lambdas) the refactoring would (or could) be something like: 1k line function => 500 line function with several lambdas => 3-8 functions totaling ~500 with some lambdas remaining.

Only those that make sense to move out for separate testing would be, and only if you also wanted to expose them for others to call.

> If tests can do this, then so can anyone else.

If I had an image loader class, I can make a test function a friend, which would allow it to call private functions on the class. This only grants access to the test function (or test class). And there are stronger ways to hide things, like the PIMPL idiom, private headers, opaque pointers.

I find it interesting we're in such different schools of thought here.

> Only those that make sense to move out for separate testing would be, and only if you also wanted to expose them for others to call.

There are so many things that you might want to hide from an interface, yet still test. Imagine if you took that to an extreme and only tested the public interface of a library. I'm all for trying to independently test any bit of code that fills a screen.

I suppose you're not implementing safety-critical code then, as AV rule 1 [1] demands that "Any one function (or method) will contain no more than 200 logical source lines of code (LSLOCs)."

Personally, I just find it frustrating, if functions don't fit on the screen anymore (and I do use portrait mode already). Further, sub functions, when named appropriately (definitely not like helperA and helperB) can aid readability (as would comments about code blocks do, but who writes those and who maintains those?).

[1] https://stroustrup.com/JSF-AV-rules.pdf

you are correct; I do not work on safety-critical code. I have worked with a lot of legacy code where functions depend on and mutate global state, often in fairly subtle ways. as much as possible, I want to see exactly which globals are being read/written and in what order. from this perspective, the "whole function" doesn't fit on one screen, whether or not sub-functions are factored out, and the name of a function cannot possibly tell me all I need to know about it.

Common image decoding code from early 2000s C++. This pattern was actually preferred because it kept the nuance of the image file structure visible within the function. Yes, it's heavy handed now... but back then it was useful to know the "blocks" and "strides" of images when you had to make pixel-perfect changes and didn't have higher-level supporting code.

Could they have abstracted MakeMegaTexture_f into something that built up leaves of tga structs they interweave in that function? Or just chunk through them with the tga data structure. Our standards for what is good code has changed with our understanding of code.

I was coding C++ (for graphics) in the early 2000s, and I (and I think most others I knew) would have considered it bad back then. But regardless, the topic isn't "C++ code that was considered good when it was written".

I have written similar code but with more optimizations. It's much faster to allocate chunks of memory onto the stack, then do small amounts of decoding into this stack memory before committing the memory back into the heap. I don't think it is good code from a performance standpoint.

That code from the 1st line was most likely converted from existing graphics code in C

Can you rewrite it in a way that's readable, performant and understandable/maintainable to someone with an understanding of the knowledge domain?

I agree, the code is ugly. I disagree it would be better if converted to some OOP hierarchy, it would be probably less understandable even if the code was tidier

Code like

    SnarfleBlaster = new SnarfleBlaster();
    SnuggleNerfer = new SnuggleNerfer();
    Guffles guffles = SnarfleBlaster->Blast();
    SnuggleNerfer->Tumbles(guffles);

Is tidy but absolutely dense

Reply on news.ycombinator.com