# C/++/# Reviewing my C code

Tags:
1. Mar 31, 2017

### doktorwho

Hi, i wrote a code and annotated it but i have few problems with it. First it won't run on Visual Studio but it runs fine on repl.it online site which uses a C ide. Second, it seems wrong when i input
5
45 6 -4 8 3
It should print out 8 3 but for some reason it misses out on 3. Could you see what's wrong with it and give me a hint on how to resolve?
Code (C):

#include <stdio.h>
#include <stdlib.h>

//Program that from the numbers in the array searches for non-fibonacci numbers and then deletes them from the list.
void main() {
int MAX_NUM_EL = 500;
int numbers[MAX_NUM_EL];
int i, n;
int k, j, f0 = 0, f1 = 1, fib = 1;

printf("Enter the size of your array: \n");
scanf("%d", &n);
if (n>MAX_NUM_EL || n<0) {
printf("You've typed an invalid lenght of the array\n");
return 0;
}
if (n == 0) {
printf("Your array has no elements to check. The program will exit now\n");
return 0;
}

printf("Enter the numbers: \n");
for (i = 0; i < n; i++)
scanf("%d", &numbers[i]);

for (i = 0; i < n; i++) {
if (numbers[i] < 0) {                       //Checking for negative numbers.
for (k = i; k < n; k++) {               //If it find a negative number it deletes it by placing the forward element.
numbers[k] = numbers[k + 1];        //into it's place and reduces the number of elements by 1.
}
n--;
i--;
continue;
}

if (numbers[i] == 0)
continue;

for (j = 0; j < numbers[i]; j++) {
if (numbers[i] == fib)
continue;
f0 = f1;
f1 = fib;
fib = f0 + f1;                      //It goes through all the fibonacci numbers to see if that element fits.
if (numbers[i] < fib) {
for (k = i; k < n; k++) {          //If we reach fibonacci that is bigger than that element does fit and we remove it.
numbers[k] = numbers[k + 1];
}
n--;
i--;
f0 = 0;
f1 = 1;
fib = 1;
continue;
}
}
}
printf("This is the resulting array: \n");
for (i = 0; i < n; i++)
printf(" %d ", numbers[i]);

return 0;
}

2. Mar 31, 2017

### ChrisVer

First, what problem do you get from VS?

Second, when you throw out the negative numbers it would be much easier to follow if you don't change the range of your iteration (n--) but your index instead to account for the missing element. Also one way to do some bug-hunting would be to introduce cout statements within your code, it will help you find where things go wrong.
Also what does it do for 8 3 ? does it still miss the 3?

3. Mar 31, 2017

### doktorwho

It turns out visual studio works if i change every return 0; to exit(0); and i also made some slight changes to the code which now looks like this:
Code (C):

#include <stdio.h>
#include <stdlib.h>

//Program that from the numbers in the array searches for non-fibonacci numbers and then deletes them from the list.
void main() {
int MAX_NUM_EL = 500;
int numbers[MAX_NUM_EL];
int i, n;
int k, j, f0 = 0, f1 = 1, fib = 1;

printf("Enter the size of your array: \n");
scanf("%d", &n);
if (n>MAX_NUM_EL || n<0) {
printf("You've typed an invalid lenght of the array\n");
exit(0);
}
if (n == 0) {
printf("Your array has no elements to check. The program will exit now\n");
exit(0);
}

printf("Enter the numbers: \n");
for (i = 0; i < n; i++)
scanf("%d", &numbers[i]);

for (i = 0; i < n; i++) {
if (numbers[i] < 0) {                       //Checking for negative numbers.
for (k = i; k < n; k++) {               //If it find a negative number it deletes it by placing the forward element.
numbers[k] = numbers[k + 1];        //into it's place and reduces the number of elements by 1.
}
n--;
i--;
continue;
}

if (numbers[i] == 0)
continue;

for (j = 0; j < numbers[i]; j++) {
if (numbers[i] == fib) {
f0 = 0;
f1 = 1;
fib = 1;
continue;
}
f0 = f1;
f1 = fib;
fib = f0 + f1;                      //It goes through all the fibonacci numbers to see if that element fits.
if (numbers[i] < fib) {
for (k = i; k < n; k++) {          //If we reach fibonacci that is bigger than that element does fit and we remove it.
numbers[k] = numbers[k + 1];
}
n--;
i--;
f0 = 0;
f1 = 1;
fib = 1;
continue;
}
}
}
printf("This is the resulting array: \n");
for (i = 0; i < n; i++)
printf(" %d ", numbers[i]);

exit(0);
}

