C programming beginner | Feedback on programs would be much apreciated

pxavier · 2012-04-23 00:19:04

Hello,

I am taking a free c programming course ( CS50 OpenCourseWare, Introduction to Computer Science 1 | http://cs50.tv/2011/fall/ ).

I did not realize that writing code was so much complicated. I find the course very challenging but also (and more importantly) "extremely" interesting :-)
However, at the present time the idea that I could even in a "really" remote future, become "programming-literate" (I hope it is a real word) :-) seems to be an utopia.
Attempting to reach a preliminary understanding which extends beyond the appearances of complexity is... disturbing.

Anyway, could it be possible to receive some feedback on the two versions of a program ( credit.c ) that I have written for the problem set 1 ?

Here is the program specification: http://ideone.com/UOjA3
the first version of credit.c : http://ideone.com/nT0xI
the second (and I hope improved) version: http://ideone.com/nT0xI

P.S. By the way, I have been using arch for a little bit more than a year ... NICE :-)
Thanks to all of you that make it possible.

ewaller · 2012-04-23 00:49:11

Moderator comment:
pxavier,
Welcome to the forums. I am obliged to point out our policy on homework.

I thank you for being candid that it is classwork. I will leave this thread open, for now. I ask that anyone helping do so in the role of a teacher, not a peer. I don't mind a bit of Socratic method to point you in the right direction. The moderators will shut this down if others start showing you how to do it.

Last edited by ewaller (2012-04-23 01:09:25)

ewaller · 2012-04-23 00:55:40

A couple comments:
First, all of your functions (with the exception of main) are void. I see that your functions report their results to the console, but do you think they might be more useful if they were to return information the routine that called them? Just because there is a return value, the called function can choose to just ignore it.
Second, I would rather not see "Magic Numbers" hard coded into case statements. I prefer to define the magic numbers in one place (in a header file, or at least, at the top of main.c)

edit: Third comment. How much do you trust the person entering the card data? Can they do something evil to you?

Last edited by ewaller (2012-04-23 01:07:29)

Blµb · 2012-04-23 08:31:05

--------------
NOTE to ewaller: I hope this is okay? I'm not really "correcting" anything (there are just 2 improvements), or really adding anything, it's more about how people generally work with C.
--------------

Okay, here you go, some general comments:

A note:

char temp_credit_card_number[17];

The 17 bytes here include the terminating null byte. So, when you later conver the number to a string,
and the number is 17 digits, the 17th digit will not be in this string, and strlen() will return 16.
But you recognize 16 as the correct length for VISA.
I know you have some other checks in there, but you should allow this check to fail as well, and use at least 18, or even more.

A change:

- snprintf(temp_credit_card_number, 17, "%lld", credit_card_number);
+ snprintf(temp_credit_card_number, sizeof(temp_credit_card_number), "%lld", credit_card_number);

Description:
sizeof: Gets the size of a type in bytes. (Note while this works well for an array, you cannot generally use it for pointers, since pointers have their own size independent from the data they're pointing to)
Reason:
If you always use sizeof, you can change the size of temp_credit_card_number at its definition without having
to worry about the places you're using it at.

A note

// "credit_card_number" converted to a string in order to determine its length
snprintf(temp_credit_card_number, 17, "%lld", credit_card_number);
int credit_card_number_length = strlen(temp_credit_card_number);

This works, however, you can actually skip the conversion, and simply count how often you can divide by 10 before the number becomes 0: count = 0; for (temp = cardnumber; temp; temp /= 10) ++count;
You then don't need a buffer, don't need to worry about passing the right size to snprintf, and don't need to actually go through printf's formatting algorithm.
(Mathematically speaking you could also use the logarithm to find out the length. However, you can get precision problems with such really high numbers since the logarithm functions use floating point numbers as parameters. In this case it might work, but in programming it is often wise to strictly seperate these things.)

And another change:
You do this a few times, consider the replacement:

- printf("%s\n", card_type[3]);
+ printf("%s\n", card_type[INVALID]);

A last note: 'INVALID' is quite a general name. In C, we tend to prefix constants and variables based on their purpose, since C has no namespaces. If at some point you add something else that can be INVALID, its INVALID could have a different number.
So you could instead do something like: enum credit_card_type { CREDIT_CARD_AMEX, ..., CREDIT_CARD_INVALID };

Last edited by Blµb (2012-04-23 08:35:37)

stqn · 2012-04-23 08:40:05

Nothing terribly wrong at first sight. A few details that don’t matter in such a program.

                        } else if ((mastercard_two_first_digits == 51)
                                   || (mastercard_two_first_digits == 52)
                                   || (mastercard_two_first_digits == 53)
                                   || (mastercard_two_first_digits == 54)
                                   || (mastercard_two_first_digits == 55)) {

This could be shorter.

Edit: “type” should probably not be a global variable. It doesn’t need to be one and it’s easy to make it local.

Last edited by stqn (2012-04-23 08:46:37)

pxavier · 2012-04-23 16:48:15

ewaller wrote:

Moderator comment:
pxavier,
Welcome to the forums. I am obliged to point out our policy on homework.
I thank you for being candid that it is classwork. I will leave this thread open, for now. I ask that anyone helping do so in the role of a teacher, not a peer. I don't mind a bit of Socratic method to point you in the right direction. The moderators will shut this down if others start showing you how to do it.

ewaller, Thanks for directing me to the homework section of the forum etiquette and for not shutting this thread down .
I do apologize for not having checked it before posting. Could I have some clarifications regarding the policy ?

The course - cs50 dot tv - is an initiative of Harvard university to make the content of its computer science introductory course - cs50 dot net -
and few others, freely available to anyone who might be interested. Videos of the lectures and sections (recitations), lecture notes, slides, source codes used during the lectures, problem sets and quizzes can be accessed from the cs50.tv website :-). A google group has even been set up which provides a place to turn to when faced with technical difficulties (the actual course instructor takes the time to offer some help).

