Roman Numerals Kata in C Here is a "Roman Numerals" kata implementation. Given a decimal number one has to return a corresponding roman number.

I have following questions:


Is memory management correct? Here I assume that the caller is responsible for freeing memory.
Does this code conform to c code style?
Does it look like idiomatic c?


Thanks!

toRoman.h:

char * toRoman(int n);

typedef struct DecimalToRoman {
    int decimal;
    const char * roman;
} DecimalToRoman;


toRoman.c:

char * toRoman(int n) {
    if (n &lt; 1) return NULL;

    const DecimalToRoman numbers[] = {
        { 1000, "M"  },
        {  900, "CM" },
        {  500, "D"  },
        {  400, "CD" },
        {  100, "C"  },
        {   90, "XC" },
        {   50, "L"  },
        {   40, "XL" },
        {   10, "X"  },
        {    9, "IX" },
        {    5, "V"  },
        {    4, "IV" },
        {    1, "I"  }
    };

    size_t resultLen = 1;
    int tmp = n;
    for (size_t i = 0; i &lt; sizeof numbers / sizeof(DecimalToRoman) &amp;&amp; tmp; i++) {
        while(tmp &gt;= numbers[i].decimal) {
            tmp -= numbers[i].decimal;
            resultLen += strlen(numbers[i].roman);
        }
    }

    char * result = malloc(resultLen);
    if (result == NULL) {
        perror("allocate");
        return NULL;
    }
    memset(result, '\0', resultLen);

    for (size_t i = 0, r = 0; r &lt; sizeof numbers / sizeof(DecimalToRoman); r++) {
        while (n &gt;= numbers[r].decimal) {
            n -= numbers[r].decimal;

            int j = 0;
            while (numbers[r].roman[j])
                result[i++] = numbers[r].roman[j++];
        }
    }
    return result;
}
toRoman.h


Is there any reason to expose DecimalToRoman to callers?



char * toRoman(int n)


Nit: it might be preferable to take unsigned rather than int as you cannot really handle negative numbers.



if (n &lt; 1) return NULL;


Seems reasonable.  If you took unsigned, then you would only have to handle 0.  In that case, it might make sense to return "" so you always return a valid string.

On second thought, there's sort of an upper-bound on what you can reasonably write as a roman numeral.  Does it make sense to write two billion in terms of thousands?  If you choose an upper bound, you can make assumptions about the length of your buffer.



const DecimalToRoman numbers[] = {
....


I love that this is written out so clearly.  Nice.



size_t resultLen = 1;


Help a reader out by saying why you init to 1.  IMO it's not obvious until you get to malloc.

int tmp = n;
for (size_t i = 0; i &lt; sizeof numbers / sizeof(DecimalToRoman) &amp;&amp; tmp; i++) {
    while(tmp &gt;= numbers[i].decimal) {
        tmp -= numbers[i].decimal;
        resultLen += strlen(numbers[i].roman);
    }
}


How about making this a function?  There's a common pattern:

Type thing = init;
...actually figure out what thing is often with a loop...

// don't change thing anymore!
... use thing ...


This is almost always more clearly written as:

Type const thing = createThing(init);
... use thing ...


But anyway back to this code.

sizeof numbers / sizeof(DecimalToRoman)


How about making this a variable named lenNumbers or something like that?  Super nit: I like that you used parens for the type and not for the expression, but IMO it looks weird with the operator/.  More clear: (sizeof numbers) / sizeof(DecimalToRoman).



if (result == NULL) {
    perror("allocate");
    return NULL;
}


Good.  I admit I usually assert in situations like this since there's not much you can do when you're out of memory (and it rarely happens in the things I do), but this is the party line.



memset(result, '\0', resultLen);


Isn't this overkill?  You're about to overwrite the memory anyway.  Could just set the last character to '\0` (and you might as well do that after you write the answer for cache locality).



        int j = 0;
        while (numbers[r].roman[j])
            result[i++] = numbers[r].roman[j++];


Better option: a for loop.  Best option: strcpy.



You could remove the calls to strlen by storing the length in DecimalToRoman.  The lengths are tiny so this is more a question of simplicity than optimization.



There's a common pattern in C APIs for returning buffers. The idea is you let the user pass in the buffer and you return an error code.  Something like:

int getRomanNumerals(int nInput, char* strOut, int* nLen);


The return value is either success, failure, or "insufficient buffer" (you probably want an enum or a preprocessor #define for this).  In any case, the user passes in a buffer and the buffer's length.  You always set nLen to the number of bytes needed, and you set strOut if you have enough space (maybe you start setting strOut assuming the buffer is long enough, but stop writing if you run out of space and only need to set nLen).

This way the user can do crazy stuff like store the result in a weird place or maybe the user knows they don't have enough memory to store the whole thing etc.