I do not know yet if it works but im gonna do some simplifications if possible. Hmm i don't follow your n-- argument. Could you explain a little more?

4. Mar 31, 2017

### ChrisVer

I mean try to input the array 8 3 and see what the output is... then try also 3 8... maybe something goes wrong with the assignments of f0,f1 once you have a decrease to the numbers in your array (since they no longer have the starting values of 0,1 in the next number's iteration).
As for the n--, I think I might be wrong there... but for example I would refrain from changing the array I am given into the final result and instead try to create a new one from the elements of the input one. So instead of manipulating the array itself within the loop, manipulate an copy of it let's say (but that's useful for example in python lists, maybe your program doesn't have a problem with it).

5. Mar 31, 2017

### haruspex

I am not entirely clear on the spec. Is it to treat each number independently and decide whether it fits in the standard Fibonacci sequence, or do you require the retained numbers to be in order within the sequence? If each is independent then the method seems strangely complicated.
(You could build a chain of Fibonacci numbers and scan down it for each presented number. If the presented number exceeds the last in the chain then extend the chain dynamically as needed. More efficient methods are possible which combine arrays with chains. That would allow an extensible structure while also letting you binary chop to find the match - or mismatch - faster.)

As regards debugging, how about finding a simpler failure case, just 8 3 possibly, then stepping through the algorithm by hand, playing the role of the computer?

6. Mar 31, 2017

### haruspex

Sounds like you need a flush call to push out the last of the print queue.

7. Mar 31, 2017

### Aufbauwerk 2045

I don't think it will be helpful to correct it myself, but here are a couple of test runs. I used gcc from mingw32 to compile since I didn't want to take the time to set up a VC++ project.

As you know, the first Fibonacci numbers are 0, 1, 1, 2, 3, 5, 8, 13, 21, 34, 55 which covers the following tests.

Enter the size of your array:
5
Enter the numbers:
45
6
-4
8
3
This is the resulting array:

nothing!

Here is your example again, but I've entered the numbers in order.

Enter the size of your array:
5
Enter the numbers:
-4
3
6
8
45
This is the resulting array:
3 8

So at least in this case, your program gives the right answer if I sort the numbers first.

Last edited: Mar 31, 2017
8. Mar 31, 2017

### Aufbauwerk 2045

Here's an easy simplification. The only output of your program is the printout of the Fibonacci numbers in the array. So just test each number in turn and print it right away if it's a Fibonacci number. You don't need to change the array itself.

Last edited: Mar 31, 2017
9. Mar 31, 2017

### Staff: Mentor

You have declared main() as a void function, which means that it is an error to have a statement of the form "return <some number>". The easy fix is to declare main() as "int main(void)". VS correctly noted the error in your code, the other software did not. It's not good programming practice to declare main() as a void function.

10. Apr 1, 2017

### doktorwho

I thought about printing out the numbers that are in agreement with the fibonacci sequence or creating a new array but the problem specifically states that the numbers are to be checked and if they don't comply are erased from the original array. As for your dynamic sequence i'm afraid that's currently beyond the scope of my abilities as i've just started learning C. Maybe you can provide some link for me to educate myself nd try to correct the code?

11. Apr 1, 2017

### doktorwho

I fixed the algortihm and it works good now. It fixed the issue of if numbers that are fibonacci come first the former are not displayed. The problem was that i didn't reset the fib number and it stayed at the value of the first fib.
Here it is:
Code (C):

#include <stdio.h>
#include <stdlib.h>

//Program that from the numbers in the array searches for non-fibonacci numbers and then deletes them from the list.
int main(void) {
static const int MAX_NUM_EL = 500;
int numbers[MAX_NUM_EL];
int i, n;
int k, j, f0 = 0, f1 = 1, fib = 1;

printf("Enter the size of your array: \n");
scanf("%d", &n);
if (n>MAX_NUM_EL || n<0) {
printf("You've typed an invalid lenght of the array\n");
exit(0);
}
if (n == 0) {
printf("Your array has no elements to check. The program will exit now\n");
exit(0);
}

printf("Enter the numbers: \n");
for (i = 0; i < n; i++)
scanf("%d", &numbers[i]);

for (i = 0; i < n; i++) {
if (numbers[i] < 0) {                       //Checking for negative numbers.
for (k = i; k < n; k++) {               //If it find a negative number it deletes it by placing the forward element.
numbers[k] = numbers[k + 1];        //into it's place and reduces the number of elements by 1.
}
n--;
i--;
continue;
}

if (numbers[i] == 0)
continue;
for (j = 0; j < numbers[i]; j++) {
if (numbers[i] == fib) {
f0 = 0;
f1 = 1;
fib = 1;
break;
}
f0 = f1;
f1 = fib;
fib = f0 + f1;                      //It goes through all the fibonacci numbers to see if that element fits.
if (numbers[i] < fib) {
for (k = i; k < n; k++) {          //If we reach fibonacci that is bigger than that element does fit and we remove it.
numbers[k] = numbers[k + 1];
}
n--;
i--;
f0 = 0;
f1 = 1;
fib = 1;
continue;
}
}
}
printf("This is the resulting array: \n");
for (i = 0; i < n; i++)
printf(" %d ", numbers[i]);

exit(0);
}

12. Apr 1, 2017

### doktorwho

But i have a different problem now. It still won't run in the VS. The results i got were in repl.it ide and this is what i get when i put it in VS:

1>------ Build started: Project: Assigments_for_PP2, Configuration: Debug Win32 ------
1>dd1.c
1>c:\users\doktorwho\desktop\projects\assigments_for_pp2\assigments_for_pp2\dd1.c(7): error C2057: expected constant expression
1>c:\users\doktorwho\desktop\projects\assigments_for_pp2\assigments_for_pp2\dd1.c(7): error C2466: cannot allocate an array of constant size 0
1>c:\users\doktorwho\desktop\projects\assigments_for_pp2\assigments_for_pp2\dd1.c(7): error C2133: 'numbers': unknown size
1>c:\users\doktorwho\desktop\projects\assigments_for_pp2\assigments_for_pp2\dd1.c(12): warning C4996: 'scanf': This function or variable may be unsafe. Consider using scanf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
1>c:\program files (x86)\windows kits\10\include\10.0.14393.0\ucrt\stdio.h(1269): note: see declaration of 'scanf'
1>c:\users\doktorwho\desktop\projects\assigments_for_pp2\assigments_for_pp2\dd1.c(24): warning C4996: 'scanf': This function or variable may be unsafe. Consider using scanf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
1>c:\program files (x86)\windows kits\10\include\10.0.14393.0\ucrt\stdio.h(1269): note: see declaration of 'scanf'
1>Done building project "Assigments_for_PP2.vcxproj" -- FAILED.
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========[/code]

13. Apr 1, 2017

### haruspex

Not sure why it doesn't like it, but try moving the decalaration of MAX_NUM_EL in front of main().
You might care to replace the deprecated scanf as suggested above.

Last edited: Apr 1, 2017
14. Apr 1, 2017

### haruspex

Looks ok. Just a few comments....

The word is "length", not "lenght".

Do you need the test for numbers<0? What would happen if you took it out?

You initialise f0, f1 and fib in three places. If you do it in the most logical place you only need do it once.

To avoid the repeated array shuffling, you could keep a separate read index and write index. If you read a Fib number then you write it back at the write index position in the array and increment both indexes. If you read a non-Fib number you only increment the read index.

Last edited: Apr 1, 2017
15. Apr 1, 2017

### Dr Transport

Not that I have time today to mess with it, I might next week, but I'd just write both of the arrays to a vector and compare vector arrays. If you find an element that isn't a Fibonacci number, it is easily eliminated by erase function.

16. Apr 1, 2017

### doktorwho

I do not know how to do what you Dr Transport said and i'm trying to solve it as a beginner. I hope you can post your solution when you find time to do it :).
It is absolutely true what you haruspex said and i do not need to check the negative numbers as they will already be checked if i only make the small correction i made and that is to make the counter go to the absolute value of the element. You will see where i did it as i will remark on it in the code.
You will also see that i added two more function and that is repetition until a negative number is inputed for the array length :) and the array sorting at the beginning. I also placed the constant declaration above the main as you suggested.
Code (C):

