# Simple but important function to check and retry given action's result

What I'm trying to achieve is Given a predicate function and an action which returns the object to check, check predicate, retry and fail after numTries .. or success .

Purpose of this function is :

During constant data collection from network resources ( web, apis, .. ) return of network operations not successful every time ( because of bad servers, network problems , etc.. ). If you need that data you should retry, and if you do this kind of ops. a lot, you should have a function to handle this.

I'm looking for are there better ways to do this.

public static async Task<bool> CheckAndRetryAsync<T>
(
T _object, //object to check state
Func<T, bool> predicate, Func<Task<T>> action, // object creation func
int sleep = 150, // sleep ms. between tries
int numTries = 6, // ..
bool rethrowExceptions = false, // should rethrow exc. during object creation
ManualResetEvent mr = null
)
{
int tries = 0;
bool success = false;
while (true)
{
try
{
_object = await action();
success = predicate(_object);
if (!success && tries < numTries)
{
tries++;
}
else
{
//goto exit;
break;
}
}
catch (Exception e)
{
if (rethrowExceptions)
throw;
if (tries < numTries){
tries++;
}
else{
goto exit;
}

}
}
exit:
if(mr != null)
mr.Set();
return success;
}


Tests for function :

public class StaticUtilFuncTests
{
ManualResetEvent mr = new ManualResetEvent(false);
[Fact]
{
int tries = 1;
mr.Reset();
Stopwatch sw = new Stopwatch();
sw.Start();
TObj ttx = null;
bool succsess = await CheckAndRetryAsync<TObj>(ttx,
(o) => (o != null && o.Value != ""),
async () =>
{
if (tries <= 4)
{
ttx = new TObj();
tries++;
}
else
{
ttx = new TObj("sdf");

}
mr.Set();

return ttx;

},
numTries: 5, sleep: 400
);
mr.WaitOne();
long elapsed = sw.ElapsedMilliseconds;
Assert.True(elapsed >= 1600l); // numtries * sleep
Assert.True(succsess);
Assert.True(tries == 5);

}
[Fact]
{
mr.Reset();

Stopwatch sw = new Stopwatch();
sw.Start();
TObj ttx = null;
bool succsess = await CheckAndRetryAsync<TObj>(
ttx,
(o) => (o != null && o.Value != ""),
async () =>
{

ttx = new TObj("");
mr.Set();

return ttx;
},
numTries: 3, sleep: 1100
);
mr.WaitOne();
long elapsed = sw.ElapsedMilliseconds;
Assert.True(elapsed >= 3300l); // numtries * sleep
Assert.False(succsess);
}
[Fact]
{
mr.Reset();
//Mock<TObj> to = new Mock<TObj>();
//to.Setup(
//    m => m.)

Stopwatch sw = new Stopwatch();
sw.Start();
TObj ttx = null;
bool succsess = await CheckAndRetryAsync<TObj>(
ttx,
(o) => (o != null && o.Value != ""),
async () =>
{

ttx = new TObj("",true);
//mr.Set();

return ttx;
},
numTries: 30, sleep: 50, mr: mr
);
mr.WaitOne();
long elapsed = sw.ElapsedMilliseconds;
Assert.True(elapsed >= 1500l); // numtries * sleep
Assert.False(succsess);
}
}
public class TObj
{
public string Value;

public TObj(string value = "" , bool testing = false)
{
if(value == "" && testing)
throw new Exception();
Value = value;
}
}
}


A real usage example :

 BittrexBtcTicker btcTicker = null;
success |= await CheckAndRetryAsync<BittrexBtcTicker>(
btcTicker, // object we need to get ( also save db )
(o) => (o != null && o.Value > 0), // predicate not null, and bigger than 0
async () =>
{
mr.Reset(); // a noise but needed during async ops.
btcTicker = await BittrexClient
.GetTicker(saveDb: true, updateDb: updateDb, _context: context, mr: mr); // call to object creation..
mr.WaitOne(); // wait to op. complete
return btcTicker;
}
);

• Welcome to Code Review. Please try to improve the first paragraph of your question. As it currently stands, I'm having trouble deciphering it. You have an action and a prediction function and something about a number of tries that's being kept track of, but I'm lost on what the actual purpose of your code is. – Mast Nov 5 '18 at 12:04
• I added extra info. Hope it helps. ( By the way, tests could also explain purpose ). Try to create object, if object is null or not in a state you desire try again, ..After n tries, return fail . – firatto Nov 5 '18 at 12:20
• 'predicte' is not spelled correctly - I assume you mean 'predicate' instead of 'predict'? – Pieter Witvoet Nov 5 '18 at 12:22
• Oh sorry for spelling, I've corrected. ( I'm not a native speaker ) – firatto Nov 5 '18 at 12:24
• @firatto you might want to look into Polly to see if it meets your needs. – CharlesNRice Nov 5 '18 at 16:10

Design

• I see you fixed the bug where the method would get stuck in an infinite loop when action throws an exception, but note how similar the normal and the exception paths are now. The method can be simplified by incrementing tries before invoking the action and by awaiting the delay after (outside) the try-catch. Try to keep code DRY (Don't Repeat Yourself).
• More simplifications include using a proper while condition: while (tries < numTries), to make the intent of the loop more clear, and the success check can be simplified to if (predicate(result)) break;.
• There's no need for goto here - break is sufficient (and easier to understand).
• The T _object parameter is useless. Currently, the only way to obtain a result is to use a closure. I assume you meant to make this a ref parameter? If so, I would rename it to TResult result. Alternately, you could return a (bool success, TResult result) value-tuple, which can be used like var (success, result) = await CheckAndRetryAsync(...);
• You may want to add ConfigureAwait(false) to your awaits, unless resuming in the same context is important.
• Are you sure you want to ignore exceptions by default? Including the last try? No logging even?
• What's the point of passing a ManualResetEvent into this method? Why not just do that in awaiting code: await CheckAndRetryAsync(...); mr.Set();?
• You may want to enforce that sleep is not negative.

Tests

• The test method names do not describe what they're testing, and none of the Assert statements contain a descriptive error message. That's not good for future maintenance.
• There's a lot of code duplication in the tests. You may want to refactor it into a data-driven approach, or write a utility method for the repetitive parts.
• What's the purpose of that ManualResetEvent? Why is it shared between all tests? You may want to add a comment explaining why it's there.
• Throwing an exception from a constructor (TObj) instead of throwing it directly inside a test method is making the tests harder to understand.
• You may want to add a timeout to your tests, to ensure that infinite loops and other such problems will get caught.
• Instead of comparing elapsed against a magic number, I'd create local variables for the number of tries and timeout, so the minimum elapsed time can be calculated from them.
• In T_010, tries will always end up being 5, even if action gets called more than 5 times.
• There's no test that checks whether exceptions are rethrown.

• All parameters are on a line of their own, except the two most important ones, so at first sight it looks like there's only one Func parameter. That's confusing!
• Personally I'd put action before predicate, to match execution order (you first need a result before you can check it, after all).
• The l prefix looks a lot like a 1 - consider using L instead, or just leave it out.
• @firatto: that infinite loop was caused by a bug in CheckAndRetryAsync. The use of a ManualResetEvent here seems pointless and confusing, and sharing it between test methods might even cause tests to influence each other, depending on how xUnit runs the tests. – Pieter Witvoet Nov 7 '18 at 9:27