Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

How can I refactor this with less code? This is homework and is cracking a Caesar cipher-text using frequency distribution.

I have completed the assignment but would like it to be cleaner.

#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <ctype.h>
#define TEXT_SIZE 100000  // Note, the longer the text the more likely you will get a good decode from the start.
#define ALEN 26         // Number of chars in ENGLISH alphabet
#define CHFREQ "ETAONRISHDLFCMUGYPWBVKJXQZ" // Characters in order of appearance in English documents.
#define ALPHABET "ABCDEFGHIJKLMNOPQRSTUVWXYZ"


/* Decode the given text using the given map and store the result in newtext */
void decode_text(char* text, char* newtext, char map[][2]) {
int i, j;

for (i = 0; i < strlen(text); i++) {
    // If the current character in text is not a letter, copy it to newtext as punctuation and digits should be maintained.
    if (!isalpha(text[i])) {
        newtext[i] = text[i];
        continue;
    }
    for (j = 0; j < ALEN; j++) {
        if (text[i] == map[j][1]) {
            newtext[i] = map [j][0];
        }
    }
}
}

char upcase(char ch){
    if(islower(ch))
    ch -= 'a' - 'A';
  return ch;
}

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

// first allocate some space for our input text (we will read from stdin).

char* text = (char*)malloc(sizeof(char)*TEXT_SIZE+1);
char textfreq[ALEN][2];
char map[ALEN][2];
char newtext[TEXT_SIZE];
char ch, opt, tmpc, tmpc2;
int i, j, tmpi;

// Check the CLI arguments and extract the mode: interactive or dump and store in opt.

if(!(argc == 2 && isalpha(opt = argv[1][1]) && (opt == 'i' || opt == 'd'))){
    printf("format is: '%s' [-d|-i]\n", argv[0]);
    exit(1);
}

// Now read TEXT_SIZE or feof worth of characters (whichever is smaller) and convert to uppercase as we do it.

for(i = 0, ch = fgetc(stdin); i < TEXT_SIZE && !feof(stdin); i++, ch = fgetc(stdin)){
    text[i] = (isalpha(ch)?upcase(ch):ch);
}
text[i] = '\0'; // terminate the string properly.

// Assign alphabet to one dimension of text frequency array and a counter to the other dimension
for (i = 0; i < ALEN; i++) {
    textfreq[i][0] = ALPHABET[i];
    textfreq[i][1] = 0;
}

// Count frequency of characters in the given text
for (i = 0; i < strlen(text); i++) {
    for (j = 0; j < ALEN; j++) {
        if (text[i] == textfreq[j][0]) textfreq[j][1]+=1;
    }
}

//Sort the character frequency array in descending order
for (i = 0; i < ALEN-1; i++) {
    for (j= 0; j < ALEN-i-1; j++) {
        if (textfreq[j][1] < textfreq[j+1][1]) {
            tmpi = textfreq[j][1];
            tmpc = textfreq[j][0];
            textfreq[j][1] = textfreq[j+1][1];
            textfreq[j][0] = textfreq[j+1][0];
            textfreq[j+1][1] = tmpi;
            textfreq[j+1][0] = tmpc;
        }
    }
}

//Map characters to most occurring English characters
for (i = 0; i < ALEN; i++) {
    map[i][0] = CHFREQ[i];
    map[i][1] = textfreq[i][0];
}

// Sort the map lexicographically
for (i = 0; i < ALEN-1; i++) {
    for (j= 0; j < ALEN-i-1; j++) {
        if (map[j][0] > map[j+1][0]) {
            tmpc = map[j][0];
            tmpc2 = map[j][1];
            map[j][0] = map[j+1][0];
            map[j][1] = map[j+1][1];
            map[j+1][0] = tmpc;
            map[j+1][1] = tmpc2;
        }
    }
}

if(opt == 'd'){
    decode_text(text, newtext, map);
} else {
// do option -i
}