#include <stdio.h>
#include <stdlib.h>

static const int MAX_NUM_EL = 500;

int main(void) {
while(1) {
int numbers[MAX_NUM_EL];
int i, n, t;
int k, j, f0 = 0, f1 = 1, fib = 1;

printf("Enter the size of your array: \n");
scanf("%d", &n);
if (n>MAX_NUM_EL || n<0) {
printf("You've typed an invalid length of the array\n");
exit(0);
}
if (n == 0) {
printf("Your array has no elements to check. The program will exit now\n");
exit(0);
}

printf("Enter the numbers: \n");
for (i = 0; i < n; i++)
scanf("%d", &numbers[i]);

for (i = 0; i < n; i++) {
for (k = i+1; k < n; k++) {
if (numbers[i] > numbers[j]) {
t = numbers[i];
numbers[i] = numbers[j];
numbers[j] = t;
}
}
}

for (i = 0; i < n; i++) {
if (numbers[i] == 0)
continue;
for (j = 0; j < abs(numbers[i]); j++) {        //The correction.
if (numbers[i] == fib) {
f0 = 0;
f1 = 1;
fib = 1;
break;
}
f0 = f1;
f1 = fib;
fib = f0 + f1;
if (numbers[i] < fib) {
for (k = i; k < n; k++) {
numbers[k] = numbers[k + 1];
}
n--;
i--;
f0 = 0;
f1 = 1;
fib = 1;
continue;
}
}
}
printf("This is the resulting array: \n");
for (i = 0; i < n; i++)
printf(" %d ", numbers[i]);

printf("\n");
}
}

