4
\$\begingroup\$

My code converts a number from one number system to another (from binary to 36 and back). Do you think my formatting is successful or not?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <locale.h>
#include <math.h>
//#include <vld.h>

#define _CRT_SECURE_NO_WARNIN
#define MAXMEMORY 10

const int EXAMPLE_ARRAY = 36; // Длина шаблонного массива

int Degree(int from,int deg) { // Функция для нахождения степени числа 
    int i = 0;
    long int stepen = 1;
    for(i = 0; i < deg; i++) {
        stepen = stepen * from;
    }
    return stepen;
}

int ConvertToDec(int *str, int from, int size) { // Переводим число из любой СС > 10 в уже в безбуквенном виде в число 10 СС
    --size;
    int i = 0;
    int temp = 0;
    int result = 0;
    int deg = from;
    while(size >= 0) {
        deg = Degree(from, i);
        result += (str[size] * deg);
        ++i;
        --size;
    }
    printf("Число в 10 СС %d\n",result); // Вывод итогового числа, в 10 СС 
    return result;
}

int *ConvertToNum(char *str, int size) { // Заменяем буквы в числе на цифры
    int k = 0;
    --size;
    int *temp = (int*)calloc(size + 1, sizeof(int)); // Заменяем память типа char на int 
    char abc[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ" ;
    while(size >= 0) {
        for(k = 0; k < EXAMPLE_ARRAY; k++) {
            if(str[size] == abc[k]) {
                break;
            }
        }
            temp[size] = k;
            --size;
        }
    return temp;
    }


void DoMyLetters(char *str, int size) { // Заменяем цифры в числе на буквы
    int i = 0; // Длина строки str
    char abc[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ"; // Создаем массив - образец, для сравнении строчки при подстановке букв в строку
    char c = 0;
    i = size;
    while(i >= 0) {
        c = str[i];
        str[i] = abc[c]; // В str должен прийти код буквы из ASCII
        i--;
    }
    return;
}

void ConvertIt(int from, char *number1, int to, int memorysize, int length , int guest) { //u - длина числа guest - переменная в 10 СС из какой-либо другой
    int ResultInDec = 0;
    int number = 0; // Против переполнения
    long long int numcopy = 0;
    if(guest == 0) {
     number = atoi(number1);
     numcopy = number;
    } else {
        number = guest; 
    }
    long int temp_numb = 0;
    long int result = 0;
    int i = 0;
    int t = 0;
    int k = 0;
    if(from == 10) { 
        int *numb = (int*)calloc(memorysize, sizeof(int)); // Выделяю память для перевода числа, с минимальным размером. 
        while(number >= to) { // Делим число в 10 СС на число конечной СС и сохраняем остатки от деления в массив. Если памяти мало, то увел. в 2 раза
            numb[i] = number % to;
            number = number / to;
            ++i; // Заодно узнаем и размер будущего массива
            if (i == memorysize){
                numb = (int*)realloc(numb, (2 * memorysize) * sizeof(int)); // Увеличиваем память экспоненициально (при заполнении увел. в 2 раза)
                memorysize *= 2;
            }
        }
        numb[i] = number; // Не забываем, про последний разряд в числе, который меньше конечной СС
        k = i; // Чтобы не потерять длину массива с числом
        int *numb2 = (int*)calloc(k + 1, sizeof(int));  // Выделяем новый массив, с остатками от деления
        for(i = 0; i <= k; i++) {
            numb2[i] = numb[i]; 
        }
        for(i = 0; i <= (k / 2); i++) { // Переворачиваем число т.к оно сохранилось наоборот
            t = numb2[i];
            numb2[i] = numb2[k - i];
            numb2[k - i] = t;
        }
        free(numb); // Старый массив очищаем за ненадобностью
        if(to < 11) { // Вывод числа, если в нем не должно быть букв
            printf("*********************\n\n");
            printf("Число ");
            for(i = 0; i < length; i++) {
                printf("%c", number1[i]);
            }
            printf(" из системы с основанием %d переведено в систему с основанием %d как ", from, to);
            for(i = 0; i <= k; i++) { // Вывод итогового числа, если в нем не должно быть букв (если СС < 11) 
                printf("%d", numb2[i]);
            }
            printf("\n");
            free(numb2);
            return;
        } else { // Мы узнали, что наша СС больше 10 СС, следовательно в строке могут быть буквы
            char *LetterStr = (char*)calloc(k + 1, sizeof(char)); // Создаем массив типа char и переносим данные из переведенной строки
            for(i = 0; i <= k; i++) {
                LetterStr[i] = numb2[i];
            }
            free(numb2); // Очистили ненужную нам больше строчку. 
            DoMyLetters(LetterStr, k);
            printf("*********************\n");
            printf("\n");
            printf("Число ");
            for(i = 0; i < length; i++) {
                printf("%c", number1[i]);
            }
            printf(" из системы с основанием %d переведено в систему с основанием %d как ", from, to);
            for(i = 0; i <= k; i++) { // Вывод итогового числа в конечной СС (с буквами) 
                printf("%c", LetterStr[i]);
            }
            printf("\n");
            free(LetterStr); // Также очистили вторую
        }
    } else {
        if(10 < from) { // Перевод числа из любой СС > 10 в 10 СС
            int *temp = 0; // Выделяем память для временного хранения числа во время перевода. НЕ ЗАБЫТЬ ОЧИСТИТЬ!
            temp = ConvertToNum(number1, length); // Избавляемся от букв для перевода в 10 СС
            ResultInDec = ConvertToDec(temp, from, length);
            free(temp);
            ConvertIt(10, number1, to, memorysize, length, ResultInDec); // Если нужен дальнейший перевод - переводим и вывод число. Иначе - просто вывод
        } else { // Перевод числа из любой СС < 10 в 10 СС 
            long int deg = from; // Обхожу использование фукции pow (для степени)
            i = 0;
            while(i < length) { // number - это число полученное из строки 
                temp_numb = number % 10;// u - длина числа изначального
                number = number / 10;
                deg = Degree(from, i);
                result += (temp_numb * deg);
                ++i;    
            }
                printf("Число в 10 СС %d\n", result); // Вывод итогового числа, в 10 СС 
                ConvertIt(10, number1, to, memorysize, length, result); 
            }
    }
    return;
}

int CheckMyNumber(char *number, int from, int size) { // Функция проверящая число на валидность. Работает только для СС <10
    int i = 0;
    int a = 0;
    int wrong = 0;
    int x = 0;
    int w = 0; // Для поиска в строке
    int newsize = 0;
    int print_check = 0;
    char abc[] =  "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ" ;
    char abc2[] = "abcdefghijklmnopqrstuvwxyz";
    while(('+' == number[x]) || ('-' == number[x])) { // Проверяем число на знак перед ним
        number[x] = ' '; 
        x++;
        i++;
    }
    size -= x; // Задаем размер строки, без учета знака
    for(x = 0; x < size; x++) { // Теперь избавляемся от знака в строке
            number[x] = number[i+x]; 
    }
    for(i = 0; i < size; i++) { // Теперь мы переводим в строчке маленькие буквы в БОЛЬШИЕ!
        for(w = 0; w < 26; w++) {
            if(number[i] == abc2[w]) {
                number[i] = abc[w + 10];
                print_check++;
                break;
            }
        }
    }
    i = 0;
    while(i < size) { // Проверка на совпадение символов (уже без знака)
        for(a = 0; a < EXAMPLE_ARRAY; a++) {
            if((number[i] != abc[a]) && (a < from)) {
                wrong++; // Несовпадение с шаблоном
            }
            if((number[i] == abc[a]) && (a < from)) {
                break;
            }
            if(a > from) {
                return -1;
            }
            if((EXAMPLE_ARRAY-1) == wrong) { // 35 раз произошло не совпадение с шаблоном, значит символ не лежит в таблице допустимых!
                return -1;
            }
        }
            i++;
            wrong = 0;  
        }
    if(0 != print_check) { // Вывод строчки, в случае если были найдены строчные буквы 
        printf("В строчке были найдены строчные буквы. Замена на прописные произошла успешно\n");
        printf("Строка была изменена: ");
        for(i = 0; i < size; i++) {
            printf("%c", number[i]);
        }
        printf("\n");
    }
    return size; // Новая длина строки (верная)
}

int main() {
    setlocale(LC_ALL, "Rus");
    int from = 0;
    int length = 0;
    int to = 0;
    char c = 0;
    int maxmemory = MAXMEMORY; // Переменная хранящая, количество необходимой памяти под входное число
    char *number = (char*)calloc(maxmemory, sizeof(char)); // Выделяем память изначально под число (10 элементов)
    printf("Добро пожаловать в Универсальный Перекодировщик Систем Счисления (УПСС) 2000!\n");
    printf("Введите систему счисления исходного числа : ");
    scanf("%d", &from); // Может быть любой от 2 до 36
    if((EXAMPLE_ARRAY < from) || (2 > from)) { // Проверка на допустимую систему счисления
        printf("Некорректная система счисления!\nВыход из приложения... \n");
        return -1;
    }
    printf("Введите само число : ");
    //fflush(stdin); // Очищаем входной поток данных, чтобы избавится от автоматического символа пробел ('\n')
    while (getchar() != '\n'); // Очищаю входной поток "правильно"
    while('\n' != (c = getchar())) { // Ввод строки неизвестной длины
        number[length] = c;
        length++;
        if(length == maxmemory) {
            number = (char*)realloc(number, (2 * maxmemory) * sizeof(char)); // Увеличиваем память экспоненициально (при заполнении увел. в 2 раза)
            maxmemory *= 2;
        }
    }
    if(0 == length) { // Проверка на пустое число
        printf("Число не введено! Выход из приложения! \n");
        return -1;
    }
    printf("Производится проверка числа... ");
    if(-1 == (length = CheckMyNumber(number, from, length))) { // Проверка числа на валидность
    printf("Некорректное число! Выход из приложения...\n");
    return -1;
    }
    printf("Число корректно!\n");
    if (10 < length){
        printf("Из-за длинного числа возможно переполнение Integer!\n");
    }
    printf("Введите основание конечной системы счисления : ");
    scanf("%d", &to);
    if((EXAMPLE_ARRAY < to) || (2 > to)) {
        printf("Некорректная система счисления!\nВыход из приложения... \n");
        return -1;
    }
    printf("Начинаю работу...\n\n");
    printf("Отладочные данные: \n");
    printf("*********************\n");  
    ConvertIt(from, number, to, maxmemory, length, 0); 
    free(number); // Очищаем строку с числом после работы
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Hi! Welcome to Code Review. I understand there's a bit of a language barrier, but can you please clarify whether or nor your code works as intended? \$\endgroup\$ – RubberDuck Nov 21 '14 at 13:31
  • 1
    \$\begingroup\$ Hi! Yes my code works, I would like to correct the formatting of my code and maybe a bit to optimize it \$\endgroup\$ – MaZaHaKa Nov 22 '14 at 3:11
  • \$\begingroup\$ Thanks!) But what about my code ? \$\endgroup\$ – MaZaHaKa Nov 22 '14 at 4:27
  • \$\begingroup\$ Oh, you don't want the VBA guy reviewing your C code. But I will recommend that you remove the commented out code. If you're afraid to remove it, it just means you need to start using source control. Hang tight. It's kind of slow on the weekends, and it takes a little while to get an answer here anyway. Good reviews take time to write. \$\endgroup\$ – RubberDuck Nov 22 '14 at 4:30
  • \$\begingroup\$ Ok, I deleted my commented code. I'll be waiting. Thanks \$\endgroup\$ – MaZaHaKa Nov 22 '14 at 4:54
4
\$\begingroup\$

Working from the top down:

Your function Degree is a bit...oddly named. It's also equivalent to pow from math.h. Unless you have a good reason not to, I'd simply use pow, which likely uses exponentiation by squaring, which will run in \$O(log \ n)\$ instead of \$O(n)\$.

ConvertToDec is a bit inefficient in that it recalculates Degree(from, i) on every loop iteration. This is eventually doing \$O(n^2)\$ work. It also has an unused variable temp. This could be written more simply by starting deg at 1, and simply multiplying it by from each time through the loop:

int ConvertToDec(int *str, int from, int size) { 
    --size;
    int result = 0;
    int deg = 1;
    while(size >= 0) {
        deg *= from;
        result += (str[size] * deg);
        --size;
    }
    printf("Число в 10 СС %d\n",result); // Вывод итогового числа, в 10 СС 
    return result;
}

ConvertToNum allocates heap memory which it returns to the caller. This should absolutely be documented somewhere, as it's very easy to leak memory. Generally, you should probably have the caller pass in a pointer to memory that they have allocated, so:

void ConvertToNum(char* str, int size, int* num)

Further, the char* passed in shouldn't be modified, so should be marked const:

void ConvertToNum(const char* str, int size, int* num)

Further, your char[] abc never changes. Instead of stack allocating it every time this function is called, you should just make it static const:

static const char[] abc = "....";

In DoMyLetters, you don't need to return from a void function. You also initialise i with 0, only to immediately change its value to size; may as well simply just declare it as int i = size;.

Your ConvertIt function is long, too long in fact. I can see a number of cases, these should be broken up into their own functions (which should probably be static, and only allow ConvertIt to call them). If you're using a C99 compliant compiler, you might want to consider using variable length arrays instead of calloc (although this seems to be written with only C89 in mind, so this may not be possible).

This loop:

for(i = 0; i <= k; i++) {
    numb2[i] = numb[i]; 
}

Could be replaced by a memcpy. The next loop:

for(i = 0; i <= (k / 2); i++) { // Переворачиваем число т.к оно сохранилось наоборот
    t = numb2[i];
    numb2[i] = numb2[k - i];
    numb2[k - i] = t;
}

should use a swap function instead of doing this inline.

Your final printf call in ConvertIt is using the wrong specifier:

printf("Число в 10 СС %d\n", result); // Вывод итогового числа, в 10 СС

Here result is a long int, and so should be %ld. You also have another unused variable in this function, numcopy.

You have a memory leak on both error paths in your main function: up the top, you have:

char *number = (char*)calloc(maxmemory, sizeof(char));

In both:

if((EXAMPLE_ARRAY < from) || (2 > from)) {

and

if(0 == length)

you return before freeing this memory (of course the OS will clean it up for you, but it's still good to remember these things).

In main, you're declaring c to be a char, and then using c = getchar(), however, getchar() actually returns an int. This can play havoc with the end of file indicator EOF. Again, potentially not a problem for a program reading from stdin, but you should still be aware of the issues it can cause.

Finally, I'd be remiss if I didn't point out that you can do everything here using the library function strtol - I assume this code is for an assignment or for learning purposes, so reinventing the wheel is fine. However, if this is not the case, stick to using standard library code rather than rolling your own.

\$\endgroup\$
  • \$\begingroup\$ Thank You, about your review! It was very informative. I used my functions instead of functions from standard library, because I think they used in main situations therefore I write my functions. They are shorter and faster than standard. \$\endgroup\$ – MaZaHaKa Nov 22 '14 at 17:40
  • \$\begingroup\$ Also function pow very big, it has a lot of things like the sequence number series and other things of mathematical analysis. Therefore I used my short function \$\endgroup\$ – MaZaHaKa Nov 22 '14 at 17:47
4
\$\begingroup\$

It is good that you divided the work into functions. However, you seem to have written a lot of code, some of which could be simplified.

I'll start with some general impressions of main():

  • setlocale() makes no difference to your program. You are not using any internationalization features of the C library.
  • You are using C99, so you don't need to declare all of those variables up front.
  • For readability, group related lines of code together, separated by blank lines.
  • The getchar() / realloc() loop is just a hard way of doing getline().
  • Most operating systems don't support negative exit codes. Positive exit codes would be more meaningful.
  • The limit of 10 digits seems arbitrary, and is likely wrong. The right thing to do, I think, is to have your number-conversion routine detect overflow as it works.
  • ConvertIt() prints output while it works, which is bad practice. It would be better if main() did all of the printing.
int main() {
    printf("Добро пожаловать в Универсальный Перекодировщик Систем Счисления (УПСС) 2000!\n");

    printf("Введите систему счисления исходного числа : ");
    int original_base;
    scanf("%d%*c", &original_base); // Может быть любой от 2 до 36
    if (original_base < 2 || original_base > MAX_BASE) {
        printf("Некорректная система счисления!\nВыход из приложения... \n");
        return -1;
    }

    printf("Введите само число : ");
    char *line = NULL;
    size_t capacity = 0;
    ssize_t line_len;
    if ((line_len = getline(&line, &capacity, stdin)) < 2) {
        printf("Число не введено! Выход из приложения! \n");
        return 1;
    }
    line[--line_len] = '\0';

    printf("Производится проверка числа... ");
    if (!IsValidNumber(line, original_base)) {
        printf("Некорректное число! Выход из приложения...\n");
        return 1;
    }
    printf("Число корректно!\n");

    printf("Введите основание конечной системы счисления : ");
    int wanted_base;
    scanf("%d%*c", &wanted_base);
    if (wanted_base < 2 || wanted_base > MAX_BASE) {
        printf("Некорректная система счисления!\nВыход из приложения... \n");
        return 1;
    }

    printf("Начинаю работу...\n");
    char *result = NULL;
    capacity = 0;
    if (!ConvertNumberBase(line, original_base, wanted_base, &result, &capacity)) {
        printf("Из-за длинного числа возможно переполнение Integer!\n");
        return 1;
    }
    printf("Число из системы с основанием %d переведено в систему с основанием %d как %s\n",
           original_base, wanted_base, result);

    free(line);
    free(result);
}

Next, CheckMyNumber():

  • Why does the size need to be passed as a parameter? Just treat number as a null-terminated string.
  • You just want to check whether every character in the string is acceptable. It should be possible to do that using just one simple loop, right?
  • To do the validation, you almost need to interpret each character's value. You might as well define a DigitValue(char c, int base) function that can be used by this code and the conversion routine.
  • A "boolean" return value would be easier to understand.
int DigitValue(char c, int base) {                                                                                                  
    int digit = c - '0';                                                                                                            
    if (0 <= digit && digit < 10 && digit < base) {                                                                                 
        return digit;                                                                                                               
    }                                                                                                                               
    digit = c - 'A' + 10;                                                                                                           
    if (10 <= digit && digit < base) {                                                                                              
        return digit;                                                                                                               
    }                                                                                                                               
    digit = c - 'a' + 10;                                                                                                           
    if (10 <= digit && digit < base) {                                                                                              
        return digit;                                                                                                               
    }                                                                                                                               
    return -1;                                                                                                                      
}                                                                                                                                   


/* Returns 1 if valid, 0 if invalid */                                                                                                      
int IsValidNumber(const char *s, int base) {                                                                                        
    if (*s == '+' || *s == '-') {                                                                                                   
        s++;                                                                                                                        
    }                                                                                                                               
    while (*s) {                                                                                                                    
        if (DigitValue(*s++, base) < 0) {                                                                                           
            return 0;                                                                                                               
        }                                                                                                                           
    }                                                                                                                               
    return 1;                                                                                                                       
}

ConvertIt() is too complicated for me to follow. I see that there is a lot of repeated code inside, and that it works recursively.

I think that it would be a lot easier if you break down the work into two parts:

/* Returns 1 if valid, 0 if overflow */
int ConvertNumberBase(const char *number, int from_base, int to_base, char **result, size_t *capacity) {
    int n;
    if (!InterpretNumber(number, from_base, &n)) {
        return 0;
    }
    FormatNumber(n, to_base, result, capacity);
    return 1;
}

Instead of printing the output, FormatNumber() would reallocate a buffer that is large enough to contain the result (similar to the way getline() works).

I encourage you to try writing InterpretNumber() and FormatNumber() yourself, and submitting them to be reviewed as separate questions.

\$\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.