5
\$\begingroup\$

My code convert text to Base64. I used the algorithm without bit operations. What do you think about my code?

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

#define CRT_SECURE_NO_WARNINGS
#define BIN 2
#define MEMORYSIZE 10

void FromSixBitNumbToDec(char *number1, FILE *result){
    char table[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
    int i = 0;
    char out = 0;
    int temp_numb = 0;
    int resul = 0;
    int number = atoi(number1);
    int deg = 1; 
    while (i < 6) { 
        temp_numb = number % 10;
        number /= 10;
        resul += (temp_numb * deg);
        deg *= BIN;
        ++i;
    }
    out = table[resul];
    fprintf(result, "%c", out);
}

void FromDecToBin(int number, FILE *output){
    int *numb = (int*)calloc(MEMORYSIZE, sizeof(int)); // Allocates memory for numbers with a minimum size
    int memory = MEMORYSIZE;
    int i = 0;
    int k = 0;
    while (number >= 2) { // Divide the number 10 number system on a finite number of the number system and the remnants of the division in the array. If memory is low, it increased by 2 times
        numb[i] = number % BIN;
        number = number / BIN;
        ++i; // Array size (numb2 array)
        if (i == memory){
            numb = (int*)realloc(numb, (BIN * memory) * sizeof(int)); 
            memory *= BIN;
        }
    }
    numb[i] = number; // The last remnant
    k = i; 
    while (k != 7){ // Supplement to the number of 8-bit
        k++;
        numb[k] = 0;
    }
    int *numb2 = (int*)calloc(k + 1, sizeof(int));  // Allocates new array for number
    memcpy(numb2, numb, (k + 1) * sizeof(int)); // Copy all digits with numb array
    free(numb); 
    for (i = 0; i <= (k / 2); i++) {  // Overturn number
        memory = numb2[i]; 
        numb2[i] = numb2[k - i];
        numb2[k - i] = memory;
    }
    for (i = 0; i <= k; i++) {
        fprintf(output, "%d", numb2[i]);
    }

}

void toBase64(char *argv[]){
    FILE *text = fopen("C:\\b.txt", "r");
    if (NULL == text) {
        printf("Error!");
        fclose(text);
        return;
    } else {
        printf("File was opened\n");
        FILE *output = fopen("C:\\output.txt", "w"); 
        /*fpos_t position;
        fgetpos(output, &position);*/
        char c = 0;
        while (fscanf(text, "%c", &c)!= EOF) { // Convert symbol to Binary
            FromDecToBin(c,output);
        }
        fclose(text);
        fclose(output);
        //fsetpos(output, &position); // Don;t work because I used fclose(output) and open output2. Please Understand how I gotta keep it real!
        //printf("pos = %d\n", position);
        FILE *output2 = fopen("C:\\output.txt", "r");
        FILE *result = fopen("C:\\result.txt", "w");
        char *number = (char*)calloc(6, sizeof(char));
        int i = 0;
        char temp = 0;
        while (!feof(output2)) {
            while (i < 6) { // Take successively six bit and convert to Base64
                if (!feof(output2)){
                    fscanf(output2, "%c", &temp);
                    number[i] = temp;
                    printf("%c", number[i]);
                    i++;
                } else {
                    i /= BIN;
                    for (; i > 0; i--){
                        fprintf(result, "%c", '=');
                    }
                    return;
                }
            }
            printf("\n");
            FromSixBitNumbToDec(number, result);
            i = 0;
        }
        fclose(output2);
        fclose(result);
    }
}

int main(int argc, char *argv[]){
    if (1 < argc){
        printf("Argument has been Received\n");
        toBase64(argv);
    } else {
        printf("Need some arguments!\nExit... \n");
        return 1;
    }
    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Please translate all relevant comments to English so we can understand them. Also, the output should either be in English or not in English. Currently it's mixed. If you want it to be not in English, please provide translations for the output text. \$\endgroup\$ – raptortech97 Nov 23 '14 at 17:33
  • 1
    \$\begingroup\$ I added some English comments and edited my output) \$\endgroup\$ – MaZaHaKa Nov 23 '14 at 17:43
  • \$\begingroup\$ There's nothing wrong with posting non-English code or comments, if that is how you actually wrote the code. \$\endgroup\$ – 200_success Nov 23 '14 at 17:54
  • \$\begingroup\$ I don't see why you convert the text to a file of '0' and '1', and then convert the file of '0' and '1' to base 64. You could do that all in one pass without the intermediate format. It would remove about half of your code as well. \$\endgroup\$ – JS1 Nov 24 '14 at 3:54
3
\$\begingroup\$
  1. You should probably take the filenames as arguments instead of hardcoding them.

  2. The code looks very nice horizontally, but is vertically crammed. I would suggest including blank lines between variable declaration blocks and things like if/for/while statements.

  3. Some variable names are kinda odd to me. Instead of "temp_numb" just do "temp_number". Instead of "resul" do "temp_result". Truncating words is usually a disaster in terms of readability.

\$\endgroup\$
  • \$\begingroup\$ Please explain me fourth paragraph because I don't understand what are you saying :( \$\endgroup\$ – MaZaHaKa Nov 25 '14 at 10:43
  • \$\begingroup\$ I removed that from my answer, it was probably too much of an opinion than an answer. \$\endgroup\$ – joe63074 Nov 25 '14 at 20:09
  • \$\begingroup\$ Why do you think so? Simply, I want understand this paragraph) \$\endgroup\$ – MaZaHaKa Nov 26 '14 at 5:00
  • \$\begingroup\$ Putting the number before the variable in an expression is harder to read for me: if(1 < argc) { ... }. (In some cases it might be a good practice, though.) Part of the original answer was an attempt at humor also, sorry for the confusion. \$\endgroup\$ – joe63074 Nov 26 '14 at 14:59
