Saker du kan förbättra

Variabler / initialisering

• Var förklarar du multiplier? Jag antar att eftersom det inte deklareras inom metoden, förklaras det som en global variabel. Försök att undvika globala variabler.

  Problemet med globala variabler är att eftersom varje funktion har tillgång till dessa blir det allt svårare att räkna ut vilka funktioner som faktiskt läser och skriver dessa variabler.

  För att förstå hur applikationen fungerar måste du i stort sett ta hänsyn till varje funktion som ändrar den globala staten. Det kan göras, men när applikationen växer kommer det att bli svårare så att det är praktiskt taget omöjligt (eller åtminstone ett helt slöseri med tid).

  Om du inte litar på globala variabler, kan skicka tillstånd mellan olika funktioner efter behov. På så sätt har du en mycket bättre chans att förstå vad varje funktion gör, eftersom du inte behöver ta hänsyn till den globala staten.

  Så istället för att använda globala variabler, initiera variablerna i main() och skicka dem som argument till funktioner om det behövs. I det här fallet ser jag inte behovet av att multiplier alls ska användas utanför funktionen, så håll det bara deklarerat inom funktionen.

• sign ska vara en int och inte en char .

Algoritm

• Just nu implementerar du en komplicerad och svår att följa metoden för att konvertera ett tecken till ett tal. Det enkla sättet är att isdigit() gör det hårda arbetet för dig. Detta hjälper dig också att implementera DRY-princip .

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

  Se hur du har två slingor som gör nästan identiska saker? Så här förenklade jag allt detta genom att använda isdigit().

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

  Du går igenom tecknen i strängen så länge de är siffror. För var och en, lägg till räknaren du behåller – värdet som ska läggas till är karaktärens helvärde. Detta görs genom att subtrahera ASCII-värdet för "0" från ascii-värdet för siffran i fråga.

• Observera att den här koden inte ”t handflöde. Om du skickar in” 89384798719061231 ”(som inte passar i en int) är resultatet odefinierat. Korrigeringen är enkel nog, använd bara en long long int för att mildra det. Vi har fortfarande problem med extremt långa siffror, men att fixa det så att funktionen fungerar som avsett är lite mer komplicerat.

Dokumentation

• Vart gick alla dina kommentarer? En nyare utvecklare skulle helt enkelt kika på en del av din kod.

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

  Kommentarer kan verkligen hjälpa andra att förstå din kod. Men gå inte överbord med dem, du måste balansera hur mycket av dem att lägga in i ditt program.

Syntax / Styling

• Det här ser ut som ett stavfel.

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

  Lägg till ett utrymme för tydlighet.

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

• Du bör inte ändra din char* du accepterar som parameter i funktionen. Förklara därför parametern som konstant.

  int my_atoi(const char* pointer) 

• Använd fler stenografiska operatorer.

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

Slutkod

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

Kommentarer

• Du bör inte ’ t ändra returtyper godtyckligt. atoi() returnerar traditionellt en int, så my_atoi() bör också. Om du vill analysera en long long, emulera sedan strtoll().
• isdigit(*c) definieras inte för *c värden mindre än 0 (annat än EOF). Bättre att while (isdigit((unsigned char) (*c) ))
• Missat hörn: När my_atoi() resultatet ska vara LLONG_MIN, value += (int) (*c-'0'); är signerat heltalsflöde (UB) när det försöker bilda LLONG_MAX + 1.
• Med isdigit är inte alls alls, eftersom det inte ’ inte har en relaterad funktion numeric_value. Därför, om din karaktärsuppsättning har två siffror (0 till 9 och ٠ till Indic), tolkas indikatorns siffror fel. Håll dig bara till '0' <= c && c <= '9' för att vara säker. Detta undviker också det odefinierade beteendet från att använda ctype-funktionen felaktigt.
• Du missade en viktig punkt när du skrev ” ASCII-värde på ’ 0 ’ ” : där ’ s ingenting som säger att värdteckenuppsättningen behöver vara ASCII (bara att 0..9 är angränsande). Det är ’ varför du skriver '0' istället för ett kodningsspecifikt kodpunktnummer.

