Hi az, > > I'd be tempted to make it an if-then with no else clause by hoisting > > the "BCC:" prefix and "\n" suffix outside of the if-then. > > hmm. i see your point, but don't entirely agree. my aim here was to > contain all the related logic within the smallest possible/sensible > horizon. > > apart from the small ugliness of having the string "bcc:" hardcoded > twice i prefer that 'if' block doing its thing (and all of its thing!) > over strings conditionally accumulating across a pageful of code or > more.
I don't quite get the comparison so think I may not have got my idea over. I don't know the context of the patch's chunk so considered it in isolation. The original, for (lp = localaddrs.m_next; lp; lp = lp->m_next) if (lp->m_bcc) allbcc = allbcc? add(concat(", ", lp->m_mbox, NULL), allbcc) : mh_xstrdup(lp->m_mbox); for (lp = netaddrs.m_next; lp; lp = lp->m_next) if (lp->m_bcc) allbcc = allbcc? add( concat(", ", lp->m_mbox, "@", lp->m_host, NULL), allbcc) : concat(lp->m_mbox, "@", lp->m_host, NULL); if (allbcc) { fprintf (out, "BCC: %s\n",allbcc); free(allbcc); } seems simpler as for (lp = localaddrs.m_next; lp; lp = lp->m_next) if (lp->m_bcc) allbcc = add(concat(", ", lp->m_mbox, NULL), allbcc); for (lp = netaddrs.m_next; lp; lp = lp->m_next) if (lp->m_bcc) allbcc = add(concat(", ", lp->m_mbox, "@", lp->m_host, NULL), allbcc); if (allbcc) { fprintf(out, "BCC: %s\n", allbcc + 1); free(allbcc); } though I haven't run it, let alone tested it, so it could be simpler but wrong. :-) > anyway, these are my personal preferences; i hope we can agree to > disagree and that somebody with commit rights will apply the patch or > equivalent to the code. Yep, absolutely. I see it's already getting attention from David and I expect Ken will look too. -- Cheers, Ralph. -- nmh-workers https://lists.nongnu.org/mailman/listinfo/nmh-workers