Context: An exercise asks me to:
10 <= x <= 100
, print that it is not a valid number and prompt the user to reinsert a - valid - number;Problem: The issue is that, as you may notice watching the code, it is incredibly unoptimized as in "I couldn't think of a better way to solve this problem, but I do know that there must be an overall better way of doing this". Specifically though, I wasn't able to design a better way to check if the newly inserted number was already inserted before printing it (hence my question).
I have written the following code in which I think I have tackled all of the given tasks.
//Esercizio 6.15 ||| Pag. 277
#include <stdio.h>
#define SIZE 20
int main()
{
int a[SIZE];
int nums_left = 20;
for_cycle:
for(int i=0; i<SIZE; ++i){
printf("Please insert %d numbers (whole)\n", nums_left);
scanf("%d", &a[i]);
//Managing Errors
if(a[i]<10 || a[i]>100){
printf("You have inserted a non valid value!\n");
goto for_cycle; //heresy! I know
}
//Effective execution of the printing | The Issue
if(a[i]!=(a[i-1])){
if(a[i]!=a[i-2]){
if(a[i]!=a[i-3]){
if(a[i]!=a[i-4]){
if(a[i]!=a[i-5]){
if(a[i]!=a[i-6]){
if(a[i]!=a[i-7]){
if(a[i]!=a[i-8]){
if(a[i]!=a[i-9]){
if(a[i]!=a[i-10]){
if(a[i]!=a[i-11]){
if(a[i]!=a[i-12]){
if(a[i]!=a[i-13]){
if(a[i]!=a[i-14]){
if(a[i]!=a[i-15]){
if(a[i]!=a[i-16]){
if(a[i]!=a[i-17]){
if(a[i]!=a[i-18]){
if(a[i]!=a[i-19]){
if(a[i]!=a[i-20]){
printf("You have inserted: %d\n", a[i]);
}}}}}}}}}}}}}}}}}}}
//Updates of the variable 'numbers left to insert' accordingly
nums_left--;
}else{i--;} //decrements the counter
}
return 0;
}
goto
is not as evil as you might think now, but here it is plain wrong. You'd re-start the loop with index 0 if it actually comes to action... bool inserted[100-10+1] = {0};
and later if(inserted[num-10]) { /*already inserted*/ } else { inserted[num-10] = true; }
Loop through the array afterwards to list those inserted. a[i] != a[i-X]
will access the array out of bounds (undefined behaviour!) for any i < X
! z
of 91 elements and set it all to zeros. Once x
is "inserted", set z[x-10]
to 1
. To check if element x
was already inserted, check z[x-10]
. for(size_t i = 0; i < SIZE; /* no automatic increment!) { a[i] = ...; for(size_t j = 0; j < i; ++j) { if(a[j] == a[i]) { goto INSERTED; } } ++i; INSERTED:; }
– so you iterate over the values already inserted and only if not found you switch to next i
. Other loop variants possible, too... scanf()
returns EOF
or 0, then OP's code is UB. scanf
– note that you cannot get out of that loop at all any more if some user provides invalid input like e.g. 10sss
– if so, then each next loop will not read any further input once 10
has been scanned as sss
cannot get converted to an integer and thus won't be consumed. Not sure if appropriate error handling is required by the task, but it is a good idea to get used to consider this possibility right from the start with every new task. fgets
and use sscanf
afterwards to extract the actual number. a[i]
might not have been written to at all – have been inattendent... Seems as if me being too much used to already doing the error handling myself ;) scanf()
does return 0 or EOF
due to end-of-file, no UB in the if(a[i]<10 || a[i]>100){
, yet certainly an infinite loop as you suggest. I suspect the meaningless zero initialization was crafted for this learner task. Somewhat necessary when weak code does not check return values. There are two main alternatives for tracking and testing which valid numbers have already been added:
You have implemented a slightly buggy and inelegant form of the first. Another answer, now deleted, demonstrated a correct and more elegant variation on this theme, using a nested loop to iterate over the array elements already assigned.
In the speed for space tradeoff category, however, there are solutions of the second type. You might, for example, use a binary search tree to record the values added so far, and then search that instead of the main array. For so few total values, however, and such a small range of valid ones, a pretty good alternative would be to maintain a simple lookup table of which values had been recorded so far. For example:
#include <stdio.h>
#define SIZE 20
#define MIN_VALID 10
#define MAX_VALID 100
int main(void) {
int a[SIZE];
_Bool seen[MAX_VALID + 1] = { 0 };
for (int next_position = 0; next_position < SIZE; ) {
printf("Please insert %d numbers (whole)\n", SIZE - next_position);
scanf("%d", &a[next_position]);
if (a[next_position] < MIN_VALID || a[next_position] > MAX_VALID){
printf("%d is not a valid value!\n", a[next_position]);
} else if (seen[a[next_position]]) {
printf("You already inserted %d!\n", a[next_position]);
} else {
printf("You have inserted: %d\n", a[next_position]);
seen[a[next_position]] = 1;
next_position += 1;
}
}
return 0;
}
Given that the range of numbers is limited to 91 options, it is reasonable to create an array of bool
initialized to false
to store flags to determine if a number has been entered.
On each number being added, if the corresponding flag is false
, accept the number and change that flag to true
.
A simple implementation of this idea might look like:
#include <stdio.h>
#include <stdbool.h>
#define N 20
#define LOWER 10
#define UPPER 100
int main(void) {
bool flags[UPPER - LOWER + 1] = { false };
int input_numbers[N];
for (size_t i = 0; i < N; i++) {
while (true) {
int n;
if (scanf("%d", &n) != 1) {
printf("Please enter a number.\n");
while (getchar() != '\n');
continue;
}
if (n >= LOWER && n <= UPPER && !flags[n-LOWER]) {
input_numbers[i] = n;
flags[n-LOWER] = true;
break;
}
printf("Invalid input.\n");
}
}
return 0;
}