[Redigera]

Förutom beteendet vid fel är atoi() ekvivalent till (int)strtol(nptr, (char **)NULL, 10). strtol() accepterar ledande vitt utrymme. OP ”s my_atoi(char* pointer) gör det inte. För att åtgärda:

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

Nedan beskrivs ett bra sätt att hantera INT_MIN.

OTOH, att lämna värden utanför [INT_MIN...INT_MAX] definieras inte av C-specifikationen, så vissa förenklingar kan göras hade. Se långt nedan.

När en sträng representerar INT_MIN, (låt oss anta 32-bitars int) som "-2147483648", kod löper till int överflöde och försöker få beräkna 2147483648. Ett enkelt sätt att lösa detta är snarare än att hitta det positiva värdet och sedan negera det, omfamna negativa sidan . Genom att göra lejonandel av matematiken i INT_MIN till 0 -området undviker vi UB. Nedåt: vissa tycker att detta tillvägagångssätt är mer utmanande att följa.

Att gå till ett bredare heltal eller unsigned det är inte alltid möjligt eftersom heltalets storlek på ”text -> heltal ”rutin kan vara den maximala storleken. Strängt taget unsigned har inte alltid ett större positivt intervall än int. I alla fall kan all matematik hanteras med önskad signerad helstorleksstorlek utan att använda andra typer.

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

Anmärkningar:

pointer%48 är förvirrande. Vad är speciellt med 48? Om du menar "0", använd sedan pointer % "0".

”sträng:” 232-19 ”. Vad ska jag gör då? ” Rekommenderar att stoppa konvertering vid ”232” och returnera värdet 232. Kunde ställa in errno, men den typiska atoi() funktionen gör inte för mycket felhantering.

Vid överflöde kan inställning av errno hända, men återigen, typisk atoi() -funktionen gör inte för mycket felhantering. Föreslå enkel återvändande INT_MAX eller INT_MIN.

Om du vill ha bättre felhantering, byt till något liknande följande och ställa in en felstatus.

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

eller plats där sakerna slutade. Om detta är bra, slutade de på "\0".

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

[Redigera] Förenklad: Borttagen upptäckt utanför området som C-specifikationen tillåter det. ”Om värdet på resultatet inte kan representeras är beteendet odefinierat.

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

Kommentarer

• INT_MIN/10 och INT_MIN%10 kräver C99-beteende.

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

Varför hänvisa till ”pekare” tre gånger? En gång räcker:

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

Kommentarer

• Välkommen till Code Review, ditt första svar ser bra ut , ha en trevlig vistelse! Men jag undrar om det gör skillnad i den genererade koden.

om du är ok med rekursion då kunde koden förkortas till en nedanför

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

Stackutmattning kunde mildras av -foptimize-sibling-calls kompilatorflagga, det vill säga stöds av både GCC- och Clang-kompilatorer.

Uppdatering:

Som nämnts av Roland Illig implementering hanterar inte felformat input. Om det är önskvärt följer du atoi semantik , nästa kod ska vara bra _{glöm inte inställningen Compile Options till en i kommentarer} .

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

Detta är fortfarande chux ”s kod där slingor ersätts med rekursion

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

Kommentarer

• Testfall: buf = malloc(65536); buf[0] = '\0'; my_atoi(buf) kommer troligen att krascha.
• Testfall: bufsize = 1 << 20; buf = malloc(bufsize); memset(buf, '0', bufsize); buf[bufsize - 1] = '\0'; my_atoi(buf) tar mycket lång tid.

För en övning i leetcode , skrev följande impl: atoi cpp-kod

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

Kommentarer

• Välkommen till kodgranskning! Du har presenterat en alternativ lösning men har inte ’ t granskat koden. Förklara ditt resonemang (hur din lösning fungerar och varför den är bättre än originalet) så att författaren och andra läsare kan lära av din tankeprocess.
• koden använder en metod, där checkMin, där ingen direkt multiplikation utförs tills resultatet valideras. att vara större än INT_MIN.