Be that as it may, in order to receive feedback on "completed" problem sets, one need to "effectively" take the course (enrolling at Harvard
and take the course on-site or online). By posting my completed problem set on the cs50 google group, I only get feedback from people
who, like me, are in the process of learning the c programming language.

Quote from the cs50 google group (someone seeking feedback):
<< - That means if i "post" my code in a mail, your team will evaluate and
give me feedback?

- that depends, but I'm going to say "usually not"
David and Glenn occasionally give clues to difficult problems (stuff like pointers and strange bugs you may have),
I don't think I've ever seen them look at someone's code and say "well commented, fast, well thought out, good logic"
if you want that from them, then you'll have to pay for the class.
[..]
There are other people on here who occasionally give feedback to code, but this is generally a "teach yourself" group.
I would say the most helpful thing is to make your code look like the sample code in the pdfs that go with every week's lectures. >>

I am not asking for solutions to the problem sets, had I been a Harvard student, this is ( unfortunately ) what I would have submitted.
I just wish to receive some comments, suggestions and criticisms which will allow me to correct the stupid things that I usually do.
The "Socratic method" is absolutely fine by me

In that context, would it be ok for me to keep on posting "completed" problem sets in the future ?

I also have to apologize regarding the links to the source codes that I have provided, they both are the same and point to the older version (credit.c),
here are the correct links:

credit.c (old version) --> http://ideone.com/nT0xI
new_credit.c (new version) --> http://ideone.com/gUDcm

ewaller wrote:

A couple comments:
First, all of your functions (with the exception of main) are void. I see that your functions report their results to the console, but do you think they might be more useful if they were to return information the routine that called them? Just because there is a return value, the called function can choose to just ignore it.

In the new version, I tried to make functions which use the return values of other functions as parameters.

ewaller wrote:

Second, I would rather not see "Magic Numbers" hard coded into case statements. I prefer to define the magic numbers in one place (in a header file, or at least, at the top of main.c)

I also tried to avoid "Magic Numbers" by using #define, however I did not think about doing so into the case statements, thanks.

ewaller wrote:

edit: Third comment. How much do you trust the person entering the card data? Can they do something evil to you?

Regarding checking users inputs, the GetLongLong() function (CS50 library - cs50.h), takes care of it for us. However in the new version I have not
used <cs50.h>, I tried to verify the input myself... It looks very shaky to me

Blµb wrote:

A note:
char temp_credit_card_number[17];
The 17 bytes here include the terminating null byte. So, when you later conver the number to a string,
and the number is 17 digits, the 17th digit will not be in this string, and strlen() will return 16.
But you recognize 16 as the correct length for VISA.
I know you have some other checks in there, but you should allow this check to fail as well, and use at least 18, or even more.

I understand, thanks :-)

Blµb wrote:

