Co można poprawić

Zmienne / inicjalizacja

• Gdzie deklarujesz multiplier? Zakładam, że skoro nie jest zadeklarowana w metodzie, jest zadeklarowana jako zmienna globalna. Staraj się unikać zmiennych globalnych.

  Problem ze zmiennymi globalnymi polega na tym, że skoro każda funkcja ma do nich dostęp, coraz trudniej jest ustalić, które funkcje faktycznie odczytują i zapisują te zmienne.

  Aby zrozumieć, jak działa aplikacja, musisz wziąć pod uwagę każdą funkcję, która modyfikuje stan globalny. Można to zrobić, ale wraz z rozwojem aplikacji będzie to coraz trudniejsze do tego stopnia, że stanie się praktycznie niemożliwe (lub przynajmniej będzie to kompletna strata czasu).

  Jeśli nie polegasz na zmiennych globalnych, może w razie potrzeby przekazywać stan między różnymi funkcjami. W ten sposób masz znacznie większą szansę zrozumienia, co robi każda funkcja, ponieważ nie musisz brać pod uwagę stanu globalnego.

  Więc zamiast używać zmienne globalne, zainicjuj zmienne w main() i przekaż je jako argumenty do funkcji, jeśli to konieczne. W tym przypadku nie widzę potrzeby używania multiplier poza funkcją, więc po prostu zachowaj to zadeklarowane w funkcji.

• sign powinno być int, a nie char .

Algorytm

• W tej chwili wdrażasz skomplikowaną i trudną do naśladowania metodę konwersji znaku na liczbę. Najłatwiejszym sposobem jest wykonanie ciężkiej pracy za Ciebie isdigit() . Pomoże Ci to również wdrożyć Zasada DRY .

  while(*pointer != "\0") { if(*pointer >= "0" && *pointer <= "9") multiplier = multiplier * 10; pointer = pointer + 1; } pointer = pointer1; while(*pointer != "\0") { if(*pointer >= "0" && *pointer <= "9") { result = result + ( (*pointer%48) * multiplier); multiplier = multiplier / 10; } pointer = pointer+1; } 

  Widzisz, jak masz dwie pętle robiące prawie identyczne rzeczy? Oto, jak uprościłem to wszystko, używając isdigit().

  while (isdigit(*c)) { value *= 10; value += (int) (*c - "0"); c++; } 

  Zapętlasz znaki w ciągu, o ile są to cyfry. Dla każdego z nich dodaj do licznika zachowujesz – wartością do dodania jest liczba całkowita znaku. Odbywa się to poprzez odjęcie wartości ASCII "0" od wartości ascii odpowiedniej cyfry.

• Zauważ, że ten kod nie „t obsłużyć przepełnienie. Jeśli przekażesz„ 89384798719061231 ”(który nie„ nie zmieści się w int), wynik jest niezdefiniowany. Rozwiązanie jest dość proste, po prostu użyj long long int, aby temu zapobiec. Nadal będziemy mieć problemy z bardzo długimi liczbami, ale naprawienie tego, aby funkcja działała zgodnie z przeznaczeniem, jest nieco bardziej skomplikowane.

Dokumentacja

• Gdzie podziały się wszystkie Twoje komentarze? Nowszy programista po prostu gapiłby się na część Twojego kodu.

  result = result + ( (*pointer%48) * multiplier); 

  Komentarze mogą naprawdę pomóc innym zrozumieć Twój kod. Nie przesadzaj z nimi, będziesz musiał zrównoważyć je umieścić w swoim programie.

Składnia / styl

• To wygląda na literówkę.

  if(*pointer == "-") sign =- 1; 

  Dodaj spację, aby zwiększyć czytelność.

  if(*pointer == "-") sign = -1; 

• Należy a nie modyfikować char*, który akceptujesz jako parametr funkcji. Dlatego zadeklaruj parametr jako stałą.

  int my_atoi(const char* pointer) 

• Użyj więcej skróconych operatorów.

  pointer++; // same as pointer = pointer+1; multiplier /= 10; // same as multiplier = multiplier / 10; multiplier *= 10; // same as multiplier = multiplier * 10; 

Kod końcowy

#include <stdio.h> #include <assert.h> #include <ctype.h> long long int my_atoi(const char *c) { long long int value = 0; int sign = 1; if( *c == "+" || *c == "-" ) { if( *c == "-" ) sign = -1; c++; } while (isdigit(*c)) { value *= 10; value += (int) (*c-"0"); c++; } return (value * sign); } int main(void) { assert(5 == my_atoi("5")); assert(-2 == my_atoi("-2")); assert(-1098273980709871235 == my_atoi("-1098273980709871235")); puts("All good."); // I reach this statement on my system } 

