Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  1. Your shuffling-algorithms is broken: See The Danger of Naïveté for the explanation, and Fisher-Yates shuffle on wikipedia for a working replacement.

    Aside from that, you really should only initialize the random-generator once, at the start of the program.
    (For debuggability, you might want to add an option to specify the seed on the command-line.)

  2. For no obvious reason, you have separated definition and initialization of multdraw, noskip and winscore in main...

  3. You could use getopt for parsing the command-line. As-is, it looks complicated enough to extract into its own (local) function.

  4. Actually, you should really break it up in more functions. If they are only used in the same TU, mark them static. And if you define them before you use them, you don't even need any forward-declaration.

  5. Your handling of cards is far too complicated.
    A deck consists of:

    • Wild card, and Wild card draw four, each four times.

      The wild-cards are special in that they don't actually have a color. Whoever plays one decides which color follows. Best to model that by letting them assume that color.

    • 0 once per color.

    • skip, reverse, draw two, 1 to 9, each twice per color.

    All cards of same color and symbol are indistinguishable, their identities are immaterial.
    So, let's encode them:

    • We have four colors, put that in the low bits.
    • And we have 15 symbols, declaring symbol 0 invalid makes 16.

    6 bits comfortably fit into a char...
    And now we can allocate a single buffer for each hand/deck/pile at the start, and use the first byte as a count. Much easier and faster.

  6. You really should test whether scanw and other input-functions actually succeed in getting the asked number of inputs, not only whether those inputs are in the range you accept...

  7. sizeof(char) is defined as 1, also Don't cast the result of malloc/callocDon't cast the result of malloc/calloc, and avoid useless zeroing of buffers.

  8. return 0; is implicit in main since C99.

  9. Your handling of single-linked-lists can be improved. Not that you should use any single-linked-list in that program, a counted or 0-terminated string is far superior...

     void clean(struct Handcard* head) { // Was quadratic. Now optimal
         for(struct Handcard* next; head; head = next) {
             next = head->link;
             free(head);
         }
     }
    
  10. I wonder why you added the superfluous null-check in length...

  11. Regarding struct Cardname, I earnestly suggest you not use a union but always two strings.

  1. Your shuffling-algorithms is broken: See The Danger of Naïveté for the explanation, and Fisher-Yates shuffle on wikipedia for a working replacement.

    Aside from that, you really should only initialize the random-generator once, at the start of the program.
    (For debuggability, you might want to add an option to specify the seed on the command-line.)

  2. For no obvious reason, you have separated definition and initialization of multdraw, noskip and winscore in main...

  3. You could use getopt for parsing the command-line. As-is, it looks complicated enough to extract into its own (local) function.

  4. Actually, you should really break it up in more functions. If they are only used in the same TU, mark them static. And if you define them before you use them, you don't even need any forward-declaration.

  5. Your handling of cards is far too complicated.
    A deck consists of:

    • Wild card, and Wild card draw four, each four times.

      The wild-cards are special in that they don't actually have a color. Whoever plays one decides which color follows. Best to model that by letting them assume that color.

    • 0 once per color.

    • skip, reverse, draw two, 1 to 9, each twice per color.

    All cards of same color and symbol are indistinguishable, their identities are immaterial.
    So, let's encode them:

    • We have four colors, put that in the low bits.
    • And we have 15 symbols, declaring symbol 0 invalid makes 16.

    6 bits comfortably fit into a char...
    And now we can allocate a single buffer for each hand/deck/pile at the start, and use the first byte as a count. Much easier and faster.

  6. You really should test whether scanw and other input-functions actually succeed in getting the asked number of inputs, not only whether those inputs are in the range you accept...

  7. sizeof(char) is defined as 1, also Don't cast the result of malloc/calloc, and avoid useless zeroing of buffers.

  8. return 0; is implicit in main since C99.

  9. Your handling of single-linked-lists can be improved. Not that you should use any single-linked-list in that program, a counted or 0-terminated string is far superior...

     void clean(struct Handcard* head) { // Was quadratic. Now optimal
         for(struct Handcard* next; head; head = next) {
             next = head->link;
             free(head);
         }
     }
    
  10. I wonder why you added the superfluous null-check in length...

  11. Regarding struct Cardname, I earnestly suggest you not use a union but always two strings.

  1. Your shuffling-algorithms is broken: See The Danger of Naïveté for the explanation, and Fisher-Yates shuffle on wikipedia for a working replacement.

    Aside from that, you really should only initialize the random-generator once, at the start of the program.
    (For debuggability, you might want to add an option to specify the seed on the command-line.)

  2. For no obvious reason, you have separated definition and initialization of multdraw, noskip and winscore in main...

  3. You could use getopt for parsing the command-line. As-is, it looks complicated enough to extract into its own (local) function.

  4. Actually, you should really break it up in more functions. If they are only used in the same TU, mark them static. And if you define them before you use them, you don't even need any forward-declaration.

  5. Your handling of cards is far too complicated.
    A deck consists of:

    • Wild card, and Wild card draw four, each four times.

      The wild-cards are special in that they don't actually have a color. Whoever plays one decides which color follows. Best to model that by letting them assume that color.

    • 0 once per color.

    • skip, reverse, draw two, 1 to 9, each twice per color.

    All cards of same color and symbol are indistinguishable, their identities are immaterial.
    So, let's encode them:

    • We have four colors, put that in the low bits.
    • And we have 15 symbols, declaring symbol 0 invalid makes 16.

    6 bits comfortably fit into a char...
    And now we can allocate a single buffer for each hand/deck/pile at the start, and use the first byte as a count. Much easier and faster.

  6. You really should test whether scanw and other input-functions actually succeed in getting the asked number of inputs, not only whether those inputs are in the range you accept...

  7. sizeof(char) is defined as 1, also Don't cast the result of malloc/calloc, and avoid useless zeroing of buffers.

  8. return 0; is implicit in main since C99.

  9. Your handling of single-linked-lists can be improved. Not that you should use any single-linked-list in that program, a counted or 0-terminated string is far superior...

     void clean(struct Handcard* head) { // Was quadratic. Now optimal
         for(struct Handcard* next; head; head = next) {
             next = head->link;
             free(head);
         }
     }
    
  10. I wonder why you added the superfluous null-check in length...

  11. Regarding struct Cardname, I earnestly suggest you not use a union but always two strings.

