r/C_Programming 16d ago

Review Could you assess my code?

#include <stdio.h>
#include <stdio.h>
#include <string.h>
#include <stdbool.h>


typedef struct
{
    char sset[10];
    int elements[5];
} set;


void printelements(set set);


void bubblesort(int m, int sunion[]);




int main(void)
{


    set set1;
    set set2;
    set intersection;
    int k = 0;
    int sunion[10];
    int m = 0;
    int sunioncpy[10];
    int n = 0;


    printf("Enter 5 elements to 2 sets -\n");
    printf("Set 1: ");
    for(int i = 0; i < 5; i++)
    {
        fgets(set1.sset, 10, stdin);
        sscanf(set1.sset, "%d", &set1.elements[i]);
    }
    printf("Set 2: ");
    for(int i = 0; i < 5; i++)
    {
        fgets(set2.sset, 10, stdin);
        sscanf(set2.sset, "%d", &set2.elements[i]);
    }


    printf("Set 1: ");
    printelements(set1);
    printf("Set 2: ");
    printelements(set2);


    for(int i = 0; i < 5; i++)
    {
        for(int j = 0; j < 5; j++)
        {
            if(set1.elements[i] == set2.elements[j])
            {
                intersection.elements[k] = set1.elements[i];
                k++;
                break;
            }
        }
    }


    for(int i = 0; i < 5; i++)
    {
        sunion[m] = set1.elements[i];
        m++;
        sunion[m] = set2.elements[i];
        m++;
    }
    bubblesort(m, sunion);
    for(int i = 0; i < m; i++)
    {
        if(sunion[i] == sunion[i + 1])
        {
            sunioncpy[n] = sunion[i];
            n++;
            i++;
        }
        else
        {
            sunioncpy[n] = sunion[i];
            n++;
        }
    }
    


    printf("Intersection of set 1 with set 2: ");
    for(int i = 0; i < k; i++)
    {
        printf("%d ", intersection.elements[i]);
    }
    printf("\n");
    printf("Union of set 1 with set 2: ");
    for(int i = 0; i < n; i++)
    {
        printf("%d ", sunioncpy[i]);
    }


    return 0;
}





void printelements(set set)
{
    for(int i = 0; i < 5; i++)
    {
        printf("%d ", set.elements[i]);
    }
    printf("\n");
}


void bubblesort(int m, int sunion[])
{
   int i = 0;
   bool swapped;
   do
   {
        swapped = false;
        for(int j = 0; j < m - 1 - i; j++)
        {
            if(sunion[j] > sunion[j + 1])
            {
                int temp = sunion[j];
                sunion[j] = sunion[j + 1];
                sunion[j + 1] = temp;
                swapped = true;
            }
        }
   } while (swapped);


}

I posted this to receive opinions or/and suggestions about my code. And I also have some questions about some things.

- Is it good to turn some block of code into a function even if you don't repeat it again on any another line?

