Calculating the minimum and maximum of a byte array I've been working with binary files with this last assignment and part of it was to determine the maximum and minimum byte. Initially this was 2 functions but a bonus portion was to make them into 1 function through usage of an enum, I imagine to reinforce their utility. I would like to know if there are better ways of doing this, be it efficient, elegant or otherwise. I'd also like tips on my C code, as this is the first real project I've done in C in several years.

I've also included the relevant bits from the header file.

#define GOOD 1
#define BAD 0

typedef unsigned int uint;
typedef unsigned char byte;

typedef enum uMaxOrMin
{
    Max,
    Min
} MaxOrMin;

typedef enum uBitStyle
{
    BitsOn,
    BitsOff
} BitStyle;

int MaxOrMinValue(byte src[], uint size, byte *val, MaxOrMin mom)
{
    uint srcTemp;
    uint srcIndex = 0;
    uint largest;
    uint smallest;
    uint* data = src;

    switch (mom) 
    {
    case Max:
    {
        largest = src[0]; //initialize largest 
        while (*data++) //iterate over binary array
        {
            srcTemp = *(src + srcIndex++); //start at beginning of array, grab first byte and increment by 1
            if (srcTemp &gt; largest) //current byte is greater than our saved largest byte
                largest = srcTemp; //set new largest 
        }

        *val = largest; //return max to caller
        return GOOD;
    }

    case Min:
    {
        smallest = src[0]; //initialize smallest
        while (srcIndex &lt; size) //iterate over binary array 
        {
            srcTemp = *(src + srcIndex++); //start at beginning of array, grab first byte and increment by 1
            if (srcTemp &lt; smallest) //current byte is less than our saved smallest byte
                smallest = srcTemp; //set new smallest
        }

        *val = smallest; //return min to caller
        return GOOD;
    }
    return BAD; //if we hit here, we've failed
    }
}


I know that under normal circumstances, C functions should return 0 upon success and -1 or otherwise upon failure, but this configuration was mandated by the assignment.

I would like to know why my while loop for BitsOn works but if I use the same construct for BitsOff, while (*data++), it fails to give me the correct results. Can someone explain this? Perhaps my understanding is lacking here. I'm trying to get more used to pointers so it seems pertinent to maximize my use of them, even in non-ideal circumstances.
Your combined function essentially looks like this:

// commonly used variables
if (min) {
    // code for computing the min
    return minValue;
} else if (max) {
    // code for computing the max
    return maxValue;
} else {
    return BAD;
}


Since there is no common code, you should either:


put the calculation for min and max in two separate functions
merge the code (which makes the code a little slower)


In the current code the variable definitions are too far away from the code that actually uses them. Understanding the code is easier when everything fits on a single screen.

Continuing with your code:

#define GOOD 1
#define BAD 0


These two constants may come from the 1980s or 1990s, when &lt;stdbool.h&gt; was not available yet. Nowadays functions that return success or failure should do so by returning bool instead of int.

typedef unsigned int uint;
typedef unsigned char byte;

typedef enum uMaxOrMin
{
    Max,
    Min
} MaxOrMin;


Since you don't use the uMaxOrMin name later, you can omit it.

typedef enum uBitStyle
{
    BitsOn,
    BitsOff
} BitStyle;


This whole enum is unused and should therefore be removed.

int MaxOrMinValue(byte src[], uint size, byte *val, MaxOrMin mom)


This function is not supposed to modify the contents of the src array. Therefore it should be declared as const byte src[].

To avoid the common confusion between * and [] for parameters (only the pointer is passed, not the whole array), it is preferable to write const byte *src instead.

The name of the size param is not as precise as possible. It should rather be src_size to make it unambiguous that it belongs to src.

{
    uint srcTemp;


The only time where temp is allowed as part of a variable name is in the popular pattern to swap two variables: tmp = a; a = b; b = tmp. In all other situations there must be a better name.

    uint srcIndex = 0;


Since there is only one thing that can be indexed here, the src in srcIndex is redundant. The code is short enough that the variable names can be short, too.

    uint largest;
    uint smallest;
    uint* data = src;

    switch (mom) 
    {
    case Max:
    {
        largest = src[0]; //initialize largest


Accessing src[0] is only allowed if src_size &gt; 0.

        while (*data++) //iterate over binary array
        {
            srcTemp = *(src + srcIndex++); //start at beginning of array, grab first byte and increment by 1


There is no point in writing *(ptr + index) since ptr[index] is much clearar to read.

            if (srcTemp &gt; largest) //current byte is greater than our saved largest byte


Ah, so srcTemp really means "the current byte". In that case, the variable should be named curr.

                largest = srcTemp; //set new largest 
        }

        *val = largest; //return max to caller


In this assignment, you are storing an unsigned int into a byte variable. Because both are unsigned, this works. Better style would be to make largest a byte variable, too. In other programming languages like Go or Kotlin, assigning values between different-sized variables is a compile-time error, and I find these quite helpful.

        return GOOD;
    }

    case Min:
    {
        smallest = src[0]; //initialize smallest
        while (srcIndex &lt; size) //iterate over binary array


Since min and max are essentially the same algorithms, their code should express this by being essentially the same. You are using two entirely different loops here, which leads to confusion and bugs.

        {
            srcTemp = *(src + srcIndex++); //start at beginning of array, grab first byte and increment by 1
            if (srcTemp &lt; smallest) //current byte is less than our saved smallest byte
                smallest = srcTemp; //set new smallest
        }

        *val = smallest; //return min to caller
        return GOOD;
    }
    return BAD; //if we hit here, we've failed
    }
}


