Dingen die je zou kunnen verbeteren

Variabelen / initialisatie

• Waar verklaar je multiplier? Ik neem aan dat aangezien het niet binnen de methode wordt gedeclareerd, het wordt gedeclareerd als een globale variabele. Probeer globale variabelen te vermijden.

  Het probleem met globale variabelen is dat aangezien elke functie hier toegang toe heeft, het steeds moeilijker wordt om erachter te komen welke functies deze variabelen daadwerkelijk lezen en schrijven.

  Om te begrijpen hoe de applicatie werkt, moet u vrijwel elke functie in overweging nemen die de globale toestand wijzigt. Dat is mogelijk, maar naarmate de toepassing groeit, wordt het moeilijker en wordt het vrijwel onmogelijk (of in ieder geval een totale verspilling van tijd).

  Als u niet op globale variabelen vertrouwt, kan waar nodig status doorgeven tussen verschillende functies. Op die manier heb je een veel betere kans om te begrijpen wat elke functie doet, aangezien je geen rekening hoeft te houden met de globale status.

  Dus in plaats van te gebruiken globale variabelen, initialiseer de variabelen in main(), en geef ze indien nodig als argumenten door aan functies. In dit geval zie ik de noodzaak niet in dat multiplier helemaal buiten de functie moet worden gebruikt, dus laat het gewoon gedeclareerd binnen de functie.

• sign moet een int zijn en niet een char .

Algoritme

• Op dit moment implementeer je een gecompliceerde en moeilijk te volgen methode om een teken in een getal om te zetten. De gemakkelijke manier is om isdigit() het harde werk voor je te laten doen. Dit zal je ook helpen bij het implementeren van de DRY-principe .

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

  Zie je hoe je twee lussen hebt die bijna identieke dingen doen? Hier is hoe ik dat allemaal vereenvoudigde door isdigit() te gebruiken.

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

  U doorloopt de tekens in de tekenreeks zolang het cijfers zijn. Voeg voor elk teken toe aan de teller je behoudt – de waarde die moet worden toegevoegd is de gehele waarde van het teken. Dit wordt gedaan door de ASCII-waarde van "0" af te trekken van de ascii-waarde van het cijfer in kwestie.

• Merk op dat deze code geen “t omgaan met overflow. Als je” 89384798719061231 “invoert (wat niet” past in een int), is het resultaat niet gedefinieerd. De oplossing is eenvoudig genoeg, gebruik gewoon een long long int om dit te verhelpen. We zullen nog steeds problemen hebben met extreem lange nummers, maar het oplossen ervan zodat de functie werkt zoals bedoeld, is een beetje ingewikkelder.

Documentatie

• Waar zijn al je opmerkingen gebleven? Een nieuwere ontwikkelaar staarde gewoon naar je code.

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

  Reacties kunnen er echt toe bijdragen anderen te helpen uw code te begrijpen. Ga er echter niet overboord mee, u zult moeten afwegen hoeveel ze om in uw programma te plaatsen.

Syntax / Styling

• Dit ziet eruit als een typfout.

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

  Voeg een spatie toe voor de duidelijkheid.

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

• Je mag niet wijzigt uw char* die u accepteert als parameter in de functie. Declareer daarom de parameter als constant.

  int my_atoi(const char* pointer) 

• Gebruik meer steno-operatoren.

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

Laatste code

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

Reacties

• Je moet niet ‘ het retourtype willekeurig wijzigen. atoi() retourneert traditioneel een int, dus my_atoi() zou dat ook moeten doen. Als je een long long wilt parseren, emuleer dan strtoll().
• isdigit(*c) is niet gedefinieerd voor *c -waarden kleiner dan 0 (behalve EOF). Beter om while (isdigit((unsigned char) (*c) ))
• Gemiste hoek: wanneer my_atoi() het resultaat LLONG_MIN, value += (int) (*c-'0'); is ondertekende integer overflow (UB) terwijl het probeert LLONG_MAX + 1 te vormen.
• Gebruik van isdigit is helemaal verkeerd, aangezien het ‘ geen gerelateerde functie heeft numeric_value. Als uw tekenset dus twee cijferreeksen heeft (0 t / m 9 en ٠ t / m the), worden de Indische getallen verkeerd geparseerd. Blijf voor de zekerheid gewoon bij '0' <= c && c <= '9'. Dit voorkomt ook dat het ongedefinieerde gedrag de ctype-functie onjuist gebruikt.
• Je hebt een belangrijk punt gemist toen je ” ASCII-waarde van ‘ 0 ‘ ” : er is ‘ is niets dat zegt dat de host-karakterset ASCII moet zijn (alleen die 0..9 zijn aaneengesloten). Dat ‘ is waarom je '0' schrijft in plaats van een coderingsspecifiek codepuntnummer.

