Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

Overall, this code doesn't look too bad, but there are a few places that can be improved.

  • This line:

     char nums[10] = {0};
    

    doesn't initialize all the elements of nums (I think some compilers will default-initialize local variables in certain build modes, but you shouldn't rely on that). Use a for loop or the standard library function memset() to do that:

     #include <string.h>
     ...
     memset(nums, 0, sizeof(nums));
    

    As 200_success notes notes, using a char as the base type of the array leaves you prone to overflow if you happen to randomly generate more than 127 of a given number (assuming your char has 8 bits, which it probably does). For counting things, it's generally a good idea to use unsigned quantities if you know your count will never be negative, so nums should be declared as:

     unsigned int nums[10];
    
  • In this code, you're adding 1 to generated_num, then subtracting 1 when you use it on the next line as an index:

     generated_num = rand() % 10 + 1;
     nums[generated_num-1] += 1;
    

    Leave that out:

     generated_num = rand() % 10;
     nums[generated_num] += 1;
    

    You could conceivably collapse the line into one, saving yourself the declaration of generated_num at the start of main:

     nums[rand() % 10] += 1;
    

    You could also use the pre- or post-increment operators to update the count for the generated number:

     nums[rand() % 10]++;
    
  • You use the magic number 10 (the size of the nums array) in several places in the code. You should declare that in a constant, one of:

     // Number of bins in the histogram of random numbers.
     static const size_t NUM_BINS = 10;
     // ... or ...
     #define NUM_BINS 10
     // ...
     char nums[NUM_BINS]; // etc.
    

    That way, if you decide you want 11 or 12 or 37 random numbers, you only have to change it in one place.

Overall, this code doesn't look too bad, but there are a few places that can be improved.

  • This line:

     char nums[10] = {0};
    

    doesn't initialize all the elements of nums (I think some compilers will default-initialize local variables in certain build modes, but you shouldn't rely on that). Use a for loop or the standard library function memset() to do that:

     #include <string.h>
     ...
     memset(nums, 0, sizeof(nums));
    

    As 200_success notes, using a char as the base type of the array leaves you prone to overflow if you happen to randomly generate more than 127 of a given number (assuming your char has 8 bits, which it probably does). For counting things, it's generally a good idea to use unsigned quantities if you know your count will never be negative, so nums should be declared as:

     unsigned int nums[10];
    
  • In this code, you're adding 1 to generated_num, then subtracting 1 when you use it on the next line as an index:

     generated_num = rand() % 10 + 1;
     nums[generated_num-1] += 1;
    

    Leave that out:

     generated_num = rand() % 10;
     nums[generated_num] += 1;
    

    You could conceivably collapse the line into one, saving yourself the declaration of generated_num at the start of main:

     nums[rand() % 10] += 1;
    

    You could also use the pre- or post-increment operators to update the count for the generated number:

     nums[rand() % 10]++;
    
  • You use the magic number 10 (the size of the nums array) in several places in the code. You should declare that in a constant, one of:

     // Number of bins in the histogram of random numbers.
     static const size_t NUM_BINS = 10;
     // ... or ...
     #define NUM_BINS 10
     // ...
     char nums[NUM_BINS]; // etc.
    

    That way, if you decide you want 11 or 12 or 37 random numbers, you only have to change it in one place.

Overall, this code doesn't look too bad, but there are a few places that can be improved.

  • This line:

     char nums[10] = {0};
    

    doesn't initialize all the elements of nums (I think some compilers will default-initialize local variables in certain build modes, but you shouldn't rely on that). Use a for loop or the standard library function memset() to do that:

     #include <string.h>
     ...
     memset(nums, 0, sizeof(nums));
    

    As 200_success notes, using a char as the base type of the array leaves you prone to overflow if you happen to randomly generate more than 127 of a given number (assuming your char has 8 bits, which it probably does). For counting things, it's generally a good idea to use unsigned quantities if you know your count will never be negative, so nums should be declared as:

     unsigned int nums[10];
    
  • In this code, you're adding 1 to generated_num, then subtracting 1 when you use it on the next line as an index:

     generated_num = rand() % 10 + 1;
     nums[generated_num-1] += 1;
    

    Leave that out:

     generated_num = rand() % 10;
     nums[generated_num] += 1;
    

    You could conceivably collapse the line into one, saving yourself the declaration of generated_num at the start of main:

     nums[rand() % 10] += 1;
    

    You could also use the pre- or post-increment operators to update the count for the generated number:

     nums[rand() % 10]++;
    
  • You use the magic number 10 (the size of the nums array) in several places in the code. You should declare that in a constant, one of:

     // Number of bins in the histogram of random numbers.
     static const size_t NUM_BINS = 10;
     // ... or ...
     #define NUM_BINS 10
     // ...
     char nums[NUM_BINS]; // etc.
    

    That way, if you decide you want 11 or 12 or 37 random numbers, you only have to change it in one place.

