refactor: replace goto with for #85
Conversation
varas
requested review from
gsanchezgavier,
roobre,
davidbrota,
paologallinaharbur,
cristianciutea and
brushknight
varas
force-pushed
the
refactor_replace-goto
branch
from
578b927
to
919a3f8
Compare
varas
force-pushed
the
refactor_replace-goto
branch
from
919a3f8
to
48b22d9
Compare
|
I'm personally not a huge fan of |
| l.logF("%s attempt %d", l.conf.Owner, tries) | ||
| if l.isBusyDeletingExpired() { | ||
| l.logF("%s failed, waiting %s", l.conf.Owner, l.conf.RetryBackoff.String()) | ||
| time.Sleep(l.conf.RetryBackoff) | ||
| goto retry | ||
| } |
There was a problem hiding this comment.
something to escape the loop is needed if not busy , could be a else+break or something similar
There was a problem hiding this comment.
what do you mean? the for condition will exit the loop
There was a problem hiding this comment.
if if l.isBusyDeletingExpired() { is not true i think it should exit the loop and continue with the execution as it was doing with the goto logic
There was a problem hiding this comment.
true! otherwise in case of long busy it will wait an extra time in the end before exiting
Co-authored-by: Roberto Santalla <roobre@users.noreply.github.com>
Co-authored-by: Guillermo Sanchez Gavier <gsanchez@newrelic.com>
publisher/lock/s3_lock.go
Outdated
| if tries >= int(l.conf.MaxRetries) { | ||
| break | ||
| } |
There was a problem hiding this comment.
i think this could be removed , the for condition prevents this case
Replace goto with for as requested #83 (comment)
I still think this is one of the few legit usages of goto, making the code more readable in this case.
wdyt?