This function started very clean but I had to add requirements:

  1. Added a Stopwatch to only update the caller periodically. I guess that responsibility could be handled elsewhere but I'm undecided about whether or not it should be.

  2. Added a Stopwatch to only check a computationally expensive operation periodically.

  3. Give the user the option to cancel the whole thing.

Additionally, I'd like for the enclosing class to be static so that calls to it look like this:

var encoded = Codec.Encode(myImage, myMessage, myCheckCancelMethod);
var decoded = Codec.Decode(myImage, myCheckCancelMethod);

There is a corresponding Encode method here that should match as closely as possible. The code works (UWP and Android so far, you have to use a .png right now).

What I see as being the problem is this function is supposed to just call out to other functions but there is so much coordination going on that the coordination function has gotten complicated and you can't really take anything out.

public static async Task<byte[]> Decode(Bitmap carrierImage, byte[] eofMarker, Func<double, bool> checkCancel)
    var shuffleSeed = FisherYates.GetSeed(eofMarker);

    bool foundEof = false;
    bool userCancelled = false;

    List<bool> messageBuilder = new List<bool>();
    byte[] decodedMessage = null;

    var userUpdateStopwatch = new Stopwatch();
    var eofStopwatch = new Stopwatch();

    await Task.Run(() => // move away from the calling thread while working
        IterateBitmap(carrierImage, shuffleSeed, (x, y) => {
            var pixelBitsAsBools = DecodePixel(carrierImage, x, y);

            if(eofStopwatch.ElapsedTicks > EOF_CHECK_RATE)
                foundEof = MessageBuilderHasEof(messageBuilder, eofMarker);

            var percentComplete = (double)messageBuilder.Count / carrierImage.BitCapacity;
            userCancelled = CheckCancelAndUpdate(userUpdateStopwatch, percentComplete, checkCancel);

            return userCancelled || foundEof;

        foundEof = MessageBuilderHasEof(messageBuilder, eofMarker); // check for eof again in case the IterateBitmap loop completed since the last time we checked.
        if (foundEof)
            decodedMessage = GetMessageWithoutEof(messageBuilder, eofMarker);

    return userCancelled || !foundEof ? null : decodedMessage;
  • 2
    \$\begingroup\$ I have a some questions about your code: 1) Why do you use stopwatch to measure intervals instead of using a timer? This looks like wasting CPU cycles for constantly calling the Func; 2) Why don't you use the CancellationToken for cancelling? \$\endgroup\$ – t3chb0t Jun 19 '18 at 7:55
  • \$\begingroup\$ Okay don't hate me but I'm not using either. I went back in with the intent of doing both, though. 1) on the encoder i'd have to move the declaration of bitsWritten and percentComplete up a couple blocks which makes it harder for me to follow what's going on. 2) it adds a fifth argument to the method which again, is just over the limit in terms of being acceptably complicated for this configuration imo. Sorry man, definitely not ignoring you. I tried it but didnt like it. \$\endgroup\$ – user875234 Jun 20 '18 at 2:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.