[Bewerken]

Behalve het gedrag bij fouten is atoi() gelijkwaardig naar (int)strtol(nptr, (char **)NULL, 10). strtol() accepteert eerste witruimte. OP “s my_atoi(char* pointer) doet dat niet. Oplossing:

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

Het onderstaande beschrijft een goede manier om INT_MIN.

OTOH, het overhandigen van waarden buiten [INT_MIN...INT_MAX] is niet gedefinieerd door de C-specificatie, dus enkele vereenvoudigingen kunnen worden had. Zie ver hieronder.

Wanneer een tekenreeks INT_MIN vertegenwoordigt, (laten we aannemen dat 32-bits int) zoals "-2147483648", komt code in int overloop om te proberen 2147483648. Een eenvoudige manier om dit op te lossen is in plaats van de positieve waarde te vinden en deze vervolgens te ontkennen, de negatieve kant van dingen te omarmen. Door het leeuwendeel van de wiskunde uit te voeren in het bereik van INT_MIN tot en met 0, vermijden we UB. Keerzijde: sommigen vinden deze benadering een grotere uitdaging om te volgen.

Als u naar een breder geheel getal of unsigned gaat, is dit niet altijd mogelijk als de gehele grootte van “tekst- -> integer “routine kan de maximale grootte zijn. Strikt genomen heeft unsigned niet altijd een groter positief bereik dan int. In elk geval kan alle wiskunde worden afgehandeld op de gewenste grootte van een geheel getal met teken zonder toevlucht te nemen tot andere typen.

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

Opmerkingen:

pointer%48 is verwarrend. Wat is er speciaal aan 48? Als je "0" bedoelt, gebruik dan pointer % "0".

“string:” 232-19 “. Wat moet ik doen dan? ” Beveel aan om de conversie te stoppen bij “232” en de waarde 232 te retourneren. Kon errno instellen, maar de typische atoi() functie doet niet te veel foutafhandeling.

Bij overloop kan het instellen van errno gebeuren, maar nogmaals, typisch atoi() functie doet niet te veel foutafhandeling. Stel voor om eenvoudig INT_MAX of INT_MIN te retourneren.

Als je een betere foutafhandeling wilt, verander dan naar iets als het volgende en stel een foutstatus in.

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

of locatie waar het eindigde. Als dit goed is, eindigden ze op de "\0".

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

[Bewerken] Vereenvoudigd: verwijderd detectie buiten bereik zoals C-specificatie dat toelaat. “Als de waarde van het resultaat niet kan worden weergegeven, is het gedrag ongedefinieerd.

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

Reacties

• INT_MIN/10 en INT_MIN%10 vereisen C99-gedrag.

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

Waarom “pointer” verwijderen drie keer? Een keer is genoeg:

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

Reacties

• Welkom bij Code Review, je eerste antwoord ziet er goed uit , geniet van je verblijf! Hoewel ik me afvraag of het een verschil maakt in de gegenereerde code.

als je akkoord gaat met recursie dan zou de code kunnen worden ingekort tot een hieronder.

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

Stack-uitputting kan worden beperkt door -foptimize-sibling-calls compilervlag, dat wil zeggen ondersteund door zowel GCC- als Clang-compilers.

Update:

Zoals vermeld door Roland Illig -implementatie verwerkt geen misvormde invoer. Als het gewenst is, volgt u atoi semantiek en de volgende code zou prima _{vergeet niet Compile Options in te stellen op één in reacties} .

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

Dit is nog steeds chux “s code waar loops vervangen worden door recursie

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")) } 

Reacties

• Testgeval: buf = malloc(65536); buf[0] = '\0'; my_atoi(buf) zal waarschijnlijk crashen.
• Testgeval: bufsize = 1 << 20; buf = malloc(bufsize); memset(buf, '0', bufsize); buf[bufsize - 1] = '\0'; my_atoi(buf) duurt erg lang.

Voor een oefening in leetcode , schreef de volgende 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; } }; 

Reacties

• Welkom bij Code Review! Je hebt een alternatieve oplossing gepresenteerd, maar ‘ heb ik de code niet bekeken. Leg alstublieft uw redenering uit (hoe uw oplossing werkt en waarom deze beter is dan het origineel) zodat de auteur en andere lezers kunnen leren van uw denkproces.
• de code gebruikt een methode, waarbij checkMin, waar geen directe vermenigvuldiging wordt uitgevoerd totdat het resultaat is gevalideerd. groter zijn dan INT_MIN.