added 284 characters in body
Source Link
Niall C.
  • 859
  • 1
  • 7
  • 19

Overall, this code doesn't look too bad, but there are a few places that can be improved.

  • This line:

     char nums[10] = {0};
    

    doesn't initialize all the elements of nums (I think some compilers will default-initialize local variables in certain build modes, but you shouldn't rely on that). Use a for loop or the standard library function memset() to do that:

     #include <string.h>
     ...
     memset(nums, 0, sizeof(nums));
    

    As 200_success notes , using a char as the base type of the array leaves you prone to overflow if you happen to randomly generate more than 127 of a given number (assuming your char has 8 bits, which it probably does). For counting things, it's generally a good idea to use unsigned quantities if you know your count will never be negative, so nums should be declared as:

     unsigned int nums[10];
    
  • In this code, you're adding 1 to generated_num, then subtracting 1 when you use it on the next line as an index:

     generated_num = rand() % 10 + 1;
     nums[generated_num-1] += 1;
    

    Leave that out:

     generated_num = rand() % 10;
     nums[generated_num] += 1;
    

    You could conceivably collapse the line into one, saving yourself the declaration of generated_num at the start of main:

     nums[rand() % 10] += 1;
    

    You could also use the pre- or post-increment operators to update the count for the generated number:

     nums[rand() % 10]++;
    
  • You use the magic number 10 (the size of the nums array) in several places in the code. You should declare that in a constant, one of:

     // Number of bins in the histogram of random numbers.
     static const size_t NUM_BINS = 10;
     // ... or ...
     #define NUM_BINS 10
     // ...
     char nums[NUM_BINS]; // etc.
    

    That way, if you decide you want 11 or 12 or 37 random numbers, you only have to change it in one place.

Overall, this code doesn't look too bad, but there are a few places that can be improved.

  • This line:

     char nums[10] = {0};
    

    doesn't initialize all the elements of nums (I think some compilers will default-initialize local variables in certain build modes, but you shouldn't rely on that). Use a for loop or the standard library function memset() to do that:

     #include <string.h>
     ...
     memset(nums, 0, sizeof(nums));
    
  • In this code, you're adding 1 to generated_num, then subtracting 1 when you use it on the next line as an index:

     generated_num = rand() % 10 + 1;
     nums[generated_num-1] += 1;
    

    Leave that out:

     generated_num = rand() % 10;
     nums[generated_num] += 1;
    

    You could conceivably collapse the line into one, saving yourself the declaration of generated_num at the start of main:

     nums[rand() % 10] += 1;
    

    You could also use the pre- or post-increment operators to update the count for the generated number:

     nums[rand() % 10]++;
    
  • You use the magic number 10 (the size of the nums array) in several places in the code. You should declare that in a constant, one of:

     // Number of bins in the histogram of random numbers.
     static const size_t NUM_BINS = 10;
     // ... or ...
     #define NUM_BINS 10
     // ...
     char nums[NUM_BINS]; // etc.
    

    That way, if you decide you want 11 or 12 or 37 random numbers, you only have to change it in one place.

Overall, this code doesn't look too bad, but there are a few places that can be improved.

  • This line:

     char nums[10] = {0};
    

    doesn't initialize all the elements of nums (I think some compilers will default-initialize local variables in certain build modes, but you shouldn't rely on that). Use a for loop or the standard library function memset() to do that:

     #include <string.h>
     ...
     memset(nums, 0, sizeof(nums));
    

    As 200_success notes , using a char as the base type of the array leaves you prone to overflow if you happen to randomly generate more than 127 of a given number (assuming your char has 8 bits, which it probably does). For counting things, it's generally a good idea to use unsigned quantities if you know your count will never be negative, so nums should be declared as:

     unsigned int nums[10];
    
  • In this code, you're adding 1 to generated_num, then subtracting 1 when you use it on the next line as an index:

     generated_num = rand() % 10 + 1;
     nums[generated_num-1] += 1;
    

    Leave that out:

     generated_num = rand() % 10;
     nums[generated_num] += 1;
    

    You could conceivably collapse the line into one, saving yourself the declaration of generated_num at the start of main:

     nums[rand() % 10] += 1;
    

    You could also use the pre- or post-increment operators to update the count for the generated number:

     nums[rand() % 10]++;
    
  • You use the magic number 10 (the size of the nums array) in several places in the code. You should declare that in a constant, one of:

     // Number of bins in the histogram of random numbers.
     static const size_t NUM_BINS = 10;
     // ... or ...
     #define NUM_BINS 10
     // ...
     char nums[NUM_BINS]; // etc.
    

    That way, if you decide you want 11 or 12 or 37 random numbers, you only have to change it in one place.