Komentarze

• Nie należy ' zmieniać dowolnie typów zwracanych wartości. atoi() tradycyjnie zwraca int, więc my_atoi() też powinno. Jeśli chcesz przeanalizować long long, emuluj strtoll().
• isdigit(*c) nie jest zdefiniowane dla *c wartości mniejszych niż 0 (innych niż EOF). Lepiej while (isdigit((unsigned char) (*c) ))
• Nieodebrany róg: gdy my_atoi() wynik powinien być LLONG_MIN, value += (int) (*c-'0'); jest przepełnieniem liczby całkowitej ze znakiem (UB) podczas próby utworzenia LLONG_MAX + 1.
• Używanie isdigit jest w ogóle błędny, ponieważ nie ' nie ma powiązanej funkcji numeric_value. Dlatego jeśli zestaw znaków zawiera dwa zakresy cyfr (od 0 do 9 i od ٠ do ٩), numery indyjskie zostaną przeanalizowane nieprawidłowo. Po prostu trzymaj się '0' <= c && c <= '9' dla bezpieczeństwa. Pozwala to również uniknąć niezdefiniowanego zachowania polegającego na nieprawidłowym użyciu funkcji ctype.
• Podczas pisania ” wartości ASCII ' 0 ' ” : there ' to nic, co mówi, że zestaw znaków hosta musi być ASCII (tylko, że 0..9 są ciągłe). To ' jest powodem, dla którego piszesz '0', a nie numer punktu kodowego specyficzny dla kodowania.

[Edytuj]

Z wyjątkiem zachowania w przypadku błędu, atoi() jest równoważne do (int)strtol(nptr, (char **)NULL, 10). strtol() akceptuje początkowe białe znaki. OP „s my_atoi(char* pointer) nie. Aby zaradzić:

