# [PATCH] CodingGuidelines: Add a note to avoid assignments inside if() 2 messages from 2008-05-22 to 2008-05-25. Participants: Miklos Vajna, Junio C Hamano. Thread: https://gitlist.dev/t/13627 ## Miklos Vajna, 2008-05-22 23:26 Subject: [PATCH] CodingGuidelines: Add a note to avoid assignments inside if() Message-ID: <1211498769-26871-1-git-send-email-vmiklos@frugalware.org> URL: https://gitlist.dev/e/1211498769-26871-1-git-send-email-vmiklos%40frugalware.org ``` Signed-off-by: Miklos Vajna --- I just had to ask Dscho about this. Better if it's documented, I suppose. Documentation/CodingGuidelines | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 994eb91..d2a0a76 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -89,6 +89,8 @@ For C programs: of "else if" statements, it can make sense to add braces to single line blocks. + - We try to avoid assignments inside if(). + - Try to make your code understandable. You may put comments in, but comments invariably tend to stale out when the code they were describing changes. Often splitting a function -- 1.5.5.1.357.g1af8b.dirty ``` ## Junio C Hamano, 2008-05-25 05:17 Subject: Re: [PATCH] CodingGuidelines: Add a note to avoid assignments inside if() Message-ID: <7vd4nbj72x.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vd4nbj72x.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1211498769-26871-1-git-send-email-vmiklos@frugalware.org> ``` Thanks. FYI, recently in a nearby mailing list, we had this gem: From: Linus Torvalds Subject: Re: [PATCH 3/5] isdn: fix integer as NULL pointer warning Date: Fri, 23 May 2008 08:08:57 -0700 (PDT) Message-ID: ... > len += sprintf(page+len, "%-16s %s\n", "type", s); > - if ((s = cinfo->version[VER_DRIVER]) != 0) > + if ((s = cinfo->version[VER_DRIVER]) != NULL) > len += sprintf(page+len, "%-16s %s\n", "ver_driver", s); For thigns like this (ie testing an assignment), I personally much prefer s = cinfo->version[VER_DRIVER]; if (s) len += sprintf(page+len, "%-16s %s\n", "ver_driver", s); over the uglier and unreadable version. IOW, testing assignments is good only when: - you have to do it because of syntax (ie notably in a "while()" loop) - there's some reason you want it to be a single statement (eg doing a macro or other thing) - of the assignment is really simple, and the test is not against NULL or zero. The reason for that "the test is not against NULL or zero" is that testing for NULL and 0 is better done with just a "if (x)", and in an assignment that just means either (a) a incomprehensible extra parenthesis just to shut the compiler up or (b) changing the simple test into a stupid test (ie doing "if (x != NULL)"). (b) is much preferable to (a), but just doing it as two statements is much preferable to either! ```