0
\$\begingroup\$

I am puzzled by your argument and file handling. Why you need to write to an intermediate file? (Even if you want to avoid bit operations, writing to an intermediate file is silly.) Why bother mentioning argv if you never use it? If given no arguments, I would expect the program to filter standard input to standard output. If there is one argument, I would expect it to treat it as the input filename, and print its output to stdout. If there are two arguments, then I would expect the second one as the output filename.

You don't check for errors when doing fprintf(). There wouldn't be any indication, if, for example, you run out of disk space.

Avoid using calloc() when a fixed-size array will do. Avoid using fprintf() and fscanf() when putc() and getc() will do. More importantly, don't read/write one character at a time. You will get better performance if you read a whole disk block at once into a buffer (about 8 kiB would be a good size, I think).

The work should be decomposed more logically. I suggest the following framework:

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

/**
 * Converts up to three octets into four encoded characters.
 * Output will be padded if len < 3.
 */
static void base64Encode(const char *in, size_t len, char out[4]) {
    assert(len <= 3);
    // TODO
}

/**
 * Reads from input, performs the encoding, and writes to output.
 * Returns -1 if unable to write to the output.
 */
int toBase64(FILE *input, FILE *output) {
    char inBuf[(8192 / 3) * 3], outBuf[(8192 / 3) * 4];
    int bytesExtra = 0, bytesRead;
    while ((bytesRead = fread(inBuf + bytesExtra, sizeof(char), sizeof(inBuf) - bytesExtra, input))) {
        int validBytes = bytesExtra + bytesRead;
        int i, o;
        for (i = 0, o = 0; i + 3 <= validBytes; i += 3, o += 4) {
            base64Encode(inBuf + i, 3, outBuf + o);
        }
        if (o != fwrite(outBuf, sizeof(char), o, output)) {
            return -1;
        }
        /* Preserve leftover input bytes for the next round */
        bytesExtra = validBytes - i;
        assert(bytesExtra < 3);
        memmove(inBuf, inBuf + i, bytesExtra);
    }
    if (bytesExtra) {
        base64Encode(inBuf, bytesExtra, outBuf);
        if (4 != fwrite(outBuf, sizeof(char), 4, output)) {
            return -1;
        }
    }
    return 0;
}

/**
 * Interprets the arguments, opens the input/output files (if any),
 * calls toBase64(), and displays any I/O errors.
 */
int main(int argc, char *argv[]) {
    FILE *input = stdin, *output = stdout;
    if (argc > 3) {
        fprintf(stderr, "Usage: ...\n");
        return 1;
    }
    if (argc > 1) {
        if (NULL == (input = fopen(argv[1], "r"))) {
            perror("Couldn't open input file");
            return 2;
        }
    }
    if (argc > 2) {
        if (NULL == (output = fopen(argv[2], "w"))) {
            perror("Couldn't open output file");
            return 2;
        }
    }
    if (-1 == toBase64(input, output)) {
        perror("Error writing output");
        return 2;
    }
}
\$\endgroup\$

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.