Last edited: Apr 1, 2017
17. Apr 1, 2017

### doktorwho

Im not sure what's wrong with the scanf function. There is still however this when i run it on VS:
1>------ Build started: Project: Assigments_for_PP2, Configuration: Debug Win32 ------
1>dd1.c
1>c:\users\doktorwho\desktop\projects\assigments_for_pp2\assigments_for_pp2\dd1.c(8): error C2057: expected constant expression
1>c:\users\doktorwho\desktop\projects\assigments_for_pp2\assigments_for_pp2\dd1.c(8): error C2466: cannot allocate an array of constant size 0
1>c:\users\doktorwho\desktop\projects\assigments_for_pp2\assigments_for_pp2\dd1.c(8): error C2133: 'numbers': unknown size
1>c:\users\doktorwho\desktop\projects\assigments_for_pp2\assigments_for_pp2\dd1.c(13): warning C4996: 'scanf': This function or variable may be unsafe. Consider using scanf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
1>c:\program files (x86)\windows kits\10\include\10.0.14393.0\ucrt\stdio.h(1269): note: see declaration of 'scanf'
1>c:\users\doktorwho\desktop\projects\assigments_for_pp2\assigments_for_pp2\dd1.c(25): warning C4996: 'scanf': This function or variable may be unsafe. Consider using scanf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
1>c:\program files (x86)\windows kits\10\include\10.0.14393.0\ucrt\stdio.h(1269): note: see declaration of 'scanf'
1>Done building project "Assigments_for_PP2.vcxproj" -- FAILED.
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========

