Reduce amount of C code

Question

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.

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;
}

If you want someone to review your code you should ask on [codereview.se] — , Aug 08 '14 at 07:59
In some places you actually have to *little* code! In your input loop, you check for end of file, but you don't check for errors. — Some programmer dude, Aug 08 '14 at 08:02
[“while( !feof( file ) )” is always wrong](http://stackoverflow.com/questions/5431941/while-feof-file-is-always-wrong) — pmg, Aug 08 '14 at 08:03
There are also other possible problems (like writing beyond array boundaries). — Some programmer dude, Aug 08 '14 at 08:03
@pmg While the OP uses `feof` in the loop condition, it's actually okay in this case as the OP uses `fgetc` in both the loop initialization and loop update sections of the `for` loop. — Some programmer dude, Aug 08 '14 at 08:05
@JoachimPileborg: except that the file (`stdin`) may have no more data and `feof()` doesnt return `true` such as when input is redirected from a network file and the connection drops. — pmg, Aug 08 '14 at 08:08

score 4 · Answer 1 · answered Aug 08 '14 at 08:08

4

Um, Refactoring != less code. Obfuscation can sometimes result in less code, if that is your objective :)

Refactoring is done to improved code readability and reduced complexity. Suggestions for improvement in your case:

Look at the chunks of logic you've implemented and consider replacing them with in built functions is usually a good place to begin. I'm convinced that some of the sorting you've performed can be replaced with qsort(). However, side note, if this is your assignment, your tutor may be a douche and want to see you write out the code in FULL VS using C's in built function, and dock you points on being too smart. (Sorry personal history here :P)
Move your logical units of work into dedicated functions, and have a main function to perform orchestration.

answered Aug 08 '14 at 08:08

rurouni88

1,165
5
10

I know refactoring does not mean less code, it means changing the internal structure while producing the same end result. Which is what i want to do, in a way that uses less code. Can qsort be used on a 2 dimensional array and only sort by 1 dimension? – Jake Barnby Aug 08 '14 at 08:12
1

Firstly, avoid the notion of writing less code. Not trying to troll, so please don't take it the wrong way, make sure your code is readable and logically segregated. (See my point about making functions) Next, you can flatten your 2 dimensional array, and then run qsort on it :) – rurouni88 Aug 08 '14 at 08:19

Reduce amount of C code

1 Answers1