added 411 characters in body
Source Link
Deduplicator
  • 19.3k
  • 1
  • 31
  • 65
  1. Your shuffling-algorithms is broken: See The Danger of Naïveté for the explanation, and Fisher-Yates shuffle on wikipedia for a working replacement.

    Aside from that, you really should only initialize the random-generator once, at the start of the program.
    (For debuggability, you might want to add an option to specify the seed on the command-line.)

  2. For no obvious reason, you have separated definition and initialization of multdraw, noskip and winscore in main...

  3. You could use getopt for parsing the command-line. As-is, it looks complicated enough to extract into its own (local) function.

  4. Actually, you should really break it up in more functions. If they are only used in the same TU, mark them static. And if you define them before you use them, you don't even need any forward-declaration.

  5. Your handling of cards is far too complicated.
    A deck consists of:

    • Wild card, and Wild card draw four, each four times.

      The wild-cards are special in that they don't actually have a color. Whoever plays one decides which color follows. Best to model that by letting them assume that color.

    • 0 once per color.

    • skip, reverse, draw two, 1 to 9, each twice per color.

    All cards of same color and symbol are indistinguishable, their identities are immaterial.
    So, let's encode them:

    • We have four colors, put that in the low bits.
    • And we have 15 symbols, declaring symbol 0 invalid makes 16.
    • And we have four colors.

    6 bits comfortably fit into a char...
    And now we can allocate a single buffer for each hand/deck/pile at the start, and use the first byte as a count. Much easier and faster.

  6. You really should test whether scanw and other input-functions actually succeed in getting the asked number of inputs, not only whether those inputs are in the range you accept...

  7. sizeof(char) is defined as 1, also Do IDon't cast the result of malloc?/calloc, and avoid useless zeroing of buffers.

  8. return 0; is implicit in main since C99.

  9. Your handling of single-linked-lists can be improved. Not that you should use any single-linked-list in that program, a counted or 0-terminated string is far superior...

     void clean(struct Handcard* head) { // Was quadratic. Now optimal
         for(struct Handcard* next; head; head = next) {
             next = head->link;
             free(head);
         }
     }
    
  10. Your shuffling-algorithms is broken: See The Danger of Naïveté for the explanation, and Fisher-Yates shuffle on wikipedia for a working replacement.

    Aside from that, you really should only initialize the random-generator once, at the start of the program.
    (For debuggability, you might want to add an option to specify the seed on the command-line.)

  11. I wonder why you added the superfluous null-check in length...

  12. Regarding struct Cardname, I earnestly suggest you not use a union but always two strings.

  1. For no obvious reason, you have separated definition and initialization of multdraw, noskip and winscore...

  2. You could use getopt for parsing the command-line. As-is, it looks complicated enough to extract into its own (local) function.

  3. Your handling of cards is far too complicated.
    A deck consists of:

    • Wild card, and Wild card draw four, each four times.

      The wild-cards are special in that they don't actually have a color. Whoever plays one decides which color follows. Best to model that by letting them assume that color.

    • 0 once per color.

    • skip, reverse, draw two, 1 to 9, each twice per color.

    All cards of same color and symbol are indistinguishable, their identities are immaterial.
    So, let's encode them:

    • We have 15 symbols, declaring symbol 0 invalid makes 16.
    • And we have four colors.

    6 bits comfortably fit into a char...

  4. You really should test whether scanw and other input-functions actually succeed in getting the asked number of inputs, not only whether those inputs are in the range you accept...

  5. sizeof(char) is defined as 1, Do I cast the result of malloc?, and avoid useless zeroing of buffers.

  6. return 0; is implicit in main since C99.

  7. Your handling of single-linked-lists can be improved. Not that you should use any single-linked-list in that program, a counted or 0-terminated string is far superior...

     void clean(struct Handcard* head) { // Was quadratic. Now optimal
         for(struct Handcard* next; head; head = next) {
             next = head->link;
             free(head);
         }
     }
    
  8. Your shuffling-algorithms is broken: See The Danger of Naïveté for the explanation, and Fisher-Yates shuffle on wikipedia for a working replacement.

    Aside from that, you really should only initialize the random-generator once, at the start of the program.
    (For debuggability, you might want to add an option to specify the seed on the command-line.)

  9. I wonder why you added the superfluous null-check in length...

  10. Regarding struct Cardname, I earnestly suggest you not use a union but always two strings.

  1. Your shuffling-algorithms is broken: See The Danger of Naïveté for the explanation, and Fisher-Yates shuffle on wikipedia for a working replacement.

    Aside from that, you really should only initialize the random-generator once, at the start of the program.
    (For debuggability, you might want to add an option to specify the seed on the command-line.)

  2. For no obvious reason, you have separated definition and initialization of multdraw, noskip and winscore in main...

  3. You could use getopt for parsing the command-line. As-is, it looks complicated enough to extract into its own (local) function.

  4. Actually, you should really break it up in more functions. If they are only used in the same TU, mark them static. And if you define them before you use them, you don't even need any forward-declaration.

  5. Your handling of cards is far too complicated.
    A deck consists of:

    • Wild card, and Wild card draw four, each four times.

      The wild-cards are special in that they don't actually have a color. Whoever plays one decides which color follows. Best to model that by letting them assume that color.

    • 0 once per color.

    • skip, reverse, draw two, 1 to 9, each twice per color.

    All cards of same color and symbol are indistinguishable, their identities are immaterial.
    So, let's encode them:

    • We have four colors, put that in the low bits.
    • And we have 15 symbols, declaring symbol 0 invalid makes 16.

    6 bits comfortably fit into a char...
    And now we can allocate a single buffer for each hand/deck/pile at the start, and use the first byte as a count. Much easier and faster.

  6. You really should test whether scanw and other input-functions actually succeed in getting the asked number of inputs, not only whether those inputs are in the range you accept...

  7. sizeof(char) is defined as 1, also Don't cast the result of malloc/calloc, and avoid useless zeroing of buffers.

  8. return 0; is implicit in main since C99.

  9. Your handling of single-linked-lists can be improved. Not that you should use any single-linked-list in that program, a counted or 0-terminated string is far superior...

     void clean(struct Handcard* head) { // Was quadratic. Now optimal
         for(struct Handcard* next; head; head = next) {
             next = head->link;
             free(head);
         }
     }
    
  10. I wonder why you added the superfluous null-check in length...

  11. Regarding struct Cardname, I earnestly suggest you not use a union but always two strings.

