undefined | Better HN

0 pointsantirez6y ago0 comments

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).

0 comments

mehrdadn6y ago

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...

j / k navigate · click thread line to collapse