Jag läste användarinmatning från stdin i vanlig C. Problemet är att jag vill ha en förnuftig implementering som är robust mot fel och begränsar användaren till en viss ingång och inte suger i termer av komplexitet. Funktionen get_strings() läser ingångsskal med char så länge det inte finns någon ny rad (\n), ingen EOF och alla tecken passerar isalpha() test. Men jag vill behålla utrymmen.
Några punkter som (jag tror) förtjänar särskild uppmärksamhet under granskningen:
- – Jag skulle gärna bli av med yttre medan loop som i grunden testar om användaren bara tryckte på Enter utan någon meningsfull inmatning.
- – Behöver jag ens använda ferror ()? fgetc returnerar redan EOF när något gick fel, men jag kommer bara sluta läsa från strömmen istället för att berätta för användaren att något gick fel.
- – Det händer inte varje gång men det händer: A \ n stannar kvar i stdin-strömmen och nästa gång vill jag få meningsfull inmatning fgetc () hoppar bara över. Det spelar ingen roll här, men det gör det när jag frågar enstaka tecken Ja / Nej. Jag kan bara bli av med detta problem med en konstruktion som rensar bort alla tidigare saker som finns kvar i stdin. Se den andra kodblocket för detta .
Så hanterar jag detta helt fel? Finns det bättre metoder att anamma? Det ser väldigt klumpigt ut för mig och klumpigt är alltid dåligt.
/** @brief Contains the dictionary */ static char **strings = NULL; /** @brief Helps with containing the dicionary */ static char *string; /* Reads input char by char with fgetc() */ static char *get_strings() { char *string = NULL; char ch; size_t len = 0; while (string == NULL && ch != EOF) { while (EOF != (ch = fgetc(in_stream)) && ch != "\n") { if (ch != " " && isalpha((int)ch) == 0) { fprintf(stderr, "Only [a-z] is a valid input. | \t" "| Input another or end with CTRL+D: "); continue; } string = (char*) realloc(string, len+2); if (string == NULL) { bail_out(EXIT_FAILURE, "realloc(3) failed"); } string[len++] = toupper(ch); if (len >= MAX_DATA) { bail_out(EXIT_FAILURE, "Input too long\n"); } } if (ferror(in_stream)) { bail_out(EXIT_FAILURE, "Error while reading from stream"); } } if(string) { string[len] = "\0"; } else { printf("\nFinished dictionary...\n"); } printf("Added string: %s | Input another or end with CTRL+D: ", string); return string; } /* Saves the returned strings from get_strings() in a linked list */ static void read_dict() { int index; for (index = 0; (string = get_strings()); ++index) { if (string[0] == "\0") continue; strings = (char**) realloc(strings, (index+1)*sizeof(*strings)); if (strings == NULL) { bail_out(EXIT_FAILURE, "realloc(3) failed"); } strings[index] = string; } /* Take a note of how many entries we have yet. */ dict_size = index; } Andra kodblock med ett enklare fall:
while(1) { char tmp; printf("Please enter your guess [a-z]: "); guess = fgetc(stdin); /* Jump back to start of loop */ if (guess == "\n") { continue; } /* HERE IS THE CLEAR FOR STDIN This part really just eats all remaining \ns from the user, so that later inputs can start uninterrupted. Can I get rid of it in some better way? */ while((tmp = getchar()) != "\n" && tmp != EOF); if(!isalpha(guess)) { fprintf(stderr, "Enter a valid letter [a-z]!\n"); continue; } } Kommentarer
Svar
Arkitektur
stdin är vanligtvis radbuffrad . Så ingenting ges till fgetc() förrän användaren träffar Enter . OP-koden ger flera felmeddelanden med inmatning som ”Hej 123”. Bättre att separera användarinmatning från ingångsvalidering. Läs raden med användarinmatning med fgets() eller någon egen version eftersom fgets() har vissa svagheter. Sedan validera ingången.
char *input; while ((input = my_gets()) != NULL) { if (valid_phrase(input)) { foo(input); } else { fprintf(stderr, "Invalid input\n"); } free(input); } När det gäller ”Jag skulle gärna bli av med den yttre medan slingan”. Den slingan finns för att tyst konsumera "\n". Om du vill att en slinga ska göra det, gick precis inre slinga med
int ch; while ((ch = fgetc()) == "\n") ; ungetc(ch, stdin); char ch
ch är inte den bästa typen. fgetc() returnerar vanligtvis 257 olika värden [0-255] och EOF. För att skilja dem ordentligt sparar du resultatet i en int.
// bad char ch; .. while (string == NULL && ch != EOF) { while (EOF != (ch = fgetc(in_stream)) && ch != "\n") { // better int ch; .. while (string == NULL && ch != EOF) { while (EOF != (ch = fgetc(in_stream)) && ch != "\n") { Samma för char tmp;
realloc()
Cast behöver inte.
Ändra för att minnet är tomt för att frigöra string – behövs inte om koden helt enkelt går ut, men ändå god praxis att sätta dina leksaker (kod ”s pekare) bort.
// string = (char*) realloc(string, len+2); char * new_string = realloc(string, len+2); if (new_string == NULL) { free(string); bail_out(EXIT_FAILURE, "Out of memory"); } string = new_string; Bra användning av sizeof(*strings) nedan. Rekommendera förenkling.
strings = (char**) realloc(strings, (index+1)*sizeof(*strings)); strings = realloc(strings, sizeof *strings * (index+1)); size_t len
Bra användning av size_t för att representera en matrisstorlek. Kodsligt nog gör inte samma sak med int index;. Rekommendera size_t index;
is...()
Med hjälp av int ch, inget behov av cast. Eftersom is är ett logiskt test, rekommendera att du använder ! snarare än aritmetik == 0.
// if (ch != " " && isalpha((int)ch) == 0) { if (ch != " " && !isalpha(ch)) { Följande kan vara lättare att förstå – färre negationer. (Utgåva för stil)
if (!(ch == " " || isalpha(ch))) { ferror()
Trevlig kontroll av if (ferror(in_stream))
Variabla namn
string, strings är lika användbart som anropar ett heltal integer. Kanske phrase, dictionary istället.
// OK /** @brief Contains the dictionary */ static char **strings = NULL; // Better // comment not truly needed static char **dictionary = NULL; get_strings() heter fel. Det låter allmänt, men kod begränsar inmatningen till bokstäver och utrymme.Kanske get_words()?
Kommentarer
- Jag känner att du skrev samma svar två gånger? Hur som helst, detta är svaret jag letade efter! Jag var helt fokuserad på att använda fgetc sind fgets fungerade ’ nt i början (på grund av skurkar \ ns i stdin). Detta verkar mycket bättre och jag kommer att införliva detta i min kod. Tack!
- @Haini du vet att förutom att acceptera kan du också rösta på ett svar om du gillade det så mycket 😉
Svar
Dålig realloc-strategi
För närvarande ringer du realloc() på varje tecken du läser. Detta resulterar i en \ $ O (n ^ 2) \ $ tid att läsa en sträng, för varje gång du ringer till realloc() kan det behöva kopiera det aktuella innehållet till den nya bufferten . Du bör antingen bara tilldela en buffert i storleken MAX_DATA och sedan använda realloc för att minska tilldelningen i slutet, eller ändra till en omfördelningsstrategi där omfördelningsstorleken ökas med en multiplikationsfaktor varje gång (som 2x).
Detta gäller också din stränguppsättning där du gör samma sak.
Konstig indrag
Din fördjupning är konstig eftersom din kapslade while -slinga är på samma indragningsnivå som den yttre while -slingan.
Använd fgets ()?
Jag skulle personligen använda fgets() (eller någon annan biblioteksfunktion som readline()) för att läsa en sträng. fgets() gör ganska mycket vad din slinga gör utan all handkodad logik.
stdbool.h, men don ’ t försök att rulla dina egna bools.