Source Link
Deduplicator
  • 19.3k
  • 1
  • 31
  • 65

  1. For no obvious reason, you have separated definition and initialization of multdraw, noskip and winscore...

  2. You could use getopt for parsing the command-line. As-is, it looks complicated enough to extract into its own (local) function.

  3. Your handling of cards is far too complicated.
    A deck consists of:

    • Wild card, and Wild card draw four, each four times.

      The wild-cards are special in that they don't actually have a color. Whoever plays one decides which color follows. Best to model that by letting them assume that color.

    • 0 once per color.

    • skip, reverse, draw two, 1 to 9, each twice per color.

    All cards of same color and symbol are indistinguishable, their identities are immaterial.
    So, let's encode them:

    • We have 15 symbols, declaring symbol 0 invalid makes 16.
    • And we have four colors.

    6 bits comfortably fit into a char...

  4. You really should test whether scanw and other input-functions actually succeed in getting the asked number of inputs, not only whether those inputs are in the range you accept...

  5. sizeof(char) is defined as 1, Do I cast the result of malloc?, and avoid useless zeroing of buffers.

  6. return 0; is implicit in main since C99.

  7. Your handling of single-linked-lists can be improved. Not that you should use any single-linked-list in that program, a counted or 0-terminated string is far superior...

     void clean(struct Handcard* head) { // Was quadratic. Now optimal
         for(struct Handcard* next; head; head = next) {
             next = head->link;
             free(head);
         }
     }
    
  8. Your shuffling-algorithms is broken: See The Danger of Naïveté for the explanation, and Fisher-Yates shuffle on wikipedia for a working replacement.

    Aside from that, you really should only initialize the random-generator once, at the start of the program.
    (For debuggability, you might want to add an option to specify the seed on the command-line.)

  9. I wonder why you added the superfluous null-check in length...

  10. Regarding struct Cardname, I earnestly suggest you not use a union but always two strings.