1
\$\begingroup\$

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 > 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 < size) //iterate over binary array 
        {
            srcTemp = *(src + srcIndex++); //start at beginning of array, grab first byte and increment by 1
            if (srcTemp < 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.

\$\endgroup\$

put on hold as off-topic by vnp, Sᴀᴍ Onᴇᴌᴀ, AJNeufeld, Ludisposed, Mast 16 hours ago

This question appears to be off-topic. The users who voted to close gave these specific reasons:

  • "Authorship of code: Since Code Review is a community where programmers improve their skills through peer review, we require that the code be posted by an author or maintainer of the code, that the code be embedded directly, and that the poster know why the code is written the way it is." – Ludisposed, Mast
  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – Sᴀᴍ Onᴇᴌᴀ, AJNeufeld
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ This is off-topic since this code is not working as intended. You could ask for help repairing it at stackoverflow.com. \$\endgroup\$ – Lundin Feb 12 at 8:04
  • 2
    \$\begingroup\$ What is BitStyle? It doesn't appear to be used anywhere in the code. \$\endgroup\$ – user673679 Feb 12 at 9:55
  • \$\begingroup\$ It's part of the assignment. It's used to differentiate between on and off bits. It's not used in this function, I mistakenly included it in the post. \$\endgroup\$ – Dumbassahedratron Feb 12 at 10:31
3
\$\begingroup\$

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 <stdbool.h> 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 > 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 > 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 < 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 < 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 <assert.h>

#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 && mom != Max) {
        return BAD;
    }

    byte mask = (byte) (mom == Max ? -1 : 0);
    byte best = src[0] ^ mask;
    for (uint i = 1; i < src_size; i++) {
        byte curr = src[i] ^ mask;
        if (curr < 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, &minmax, Min) == GOOD);
    assert(minmax == 0);

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

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

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

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

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

    // Passing neither Min nor Max is bad.
    assert(MaxOrMinValue(arr2, 3, &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 && mom != Max) {
        return BAD;
    }

    byte best = src[0];
    byte best_possible = mom == Max ? (byte) -1 : (byte) 0;
    for (uint i = 1; i < src_size; i++) {
        byte curr = src[i];
        if (mom == Max ? curr > best : curr < 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.
\$\endgroup\$
  • \$\begingroup\$ Thanks for your criticism. Many of the things you mentioned, such as the #define and separating the 2 cases into their own functions, were constraints of the assignment. Otherwise, I would not have written them that way. I appreciate the reasons behind what you said, however. I will respond to the rest of the comment once finished analyzing. \$\endgroup\$ – Dumbassahedratron Feb 12 at 11:33
  • \$\begingroup\$ So I've went through and changed my code to reflect your suggestions, and I want to know how you came to this algorithm, using XORs? For example, what was your thought process? I'm only now actually diving into digital logic and I'm finding it difficult to think about algorithms in these terms. For everything else, thanks. Appreciate the criticism. Answer accepted. \$\endgroup\$ – Dumbassahedratron Feb 12 at 11:59
  • \$\begingroup\$ Also, how does the function know to do the minimum in this scenario? I see the ternary expression, but I would have assumed it would be mom == Max ? 1 : 0, yet it still works. How? \$\endgroup\$ – Dumbassahedratron Feb 12 at 12:04
  • \$\begingroup\$ I added some more paragraphs to the answer. Sorry it got so long. ;) \$\endgroup\$ – Roland Illig Feb 12 at 12:43
  • 1
    \$\begingroup\$ "This function is not supposed to modify the contents of the src array. Therefore it must be declared as const byte src[]." --> must is a bit strong here - yet still a good idea to use const. Function would work the same without const, yet with const 1) may perform faster and 2) work with const * arguments. \$\endgroup\$ – chux Feb 14 at 4:27
0
\$\begingroup\$

Program does not work correctly

Your function says the maximum byte is 0 for the simple test case byte input[] = { 0,0,0,0,0,0,0,1,2,3,4 }. The issue is this line: while (*data++). You have it correct for the Min case.

Keep it simple

A simple for loop is sufficient to iterate through the array, it is much easier to understand. You don't have to deal with pointer arithmetic like *(src + srcIndex++), just use indexing.

for (index = 0; index < size; ++index)
{
    if (src[index] > largest)
    {
        largest = src[index];
    }
}

This is still using pointers.

Check for NULL.

You have not checked if src or val is NULL. This results in program crash due to dereferencing NULL pointer.

uint* data = src;

This is dangerous because you are trying to access a char using pointer to int. The result is not what you expect it to be. It should be byte * data = src;. uint largest and uint smallest should be byte largest and byte smallest.

Swap the order of switch condition and loop

You can simplify your function by swapping the order of iterating over the array and checking if you want maximum or minimum. This way reduces the chance of making copy and paste error like the bug with maximum case.

Your last question is missing some context, you should see if the above solves your problem. If not then post a new question.

\$\endgroup\$
  • \$\begingroup\$ "Check for NULL" That's nonsense, it is perfectly fine for a function to assume that parameters passed are valid and a superfluous branch checking for NULL only slows the code down. \$\endgroup\$ – Lundin Feb 12 at 7:53
  • \$\begingroup\$ I put check for null because the function return a failure code which I assumed the function should do validation of all its input parameters. Which I now realized the function didn't handle case where mom wasn't min or max. \$\endgroup\$ – wooooooooosh Feb 12 at 23:50

Not the answer you're looking for? Browse other questions tagged or ask your own question.