Is there anything that could help?

18. Apr 1, 2017

### Dr Transport

I ran your code in VS2015 without making any changes what so ever....

19. Apr 1, 2017

### doktorwho

I have VS2017. Maybe some of my settings are messed up, should i reset them?

20. Apr 1, 2017

### Dr Transport

couldn't tell you, i just opened a new project witha precompiled header then copied your code in and it compiled without any errors.

21. Apr 2, 2017

### Staff: Mentor

I'm running VS 2015, and I'm getting the same errors you got. VS doesn't like your declaration for MAX_NUM_EL, but I don't see why at the moment.

22. Apr 2, 2017

### Staff: Mentor

You can't do this...
Code (C):
#include <stdio.h>
#include <stdlib.h>

static const int MAX_NUM_EL = 500; <<<<

int main(void) {
while(1) {
int numbers[MAX_NUM_EL];
...
The size of the array has to be known at compile time. MAX_NUM_EL is a variable, and the values of variables are known at run time, which always happens after compile time. That's the answer for C90, the specification from 1990. For compilers that support the C99 and C11 standards, you can do the above if the array could be a VLA (variable length array).

Here's an example from the MSDN documentation for the C2057 compiler error, the first one in your list of errors.
Code (C):

// C2057b.c
#define ArraySize1 10
int main() {
const int ArraySize2 = 10;
int h[ArraySize2];   // C2057 - C does not allow variables here
int h[ArraySize1];   // OK - uses preprocessor constant
}
BTW, now that main() is declared as "int main(void)" you really should have a return statement down at the bottom.

23. Apr 2, 2017

### haruspex

Some thoughts on optimisation, though not needed for this exercise I'm sure.

If you wanted it to perform well with very large numbers as inputs, there is an interesting way to avoid having to generate all the Fibonacci numbers up that number.

The nth Fibonacci number is given by the formula $\frac 1{\sqrt 5}(\lambda^n-(1-\lambda)^n)$, where $\lambda=\frac{1+\sqrt 5}2$. Note that |1-λ|<1, so as n tends to infinity that term tends to zero. Indeed, it is very quickly the case that we can discard that term and just take the nearest whole number to what results. Consequently we can invert the formula to find whereabouts in the sequence a given Fibonacci x number belongs: its index is the nearest whole number to $\frac{\ln(x\sqrt 5)}{\ln(\lambda)}$.
Assuming the built-in exp and ln functions are rather faster than generating the Fibonacci sequence in a loop, we can calculate the n for a proposed Fibonacci number x, then reverse the process to find the actual nth Fibonacci number and compare it with x.

24. Apr 2, 2017

### haruspex

Ah, of course! There may be other local scope variables, and the compiler has to know the offset of each from the current stack position.

An alternative solution would be to "new" the array.

25. Apr 2, 2017

### doktorwho

Note that in my posted code above i have a little mistake where i for sorting somehow used $j$ instead of $k$ but that doesnt change the way VS behaves.
I believe that as a part of the problem we are tested to see if we could generate the sequence as a check but i found that too and it's quite interesting. There is also a check that involves perfect squares.
If a number $n$ is a Fib number then one of the following must be a perfect square $(5n^2 \pm 4)$. So i was thinking that if a am to implement this as a check i would do this calculation take the root then round up to the nearest whole number and if it matches n it is a fib number? Is there any faster built in function?