Well, that is one way of doing it...



  • Found in production code (not mine, obviously, and slightly anonymised). It works, but...

    unsigned int errmsk=1;
    ...
        while(errmsk > 0)
        {
            if((errstatus & errmsk) ^ (errstatus_old & errmsk))
            {
                switch(errmsk)
                {
                    case 1:
                        sendResponse(address1,(long)(errstatus & errmsk));
                        break;
                    case 2:
                        sendResponse(address2,(long)((errstatus & errmsk)>>1));            
                                            break;
                    case 4:
                        sendResponse(address3,(long)((errstatus & errmsk)>>2));    
                        break;
                    default:
                        break;
                }
            }
            errmsk<<=1;
        }



  • The responses are being sent to different addresses. There's no WTF here. Except possibly the for-switch loop and the fact that it runs forever if unsigned int has infinite bits.



  • Well, as I said, "it works". But let me count the "whyTF did they do it in this way?"s...

    Using a shifted mask of 1 bit when it all ends up with a huge hardcoded address thing anyway (just use if and &, it's not that many checks...),
    Using an XOR every frigging time instead of just checking if the status has changed in the first line (IIRC the function is only called if the status has changed, even),
    Switching by the (magic) position of the mask,
    Sending "1" to the respective address in the most complicated way I have ever seen, using hardcoded shiftcount
    And using this whole thing to send three(!) possible things.

    Did I forget anything?

    You could code the whole thing a lot easier, better, more readable, and easier to expand. That is the WTF.



  • It does not only send 1s, and it does not always send to all addresses.

    Dunno what it should do, but ATM when errstatus_old is 6 and errstatus is 5, it will send a 1 to address1, a 0 to address2 and nothing (since that bit did not change) to address3.

    The code may not be the clearest (especially since you can stop after the three bits), and unrolling might make it more readable, but I don't think you can remove the logic to determine whether to send 1 or 0, or the if clause whether a message should be sent.

    Of course I don't know the protocol, maybe the data or whether something is sent does not matter at all...



  • @mihi said:

    Dunno what it should do

    It should do exactly what it does do, except that it should do it like this:

    {
        int changed = errstatus ^ errstatus_old;
        if (changed & 1) sendResponse(address1, (long)!!(errstatus & 1));
        if (changed & 2) sendResponse(address2, (long)!!(errstatus & 2));
        if (changed & 4) sendResponse(address3, (long)!!(errstatus & 4));
    }
    

Log in to reply