Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

I advised you that it is called main and not everything it is called main and not everything, and you did a good job of moving code out of main. Now main looks a lot closer to what I'd expect to see.

But, we still have functions that are doing too much. Plus, we've made a bad problem worse. We've replaced doing everything in the main function with global variables. And we don't want to do that.


One indication that we have functions that are doing too much is that our functions don't have very great names and there's not a good name to give to them.

For example, void input(). It's not very clear what exactly this function does (from name alone), and we can't really give it a more clear name without it being miles long (and filled with ands... "and" and "or" in function names is generally a code smell). From the name alone, we'd guess that this function gets input from the user. But what does it do with that input? Who knows? Because this returns void and it doesn't take anything by reference to return either.

So let me repeat the method from earlier more clearly (this you can and should copy-and-paste this exactly and use it):

int getUserInput(const char * prompt) {
 int input;
 printf(prompt);
 scanf("%d", input);
 return input;
}

Do not mistake this. You're repeatedly getting integer input from the user. This is a clear indication that you should have a function for doing this. If you don't have this in your next post, you're doing something very wrong.

The section in which we gather inputs from the user should look basically exactly like this:

int denominationCount = getUserInput("How many different denominations? ");
for (int i = 0; i < denominationCount; ++i) {
 denominations[i] = getUserInput("Enter the next denomination: ");
}
int makingChangeFor = getUserInput("How much are we making change for? ");

And now you've got all the values for which to do the calculation. You can feed them to the function that does the calculation and get an output from that. And you don't need the global variables.

Until all of your functions that are accessing the global variables are both taking and returning arguments, you shouldn't be comfortable using the global variables (as a rough guideline). Global variables are almost always unnecessary. Global variables are the path to rats nests, madness, and lunacy.

I advised you that it is called main and not everything, and you did a good job of moving code out of main. Now main looks a lot closer to what I'd expect to see.

But, we still have functions that are doing too much. Plus, we've made a bad problem worse. We've replaced doing everything in the main function with global variables. And we don't want to do that.


One indication that we have functions that are doing too much is that our functions don't have very great names and there's not a good name to give to them.

For example, void input(). It's not very clear what exactly this function does (from name alone), and we can't really give it a more clear name without it being miles long (and filled with ands... "and" and "or" in function names is generally a code smell). From the name alone, we'd guess that this function gets input from the user. But what does it do with that input? Who knows? Because this returns void and it doesn't take anything by reference to return either.

So let me repeat the method from earlier more clearly (this you can and should copy-and-paste this exactly and use it):

int getUserInput(const char * prompt) {
 int input;
 printf(prompt);
 scanf("%d", input);
 return input;
}

Do not mistake this. You're repeatedly getting integer input from the user. This is a clear indication that you should have a function for doing this. If you don't have this in your next post, you're doing something very wrong.

The section in which we gather inputs from the user should look basically exactly like this:

int denominationCount = getUserInput("How many different denominations? ");
for (int i = 0; i < denominationCount; ++i) {
 denominations[i] = getUserInput("Enter the next denomination: ");
}
int makingChangeFor = getUserInput("How much are we making change for? ");

And now you've got all the values for which to do the calculation. You can feed them to the function that does the calculation and get an output from that. And you don't need the global variables.

Until all of your functions that are accessing the global variables are both taking and returning arguments, you shouldn't be comfortable using the global variables (as a rough guideline). Global variables are almost always unnecessary. Global variables are the path to rats nests, madness, and lunacy.

I advised you that it is called main and not everything, and you did a good job of moving code out of main. Now main looks a lot closer to what I'd expect to see.

But, we still have functions that are doing too much. Plus, we've made a bad problem worse. We've replaced doing everything in the main function with global variables. And we don't want to do that.


One indication that we have functions that are doing too much is that our functions don't have very great names and there's not a good name to give to them.

For example, void input(). It's not very clear what exactly this function does (from name alone), and we can't really give it a more clear name without it being miles long (and filled with ands... "and" and "or" in function names is generally a code smell). From the name alone, we'd guess that this function gets input from the user. But what does it do with that input? Who knows? Because this returns void and it doesn't take anything by reference to return either.

So let me repeat the method from earlier more clearly (this you can and should copy-and-paste this exactly and use it):

int getUserInput(const char * prompt) {
 int input;
 printf(prompt);
 scanf("%d", input);
 return input;
}

Do not mistake this. You're repeatedly getting integer input from the user. This is a clear indication that you should have a function for doing this. If you don't have this in your next post, you're doing something very wrong.

The section in which we gather inputs from the user should look basically exactly like this:

int denominationCount = getUserInput("How many different denominations? ");
for (int i = 0; i < denominationCount; ++i) {
 denominations[i] = getUserInput("Enter the next denomination: ");
}
int makingChangeFor = getUserInput("How much are we making change for? ");

And now you've got all the values for which to do the calculation. You can feed them to the function that does the calculation and get an output from that. And you don't need the global variables.

Until all of your functions that are accessing the global variables are both taking and returning arguments, you shouldn't be comfortable using the global variables (as a rough guideline). Global variables are almost always unnecessary. Global variables are the path to rats nests, madness, and lunacy.

Source Link
nhgrif
  • 25.4k
  • 3
  • 64
  • 129

I advised you that it is called main and not everything, and you did a good job of moving code out of main. Now main looks a lot closer to what I'd expect to see.

But, we still have functions that are doing too much. Plus, we've made a bad problem worse. We've replaced doing everything in the main function with global variables. And we don't want to do that.


One indication that we have functions that are doing too much is that our functions don't have very great names and there's not a good name to give to them.

For example, void input(). It's not very clear what exactly this function does (from name alone), and we can't really give it a more clear name without it being miles long (and filled with ands... "and" and "or" in function names is generally a code smell). From the name alone, we'd guess that this function gets input from the user. But what does it do with that input? Who knows? Because this returns void and it doesn't take anything by reference to return either.

So let me repeat the method from earlier more clearly (this you can and should copy-and-paste this exactly and use it):

int getUserInput(const char * prompt) {
 int input;
 printf(prompt);
 scanf("%d", input);
 return input;
}

Do not mistake this. You're repeatedly getting integer input from the user. This is a clear indication that you should have a function for doing this. If you don't have this in your next post, you're doing something very wrong.

The section in which we gather inputs from the user should look basically exactly like this:

int denominationCount = getUserInput("How many different denominations? ");
for (int i = 0; i < denominationCount; ++i) {
 denominations[i] = getUserInput("Enter the next denomination: ");
}
int makingChangeFor = getUserInput("How much are we making change for? ");

And now you've got all the values for which to do the calculation. You can feed them to the function that does the calculation and get an output from that. And you don't need the global variables.

Until all of your functions that are accessing the global variables are both taking and returning arguments, you shouldn't be comfortable using the global variables (as a rough guideline). Global variables are almost always unnecessary. Global variables are the path to rats nests, madness, and lunacy.

lang-c

AltStyle によって変換されたページ (->オリジナル) /