Zev Weiss <zev@xxxxxxxxxxxxxxxxx> writes:
> Subject: Re: [PATCH 2/5] log: Refactor duplicated code to headerize recipient lists
Style: "log: Refactor" -> "log: refactor"
cf. Documentation/SubmittingPatches[[summary-section]]
> The To and Cc headers are handled identically (the only difference is
> the header name tag), so we might as well reuse the same code for both
> instead of duplicating it.
Makes tons of sense. But seeing this one ...
> + recipients_to_header_buf("To", &buf, &extra_to);
> + recipients_to_header_buf("Cc", &buf, &extra_cc);
... the parameters to the function probably should be ...
> +void recipients_to_header_buf(const char *hdr, struct strbuf *buf,
> + const struct string_list *recipients);
... in this order, instead:
format_recipients(&buf, "To", &extra_to);
That is, "To" and &extra_to are much closely related to each other
than they are to &buf in the sense that they are both input to the
helper function to work on, while &buf is an output buffer, and we
tend to place closer things together, next to each other.
Other than that, removal of repetition is very good.
Thanks.
