Base64 encoding implementation

Question

I am learning C and decided to make an implementation of Base64 encoding according to the info Wikipedia provides about it.

My main question is: should I declare the index_table array inside the get_base64_digit function like static char index_table[] = "blabla"?

A general code review would also be appreciated.

funcs.c

static char index_table[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";

static char get_index_digit(int x) {
    return index_table[x];
}

char get_base64_digit(const unsigned int base64, int digit_number){

    switch (digit_number){
            case 0:
                    return get_index_digit(base64 >> 18);
            case 1:
                    return get_index_digit((base64 >> 12) & 077);
            case 2:
                    return get_index_digit((base64 >> 6) & 077);
            case 3:
                    return get_index_digit(base64 & 077);
    }

    return '\0';
}

funcs.h

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

char get_base64_digit(unsigned int base64, int digit_number);

main.c

 #include "funcs.h"

int main(int argc, char* argv[]){

    char* text = argc == 2 ? argv[1] : '\0';

    unsigned final, temp;

    int text_len  = strlen(text);
    int remainder = text_len % 3;

    //Iterate over the input string, three characters at a time.
    for (int x = final = temp = 0; x < (remainder == 0 ? text_len : text_len - remainder); x += 3) {

            final = (text[x] << 16) | (text[x+1] << 8) | text[x+2];

            printf("%c%c%c%c",  get_base64_digit(final, 0), get_base64_digit(final, 1), get_base64_digit(final, 2), get_base64_digit(final, 3));

    }

    //Handle the last bytes in case text_len wasn't a multiple of 3
    if (remainder == 1) {
            final = text[text_len-1] << 16; 
            printf("%c%c==\n", get_base64_digit(final, 0), get_base64_digit(final, 1));
    } else if (remainder == 2){ 
            final = (text[text_len-2] << 16) | (text[text_len-1] << 8); 
            printf("%c%c%c=\n",  get_base64_digit(final, 0), get_base64_digit(final, 1), get_base64_digit(final, 2));
    } else
            return 0;


    return 0;
}

Answer 1

funcs.h

Neither funcs.c nor funcs.h require anything from stdio.h or string.h, so you shouldn't be including those there, move them to main.c instead. But you should get into the habit of adding include guards to your headers. So either use this at the top:

#pragma once

Or, to be completely standard and portable:

#ifndef FUNCS_H
#define FUNCS_H

// contents of header here

#endif // FUNCS_H

funcs.c

The static encode mapping is fine as you have it, but would also be fine inside the get_index_digit function (static also of course). Since that is the only direct user, it would be slightly better inside in my opinion.

I'd change your signature and preamble for the encoder to:

#include <assert.h>
// ...

char get_base64_digit(const unsigned int base64, unsigned int digit_number)
{
  assert(digit_number < 4);
  // ...

Passing a negative index or anything greater than 3 is a programming error, it should be caught as early as possible and be a hard failure.

You can get rid of the switch by collapsing all cases:

return get_index_digit((base64 >> (18 - 6*digit_number)) & 077);

Note that this masks the bits even for the zero case, which is safer. But you might want instead to assert on the fact that the bits above index 18 are all zeros - slightly different contract. (I'd prefer just masking them off though.)

main.c

char* text = argc == 2 ? argv[1] : '\0';

That '\0' is bad. Happens to work, but doesn't convey the right information. text is a pointer, not a char. Replace with:

char* text = argc == 2 ? argv[1] : NULL;

But that's actually not a good idea either. What you actually need is an empty string (or bail out).

const char* text = argc == 2 ? argv[1] : "";

Then you have:

unsigned final, temp;

But you're not using temp anywhere, except for setting it to zero. And you're always initializing final to zero in the for loop. So to help simplifying that for a bit:

unsigned final = 0;

Also a slight inconsistency: you're using unsigned here, but used unsigned int in the declaration for get_base64_digit - pick one version and stick to that (they are completely identical).

As for the loop:

for (int x = final = temp = 0; x < (remainder == 0 ? text_len : text_len - remainder); x += 3) {

That line is too long, and more complex than it needs to be. Removing final and temp will help:

for (int x = 0; x < (remainder == 0 ? text_len : text_len - remainder); x += 3) {

And might help you to see that the conditional is unnecessary. If remainder is zero, text_len and text_len - remainder have the same value. So:

for (int x = 0; x < text_len - remainder; x += 3) {

Or move that subtraction out of the loop for an even simpler structure.

The last lines are a bit odd:

} else
        return 0;

return 0;

Remove the else, it is not useful. Also mixing braced and non-braced blocks in an if/else chain isn't a good idea. This is a matter of style, but always using braces is usually safer. If you want to omit them for single-statement blocks, only do that if you only have single-statement blocks in the if/else chain.

Last tidbit: you sometimes have ){ (no space), and sometimes ) { (one space). Stick to one, preferably the one with the space. You also sometimes have an empty line at the start of blocks, sometimes not. If you feel like a bit of vertical separation is good there, try putting the { that starts the block on its own line (as I did in the get_base64_digit above), and look at the Linux coding style guidelines for some other tips that you may, or may not :-), like.

clang-format is an excellent auto-format tool, and a lot of dev tools (including Vim and Emacs) can be made to use it. Try it out.

Answer 2

Some additional thoughts after @Mat fine answer.

1. When implementing an algorithm that counts on a particular size of integer, better to use a size that is guaranteed to work, or consider fixed size types. unsigned may only be 16-bit. Insufficient for the task.

   #include <stdint.h>
   
   // unsigned finalfinal;
   // Options in order of (my) preference
   uint32_t      finalfinal;
   unsigned long finalfinal;
   uint_fast32_t finalfinal;
   

2. For maximum portability, consider that 16-bit machines are very popular in the embedded world in 2016, so do not assume unsigned/int math is at least 32-bit, code to C spec which says an unsigned may only be 16-bit. A 16-bit shift of text[x], which is promoted to int, may not work.

   // finalfinal = (text[x] << 16) | (text[x+1] << 8) | text[x+2];
   finalfinal = (text[x]*1UL << 16) | (text[x+1]*1UL << 8) | text[x+2];
   //  or 
   finalfinal = text[x];
   finalfinal <<= 8; 
   finalfinal |= text[x+1];
   finalfinal <<= 8; 
   finalfinal |= text[x+2];
   
   // final = text[text_len-1] << 16;
   final = text[text_len-1]*1UL << 16;
   

3. Recommend similar thoughts for the function parameters, variables too.

   // char get_base64_digit(const unsigned int base64, int digit_number)
   int get_base64_digit(const uint32_t base64, unsigned digit_number)
   

4. Extending the code to non-null character terminated sources. Current code works fine for test, yet all the following code should be wrapped in a function with the data pointer and size passed in.

   char* text = ...
   size_t text_len = strlen(text);
   
   // Something like this is the real function you need
   int status = NMK64_encode(text, text_len);
   

5. funcs.c is not distinctive. How about NMK_Base64.c?

6. Minor stuff follows

7. Use matching type. strlen() return type size_t, not int.

   // int text_len  = strlen(text);
   // int remainder = text_len % 3;
   size_t text_len  = strlen(text);
   size_t remainder = text_len % 3;
   

8. Make const

   // static char index_table[] = ...
   static const char index_table[] = ...
   

9. Make static. Would not expect this helper function to have global usage. Expect NMK64_encode(text, text_len) to be the global interface.

   // get_base64_digit(
   static int get_base64_digit( ...