Looking for critique. Teaching myself C.

jmgibson1981 · 01-06-2023, 06:21 PM

https://gitlab.com/jmgibson1981/mycprogress

Thus far I've only made a very basic calculator. It works. I'm sure it won't be my last. I'm hoping it's ok to ask for constructive criticism. Thank you for your help and interest.

EdGr · 01-06-2023, 10:33 PM

That link leads to an endless spinner.
Ed

ntubski · 01-06-2023, 11:36 PM

It would be good to handle unrecognized operators with some sort of error indication.

You don't seem to be returning a value from math_line or main? I guess the compiler should be warning you about that.

Quote:

Originally Posted by EdGr

That link leads to an endless spinner.

You have JavaScript disabled? Try https://gitlab.com/jmgibson1981/mycp...culator/calc.c

EdGr · 01-07-2023, 11:26 AM

The direct link works. Otherwise, there is some problem with SeaMonkey and GitLab.

jmgibson1981 - Compile the program with -Wall. Gcc will complain about the lack of return values from functions.
Ed

pan64 · 01-07-2023, 11:45 AM

avoid code duplication, so

Code:

	switch (b) {
		case '+':
			t = a + c; break;
		case '-':
			t = a - c; break;
		case '*':
			t = a * c; break;
		case '/':
			t = a / c; break;
	}
        math_ret = print_line(t);

Also use a default for that switch (in case something different was entered).
multiplication is ok, just the shell itself interprets * before passing to your app. As you wrote you can avoid it using '*'
Check division by zero.

rtmistler · 01-07-2023, 12:30 PM

Use real, meaningful variable names.

Do not combine multiple lines of code in one physical line.

sundialsvcs · 01-07-2023, 01:28 PM

"The very first computer program that I ever wrote was eight lines long, took me six months to write, and had a bug in it."

#define PREACHER_MODE

When you are writing any "algorithm," whether it be large or small, you should also be thinking about test cases. You should be thinking, simultaneously, about cases which demonstrate correct behavior, and(!) about cases which represent errors. "A calculator" should respond "intelligently" to any(!) string of characters that is given to it. It should never "crash." No matter what.

In many languages – notably, Perl – packages are accompanied by sometimes-many-dozens of "automatic tests" which are applied before the package will consent to be installed. These tests help to verify that the package will, in fact, perform as intended on your computer. The formulation of these tests is somewhat of an art, but I have found that it is a very important discipline to develop. These disciplines will help you to say with confidence, not only that "your algorithm works," but that "it still works." (I promise you that you will be surprised when an "unrelated" change suddenly causes a test to fail. But, you will be very glad that the computer just caught it.)

#undef PREACHER_MODE

Welcome aboard. ("Welcome to my nightmare ...") Never be too hard on yourself. It can be a crazy way to make a living.

jmgibson1981 · 01-07-2023, 07:54 PM

Ok I'm still not sure about those test cases but I did make suggested changes. I also added the ability for it to detect remainders on a division solution. changed up the commenting a bit as well.

As far as the returns. I have been compiling with gcc and haven't seen any errors however i did add some to a few places. Trial and error a bit but it seems to be working just fine now. I await further guidance is you have any.

https://gitlab.com/jmgibson1981/mycprogress

ntubski · 01-07-2023, 11:12 PM

Quote:

Originally Posted by jmgibson1981

As far as the returns. I have been compiling with gcc and haven't seen any errors

Try adding -Wall and -Wextra flags:

Code:

cc -Wall -Wextra    lq-calc.c   -o lq-calc
lq-calc.c: In function 'main':
lq-calc.c:27:18: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
   27 |  int num1, num2, ret;
      |                  ^~~
lq-calc.c: In function 'math_func':
lq-calc.c:49:6: warning: variable 'math_ret' set but not used [-Wunused-but-set-variable]
   49 |  int math_ret;
      |      ^~~~~~~~

Also, if you add -O1 optimization, the compiler will notice another problem:

Code:

lq-calc.c:73:13: warning: 'solution' may be used uninitialized in this function [-Wmaybe-uninitialized]
   73 |  math_ret = print_func(solution);
      |             ^~~~~~~~~~~~~~~~~~~~

jmgibson1981 · 01-07-2023, 11:45 PM

Was able to remove the math_ret and ret

If I comment out solution int then it fails to compile saying that it isn't aren't initialized.

Code:

$ gcc -Wall -O1 -o calc.e calc.c
calc.c: In function ‘math_func’:
calc.c:82:2: warning: ‘solution’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   82 |  printf("answer is %i\n", solution);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I've got lots to learn. thank you again for the assistance and information.

As it sits now.

https://gitlab.com/jmgibson1981/mycprogress

pan64 · 01-08-2023, 04:13 AM

this is much better, although other operations (illegal chars) are still not checked.
For example what will print: calc.e 1 q 5 ?
also the main function should return 0 only if everything was ok, otherwise something else (1).

ntubski · 01-08-2023, 10:32 AM

Quote:

Originally Posted by jmgibson1981

If I comment out solution int then it fails to compile saying that it isn't aren't initialized.

Quote:

Originally Posted by pan64

this is much better, although other operations (illegal chars) are still not checked.
For example what will print: calc.e 1 q 5 ?

Yes, these two things are related. Specifically, the problem with solution not being initialized will be triggered when the user passes an illegal operator (and none of the switch cases are chosen).

jmgibson1981 · 01-08-2023, 02:29 PM

Very good. Did those. Got an empty response on

Code:

gcc -Wall -o calc calc.c

Thank you. Have learned a lot from this one alone.

business_kid · 01-08-2023, 03:11 PM

Reading this thread makes me very glad I never pursued software. Hardware was bad enough.

ntubski · 01-08-2023, 04:52 PM

Quote:

Originally Posted by jmgibson1981

Very good. Did those. Got an empty response on

Code:

gcc -Wall -o calc calc.c

Hmm, I think that's only because gcc doesn't do the analysis needed to make that warning unless you ask for optimization; I still get the warning if I include -O1. The problem is that gcc doesn't analyse across functions, so it doesn't see what you handle the wrong operator in the upper level. In this case I think it would be better to check for a wrong operator in math_func, because you currently have two places where the valid operators are listed which means you can have bugs if they don't match.