Looking for a more elegant DO WHILE solution

Question

So I have some code centered around a do while loop.

string token;
var count = 0;      
var checkDataLength= data.Length == 16;

do
{              
    count = 0;
    token = GenerateToken(data, start, end);

    if (checkDataLength)
    {
        if (Mod120Check(token))
        {                      
            count += 1;
        }
    }

    if (IsCompliant(token))
    {
        count += 1;
    }
} 
while (count > 0);

return token;

Basically, I am generating a token and for this token to be valid, has to FAIL the Mod120Check and the IsCompliant check. I cannot change how those methods return the data.

While the above code works, I feel that its ugly and I was wondering if anyone had a better way of doing it?

Thanks

I feel this is the best way. Other solutions would be uglier. It matches your requirement which is to execute it atleast once. — Mohit Rustagi, Jul 18 '18 at 20:24
I don't find anything bad about this code. The only thing that I would improve is the use of the `count` variable. It's not a count at all! It should be a simple boolean flag. — Alejandro, Jul 18 '18 at 20:27
@Alejandro I'd say Nathalia's answer makes this code look pretty bad by comparison :) — Max, Jul 18 '18 at 20:33
Fugly loops tend to be fixable with `for (;;) {}` + `break`. The explicit break makes it easy to reason about the flow and debug the break condition. You might like `continue` in this case as well. Some programmers might consider `for (;;)` too grating, but nothing screams loop-forever louder :) — Hans Passant, Jul 18 '18 at 21:20

Nathalia Soragge · Answer 1 · 2018-07-20T05:29:23.807

17

Try this:

do
{
    token = GenerateToken(data, start, end);
} 
while (checkDataLength && Mod120Check(token) || IsCompliant(token))

Just moving your conditions to the while.

(!) Notice that IsCompliant(token) will only be called if checkDataLength && Mod120Check(token) states false. It shouldn't cause any colateral effects, but it could, depending on what your IsCompliant method does.

edited Jul 20 '18 at 05:29

answered Jul 18 '18 at 20:28

Nathalia Soragge

1,415
6
21

2

Much neater! The OP may want to note that in the case where the `Mod120Check` passes, the original code still calls `IsCompliant` whereas this rewrite does not. This is almost certainly another point in the rewrite's favor, but worth pointing out just in case that call has some goofy side effects or something. – Joe Farrell Jul 18 '18 at 20:47
@JoeFarrell Good point! i hadn't noticed this detail. Added it to the answer – Nathalia Soragge Jul 18 '18 at 21:03

score 2 · Answer 2 · answered Jul 18 '18 at 20:32

You're right, it is ugly. You are using count in an unexpected way (it gets reset to zero at the top of every loop, and can go positive for two different reasons). When I see count, I expect something to start at zero and count up (or start high and count done). Try this instead:

Change var count = 0; to var goodEnough = false; up at the top
Remove the count = 0; statement
Change the two count += 1; statements to goodEnough = true;
Change the while (count > 0); to while (!goodEnough);

This emphasizes that you are starting in a "not good enough" state, and you will loop until some condition makes it good enough to continue past the loop.

Looking for a more elegant DO WHILE solution

2 Answers2