All in all, your code does not take any advantage of the algorithms being so similar. To combine them in a single piece of code, I suggest this:

#include &lt;assert.h&gt;

#define GOOD 1
#define BAD 0

typedef unsigned int uint;
typedef unsigned char byte;

typedef enum {
    Max,
    Min
} MaxOrMin;

int MaxOrMinValue(const byte *src, uint src_size, byte *result, MaxOrMin mom) {
    if (src_size == 0) {
        return BAD;
    }
    if (mom != Min &amp;&amp; mom != Max) {
        return BAD;
    }

    byte mask = (byte) (mom == Max ? -1 : 0);
    byte best = src[0] ^ mask;
    for (uint i = 1; i &lt; src_size; i++) {
        byte curr = src[i] ^ mask;
        if (curr &lt; best) {
            best = curr;
            if (best == 0)
                break;
        }
    }

    *result = best ^ mask;
    return GOOD;
}

int main() {
    byte arr1[] = {0, 1, 2, 3, 4};
    byte arr2[] = {200, 100, 222};
    byte minmax = 13;

    assert(MaxOrMinValue(arr1, sizeof arr1, &amp;minmax, Min) == GOOD);
    assert(minmax == 0);

    assert(MaxOrMinValue(arr1, sizeof arr1, &amp;minmax, Max) == GOOD);
    assert(minmax == 4);

    assert(MaxOrMinValue(arr2, 0, &amp;minmax, Max) == BAD);
    assert(minmax == 4); // unchanged

    assert(MaxOrMinValue(arr2, 1, &amp;minmax, Max) == GOOD);
    assert(minmax == 200);

    minmax = 123; // to make sure it is overwritten again
    assert(MaxOrMinValue(arr2, 2, &amp;minmax, Max) == GOOD);
    assert(minmax == 200);

    assert(MaxOrMinValue(arr2, 3, &amp;minmax, Max) == GOOD);
    assert(minmax == 222);

    // Passing neither Min nor Max is bad.
    assert(MaxOrMinValue(arr2, 3, &amp;minmax, 123) == BAD);
    assert(minmax == 222); // unchanged
}


Admitted, using a bitmask to combine the min and max algorithms is tricky. Without using this trick, the code might look like this:

int MaxOrMinValue(const byte *src, uint src_size, byte *result, MaxOrMin mom) {
    if (src_size == 0) {
        return BAD;
    }
    if (mom != Min &amp;&amp; mom != Max) {
        return BAD;
    }

    byte best = src[0];
    byte best_possible = mom == Max ? (byte) -1 : (byte) 0;
    for (uint i = 1; i &lt; src_size; i++) {
        byte curr = src[i];
        if (mom == Max ? curr &gt; best : curr &lt; best) {
            best = curr;
            if (best == best_possible) {
                break;
            }
        }
    }

    *result = best;
    return GOOD;
}


Oh, nice. The code even got simpler. One thing that bothered me though about this code is the complicated condition in the if (mom == Max ?. I wanted to avoid checking this condition in every loop again because of branch prediction and possible performance.

During one call of this function, the outcome of the outer condition will always be the same though (mom does not change at all). Therefore the actual performance penalty might be so small that it is barely measurable. Choosing the faster code would need very careful benchmarks in this case. Therefore, either variant is probably good enough.

Anyway, out of intuition I wanted to avoid as many branches as possible. To avoid the mom == Max condition inside the loop, I quickly analyzed in my head that min == bitinvert(max), at least for unsigned numbers. And since you defined the byte type as unsigned char, it seemed a good fit. Plus, I wanted to try out this bitmasking idea since I had not seen it anywhere else before.

Finding the idea of using a bitmask to convert between min and max algorithms is something that probably comes with experience, I would be very surprised if any programming beginner would find this by themselves. Even understanding it is difficult enough. For more crazy stuff, have a look at the book Hacker's Delight.

One nice last thought about this bitmasking technique is that I think it can be extended even further to also allow MinSigned (mask 0b1000_0000) and MaxSigned (mask 0b0111_1111). There might also be even stranger use cases like mask 0b0011_0000 to find the smallest ASCII digit.

Things that the bitmasking technique cannot do are:


find the first uppercase letter from the alphabet (that is, 'A' if it exists somewhere, otherwise 'B', otherwise 'C'). This is because there are 26 uppercase letters, and the code point of the 'A' is U+0041, which in binary is 0100_0001. Using this as the bitmask, the smallest bytes would be 'A@CBEDHG', in this order.
find the largest ASCII digit, for similar reasons.