| Message ID | 20200226101429.81327-2-mirucam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Finish converting git bisect to C part 2 | expand |
Miriam Rubio <mirucam@gmail.com> writes: > Let's refactor code adding a new `write_in_file()` function > that opens a file for writing a message and closes it. > > This removes some duplicated code and makes the code simpler, > clearer and easier to understand. I find it a bit too much self promotion ;-) With what we see only in this step, the code is not so much simpler nor easier to understand. Perhaps when we see more callers in the remainder of the series, we may start to appreciate it, but if that is the case, we should clarify it here, e.g. This helper will be used in later steps and makes the code simpler, ... > +static int write_in_file(const char *filepath, const char *content, int append) > +{ > + FILE *fp = NULL; > + const char *mode = append ? "a" : "w"; > + > + fp = fopen(filepath, mode); > + if (!fp) > + return error_errno(_("could not open the file '%s'"), filepath); > + if (!fprintf(fp, "%s\n", content)) > + return error_errno(_("could not write in file '%s'"), filepath); Use and non-use of definite article being inconstent between "open the file X" vs "write in file X" bothers me as a reader here. Wouldn't it be more common to say "write TO" here, not "write IN"? > + return fclose(fp); We didn't use to say error_errno(_("could not close ...")) in the original, so we stay silent and return an error (EOF, which is a negative integer) from here. > +} > + > static int check_term_format(const char *term, const char *orig_term) > { > int res; > @@ -104,7 +117,7 @@ static int check_term_format(const char *term, const char *orig_term) > > static int write_terms(const char *bad, const char *good) > { > - FILE *fp = NULL; > + char *content = xstrfmt("%s\n%s", bad, good); > int res; > > if (!strcmp(bad, good)) > @@ -113,12 +126,9 @@ static int write_terms(const char *bad, const char *good) > if (check_term_format(bad, "bad") || check_term_format(good, "good")) > return -1; > > - fp = fopen(git_path_bisect_terms(), "w"); > - if (!fp) > - return error_errno(_("could not open the file BISECT_TERMS")); > + res = write_in_file(git_path_bisect_terms(), content, 0); > + free(content); > > - res = fprintf(fp, "%s\n%s\n", bad, good); With just this one callsite, I do not think it is a good idea to lose the "write a formatted output without an extra allcoation" by introducing this wrapper. The equation may become different if we see more (a lot more) callsites that already have strings ready to be written that can use this helper, though. > - res |= fclose(fp); > return (res < 0) ? -1 : 0; The return value is a bit strange here, but it is inherited from the original, so I'll let it pass. > }
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index c1c40b516d..ee1be630da 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -74,6 +74,19 @@ static int one_of(const char *term, ...) return res; } +static int write_in_file(const char *filepath, const char *content, int append) +{ + FILE *fp = NULL; + const char *mode = append ? "a" : "w"; + + fp = fopen(filepath, mode); + if (!fp) + return error_errno(_("could not open the file '%s'"), filepath); + if (!fprintf(fp, "%s\n", content)) + return error_errno(_("could not write in file '%s'"), filepath); + return fclose(fp); +} + static int check_term_format(const char *term, const char *orig_term) { int res; @@ -104,7 +117,7 @@ static int check_term_format(const char *term, const char *orig_term) static int write_terms(const char *bad, const char *good) { - FILE *fp = NULL; + char *content = xstrfmt("%s\n%s", bad, good); int res; if (!strcmp(bad, good)) @@ -113,12 +126,9 @@ static int write_terms(const char *bad, const char *good) if (check_term_format(bad, "bad") || check_term_format(good, "good")) return -1; - fp = fopen(git_path_bisect_terms(), "w"); - if (!fp) - return error_errno(_("could not open the file BISECT_TERMS")); + res = write_in_file(git_path_bisect_terms(), content, 0); + free(content); - res = fprintf(fp, "%s\n%s\n", bad, good); - res |= fclose(fp); return (res < 0) ? -1 : 0; }
Let's refactor code adding a new `write_in_file()` function that opens a file for writing a message and closes it. This removes some duplicated code and makes the code simpler, clearer and easier to understand. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Miriam Rubio <mirucam@gmail.com> --- builtin/bisect--helper.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)