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\$
4
  • 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\$ Commented Nov 23, 2014 at 17:33
  • 1
    \$\begingroup\$ I added some English comments and edited my output) \$\endgroup\$ Commented Nov 23, 2014 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\$ Commented Nov 23, 2014 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\$ Commented Nov 24, 2014 at 3:54

2 Answers 2

Reset to default
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\$
4
  • \$\begingroup\$ Please explain me fourth paragraph because I don't understand what are you saying :( \$\endgroup\$ Commented Nov 25, 2014 at 10:43
  • \$\begingroup\$ I removed that from my answer, it was probably too much of an opinion than an answer. \$\endgroup\$ Commented Nov 25, 2014 at 20:09
  • \$\begingroup\$ Why do you think so? Simply, I want understand this paragraph) \$\endgroup\$ Commented Nov 26, 2014 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\$ Commented Nov 26, 2014 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\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.