A change:
- snprintf(temp_credit_card_number, 17, "%lld", credit_card_number);
+ snprintf(temp_credit_card_number, sizeof(temp_credit_card_number), "%lld", credit_card_number);
Description:
sizeof: Gets the size of a type in bytes. (Note while this works well for an array, you cannot generally use it for pointers, since pointers have their own size independent from the data they're pointing to)
Reason:
If you always use sizeof, you can change the size of temp_credit_card_number at its definition without having
to worry about the places you're using it at.

Should I also use sizeof in the new version http://ideone.com/gUDcm at line 51, instead of BUFFER_SIZE ? What about line 53 ?
In such cases what are the pros and cons of using either sizeof or #define BUFFER_SIZE ?

Blµb wrote:

A note
// "credit_card_number" converted to a string in order to determine its length
snprintf(temp_credit_card_number, 17, "%lld", credit_card_number);
int credit_card_number_length = strlen(temp_credit_card_number);
This works, however, you can actually skip the conversion, and simply count how often you can divide by 10 before the number becomes 0: count = 0; for (temp = cardnumber; temp; temp /= 10) ++count;
You then don't need a buffer, don't need to worry about passing the right size to snprintf, and don't need to actually go through printf's formatting algorithm.

That's what I have done in the new version.

Blµb wrote:

And another change:
You do this a few times, consider the replacement:
- printf("%s\n", card_type[3]);
+ printf("%s\n", card_type[INVALID]);

I do need to think about that one...

Blµb wrote:

A last note: 'INVALID' is quite a general name. In C, we tend to prefix constants and variables based on their purpose, since C has no namespaces. If at some point you add something else that can be INVALID, its INVALID could have a different number.
So you could instead do something like: enum credit_card_type { CREDIT_CARD_AMEX, ..., CREDIT_CARD_INVALID };

Thanks, I will.

stqn wrote:

                        } else if ((mastercard_two_first_digits == 51)
                                   || (mastercard_two_first_digits == 52)
                                   || (mastercard_two_first_digits == 53)
                                   || (mastercard_two_first_digits == 54)
                                   || (mastercard_two_first_digits == 55)) {

This could be shorter.

Edit: “type” should probably not be a global variable. It doesn’t need to be one and it’s easy to make it local.

Ok, I will try to shorten the elseif statement and make "type" a local variable". thanks

Blµb · 2012-04-26 09:21:07

pxavier wrote:

(...)
Blµb wrote:
A change:
- snprintf(temp_credit_card_number, 17, "%lld", credit_card_number);
+ snprintf(temp_credit_card_number, sizeof(temp_credit_card_number), "%lld", credit_card_number);
Description:
sizeof: Gets the size of a type in bytes. (Note while this works well for an array, you cannot generally use it for pointers, since pointers have their own size independent from the data they're pointing to)
Reason:
If you always use sizeof, you can change the size of temp_credit_card_number at its definition without having
to worry about the places you're using it at.
Should I also use sizeof in the new version http://ideone.com/gUDcm at line 51, instead of BUFFER_SIZE ? What about line 53 ?
In such cases what are the pros and cons of using either sizeof or #define BUFFER_SIZE ?

I'd say it's partially your personal preference.
Personally I'd stick to sizeof in expressions, and BUFFER_SIZE in definitions. Mainly because when your codebase grows, it'll get harder to find all expressions using BUFFER_SIZE should you ever need to change them.

An example would be if you define several buffers in your program, then you can use BUFFER_SIZE in each definition.
In theory, you could also do something like:

char foo[sizeof(other_foo)];

But that would rely on other_foo being available in that file. I would use sizeof there if it makes sense "semantically" that it has the same size, or if other_foo doesn't come from your code, but from an external library.

So, yes, in lines 51 and 53, I would suggest using sizeof for its flexibility.

Arch Linux

#1 2012-04-23 00:19:04

C programming beginner | Feedback on programs would be much apreciated

#2 2012-04-23 00:49:11

Re: C programming beginner | Feedback on programs would be much apreciated

#3 2012-04-23 00:55:40

Re: C programming beginner | Feedback on programs would be much apreciated

#4 2012-04-23 08:31:05

Re: C programming beginner | Feedback on programs would be much apreciated

#5 2012-04-23 08:40:05

Re: C programming beginner | Feedback on programs would be much apreciated

#6 2012-04-23 16:48:15

Re: C programming beginner | Feedback on programs would be much apreciated

#7 2012-04-26 09:21:07

Re: C programming beginner | Feedback on programs would be much apreciated

Board footer