Skip to main content
added 4 characters in body
Source Link
Jonah
  • 4.4k
  • 14
  • 22

Other than the cosmetic suggestion we discussed about removing the unnecessary self prefixes, my main commentcentral critique is that you're mixing concerns in this code.

Currently, you have specific logging strategies and even logging messages hardcoded in the retryer. Which will make it hard to use the class in other contexts.

The retryer should do just one thing: retry X number of times and return a result.

Exactly what you want the return values to be is up to you (you could even make a RetryResult class), but one simple option would be an array of results from each attempt: true would be the final array value if it eventually succeeds; and anthe array would rescued errors representingfrom each failed attempt.

The calling code could then decide how to process that information: logging it, sending emails, ignoring it, etc.

Other than cosmetic suggestion we discussed about removing the unnecessary self prefixes, my main comment is that you're mixing concerns in this code.

Currently, you have specific logging strategies and even logging messages hardcoded in the retryer. Which will make it hard to use the class in other contexts.

The retryer should do just one thing: retry X number of times and return a result.

Exactly what you want the return values to be is up to you (you could even make a RetryResult class), but one simple option would be an array of results from each attempt: true would be the final array value if it eventually succeeds; and an array rescued errors representing each failed attempt.

The calling code could then decide how to process that information: logging it, sending emails, ignoring it, etc.

Other than the cosmetic suggestion we discussed about removing the unnecessary self prefixes, my central critique is that you're mixing concerns in this code.

Currently, you have specific logging strategies and even logging messages hardcoded in the retryer. Which will make it hard to use the class in other contexts.

The retryer should do just one thing: retry X number of times and return a result.

Exactly what you want the return values to be is up to you (you could even make a RetryResult class), but one simple option would be an array of results from each attempt: true would be the final array value if it eventually succeeds; and the array would rescued errors from each failed attempt.

The calling code could then decide how to process that information: logging it, sending emails, ignoring it, etc.

Source Link
Jonah
  • 4.4k
  • 14
  • 22

Other than cosmetic suggestion we discussed about removing the unnecessary self prefixes, my main comment is that you're mixing concerns in this code.

Currently, you have specific logging strategies and even logging messages hardcoded in the retryer. Which will make it hard to use the class in other contexts.

The retryer should do just one thing: retry X number of times and return a result.

Exactly what you want the return values to be is up to you (you could even make a RetryResult class), but one simple option would be an array of results from each attempt: true would be the final array value if it eventually succeeds; and an array rescued errors representing each failed attempt.

The calling code could then decide how to process that information: logging it, sending emails, ignoring it, etc.