Skip to main content
added 442 characters in body
Source Link
Corbin
  • 10.6k
  • 2
  • 31
  • 51

Jamal had a lot of good points, so just a few short things to add:


wordstogether is a confusing naming scheme. You should do one of the more standard naming schemes. In particular, either underscore_separator, or camelCase. It's also fairly common to use PascalCase for structs in C.


Rather than celsconv, I would state what the conversion is from and to. For example, fahrenheit_to_celsius and celsius_to_fahrenheit. Those make it immediately clear what the input is and what the output is. From the declaration, it's easy to see what they take, but their usage is more ambiguous (fahconv(deg) -- is deg Celsius? Kelvin?).


Since the performance of the switch is not a concern (it's not in a tight loop for example), I would be tempted to use tolower (from ctype.h) to simplify the switch to only have 1 case per character:

switch (tolower(choice)) {
    case 'c':
        /* celsius stuff */
        break;
    case 'f':
        /* ... */
        break;
    default:
        /* ... */
}

You should verify that all of the readings are successful. Otherwise, your later use of those variables are pretty dangerous/pointless. scanf returns the number of tokens successfully extracted, so the simplest way to verify it's success is to check number read == number expected.

Example:

double example;
if (scanf("%lf", &example) == 1) {
    /* success */
} else {
    /* failed :( */
}

Jamal had a lot of good points, so just a few short things to add:


wordstogether is a confusing naming scheme. You should do one of the more standard naming schemes. In particular, either underscore_separator, or camelCase. It's also fairly common to use PascalCase for structs in C.


Rather than celsconv, I would state what the conversion is from and to. For example, fahrenheit_to_celsius and celsius_to_fahrenheit. Those make it immediately clear what the input is and what the output is. From the declaration, it's easy to see what they take, but their usage is more ambiguous (fahconv(deg) -- is deg Celsius? Kelvin?).


Since the performance of the switch is not a concern (it's not in a tight loop for example), I would be tempted to use tolower (from ctype.h) to simplify the switch to only have 1 case per character:

switch (tolower(choice)) {
    case 'c':
        /* celsius stuff */
        break;
    case 'f':
        /* ... */
        break;
    default:
        /* ... */
}

Jamal had a lot of good points, so just a few short things to add:


wordstogether is a confusing naming scheme. You should do one of the more standard naming schemes. In particular, either underscore_separator, or camelCase. It's also fairly common to use PascalCase for structs in C.


Rather than celsconv, I would state what the conversion is from and to. For example, fahrenheit_to_celsius and celsius_to_fahrenheit. Those make it immediately clear what the input is and what the output is. From the declaration, it's easy to see what they take, but their usage is more ambiguous (fahconv(deg) -- is deg Celsius? Kelvin?).


Since the performance of the switch is not a concern (it's not in a tight loop for example), I would be tempted to use tolower (from ctype.h) to simplify the switch to only have 1 case per character:

switch (tolower(choice)) {
    case 'c':
        /* celsius stuff */
        break;
    case 'f':
        /* ... */
        break;
    default:
        /* ... */
}

You should verify that all of the readings are successful. Otherwise, your later use of those variables are pretty dangerous/pointless. scanf returns the number of tokens successfully extracted, so the simplest way to verify it's success is to check number read == number expected.

Example:

double example;
if (scanf("%lf", &example) == 1) {
    /* success */
} else {
    /* failed :( */
}
Source Link
Corbin
  • 10.6k
  • 2
  • 31
  • 51

Jamal had a lot of good points, so just a few short things to add:


wordstogether is a confusing naming scheme. You should do one of the more standard naming schemes. In particular, either underscore_separator, or camelCase. It's also fairly common to use PascalCase for structs in C.


Rather than celsconv, I would state what the conversion is from and to. For example, fahrenheit_to_celsius and celsius_to_fahrenheit. Those make it immediately clear what the input is and what the output is. From the declaration, it's easy to see what they take, but their usage is more ambiguous (fahconv(deg) -- is deg Celsius? Kelvin?).


Since the performance of the switch is not a concern (it's not in a tight loop for example), I would be tempted to use tolower (from ctype.h) to simplify the switch to only have 1 case per character:

switch (tolower(choice)) {
    case 'c':
        /* celsius stuff */
        break;
    case 'f':
        /* ... */
        break;
    default:
        /* ... */
}