0
\$\begingroup\$

I have much more experience with PAWN scripting language, but I'm digging into K&R book (I think it is it), and trying some stuff on my own.

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

#define ITEMS_COUNT 16

unsigned int randr(unsigned int min, unsigned int max);
inline short randomType();
inline char* randomCompanyName();
inline char* randomPersonName();
inline char* randomPersonSurname();
void populateItems();
void dumpItems();

typedef unsigned short int DBID;

typedef struct {
    char name[64];
    char surname[64];
} Person;

typedef struct {
    char name[128];
} Company;

typedef struct {
    unsigned short int type;
    DBID dbID;
    union {
        Person PersonItem;
        Company CompanyItem;
    } chosenItem;
} Item;

enum E_TYPE {
    TYPE_PERSON,
    TYPE_COMPANY
};

Item
    itemList[ITEMS_COUNT];

int main(int argc, char* argv[]) {
    /* Init stuff */
    srand(time(NULL));

    populateItems();
    dumpItems();
    return 0;
}

void populateItems() {
    Item
        singleItem;
    size_t
        i = 0;

    for(; i != ITEMS_COUNT; ++i) {
        singleItem.dbID =   i + 1;
        singleItem.type = randomType();

        if(TYPE_PERSON == singleItem.type) {
            strncpy(singleItem.chosenItem.PersonItem.name, randomPersonName(), sizeof(singleItem.chosenItem.PersonItem.name));
            strncpy(singleItem.chosenItem.PersonItem.surname, randomPersonSurname(), sizeof(singleItem.chosenItem.PersonItem.surname));
        } else {
            strncpy(singleItem.chosenItem.CompanyItem.name, randomCompanyName(), sizeof(singleItem.chosenItem.CompanyItem.name));
        }
        memcpy(&itemList[i], &singleItem, sizeof(singleItem));
    }
}

void dumpItems() {
    Item
        *currentItem;
    size_t
        i = 0;
    char
        str[256];

    for(; i != ITEMS_COUNT; ++i) {
        currentItem = &itemList[i];

        //Not safe sprintf!
        sprintf(str, "Row %d: { dbID: %d,", i + 1, currentItem->dbID);

        if(TYPE_PERSON == currentItem->type) {
            sprintf(str, "%s name: %s, surname: %s }", str, currentItem->chosenItem.PersonItem.name, currentItem->chosenItem.PersonItem.surname);
        } else {
            sprintf(str, "%s company name: %s }", str, currentItem->chosenItem.CompanyItem.name);
        }

        printf("%s\n", str);
    }
}

inline short randomType() {
    return randr(0, 1);
}

inline char* randomCompanyName() {
    static char* 
        nameList[32] = {
        "Microsoft",
        "Valve",
        "Vamonos pest",
        "Hello folks!",
        "Just a program inc"
    };
    return nameList[randr(0, 4)];
}

inline char* randomPersonName() {
    static char* 
        nameList[32] = {
        "John",
        "Mike",
        "Emily",
        "Jessica",
        "Steven"
    };
    return nameList[randr(0, 4)];
}

inline char* randomPersonSurname() {
    static char* 
        nameList[32]  = {
        "Ross",
        "Litt",
        "Hellyeah"
    };
    return nameList[randr(0, 2)];
}

unsigned int randr(unsigned int min, unsigned int max)
{
    double 
        scaled = (double) rand() / RAND_MAX;

    return (max - min + 1) * scaled + min;
}

I'd like you to tell me if I've made any major mistakes, and how's my coding style. Additionally I have a question about unions. I thought that it simply pointed to regions, and that you didn't have to care later which one you are using, like:

//instead of 
singleItem.chosenItem.PersonItem.name
//use
singleItem.chosenItem.name

which would point at either PersonItem.name, or CompanyItem.name. How to achieve somthing similar?

\$\endgroup\$
1
  • \$\begingroup\$ K&R is about the C language, inline is C++'s alternative/answer/attempt at solving macro issues \$\endgroup\$ Nov 12 '13 at 12:54
2
\$\begingroup\$

My little bit of input:

  1. inline is a C++ keyword, and it's only a hint to the compiler. The only reason to explicitly use inline is when you define function's body inside a header file, which is included in more than one source file.

  2. Avoid basic data types like int, use types from stdint.h instead

  3. Use comments to explain why are you doing something, and what are function arguments for

  4. No reason to break a line after declaring a type.

  5. No reason to give nameList[32] a size - the compiler will guess it on its own. Just do it as

    static char* nameList[]  = {
        "Ross",
        "Litt",
        "Hellyeah"
    }; 
    
  6. Use sizeof(nameList) - 1 instead of a magic number to indicate last array index - there will be less changes if you ever add more names.

Regarding your union questions:

union is kinda like a struct; the only difference is how it's handled in memory. Since your union has two fields (PersonItem and CompanyItem), you must say which one is to be used. Every object created with your struct will contain both fields. What you're trying to do is called polymorphism, and you have it with classes in C++. Original C does not have such option.

