He has some decent points, but on other points I'd say "he'll get wiser"
- Carmack himself has
also replied, stating that "
In some ways, I still think the Quake 3 code is cleaner, as a final evolution of my C style, rather than the first iteration of my C++ style" and also "
In retrospect, I very much wish I had read Effective C++ and some other material." - which to me translates as "this is not how I'd do it today" and definitely not being idiomatic C++ all the way through.
A few comments...
Unified Parsing and Lexical Analysis - i.e., using (the same) text format for all resources). Shawn praises that, but here's what
Carmack has to say about it:
Fabien Sanglard - So far only .map files were text-based but with idTech4 everything is text-based: Binary seems to have been abandoned. It slows down loading significantly since you have to idLexer everything....and in return I am not sure what you got. Was it to make it easier to the mod community ?
John Carmack - In hindsight, this was a mistake. There are benefits during development for text based formats, but it isn't worth the load time costs. It might have been justified for the animation system, which went through a significant development process during D3 and had to interact with an exporter from Maya, but it certainly wasn't for general static models.
...might have been a decent compromise keeping source material in text format, but create a binary representation as well - not necessarily fully specialized formats for each resource type, but a model like XAML/BAML might have been a natural fit?
Const and Rigid Parameters - pretty much spot on. C++ style const specifiers is something I miss in other languages. It's also nice to see that Carmack uses const-ref for input and pointers for output, it's IMHO good practice. It does mean you need null-checking, but IMHO it's an OK compromise (the stuff I do with output parameters tends to be hard to end up with a nullptr for).
Minimal Comments - pretty spot on, IMHO.
Spacing - disagree. The additional code-on-screen I'd get from putting braces on the same line doesn't matter too much... the readbility drop from cramped code and not being able to line up braces visually weighs a lot more. And I like blank lines between logical chunks of code. Dunno if there's been done any studies on this or if it's just down to personal preference, but my approach works a lot better for me :-). Oh, I fully agree with always using braces, even for single-line statements.
Minimal Templates - I'm a bit mixed with regards to this. Parts of the STL are somewhat sucky (remove+erase is a good example), and before C++11's lambdas, using std::algorithm was often extremely clunky and ugly. OTOH, for the most part the STL datatypes are easy to use and you get decent enough performance out of the box. Now, if you have code that's extremely sensitive to
LORw or benefits massively from pooled allocation (either for speed or for avoiding heap fragmentation), it might make more sense to roll your own rather than mucking around with allocators and whatnot. But I'd definitely default to STL for 'anything normal'. And
auto is a really great new feature, it doesn't make code hard to read (quite the opposite!) unless abused.
Anyway, Carmack being sceptical of STL probably made a lot of sense back when they started the doom3 codebase (game released in 2004, so several years before that - that would probably mean VC++ from VS.NET2002 (at least during start of development, perhaps VS.NET2003 for release?)), there's been several bugs and performance problems in STL implementations over the years... but it's 2012 now.
Remnants of C - getters/setters are often overkill, but I'm not fond of Shawn's examples. For immutable objects, having fields public can be OK (though one might argue that for the sake of binary compatibility for future upgrades, it might be better to use an accessor function anyway). But direct access to mutable fields? Ugh. I guess it's mostly a code smell to me since I tend to belive mutable objects implies "complex stuff", where you'd want some logic attached to the action of mutating.
StringStreams are ugly, but printf is unsafe - solution? use some safe formatting code. Been a while since I took a look, but there's several to choose from depending on your speed/flexibility needs.
Horizontal Spacing - pretty much agree.
Method Names - somewhat agree. I do prefer function names that read like English, but for simple & common & well-defined methods like size() and length(), I prefer not having the get prefix. In general, I'm not fond of getters/setters, I find that they read less naturally - still not sure what the most elegant solution is. I've toyed around with the idea of simply naming the accessor methods from the field name, which does read nicely... but is somewhat non-standard. Oh, and it feels wrong that the 'setter' functions are hard to discern from other functions, and you lose the value of having getXxx and setXxx methods grouped in auto-completion (which is nice for discoverability in a big codebase). ObjectPascal and C# properties are nice.
And finally,
Yes, it's Beautiful - the codebase might very well be, but I don't find any of Shawn's examples
beautiful in themselves, more along the lines of "this looks like decently engineered code"
