4 ms·

What are your issues with it? That the variable names are not descriptive enough to be understood when the code is taken out of context? That the if-substatemen

by cremno 12y ago

What are your issues with it? That the variable names are not descriptive enough to be understood when the code is taken out of context? That the if-substatements aren't compound statements? The last line? What would you have done instead (back then)? Every statement on a new line makes it more difficult to understand it since it belongs together, a macro obscures the code and also has to be named, and a function only for this is (was) too expensive speed and memory wise.

viraptor 12y ago
Not the gp, but I agree with him. I dislike everything you listed, but think that even with context it's unreadable. Context is hard to find too since this file has 3 lines of comments only. (For an enum equivalent) Also, I don't buy the "function too expensive" idea - that looks like a crazy micro optimization. If it was needed, I'd say it deserves a comment.
- cremno 12y ago
  Disliking omitting braces or really short names is just a matter of taste (however both is classic C coding style), but you can easily find out what they store without comments or a longer name. I personally would have chosen l instead of x, which is a pointer to a character in the current read _l_ine (see L202). Can you tell me what comments you're expecting? qmail comes with a bunch of man pages (including ones for functions and they're well written). The file is called qmail-pw2u.c and there's qmail-pw2u(8). The function is called dosubuser, so let's look up subuser in it: > Extra addresses. Each line has the form > sub:user:pre: That gives us a pretty good idea about the purpose of the posted code (plus the previous lines). I agree, a function or macro makes sense (since there are many other equivalent constructs in the same file), but that's "just" giving a bunch of statement a name. Does field_next explain what they do? static int field_next(int *i, char **x, unsigned *xlen) { *++i; *x += *i; *xlen -= *i; *i = byte_chr(*x,*xlen,':'); return i == xlen; } / * later */ if (field_next(&i, &x, &xlen)) return; The rest of the function is writing something. Maybe we should find out the purpose of the program at first? The man page tells us this too: >qmail-pw2u reads a V7-format passwd file from standard input and prints a qmail-users-format assignment file.

by cremno 12y ago

viraptor 12y ago

Not the gp, but I agree with him. I dislike everything you listed, but think that even with context it's unreadable. Context is hard to find too since this file has 3 lines of comments only. (For an enum equivalent) Also, I don't buy the "function too expensive" idea - that looks like a crazy micro optimization. If it was needed, I'd say it deserves a comment.

cremno 12y ago
Disliking omitting braces or really short names is just a matter of taste (however both is classic C coding style), but you can easily find out what they store without comments or a longer name. I personally would have chosen l instead of x, which is a pointer to a character in the current read _l_ine (see L202). Can you tell me what comments you're expecting? qmail comes with a bunch of man pages (including ones for functions and they're well written). The file is called qmail-pw2u.c and there's qmail-pw2u(8). The function is called dosubuser, so let's look up subuser in it: > Extra addresses. Each line has the form > sub:user:pre: That gives us a pretty good idea about the purpose of the posted code (plus the previous lines). I agree, a function or macro makes sense (since there are many other equivalent constructs in the same file), but that's "just" giving a bunch of statement a name. Does field_next explain what they do? static int field_next(int *i, char **x, unsigned *xlen) { *++i; *x += *i; *xlen -= *i; *i = byte_chr(*x,*xlen,':'); return i == xlen; } / * later */ if (field_next(&i, &x, &xlen)) return; The rest of the function is writing something. Maybe we should find out the purpose of the program at first? The man page tells us this too: >qmail-pw2u reads a V7-format passwd file from standard input and prints a qmail-users-format assignment file.