// Print alphabet and map to stderr and the decoded text to stdout
fprintf(stderr, "\n%s\n", ALPHABET);
for (i = 0; i < ALEN; i++) {
    fprintf(stderr, "%c", map[i][1]);
}
printf("\n%s\n", newtext);

return 0;
}
share|improve this question

5 Answers 5

A few simple comments :

Listen to your compiler

The compiler can give you a lot of really interesting information. Activate all warnings (I use gcc -Wall -Wextra -std=c99 cipher.c -o cipher_out) and you'll see a few things to fix :

cipher.c: In function ‘decode_text’:
cipher.c:13:16: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  for (i = 0; i < strlen(text); i++) {
                ^
cipher.c: In function ‘main’:
cipher.c:37:2: warning: implicit declaration of function ‘malloc’ [-Wimplicit-function-declaration]
  char* text = (char*)malloc(sizeof(char)*TEXT_SIZE+1);
  ^
cipher.c:37:22: warning: incompatible implicit declaration of built-in function ‘malloc’ [enabled by default]
  char* text = (char*)malloc(sizeof(char)*TEXT_SIZE+1);
                      ^
cipher.c:47:3: warning: implicit declaration of function ‘printf’ [-Wimplicit-function-declaration]
   printf("format is: '%s' [-d|-i]\n", argv[0]);
   ^
cipher.c:47:3: warning: incompatible implicit declaration of built-in function ‘printf’ [enabled by default]
cipher.c:48:3: warning: implicit declaration of function ‘exit’ [-Wimplicit-function-declaration]
   exit(1);
   ^
cipher.c:48:3: warning: incompatible implicit declaration of built-in function ‘exit’ [enabled by default]
cipher.c:53:2: warning: implicit declaration of function ‘fgetc’ [-Wimplicit-function-declaration]
  for(i = 0, ch = fgetc(stdin); i < TEXT_SIZE && !feof(stdin); i++, ch = fgetc(stdin)){
  ^
cipher.c:53:24: error: ‘stdin’ undeclared (first use in this function)
  for(i = 0, ch = fgetc(stdin); i < TEXT_SIZE && !feof(stdin); i++, ch = fgetc(stdin)){
                        ^
cipher.c:53:24: note: each undeclared identifier is reported only once for each function it appears in
cipher.c:53:2: warning: implicit declaration of function ‘feof’ [-Wimplicit-function-declaration]
  for(i = 0, ch = fgetc(stdin); i < TEXT_SIZE && !feof(stdin); i++, ch = fgetc(stdin)){
  ^
cipher.c:65:16: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  for (i = 0; i < strlen(text); i++) {
                ^
cipher.c:112:2: warning: implicit declaration of function ‘fprintf’ [-Wimplicit-function-declaration]
  fprintf(stderr, "\n%s\n", ALPHABET);
  ^
cipher.c:112:2: warning: incompatible implicit declaration of built-in function ‘fprintf’ [enabled by default]
cipher.c:112:10: error: ‘stderr’ undeclared (first use in this function)
  fprintf(stderr, "\n%s\n", ALPHABET);
          ^
cipher.c:116:2: warning: incompatible implicit declaration of built-in function ‘printf’ [enabled by default]
  printf("\n%s\n", newtext);
  ^

You are missing :

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

And you should define i and j as unsigned int.

Format your code

The indentation seems a bit off in your question. I don't know if it's because you pasted the code in a wrong way or whatever but your favorite text editor can fix this easily.

Tell what you want to do

You are not describing anywhere how your program works : format is: program_name [-d|-i] is not enough if you don't explain what the options are for.

Also, it should be described somewhere that your program will affect only letters for instance.

Move variables to the smallest possible scope

It keeps things easier to understand and to split into smaller functions.

At this stage, the code looks like :

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <ctype.h>
#define TEXT_SIZE 100000  // Note, the longer the text the more likely you will get a good decode from the start.
#define ALEN 26         // Number of chars in ENGLISH alphabet
#define CHFREQ "ETAONRISHDLFCMUGYPWBVKJXQZ" // Characters in order of appearance in English documents.
#define ALPHABET "ABCDEFGHIJKLMNOPQRSTUVWXYZ"


/* Decode the given text using the given map and store the result in newtext */
void decode_text(char* text, char* newtext, char map[][2]) {
    for (unsigned int i = 0; i < strlen(text); i++) {
        // If the current character in text is not a letter, copy it to newtext as punctuation and digits should be maintained.
        if (!isalpha(text[i])) {
            newtext[i] = text[i];
            continue;
        }
        for (unsigned int j = 0; j < ALEN; j++) {
            if (text[i] == map[j][1]) {
                newtext[i] = map [j][0];
            }
        }
    }
}

char upcase(char ch){
    if(islower(ch))
        ch -= 'a' - 'A';
    return ch;
}

int main(int argc, char **argv){
    // first allocate some space for our input text (we will read from stdin).
    char* text = (char*)malloc(sizeof(char)*TEXT_SIZE+1);
    char opt;
    int tmpi;

    // Check the CLI arguments and extract the mode: interactive or dump and store in opt.

    if(!(argc == 2 && isalpha(opt = argv[1][1]) && (opt == 'i' || opt == 'd'))){
        printf("format is: '%s' [-d|-i]\n", argv[0]);
        exit(1);
    }

    // Now read TEXT_SIZE or feof worth of characters (whichever is smaller) and convert to uppercase as we do it.
    {
        unsigned int i;
        for(char ch = fgetc(stdin); i < TEXT_SIZE && !feof(stdin); i++, ch = fgetc(stdin)){
            text[i] = (isalpha(ch)?upcase(ch):ch);
        }
        text[i] = '\0'; // terminate the string properly.
    }

    // Assign alphabet to one dimension of text frequency array and a counter to the other dimension
    char textfreq[ALEN][2];
    for (unsigned int i = 0; i < ALEN; i++) {
        textfreq[i][0] = ALPHABET[i];
        textfreq[i][1] = 0;
    }

    // Count frequency of characters in the given text
    for (unsigned int i = 0; i < strlen(text); i++) {
        for (unsigned int j = 0; j < ALEN; j++) {
            if (text[i] == textfreq[j][0]) textfreq[j][1]+=1;
        }
    }

    //Sort the character frequency array in descending order
    for (unsigned i = 0; i < ALEN-1; i++) {
        for (unsigned j= 0; j < ALEN-i-1; j++) {
            if (textfreq[j][1] < textfreq[j+1][1]) {
                tmpi = textfreq[j][1];
                char tmpc = textfreq[j][0];
                textfreq[j][1] = textfreq[j+1][1];
                textfreq[j][0] = textfreq[j+1][0];
                textfreq[j+1][1] = tmpi;
                textfreq[j+1][0] = tmpc;
            }
        }
    }

    //Map characters to most occurring English characters
    char map[ALEN][2];
    for (unsigned int i = 0; i < ALEN; i++) {
        map[i][0] = CHFREQ[i];
        map[i][1] = textfreq[i][0];
    }

    // Sort the map lexicographically
    for (unsigned int i = 0; i < ALEN-1; i++) {
        for (unsigned int j = 0; j < ALEN-i-1; j++) {
            if (map[j][0] > map[j+1][0]) {
                char tmpc = map[j][0];
                char tmpc2 = map[j][1];
                map[j][0] = map[j+1][0];
                map[j][1] = map[j+1][1];
                map[j+1][0] = tmpc;
                map[j+1][1] = tmpc2;
            }
        }
    }

    char newtext[TEXT_SIZE];
    if(opt == 'd'){
        decode_text(text, newtext, map);
    } else {
        // do option -i
    }

    // Print alphabet and map to stderr and the decoded text to stdout
    fprintf(stderr, "\n%s\n", ALPHABET);
    for (unsigned int i = 0; i < ALEN; i++) {
        fprintf(stderr, "%c", map[i][1]);
    }
    printf("\n%s\n", newtext);

    return 0;
}

Split your code into re-usable functions

I just realised that vnp comment explains this already. nothing to add here.

A few tiny code comments

You don't need to use malloc in your case.

Also, you don't need a continue in your decode function : an else would do the trick here.

share|improve this answer
1  
-Wall is a great suggestion. Passing -Wall -Werror is usually a good idea unless you have a specific reason not to. –  ruds Aug 8 at 12:17

Comments

Anything in a program must have a reason to exist, and the only reason for a comment to exist is to clarify a code otherwise incomprehensible. Do not add gratuitous comments (but avoid incomprehensible code, such as how you inspect options).

Dread nought functions

Fragments at lines 74-85 and 94-105 are pretty much identical. Factor them out to a properly called function (and see how comments will disappear).

Misc

Try to declare variables as close to their use as possible. It helps very much to identify reusable pieces of code, aka functions.

upcase doesn't have a right to exist. There's a standard library toupper.

share|improve this answer
    
The comment advice is very good, but it's homework, so I'm willing to bet Professor wants it that way. It looks like they're teaching "pseudocode first". –  RubberDuck Aug 8 at 10:30

Usage of #define

There are some places you have to use the preprocessor, but prefer to limit its usage as much as possible. For both your character strings, use const char* instead:

const char* CHFREQ = "ETAONRISHDLFCMUGYPWBVKJXQZ";
const char* ALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";

Make use of the standard library

There's no need to create your own upcase function: one already exists in ctypes.h: toupper. This is also true for sorting routines (although I don't know if you've been shown that yet). The standard library contains a routine that will perform sorts for you: qsort (this has a bit of a strange looking declaration if you haven't seen it before however, so if you haven't been introduced to it, don't worry about it too much).

Break things into functions

Instead of putting all the logic into main, you should break things into functions. Just about everywhere you have a long comment could be a function. Also, don't be afraid to break things out a bit. For example:

for(i = 0, ch = fgetc(stdin); i < TEXT_SIZE && !feof(stdin); i++, ch = fgetc(stdin)){

There is quite a bit going on there. Some examples of things that should be functions:

  • Reading in the input and placing it in an array
  • Parsing arguments and printing out program command line usage
  • Building the frequency array

These could look something like:

// Reads input from stdin into the character buffer text. 
// Input is read either until an end of file marker is seen,
// or text_len characters have been read.
size_t read_input(char* text, size_t text_len)
{
    size_t i = 0;
    int ch;
    do {
        ch = fgetc(stdin);
        text[i] = (isalpha(ch) ? toupper(ch) : ch);
        ++i;
    } while(i < text_len && ch != EOF);
    text[i] = '\0'; 
    return i+ 1;
}

void print_usage(const char* name)
{
    printf("format is: '%s' [-d|-i]\n", name);
    exit(1);
}

char parse_arguments(char** argv)
{
    char opt = argv[1][1];
    if(!isalpha(opt) && (opt != 'i' || opt != 'd')) {
        print_usage(argv[0]);
    }
    return opt;
}

void build_freq_array(char freq[][2])
{
    for (int i = 0; i < ALEN; i++) {
        freq[i][0] = ALPHABET[i];
        freq[i][1] = 0;
    }
}

This makes main a lot cleaner:

int main(int argc, char** argv)
{
    char* text = (char*)malloc(sizeof(char)*TEXT_SIZE+1);
    char textfreq[ALEN][2];
    char map[ALEN][2];
    char newtext[TEXT_SIZE];

    if(argc != 2) {
        print_usage(argv[0]);
    }
    char opt = parse_arguments(argv);
    size_t len = read_input(text, TEXT_SIZE);
    build_freq_array(textfreq);    
    count_frequency(text, len, textfreq);

    ....
 }

Beware of memory leaks

At the top of main, you have:

char* text = (char*)malloc(sizeof(char)*TEXT_SIZE+1);

Firstly, there is some redundancy here. sizeof(char) is guaranteed to be 1. Further, the cast is not necessary in C:

char *text = malloc(TEXT_SIZE + 1);

However, the major issue is that this memory is malloced, but never freed. In a small program like this that exits after doing a bit of work, this isn't a big deal as the OS will reclaim the memory. However, part of learning C is learning manual memory management. There's actually 2 potential places you need to have a free here:

if(!(argc == 2 && isalpha(opt = argv[1][1]) && (opt == 'i' || opt == 'd'))){
    printf("format is: '%s' [-d|-i]\n", argv[0]);
    free(text);   // About to call exit(), free malloced memory
    exit(1);
}

and:

...
free(text);  // Just before we return, free memory
return 0;

Of course, knowing when you need to use malloc/free and when you don't is also part of learning C. Here, there's no real reason to use malloc:

char text[TEXT_SIZE];

This will allocate text on the stack; it will be automatically reclaimed (so you don't need to worry about needing to call free).

share|improve this answer
    
I suggest preferring const char[] to const char*. For one thing, const char* can be reassigned! For another, it's easier for the compiler to optimize away strlen for arrays than pointers (though I understand some still will). –  ruds Aug 8 at 11:46
    
@ruds First problem can always be solved by using const char* const. I prefer using const char* over the array variant as it is always safe to use its address (even from a function scope that it is defined in). –  Yuushi Aug 8 at 11:57
    
You mean something like const char* foo() { const char* f = "foo"; return f; }? I mean, that's kind of gross. The first problem could be solved by const char* const, but that's not what you recommended and it's easy to forget. –  ruds Aug 8 at 12:01
    
@ruds Yes, it's kind of gross, but it is also safe. Anyway, I consider the difference between the two incredibly minor at best. Worrying about whether the compiler may optimise away a call to strlen is pretty much the definition of premature optimisation. –  Yuushi Aug 8 at 12:05
    
I think on the contrary that choosing to use as a general rule a less efficient construct with no clear readability benefit is premature pessimization. Premature optimization is writing less readable code in the name of efficiency without first determining that the code in question requires that efficiency. const char[] does not produce less readable code. –  ruds Aug 8 at 12:13

My number one piece of advice: More functions! One of your goals when writing a program should be to minimize the number of comments that are necessary by organizing the code in such a way as to make the functionality obvious.

Let's take a top-down approach to begin: I'll rewrite main with this principle in mind.

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

// Number of characters in our input alphabet (A-Z).
#define ALEN 26
// Characters in decreasing order of frequency in English.
const char charsByFrequency[] = "ETAONRISHDLFCMUGYPWBVKJXQZ";

enum OperationMode {
    DECODE,
    I,  // I'm not sure what -i does
};

struct ProgramOptions {
    enum OperationMode operation_mode;
};

// Prints usage statement to stderr and returns non-zero on error.
int read_options(int argc, const char** argv, struct ProgramOptions* options);

// Allocates space for text and reads text from in. Returns the number of
// characters in *text, or 0 on a read error. *text must be deallocated with free().
size_t read_text(FILE* in, char** text);

struct CharacterFrequency {
    char c;
    int count;
};

// counts must have dimension ALEN.
void count_character_frequencies(const char* text, size_t num_chars, struct CharacterFrequency counts[]);

void print_character_frequencies(FILE* out, const struct CharacterFrequency counts[]);

// Use the frequency counts of characters (as given by counts)
// to decode encoded into decoded.
//
// decoded must have space for at least num_chars + 1 characters.
// counts must have dimension ALEN and will be modified.
void decode_text(const char* encoded, char* decoded, size_t num_chars,
                struct CharacterFrequency counts[]);

int main(int argc, char** argv) {
    struct CharacterFrequency counts[ALEN];
    struct ProgramOptions program_options;
    char* text;
    size_t text_length;

    if (read_options(argc, argv, &program_options)) {
        exit(1);
    }

    if (!(text_length = read_text(stdin, &text)) {
        free(text);
        exit(1);
    }

    count_character_frequencies(text, text_length, counts);

    print_character_frequencies(stderr, counts);

    if (program_options.operation_mode == DECODE) {
        char* decoded = (char *) malloc(sizeof(char) * (text_length + 1));
        decode_text(text, decoded, text_length, counts);
        printf("%s\n", decoded);
        free(decoded);
    } else {
        fprintf(stderr, "-i is unimplemented");
        free(text);
        exit(1);
    }
    free(text);
    return 0;
}

Now the full functionality of the program can be understood by reading a couple of dozen lines instead of the whole program! Now let's implement our helper functions.

int read_options(int argc, const char** argv, struct ProgramOptions* options) {
    static const char usage_format_str[] =
        "Usage:\n"
        "    %s [-d|-i]\n"
        "  Options:\n"
        "    -d: Print a best-guess decryption of the input.\n"
        "    -i: ????\n";

    if (argc != 2 || argv[1][0] != '-') {
        fprintf(stderr, usage_format_str, argv[0]);
        return 1;
    }
    switch (argv[1][1]) {
        case 'i':
            options->operation_mode = I;
            break;

        case 'd':
            options->operation_mode = DECODE;
            break;

        default:
            fprintf(stderr, usage_format_str, argv[0]);
            return 1;
    }
    return 0;
}

Not much has changed here. I've given the options names so that I can localize the knowledge that "-d" means decode and "-i" means whatever to this single function.

// Increasing this size increases the minimum memory requirements
// but decreases time spent re-allocating memory for large inputs.
// I've chosen 1MB as a reasonable buffer size.
#define INITIAL_BUFSIZE 1048576
// If there's not at least this much free space in the buffer between
// reads, the buffer will be reallocated.
#define MIN_BUF_OVERHEAD 1024
size_t read_text(FILE* in, char** text) {
    size_t total_read = 0, alloc_size = INITIAL_BUFSIZE;
    assert(text);

    *text = (char *) malloc(INITIAL_BUFSIZE);

    while (*text) {
        // Loop invariant: *text points to a buffer that is of length at least
        // total_read + MIN_BUF_OVERHEAD
        assert(alloc_size >= total_read + MIN_BUF_OVERHEAD);
        total_read += fread(*text + total_read, 1, alloc_size - total_read - 1, in);

        if (feof(in)) {
            (*text)[total_read] = '\0';
            return total_read;
        } else if (ferror(in)) {
            fprintf(stderr, "Error reading in text.\n");
            return 0;
        }

        if (alloc_size - total_read < MIN_BUF_OVERHEAD) {
            // Doubling the allocation size makes this total read take O(N) time.
            size_t new_alloc_size = 2 * alloc_size;
            void* newptr;
            if (new_alloc_size < alloc_size) {
                fprintf(stderr, "Goodness, we've run out of virtual memory!\n");
                return 0;
            }
            newptr = realloc(*text, new_alloc_size);
            if (!newptr) {
                fprintf(stderr, "Error allocating %zu bytes.\n", new_alloc_size);
                return 0;
            }
            *text = (char *) newptr;
            alloc_size = new_alloc_size;
        }
    }
    // We're only here if the initial allocation failed.
    fprintf(stderr, "Couldn't allocate any memory for text.\n");
    return 0;
}

A couple things here. First, using fread instead of fgetc should make your reads considerably faster. This is for two main reasons: fewer function calls means less overhead, and fgetc copies one byte at a time while fread uses memcpy, which generally copies multiple bytes per instruction. Second, this allocates enough memory to read in whatever file is passed on stdin; it's not limited to 10000 bytes. There is a tradeoff: The code is, as a result, more complicated and less obviously correct.

int is_decodable_character(c) {
    // NOTE: isalpha returns true for more than just A-Z in some locales.
    // However, our code assumes that A-Z (26 letters) is our entire alphabet,
    // so isalpha is not appropriate here.
    return 'A' <= c && c <= 'Z';
}

void count_character_frequencies(const char* text, size_t num_chars, struct CharacterFrequency counts[]) {
    for (size_t i = 0; i < ALEN; ++i) { 
        counts[i].c = 'A' + i;
        counts[i].count = 0;
    }
    for (size_t i = 0; i < num_chars; ++i) {
        char c = toupper(text[i]);
        if (is_decodable_character(c)) {
            ++counts[c - 'A'].count;
        }
    }
}

OK, some big differences here. I've built in the assumption that the alphabet is the 26 Roman letters A-Z. This should make the code considerably faster (not a big deal on small inputs, but should be noticeable on large inputs). If you want to allow arbitrary character sets, consider using a map data structure of some type instead of an array so that you can have efficient lookup.

void print_character_frequencies(FILE* out, const struct CharacterFrequency counts[]) {
    fprintf(out, "Character Frequencies:\n");
    for (size_t i = 0; i < ALEN; ++i) {
        fprintf(out, "%c: %d\n", counts[i].c, counts[i].count);
    }
}

Your code appeared to print out the ASCII character corresponding to the count for each letter (e.g. if a letter appeared 33 times, '!' would be printed). You also printed the translated character frequencies; that is, 'E' would always have the highest count because you swapped out characters before printing the frequencies.

That seems like it would be less useful to the user than printing the frequencies of the encoded characters, so I've done the latter.

Here's an alternative implementation:

void print_character_frequencies(FILE* out, const struct CharacterFrequency counts[]) {
    fprintf(out, "Character Frequencies:\n");
    for (size_t i = 0; i < ALEN; ++i) {
        fprintf(out, "%3c", counts[i].c);
    }
    fprintf(out, "\n");
    for (size_t i = 0; i < ALEN; ++i) {
        fprintf(out, "%3d", counts[i].count);
    }
    fprintf(out, "\n");
}

This version will be more compact, but you'll run into trouble if any character appears more than 999 times (and the numbers will run together if any character appears more than 99 times).

int count_greater(const void* l, const void* r) {
    const struct CharacterFrequency *lc = (const struct CharacterFrequency *) l,
                                    *rc = (const struct CharacterFrequency *) r;
    return rc->count - lc->count;
}

void decode_text(const char* encoded, char* decoded, size_t num_chars,
                 struct CharacterFrequency counts[]) {
    char translationMap[ALEN];

    qsort(counts, ALEN, sizeof(counts[0]), &count_greater);

    for (size_t i = 0; i < ALEN; ++i) {
        translationMap[counts[i].c - 'A'] = charsByFrequency[i];
    }

    for (size_t i = 0; i < num_chars; ++i) {
        char c = toupper(encoded[i]);
        if (is_decodable_character(c)) {
            decoded[i] = translationMap[c];
        } else {
            decoded[i] = c;
        }
    }
    decoded[num_chars] = '\0';
}

This is the biggest departure from your code. First, I've used a library function to sort the frequency counts in descending order. qsort is defined in stdlib.h and uses a much better algorithm than bubble sort. One of the advantages to extracting functions from your code is that it makes it easier to see when you're doing a common operation. In this case, extracting your sorting loop into a function, and calling it "sort_frequencies" (or whatever), makes it easy to see that you're sorting a list. This is such a common operation that even C's standard library provides a sorting function.

Second, I've computed a forward mapping (encoded character -> decoded character) rather than a reverse mapping (decoded character -> encoded character) as you have. This again allows me to do a direct lookup rather than a search through the mapping on each character. Design your data structures so that your most common operations are natural and efficient.


Here's some general-approach advice.

  • Use the type system appropriately. For example, textfreq isn't really a 2-dimensional array of characters, it's a mapping from characters to counts. Make it an array of an appropriate struct instead of a 2-d array of char.

  • Encapsulate knowledge. For example, ou store opt as a character -- now all readers of this code need to remember that when opt is 'd', it means that a decryption of the text should be printed at the end of the program. Instead, limit that knowledge to the smallest piece of code possible (in this case, part of a single function) and use well-named variables and the type system to pass this knowledge elsewhere.

  • Design your data structures so that you don't have to fight against them. textfreq and map were both written so that you needed to write a loop just to do a simple lookup. Now, I chose to rewrite them so that I could just index them by c - 'A', but even if you wanted to keep your approach, you should write accessors with signatures like increment_frequency(char[][2] counts, char c) or char get_frequency(char[][2] counts, char c) or translate_char(char[][2] map, char c).

  • Use library functions when you can (e.g. toupper instead of your upcase, qsort instead of your bubble sort). They're generally at least as fast as anything you can write -- standard library writers have a lot of experience writing efficient code. They're also well-tested and widely used, and thus less likely to have a bug than ad hoc code.

  • Use size_t when indexing into arrays or (especially) when referring to buffer sizes.

  • When you know the length of a string, you should probably pass it along rather than counting on strlen. Besides arguments about efficiency, you should also consider: what if my string is binary data that may contain NULs? Alternatively, what if I forgot to NUL-terminate my string? It's good general practice to pass a string's length around with the string.

One last note: I haven't tried to compile my code, much less tested it. Drop a comment if you have any questions, and good luck!

share|improve this answer

Here your code can be simplified and made more clear. You are checking if the character is alphabetical and then looking into your map to find the corresponding translation:

    if (!isalpha(text[i])) {
        newtext[i] = text[i];
        continue;
    }
    for (unsigned int j = 0; j < ALEN; j++) {
        if (text[i] == map[j][1]) {
            newtext[i] = map [j][0];
        }
    }

It is more clear and less error prone to first make the search in the table and then fallback if you don't find the character. I would use an empty for for the search, this might be controversial:

// find j such that text[i] == map[j][1]
for (unsigned int j = 0; j < ALEN && text[i] == map[j][1]); j++);

if (j < ALEN) { // found!
    newtext[i] = map[j][0];
} else {
    newtext[i] = text[i];
}

This makes it clear that newtext[i] is assigned one and only one value (while in your code this depends on the content of map). Notice also that the code is simpler and the continue is no longer necessary.

The following code is ugly:

if(!(argc == 2 && isalpha(opt = argv[1][1]) && (opt == 'i' || opt == 'd'))){
    printf("format is: '%s' [-d|-i]\n", argv[0]);
    exit(1);
}

even if you have only a single option it would be good to write your code as if there were many. This could be simpler to understand and easier to extend. I would keep the options (in your case opt which you could transform in a boolean variable interactive_mode) as global variables, and parse them in a dedicated function. Once you have extracted that code from the main function you will feel more eager to add other options (for example --help to print the usage message).

You main function is definitely too large. Extract the arg parsing code. Put the logic into a separate function and let main just be glueing code.

Here you are reinventing the wheel:

for(i = 0, ch = fgetc(stdin); i < TEXT_SIZE && !feof(stdin); i++, ch = fgetc(stdin)){
    text[i] = (isalpha(ch)?upcase(ch):ch);
}
text[i] = '\0'; // terminate the string properly.

use a single call to fgets followed by a call to toupper to get the same result.

In the rest of the code I would suggest to take advantage of the fact that ALPHABET characters have consecutive ASCII codes. This would simplify (and speed up) the code significantly. In this case you would silently assume that alphabet is given by ascii codes from 'A' to 'Z' and to find the index of a character c in the alphabet just compute c-'A'. Of course this won't apply if you plan to differentiate frequency of lower/uppercase characters... but it would not be too bad to extend your frequency map to all 128 ASCII characters to anyway take advantage of this hint.

share|improve this answer
    
fgets will only work if there are no line breaks in the input. –  ruds Aug 8 at 12:18

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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