int my_atoi(const char* pointer) { while (isspace((unsigned char) *pointer)) { pointer++; } ... 

Poniżej opisano dobry sposób obsługi INT_MIN.

OTOH, przekazywanie wartości poza [INT_MIN...INT_MAX] nie jest zdefiniowane w specyfikacji C, więc można zastosować pewne uproszczenia miał. Patrz niżej.

Gdy ciąg reprezentuje INT_MIN, (załóżmy, że 32-bitowy int), na przykład "-2147483648", kod napotyka int przepełnienie, próbując obliczyć 2147483648. Prostym sposobem rozwiązania tego problemu jest zamiast znaleźć wartość dodatnią, a następnie ją zanegować, przyjąć stronę ujemną rzeczy. Wykonując lwią część obliczeń w zakresie od INT_MIN do 0, unikamy UB. Wada: niektórzy uważają to podejście za trudniejsze do naśladowania.

Przechodzenie do szerszej liczby całkowitej lub unsigned nie zawsze jest możliwe, ponieważ rozmiar całkowity „text- -> integer ”może mieć maksymalny rozmiar. Ściśle mówiąc, unsigned nie zawsze ma szerszy zakres dodatni niż int. W każdym razie cała matematyka może być obsługiwana przy żądanym rozmiarze liczby całkowitej ze znakiem bez uciekania się do innych typów.

#include <ctype.h> #include <limits.h> int my_atoi(const char* pointer) { // good idea to make the `const` int result = 0; while (isspace((unsigned char) *pointer)) { pointer++; } char sign = *pointer; if (*pointer == "-" || *pointer == "+") { // text could lead with a "+" pointer++; } int ch; // isdigit() expects an unsigned char or EOF, not char while ((ch = (unsigned char)(*pointer)) != 0) { if (!isdigit(ch)) break; ch -= "0"; // Will overflow occur? if ((result < INT_MIN/10) || (result == INT_MIN/10 && ch > -(INT_MIN%10))) Handle_Overflow(); result *= 10; result -= ch; // - , not + pointer++; } if (sign != "-") { if (result < -INT_MAX) Handle_Overflow(); result = -result; } return result; } 

Uwagi:

pointer%48 jest mylące. Co jest specjalnego w 48? Jeśli masz na myśli "0", użyj pointer % "0".

„string:” 232-19 „. Co powinienem więc zrobić? ” Zalecamy zatrzymanie konwersji na „232” i zwrócenie wartości 232. Może ustawić errno, ale typowe atoi() funkcja nie obsługuje zbyt wiele błędów.

W przypadku przepełnienia ustawienie errno może się zdarzyć, ale znowu typowe atoi() nie obsługuje zbyt wielu błędów. Zaproponuj proste zwracanie INT_MAX lub INT_MIN.

Jeśli potrzebujesz lepszej obsługi błędów, zmień na coś podobnego do poniższego i ustawić stan błędu.

int my_atoi(const char *s, int *ErrorCode); 

lub lokalizacja gdzie wszystko się skończyło. Jeśli wszystko jest w porządku, kończyły się na "\0".

int my_atoi(const char *s, const char **endptr); 

[Edytuj] Uproszczony: Usunięty wykrywanie poza zakresem, na co pozwala specyfikacja C. „Jeśli wartość wyniku nie może być przedstawiona, zachowanie jest niezdefiniowane.

int my_atoi(const char* pointer) { int result = 0; while (isspace((unsigned char) *pointer)) { pointer++; } char sign = *pointer; if (*pointer == "-" || *pointer == "+") { pointer++; } while (isdigit((unsigned char)*pointer)) { result = result*10 - (*pointer++ - "0"); } if (sign != "-") { result = -result; } return result; } 

Komentarze

• INT_MIN/10 i INT_MIN%10 wymagają zachowania C99.

 char sign = *pointer; if (*pointer == "-" || *pointer == "+") { pointer++; } 

Po co usuwać odniesienia „pointer” trzy razy? Wystarczy jeden raz:

 char sign = *pointer; if (sign == "-" || sign == "+") { pointer++; } 

Komentarze

• Witamy w Code Review, Twoja pierwsza odpowiedź wygląda dobrze , miłego pobytu! Chociaż zastanawiam się, czy ma to wpływ na generowany kod.

jeśli nie przeszkadza Ci rekurencja wtedy kod można skrócić do jednego poniżej

#include <string.h> #include <math.h> #include <stdbool.h> int natural_number(const char* string) { int index = strlen(string) - 1; int number = pow(10, index) * (*string - "0"); return (index == 0) ? number : number + natural_number(string + 1); } int my_atoi(const char* string) { int sign = (*string == "-") ? -1 : 1; int offset = (*string == "-") ? 1 : 0; return sign * natural_number(string + offset); } /* test cases */ my_atoi("-100") == -100; my_atoi("0") == 0; my_atoi("100") == 100; 

Wyczerpanie stosu można złagodzić przez -foptimize-sibling-calls flagę kompilatora, która jest obsługiwane przez kompilatory GCC i Clang.

Aktualizacja:

Jak wspomniano implementacja Roland Illig nie obsługuje zniekształconych danych wejściowych. Jeśli jest to pożądane, ściśle przestrzegaj semantyki atoi , a następnie następnym kodem powinien być dobrze _{nie zapomnij ustawić Compile Options na jeden w komentarzach} .

int digit(char symbol) { return symbol - "0"; } /* tail call optimized */ int natural_number_tc(const char* string, int number) { return !isdigit(*string) ? number : natural_number_tc(string + 1, 10 * number + digit(*string)); } int natural_number(const char* string) { return natural_number_tc(string, 0); } const char* left_trim_tc(const char* string, const char* symbol) { return !isspace(*string) ? symbol : left_trim_tc(string + 1, symbol + 1); } const char* left_trim(const char* string) { return left_trim_tc(string, string); } int my_atoi(const char* string) { const char* symbol = left_trim(string); int sign = (*symbol == "-") ? -1 : 1; size_t offset = (*symbol == "-" || *symbol == "+") ? 1 : 0; return sign * natural_number(symbol + offset); } 

To wciąż jest kod chux „, w którym pętle zastępowane są rekurencją

int result = 0; while (isdigit((unsigned char)*pointer)) { result = 10 * result + (*pointer - "0"); pointer++; } // VS int loop(const char* pointer, int result) { return !isdigit((unsigned char)*pointer) ? result : loop(pointer + 1, 10 * result + (*pointer - "0")) } 

Komentarze

• Przypadek testowy: buf = malloc(65536); buf[0] = '\0'; my_atoi(buf) prawdopodobnie ulegnie awarii.
• Przypadek testowy: bufsize = 1 << 20; buf = malloc(bufsize); memset(buf, '0', bufsize); buf[bufsize - 1] = '\0'; my_atoi(buf) zajmie bardzo dużo czasu.

Aby wykonać ćwiczenie w leetcode , napisał następujący impl: atoi cpp code

 class Solution { private: bool checkMin(int a, int b=10, int c=0, int min_val=INT_MIN) { /* accepts a*b + c, min a>min; b>min; c>min check a*b+c > min or not b>0; a<0 -ive; c<0 a!=0 */ min_val = min_val -c; //std::cout<<"new min input: "<<a <<" , "<< c<<" iter: "<<b << " "<<min_val <<std::endl; //compare with a now if(a<min_val) return false; int cur_prod = 0; if(a==0) return true; for(;b>1;b--) { cur_prod += a; int curr_diff = min_val-cur_prod; /* subtraction possible because min_val<prod, min_val-prod<prod-prod min_val-prod<0 ---1 prod<0 -prod>0 min_val+(-prod )> min_val+0 [x+ (+ive quantity)>x ] min_val-prod>min_val --2 from 1, 2 min_val< min_val-prod < 0 ---3 from 3, min_val-prod can be expressed in integer check if curr_diff still can hold a deduction of a which means: curr_diff<a should hold, for a further a deduction in prod -5, -6 for ex of min_val = 59, a = -6 at b = 2 (9th iteration) prod = -54 you can"t add -6 now, since it will cross definable limit only b-1 iterations because at i-1 th iteration, ith product formation is checked */ //std::cout<<"check function for input: "<<a <<" , "<< c<<" iter: "<<b << " prod now = " //<< cur_prod << " diff = " <<curr_diff<<" is curr_dif<a "<<(curr_diff<a)<<std::endl; if(curr_diff>a) { //std::cout<<" not possible"<<std::endl; return false; } } return true; } bool checkMax(int a, int b=10, int c=0, int max_val=INT_MAX) { /* accepts a*b + c, min a<max; b<max; c<max check a*b+c < max or not b>0; a>0, c>0 */ max_val = max_val -c; //std::cout<<"new max input: "<<a <<" , "<< c<<" iter: "<<b << " "<<max_val <<std::endl; //compare with a now if(a>max_val) return false; int cur_prod = 0; if(a==0) return true; for(;b>1;b--) { cur_prod += a; int curr_diff = max_val-cur_prod; /* subtraction possible because max_val>prod, max_val-prod>prod-prod max_val-prod>0 ---1 prod>0 -prod<0 max_val+(-prod )< max_val+0 [x+ (-ive quantity)<x ] max_val-prod<max_val --2 from 1, 2 0< max_val-prod < max_val ---3 from 3, max_val-prod can be expressed in integer check if curr_diff still can hold a increment of a which means: curr_diff>a should hold, for a further a deduction in prod 5>6 fails for ex of max_val = 59, a = 6 at b = 2 (9th iteration) prod = 54 you can"t add 6 now, since it will cross definable limit only b-1 iterations because at i-1 th iteration, ith product formation is checked */ //std::cout<<"check function for input: "<<a <<" , "<< c<<" iter: "<<b << " prod now = " // << cur_prod << " diff = " <<curr_diff<<" is curr_dif<a "<<(curr_diff>a)<<std::endl; if(curr_diff<a) { //std::cout<<" not possible"<<std::endl; return false; } } return true; } public: int myAtoi(string str) { //code to trim string int i =0, end=str.length()-1; //std::cout<<i<<" "<<end<<std::endl; while(i<end && str.at(i)==" ") {i++;continue;} while(end>-1 && str.at(end)==" ") {end--;continue;} if(end<i) return 0; int sign=1; if(str.at(i)=="-") {sign = -1; i++;} else if(str.at(i)=="+") {i++;} string tr_str = str.substr(i, end-i+1); int num = 0; for(char& digit : tr_str) { if(digit<"0" || digit>"9") return num; // not convertable character - exit int c= digit-"0"; if(sign==-1) { //std::cout<<"Evaluating "<<c<<std::endl; //number cannot be lower than INT_MIN // do a check of num * 10 - c //num<0 already if(checkMin(num, 10, -c, INT_MIN)) num = num*10 -c; else { num = INT_MIN; break; } //std::cout<<"number is"<<num<<std::endl; } else { if(checkMax(num, 10, c, INT_MAX)) num = num*10 +c; else { num = INT_MAX; break; } //std::cout<<"number is"<<num<<std::endl; } } return num; } }; 

Komentarze

• Witamy w Code Review! Przedstawiłeś alternatywne rozwiązanie, ale nie ' nie sprawdziłeś kodu. Wyjaśnij swoje uzasadnienie (jak działa Twoje rozwiązanie i dlaczego jest lepsze od oryginału), aby autor i inni czytelnicy mogli wyciągnąć wnioski z Twojego procesu myślowego.
• kod używa metody, gdzie checkMin, gdzie nie bezpośrednie mnożenie jest wykonywane do momentu zatwierdzenia wyniku. być większe niż INT_MIN.