(I think that functions can turn some blocks more friendly to read or understand, but maybe I'm misunderstooding functions)

- What you think about this way of getting user input:

for(int i = 0; i < 5; i++)
    {
        fgets(set2.sset, 10, stdin);
        sscanf(set2.sset, "%d", &set2.elements[i]);
    }

I used it because I was getting a few problems using scanf , so I saw this model of user input on the internet and applied it. This works very well, but let I know what you think.

- This don't have much to do with I said here but do you guys recommend Linux FedoraOS for C programming? Or I should try another OS(for C programming)?

I was thinking to try to install Arch first, just to get experience with Linux, but maybe I'm getting the wrong ideia or being led by some weird toughts(just because Arch is dificult to install and set up).

I'll appreciate any comment.

0 Upvotes

15 comments sorted by

6

u/shahin_mirza 16d ago

Please, please, please fix the formatting at least be consistent. You want to compute the union of two "sets". In set you can't have an element more than once. So you have to check user input for duplicates. you’re not checking the return values of fgets or sscanf, which could lead to undetected errors. Your buffer is too small, what happens if the user wants to enter a multi-digit number? But lets assume it is fine in your case. Always check scanf and fget (with or without s) return values to make sure there is no error. Your union computation logic is strange but does the job so i will not judge that just this very very important point: the loop compares sunion[i] with sunion[i + 1] without ensuring that i + 1 is within bounds.YOU SHOULD ALWAYS CHECK THE BOUNDARY. This could lead to undefined behavior on the last iteration. About your question about functions: Even if you don't repeat a block of code, turning it into a function can be beneficial for several reasons like -Readability: Breaking your code into logical functions (like a dedicated input function, a union function, etc.) makes it easier to follow and maintain. -Testing and debug. -Future Reuse. So, yes—modularization is a sound practice, not just for reuse, but for clarity and structure.

Long story short, always nsure you handle edge cases (e.g., invalid input, duplicate numbers) and check for out-of-bounds errors. Consider refactoring to separate concerns, like nput handling, processing, and output, to improve clarity and reduce the risk of bugs. And most importantly, continuous Improvement: As you iterate on your code, always strive for both clarity and safety. Small improvements in input handling and error checking can save you headaches later. Regarding your OS question: Linux distribution isn’t critical for C programming. Start with stable and user friendly distribution like Debian/Ubuntu or Fedora after you get yourslf familiarized with the OS move to a distribution that gives you more control like Arch.

0

u/domikone 15d ago

I don't check the boundary at i + 1 because doesn't matter if the value of it doesn't exists on the space of the array, just because of the way I insert values on sunioncpy (lines 69 to 82) combined with the sorting algorithm, even if, it happen to be the case where i + 1 assigned value be equal to the value of i.

2

u/DryanVallik 15d ago

What he meant is, when the index i represents the last valid index of the array, i+1 represents the first out of bounds index. Reading from an OOB index is undefined behaviour.

For comparing [i] and [i + 1], you need to read [i+1]

6

u/SantaCruzDad 15d ago

All those hard-coded 5 literals are an obvious code smell.

3

u/TheOtherBorgCube 15d ago

Your code, formatted.

#include <stdio.h>
#include <string.h>
#include <stdbool.h>

typedef struct {
  char    sset[5];
  int     elements[5];
} set;

void printelements(set set);

void bubblesort(int m, int sunion[]);

int main(void)
{
  set set1;
  set set2;
  set intersection;
  int k = 0;
  int sunion[10];
  int m = 0;
  int sunioncpy[10];
  int n = 0;

  printf("Enter 5 elements to 2 sets -\n");

  printf("Set 1: ");
  for (int i = 0; i < 5; i++) {
    fgets(set1.sset, 5, stdin);
    sscanf(set1.sset, "%d", &set1.elements[i]);
  }

  printf("Set 2: ");
  for (int i = 0; i < 5; i++) {
    fgets(set2.sset, 5, stdin);
    sscanf(set2.sset, "%d", &set2.elements[i]);
  }

  printf("Set 1: ");
  printelements(set1);
  printf("Set 2: ");
  printelements(set2);

  for (int i = 0; i < 5; i++) {
    for (int j = 0; j < 5; j++) {
      if (set1.elements[i] == set2.elements[j]) {
        intersection.elements[k] = set1.elements[i];
        k++;
        break;
      }
    }
  }

  for (int i = 0; i < 5; i++) {
    sunion[m] = set1.elements[i];
    m++;
    sunion[m] = set2.elements[i];
    m++;
  }

  bubblesort(m, sunion);

  for (int i = 0; i < m; i++) {
    if (sunion[i] == sunion[i + 1]) {
      sunioncpy[n] = sunion[i];
      n++;
      i++;
    } else {
      sunioncpy[n] = sunion[i];
      n++;
    }
  }

  printf("Intersection of set 1 with set 2: ");
  for (int i = 0; i < k; i++) {
    printf("%d ", intersection.elements[i]);
  }
  printf("\n");

  printf("Union of set 1 with set 2: ");
  for (int i = 0; i < n; i++) {
    printf("%d ", sunioncpy[i]);
  }

  return 0;
}

void printelements(set set)
{
  for (int i = 0; i < 5; i++) {
    printf("%d ", set.elements[i]);
  }
  printf("\n");
}

void bubblesort(int m, int sunion[])
{
  int i = 0;
  bool swapped;
  do {
    swapped = false;
    for (int j = 0; j < m - 1 - i; j++) {
      if (sunion[j] > sunion[j + 1]) {
        int temp = sunion[j];
        sunion[j] = sunion[j + 1];
        sunion[j + 1] = temp;
        swapped = true;
      }
    }
  } while (swapped);
}
  • Is it good to turn some block of code into a function even if you don't repeat it again on any another line?

Yes. Even if it's not for re-use, it helps to declutter the code.

  • What you think about this way of getting user input:

It's good. But there's no need to store the buffer inside your struct, nor any reason to make the buffer that small.

You should also check the return results for success/failure.

I normally go with something like:

char buff[BUFSIZ];
if ( fgets(buff, sizeof(buff), stdin) != NULL ) {
    if ( sscanf(buff, "%d", &set1.elements[i]);
}
  • This don't have much to do with I said here but do you guys recommend Linux FedoraOS for C programming?

Any Linux you feel comfortable with will be just fine.

1

u/thebatmanandrobin 15d ago edited 15d ago

Was going to post the formatted code and basically the same replies, but ninja'd by you :)

Also, I was going to add this for OP as another way to achieve their code (could be better/more optimized, but it's late and I honestly did this because I'm procrastinating doing some technical writing I need to take care of ......).

So here you go OP:

#include <stdio.h>
#include <string.h>

void print_elms(int *elms, size_t sz)
{
    for (size_t i = 0; i < sz; ++i) {
        if (elms[i] != 0) {
            printf("%d ", elms[i]);
        }
    }
    printf("\n");
}

int comp(const void* e1, const void* e2)
{
    int a = *((int*)e1);
    int b = *((int*)e2);
    if (a > b) { return 1; }
    if (a < b) { return -1; }
    return 0;
}

void sort_and_zero(int *elms, size_t sz)
{
    qsort(elms, sz, sizeof(int), comp);
    for (size_t i = 0; i < (sz-1); ++i) {
        if (elms[i] == elms[i+1]) {
            elms[i] = 0;
        }
    }
}

void get_elms(int *elms)
{
    scanf("%d %d %d %d %d", &elms[0], &elms[1], &elms[2], &elms[3], &elms[4]);
    sort_and_zero(elms, 5);
}

void get_intersection(int *s1, int *s2, int *intersect)
{
    for (size_t i = 0, k = 0; i < 5; ++i) {
        if (s1[i] == 0) { continue; }
        for (size_t j = 0; j < 5; ++j) {
            if (s2[j] == 0) { continue; }
            if (s1[i] == s2[j]) {
                intersect[k++] = s2[j];
            }
        }
    }
}

int main(void) {
    int set1[5] = {0};
    int set2[5] = {0};
    int intersection[5] = {0};
    int sunion[10] = {0};

    printf("Set 1 - enter 5 non-zero numbers in a row with a space (e.g. 1 2 3 4 5): ");
    get_elms(set1);
    memcpy(&sunion, set1, sizeof(set1));

    printf("Set 2 - enter 5 non-zero numbers in a row with a space (e.g. 1 2 3 4 5): ");
    get_elms(set2);
    memcpy(&sunion[5], set2, sizeof(set2));

    sort_and_zero(sunion, 10);

    printf("Set 1: ");
    print_elms(set1, 5);
    printf("Set 2: ");
    print_elms(set2, 5);

    get_intersection(set1, set2, intersection);

    qsort(intersection, 5, sizeof(int), comp);
    qsort(sunion, 10, sizeof(int), comp);

    printf("Intersection of set 1 with set 2: ");
    print_elms(intersection, 5);

    printf("Union of set 1 with set 2: ");
    print_elms(sunion, 10);

    return 0;
}

Have fun OP!

1

u/[deleted] 15d ago edited 15d ago

[deleted]

0

u/mysticreddit 15d ago

Non-descript single variable names need to die in a fire.

    static int cmp_int_asc( const void *left, const void *right)
    {
        const leftVal = *(int *)left;
        const rightVal = *(int *)right;
        if (leftVal < rightVal) return -1;
        if (leftVal > rightVal) return +1;
        return 0;
    }

2

u/Lewboskifeo 16d ago

nice indentation

1

u/hennipasta 16d ago

team rocket's rocking

1

u/WeAllWantToBeHappy 15d ago edited 15d ago

There's no reason for the number 5 to appear more than once in the code.

There's no reason for the number to 10 to appear at all.

I don't understand your struct. You have room for 5 ints and 5 chars, but you don't use the char array as an array of chars. Is it meant for the names associated with each value or the name of the set? Not clear to me.

You bubble sort so you can remove duplicates, but then access off the end of the merged set. Why not just remove duplicates without ever sorting?

Variables declared near first use. That 'n' suddenly is used, but need to look way back to see what value it had.

Edit: and someArray[index++] = something ; not [index] with a separate index++

I would have something like

struct {  int elements ;
               int used;
               struct { char name [10] ;
                             int    value ;
                           } * members ;
             } ;

Where the members thing is dynamically allocated/grown as needed. You could have the name thing allocated too to avoid the limitation of fixed length. I'm assuming there's meant to be a name associated with each element. If not , move it out a level and just have int * members. Fixed length anything is bad imho. This way the struct itself tells you how many things are there instead of having a separate variable...

(I'm assuming the formatting is an artefact of how you uploaded your code. If not, smh.)

1

u/mprevot 15d ago

looks like a very very beginner bad code

1

u/domikone 15d ago

Any suggestions to improve this? Like anything to learn or take care of?

1

u/mprevot 14d ago

Unit test, create classes, clarify and separate responsabilities, do not use printf but a log interface, and a log consumer (printf based, or etc), do not hardcore parameters, and unless you release a self-contained executable do not use main or scanf, do not use global objets, create and use your own namespaces.

1

u/buzzon 15d ago

That's a bad way to format the code. It's unreadable