Ask HN: What are the “best” codebases that you've encountered?

> Assuming argv[++i] is not out of bounds of course.

Well, you cannot assume that - look at the code (there are links to it) - there are several different ways in which it will go out of bounds.

What's more, even if it were the case that the worst that could happen is that the variable gets set to zero and the program terminates gracefully without doing anything, that would not amount to a justification for not checking for an error, it would merely be an observation that, fortuitously, this particular bug is relatively harmless, perhaps doing nothing worse than leaving the user unnecessarily puzzled over what went wrong.

Even the example you give here (which is not actually to be found in the code in question, and so could not possibly be a candidate for the worst thing about it) is relatively harmless compared to potentially causing undefined behavior, or (what is sometimes at least as bad) defined behavior that apparently works but which gives subtly wrong answers. And, by the way, code can lie too: identifiers can make claims about the semantics of a variable or function that are not always true. The only way to avoid that is to use meaningless identifiers.

The more you try to brush this off and argue that comments are worse than actual errors, the more obvious it becomes that your priorities are askew here.

Redis code quality could be improved significantly, but you picked the wrong file to evaluate it: the first file created, and the one that for the nature of system software will be the less "overall designed", because it is the place where we call almost everything else sequentially. Still, if I had more time, I could improve it and other parts a lot. However to see how modern Redis was coded check the following:

* hyperloglog.c

* rax.c

* acl.c (unstable branch, the Github default)

* even cluster.c

Everything you'll pick will likely be a lot better than server.c

Other things you mentioned are a matter of taste. For instance things like:

    if (foo) bar();

Is my personal taste and I enforce it everywhere I can inside the Redis code, even modifying PRs received.

The line breaks are to stay under 80 cols. And so forth. A lot of the things you mentioned about the "style" are actually intentional. The weakness of server.c is in the overall design because it is the part of Redis that evolved by "summing" stuff into it in the course of 10 years, without ever getting a refactoring for some reason (it is one of the places where you usually don't have bugs or alike).

Ah I see. I didn't realize it was old, I just picked server.c since it just sounded like it'd have a decent mix of stuff. I think the only 'taste' things I mentioned is the lack of braces and possibly the function name, which I'm happy to ignore. But the rest were just inconsistencies, regardless of which way anyone's tastes lean -- the spaces after commas are entirely inconsistent (although I would argue if you took the stance that there shouldn't be a space at all, that's bordering on just being wrong, not merely a matter of taste!), and some lines are far longer than 80 columns [1] and some are broken at really inconvenient points just to fit them into 80 columns, which gets you the worst of both worlds.

Thankfully, like you said, the other files do seem better. :) However, they do have the same problems I just pointed out above: spaces are inconsistent everywhere (after commas, after 'while', after casts, etc.) and lacking in awful places (honestly, how am I supposed to read a function call with eight identifiers and zero spaces? [2]) and some lines are broken and others longer than 80 [3][4].

A couple other immediate issues I have on syntactic things (semantic analysis would take me a long time so it's hard to give me feedback on that):

- One comment I have that some others would vehemently disagree with me on is: I would cut down on the early returns. Personally, I really hate them, for multiple reasons -- the most practical one of which is that they prevent you from inserting a single breakpoint in a debugger (or printf(), or whatever you feel like doing) and being able to see what the function returns easily. Instead, you have to hunt through the entire function and place a dozen breakpoints to make sure you didn't miss any return path. And it becomes the most confusing thing in the world when you inevitably miss one. To me that's bad enough, but some people still insist on them. But if you're going to do that, I would at least make it easier for people to debug them. On my first reading, for example, I completely missed the return on line [5] -- because it's right after the if condition and visually blends in. That also makes it hard if not impossible to put a breakpoint on it, which wouldn't be an issue if it were on a separate line. (Maybe you don't do that because you don't want to forget braces, but you know my opinion on that too. :P) That makes it even more painful to debug this function.

- I'm not a fan of the liberal macro usage. I'm not saying you should never use macros -- I know that sometimes it's outright impossible not to (especially in C), and sometimes they're the perfect tool (code deduplication) -- but you shouldn't be using them to use them to define constants and perform normal function calls. [6] [7] Not only is it often possible to break them because of their textual nature, and not only do they lack type information that would be extremely useful, but they also make it harder to debug, since you can't just type them into a debugger at runtime and get their values. And you can't step through them like normal code either.

Hope some of this is helpful. All of it said though, they're pretty minor things overall, and the code does seem good quality, at least as far as I can tell in in this timespan. The fact that it's in C does always leave me with this nagging feeling that there's always going to be some resource leak somewhere (especially with early returns!), which I wouldn't have in most other languages, but that's not really an indictment of the code but of the language. And I love, love, love the comments. I don't leave comments nearly that good myself, and I will almost certainly refer back to them later.

[1] https://github.com/antirez/redis/blob/583933e2d6b4c2721554ab...

[2] https://github.com/antirez/redis/blob/unstable/src/rax.c#L10...

[3] https://github.com/antirez/redis/blob/unstable/src/rax.c#L50...

[4] https://github.com/antirez/redis/blob/unstable/src/rax.c#L45...

[5] https://github.com/antirez/redis/blob/unstable/src/rax.c#L13...

[6] https://github.com/antirez/redis/blob/unstable/src/hyperlogl...

[7] https://github.com/antirez/redis/blob/unstable/src/quicklist...

I have to admit that until seeing this example, I thought that code commented this way did not actually exist, and that the possibility of it was just a straw man invented for the sake of argument!