9 ms·

I suggest the Doom3 source code. https://github.com/TTimo/doom3.gpl https://github.com/TTimo/doom3.gpl I find it to be extremely readable, and it has a C with

I suggest the Doom3 source code. https://github.com/TTimo/doom3.gpl https://github.com/TTimo/doom3.gpl I find it to be extremely readable, and it has a C with classes approach that I tend to gravitate towards when I'm developing in C++. It is not an example of "modern C++".

jstimpfle 6y ago
Second that, it's beautiful.
tayistay 6y ago
Not terribly impressed. This wouldn't pass my code review: https://github.com/TTimo/doom3.gpl/blob/aaa855815ab484d5bd095f347163194ac569abcc/neo/renderer/MegaTexture.cpp#L748 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/aaa855815ab484d5bd095f347163194ac569abcc/neo/game/physics/Physics_Player.cpp#L168 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_programming_in_C.php 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 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.
- Guzba 6y ago
  Counter argument: http://number-none.com/blow/john_carmack_on_inlined_code.html http://number-none.com/blow/john_carmack_on_inlined_code.htm...
  - typon 6y ago
    I hope you read his 2014 update.
    - free_rms 6y ago
      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?
      - typon 6y ago
        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.
        free_rms 6y ago
        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.
  - leetcrew 6y ago
    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.
- gabereiser 6y ago
  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.
  - tayistay 6y ago
    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".
  - Nuzzerino 6y ago
    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.
- stagger87 6y ago
  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.
  - tayistay 6y ago
    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.
- raverbashing 6y ago
  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
- arriu 6y ago
  I've always encouraged people to check out Jonathan Blow's talk on this topic ( https://youtu.be/JjDsP5n2kSM?t=817 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 https://github.com/ImageMagick/dcraw/blob/master/dcraw.c
  - tayistay 6y ago
    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.
    - unionpivo 6y ago
      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.
kartayyar 6y ago
The problem with this codebase is it's not modern C++. Modern C++ and it's use of std::unique_ptr / std::move is so much nicer vs. manual memory allocation.

jstimpfle 6y ago

Second that, it's beautiful.

tayistay 6y ago

Not terribly impressed. This wouldn't pass my code review: https://github.com/TTimo/doom3.gpl/blob/aaa855815ab484d5bd095f347163194ac569abcc/neo/renderer/MegaTexture.cpp#L748 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/aaa855815ab484d5bd095f347163194ac569abcc/neo/game/physics/Physics_Player.cpp#L168 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_programming_in_C.php 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 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.

Guzba 6y ago
Counter argument: http://number-none.com/blow/john_carmack_on_inlined_code.html http://number-none.com/blow/john_carmack_on_inlined_code.htm...
- typon 6y ago
  I hope you read his 2014 update.
  - free_rms 6y ago
    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?
    - typon 6y ago
      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.
      - free_rms 6y ago
        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.
- leetcrew 6y ago
  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.
gabereiser 6y ago
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.
- tayistay 6y ago
  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".
- Nuzzerino 6y ago
  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.
stagger87 6y ago
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.
- tayistay 6y ago
  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.
raverbashing 6y ago
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
arriu 6y ago
I've always encouraged people to check out Jonathan Blow's talk on this topic ( https://youtu.be/JjDsP5n2kSM?t=817 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 https://github.com/ImageMagick/dcraw/blob/master/dcraw.c
- tayistay 6y ago
  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.
  - unionpivo 6y ago
    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.

kartayyar 6y ago

The problem with this codebase is it's not modern C++. Modern C++ and it's use of std::unique_ptr / std::move is so much nicer vs. manual memory allocation.