\$\endgroup\$
9
  • \$\begingroup\$ Also: read this, it should help you with code quality, even though it's not for C :) net.tutsplus.com/tutorials/html-css-techniques/… \$\endgroup\$ Nov 8 '13 at 22:42
  • \$\begingroup\$ You might cringe a little, but I'm a professional webdeveloper (+ php scripter) (as in earn money for living with, not skill level yet) - don't worry though, I stick to php-psr. I didn't know that inline is C++ only, and I had no clue about badness of builtin datatypes - thanks for that. \$\endgroup\$
    – Misiur
    Nov 8 '13 at 22:53
  • \$\begingroup\$ I know that unions are structs with each element pointing to index 0 (I guess). And those 32 were to tell future coders that you can't go over 31 + NUL limit. I wanted to master C (at least syntax) before moving on to C++, but I think I know what you're talking about (virtual classes and stuff) \$\endgroup\$
    – Misiur
    Nov 8 '13 at 22:55
  • 1
    \$\begingroup\$ Ok :) In that case, since you know what you're doing I have nothing to add :) Good luck :) \$\endgroup\$ Nov 8 '13 at 22:58
  • 1
    \$\begingroup\$ @DarthHunterix your point 1 is untrue, inline is in c99. Your point 2 is not appropriate here. There is no reason not to use int/char etc in this code. Your point 6 is wrong - sizeof(nameList) is the size in bytes of nameList, not the number of char* entries in the array. \$\endgroup\$ Nov 9 '13 at 0:39
2
\$\begingroup\$

Your code has a number of issues.

It is often better to layout the code in reverse order. This avoids the need for local prototypes. For example, main goes last in the file and the functions it calls are above it. inline functions must be defined before they are used - another reason for doing things backwards. Note that the compiler can decide for itself what should be inline and what not, so using 'inline' is normally unnecessary.

You have defined emum E_TYPE but not used it in struct Item, where it is needed. Instead you use unsigned short int type. Note that all-caps names are usually constants, so E_TYPE would be better as type. Function randomType should return an enum type, not a short and should use the values defined for the enum. The union in your Item struct need not have a name and its field names are verbose. Here is how I would simplify it:

typedef struct {
    enum type type;
    DBID id;
    union {
        Person person;
        Company company;
    };
} Item;

General point: don't put the type and variable on separate lines. Nobody does that.

Functions with no parameters should be defined with a void in the parameter list. The compiler should warn you about that (compile with warnings turned on - use at least the option -W with gcc).

In your randomCompanyName etc functions, the name list should be const. When selecting the random name using

return nameList[randr(0, 4)];

instead of an explicit 4, you should derive the value from the array:

const char* nameList[] = {
    "Microsoft",
    "Valve",
    "Vamonos pest",
    "Hello folks!",
    "Just a program inc"
};
const int listSize = sizeof nameList / sizeof nameList[0];
return nameList[randr(0, listSize - 1)];

You can do this only if you omit the size of the array.

Your randr is almost correct, but should divide by RANDMAX + 1

double scaled = rand() / (1.0 + RAND_MAX);

Your populateItems function is not very readable. For a start, it is generally best not to exceed about 80 characters per line. It is also odd to create an Item and then copy that into the global array. Why not create it directly in the correct array entry? Also, define the loop variable in the loop, not above it.

Your use of strncpy in the loop is a problem. strncpy does not do what you might expect when there is not enough space. It does not terminate the truncated string! To do it properly you must terminate each string explicitly. This is messy. The alternative copy function strlcpy that gets it right but is not universally available (and hence is non-portable). An alternative, perhaps nicer , is to get your randomCompanyName etc functions, to fill the string buffer themselves:

static char* randomCompanyName(char *buf, size_t size)
{
    static char* nameList[] = { ...
    };
    const int listSize = sizeof nameList / sizeof nameList[0];
    const char *name = nameList[randr(0, listSize - 1)];
    strncpy(buf, name, size);
    buf[size - 1] = '\0';
    return buf;
}

Putting these together, populateItems then becomes rather easier to read:

static void populateItems(void)
{
    for (int i = 0; i < ITEMS_COUNT; ++i) { // note < not !=
        Item *it = &itemList[i];
        it->id = i + 1;
        it->type = randomType();

        if (it->type = TYPE_PERSON) {
            randomPersonName(it->person.name, sizeof it->person.name);
            randomPersonSurname(it->person.surname, sizeof it->person.surname);
        } else {
            randomCompanyName(it->company.name, sizeof it->company.name);
        }
    }
}

Your dumpItems uses sprintf to create a string but overwrites the first part ("Row %ld: { dbID: %d,") with the second part ("%s name: %s, surname: %s }") as it does not index into str. sprintf like strcpy etc is a common source of buffer overruns. You could use snprintf instead, but really why bother? It would be much easier just to print the two parts of the string directly to stdout.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your feedback! I have question though: is using static for every function some kind of convention? (even if the program is single file) Didn't know that bit about strncpy, I was a little confused when I saw 20 diffrent strcpy variations. Oh, and I don't know why I used the new instance -> memcpy thingy, I think I didn't know then about the -> operator and thought I'd have to use (*it).stuff all the time. pastebin.com/Tp30jhTE - much more readable now. \$\endgroup\$
    – Misiur
    Nov 9 '13 at 2:50
  • \$\begingroup\$ Functions should be static if they will never be used outside the file that defines them. For a single-file program this doesn't matter, but adding the static to each function keeps my compiler silent (gcc warning -Wmissing-prototypes). I just forgot to remove them. \$\endgroup\$ Nov 9 '13 at 11:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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