added 284 characters in body
Source Link
Niall C.
  • 859
  • 1
  • 7
  • 19

Overall, this code doesn't look too bad, but there are a couple offew places that can be improved.

  • This line:

     char nums[10] = {0};
    

    doesn't initialize all the elements of nums (I think some compilers will default-initialize local variables in certain build modes, but you shouldn't rely on that). Use a for loop or the standard library function memset() to do that:

     #include <string.h>
     ...
     memset(nums, 0, sizeof(nums));
    
  • In this code, you're adding 1 to generated_num, then subtracting 1 when you use it on the next line as an index:

     generated_num = rand() % 10 + 1;
     nums[generated_num-1] += 1;
    

    Leave that out:

     generated_num = rand() % 10;
     nums[generated_num] += 1;
    

    You could conceivably collapse the line into one, saving yourself the declaration of generated_num at the start of main:

     nums[rand() % 10] += 1;
    

    You could also use the pre- or post-increment operators to update the count for the generated number:

     nums[rand() % 10]++;
    
  • You use the magic number 10 (the size of the nums array) in several places in the code. You should declare that in a constant, one of:

     // Number of bins in the histogram of random numbers.
     static const size_t NUM_BINS = 10;
     // ... or ...
     #define NUM_BINS 10
     // ...
     char nums[NUM_BINS]; // etc.
    

    That way, if you decide you want 11 or 12 or 37 random numbers, you only have to change it in one place.

Overall, this code doesn't look too bad, but there are a couple of places that can be improved.

  • This line:

     char nums[10] = {0};
    

    doesn't initialize all the elements of nums (I think some compilers will default-initialize local variables in certain build modes, but you shouldn't rely on that). Use a for loop or the standard library function memset() to do that:

     #include <string.h>
     ...
     memset(nums, 0, sizeof(nums));
    
  • In this code, you're adding 1 to generated_num, then subtracting 1 when you use it on the next line as an index:

     generated_num = rand() % 10 + 1;
     nums[generated_num-1] += 1;
    

    Leave that out:

     generated_num = rand() % 10;
     nums[generated_num] += 1;
    

    You could conceivably collapse the line into one, saving yourself the declaration of generated_num at the start of main:

     nums[rand() % 10] += 1;
    

    You could also use the pre- or post-increment operators to update the count for the generated number:

     nums[rand() % 10]++;
    

Overall, this code doesn't look too bad, but there are a few places that can be improved.

  • This line:

     char nums[10] = {0};
    

    doesn't initialize all the elements of nums (I think some compilers will default-initialize local variables in certain build modes, but you shouldn't rely on that). Use a for loop or the standard library function memset() to do that:

     #include <string.h>
     ...
     memset(nums, 0, sizeof(nums));
    
  • In this code, you're adding 1 to generated_num, then subtracting 1 when you use it on the next line as an index:

     generated_num = rand() % 10 + 1;
     nums[generated_num-1] += 1;
    

    Leave that out:

     generated_num = rand() % 10;
     nums[generated_num] += 1;
    

    You could conceivably collapse the line into one, saving yourself the declaration of generated_num at the start of main:

     nums[rand() % 10] += 1;
    

    You could also use the pre- or post-increment operators to update the count for the generated number:

     nums[rand() % 10]++;
    
  • You use the magic number 10 (the size of the nums array) in several places in the code. You should declare that in a constant, one of:

     // Number of bins in the histogram of random numbers.
     static const size_t NUM_BINS = 10;
     // ... or ...
     #define NUM_BINS 10
     // ...
     char nums[NUM_BINS]; // etc.
    

    That way, if you decide you want 11 or 12 or 37 random numbers, you only have to change it in one place.

Source Link
Niall C.
  • 859
  • 1
  • 7
  • 19
Loading
lang-c

AltStyle によって変換されたページ (->オリジナル) /