LinuxQuestions.org
Visit Jeremy's Blog.
Go Back   LinuxQuestions.org > Forums > Non-*NIX Forums > Programming
User Name
Password
Programming This forum is for all programming questions.
The question does not have to be directly related to Linux and any language is fair game.

Notices


Reply
  Search this Thread
Old 01-06-2023, 06:21 PM   #1
jmgibson1981
Member
 
Registered: Jun 2015
Location: Tucson, AZ USA
Distribution: Debian
Posts: 952

Rep: Reputation: 325Reputation: 325Reputation: 325Reputation: 325
Looking for critique. Teaching myself C.


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.
 
Old 01-06-2023, 10:33 PM   #2
EdGr
Member
 
Registered: Dec 2010
Location: California, USA
Distribution: I run my own OS
Posts: 912

Rep: Reputation: 436Reputation: 436Reputation: 436Reputation: 436Reputation: 436
That link leads to an endless spinner.
Ed
 
Old 01-06-2023, 11:36 PM   #3
ntubski
Senior Member
 
Registered: Nov 2005
Distribution: Debian, Arch
Posts: 3,683

Rep: Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008
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 View Post
That link leads to an endless spinner.
You have JavaScript disabled? Try https://gitlab.com/jmgibson1981/mycp...culator/calc.c
 
Old 01-07-2023, 11:26 AM   #4
EdGr
Member
 
Registered: Dec 2010
Location: California, USA
Distribution: I run my own OS
Posts: 912

Rep: Reputation: 436Reputation: 436Reputation: 436Reputation: 436Reputation: 436
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
 
Old 01-07-2023, 11:45 AM   #5
pan64
LQ Addict
 
Registered: Mar 2012
Location: Hungary
Distribution: debian/ubuntu/suse ...
Posts: 19,594

Rep: Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629
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.
 
Old 01-07-2023, 12:30 PM   #6
rtmistler
Moderator
 
Registered: Mar 2011
Location: USA
Distribution: MINT Debian, Angstrom, SUSE, Ubuntu, Debian
Posts: 9,760
Blog Entries: 13

Rep: Reputation: 4803Reputation: 4803Reputation: 4803Reputation: 4803Reputation: 4803Reputation: 4803Reputation: 4803Reputation: 4803Reputation: 4803Reputation: 4803Reputation: 4803
Use real, meaningful variable names.

Do not combine multiple lines of code in one physical line.
 
Old 01-07-2023, 01:28 PM   #7
sundialsvcs
LQ Guru
 
Registered: Feb 2004
Location: SE Tennessee, USA
Distribution: Gentoo, LFS
Posts: 10,101
Blog Entries: 4

Rep: Reputation: 3662Reputation: 3662Reputation: 3662Reputation: 3662Reputation: 3662Reputation: 3662Reputation: 3662Reputation: 3662Reputation: 3662Reputation: 3662Reputation: 3662
"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.

Last edited by sundialsvcs; 01-07-2023 at 01:31 PM.
 
Old 01-07-2023, 07:54 PM   #8
jmgibson1981
Member
 
Registered: Jun 2015
Location: Tucson, AZ USA
Distribution: Debian
Posts: 952

Original Poster
Rep: Reputation: 325Reputation: 325Reputation: 325Reputation: 325
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
 
Old 01-07-2023, 11:12 PM   #9
ntubski
Senior Member
 
Registered: Nov 2005
Distribution: Debian, Arch
Posts: 3,683

Rep: Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008
Quote:
Originally Posted by jmgibson1981 View Post
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);
      |             ^~~~~~~~~~~~~~~~~~~~
 
Old 01-07-2023, 11:45 PM   #10
jmgibson1981
Member
 
Registered: Jun 2015
Location: Tucson, AZ USA
Distribution: Debian
Posts: 952

Original Poster
Rep: Reputation: 325Reputation: 325Reputation: 325Reputation: 325
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

Last edited by jmgibson1981; 01-07-2023 at 11:52 PM.
 
Old 01-08-2023, 04:13 AM   #11
pan64
LQ Addict
 
Registered: Mar 2012
Location: Hungary
Distribution: debian/ubuntu/suse ...
Posts: 19,594

Rep: Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629Reputation: 6629
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).
 
Old 01-08-2023, 10:32 AM   #12
ntubski
Senior Member
 
Registered: Nov 2005
Distribution: Debian, Arch
Posts: 3,683

Rep: Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008
Quote:
Originally Posted by jmgibson1981 View Post
If I comment out solution int then it fails to compile saying that it isn't aren't initialized.
Quote:
Originally Posted by pan64 View Post
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).
 
Old 01-08-2023, 02:29 PM   #13
jmgibson1981
Member
 
Registered: Jun 2015
Location: Tucson, AZ USA
Distribution: Debian
Posts: 952

Original Poster
Rep: Reputation: 325Reputation: 325Reputation: 325Reputation: 325
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.
 
Old 01-08-2023, 03:11 PM   #14
business_kid
LQ Guru
 
Registered: Jan 2006
Location: Ireland
Distribution: Slackware, Slarm64 & Android
Posts: 14,719

Rep: Reputation: 2029Reputation: 2029Reputation: 2029Reputation: 2029Reputation: 2029Reputation: 2029Reputation: 2029Reputation: 2029Reputation: 2029Reputation: 2029Reputation: 2029
Reading this thread makes me very glad I never pursued software. Hardware was bad enough.
 
Old 01-08-2023, 04:52 PM   #15
ntubski
Senior Member
 
Registered: Nov 2005
Distribution: Debian, Arch
Posts: 3,683

Rep: Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008Reputation: 2008
Quote:
Originally Posted by jmgibson1981 View Post
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.
 
  


Reply


Thread Tools Search this Thread
Search this Thread:

Advanced Search

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is Off
HTML code is Off



Similar Threads
Thread Thread Starter Forum Replies Last Post
Building a new computer, looking for suggestions and build critique largedon Linux - Hardware 17 03-20-2013 02:03 PM
Teaching myself C++ Amdx2_x64 Programming 28 07-21-2008 02:08 PM
Teaching myself Linux. Hit a road block dcj1978 Programming 5 08-06-2007 06:33 AM
teaching active directory w/o teaching network concept (possible???) Tafta General 4 01-21-2004 07:12 PM
teaching myself perl -- need feedback farhanali Programming 2 06-28-2003 12:36 PM

LinuxQuestions.org > Forums > Non-*NIX Forums > Programming

All times are GMT -5. The time now is 11:03 PM.

Main Menu
Advertisement
My LQ
Write for LQ
LinuxQuestions.org is looking for people interested in writing Editorials, Articles, Reviews, and more. If you'd like to contribute content, let us know.
Main Menu
Syndicate
RSS1  Latest Threads
RSS1  LQ News
Twitter: @linuxquestions
Open Source Consulting | Domain Registration