Skip to main content
Code Review

Return to Answer

added 964 characters in body
Source Link
Toby Speight
Toby Speight
  • 81.7k
  • 14
  • 101
  • 308

Use the right type - strlen() returns a size_t. But we don't need to measure the string anyway, as we're walking it character by character - we should loop like a native with for (const char *s = string; *s; ++) (or just changeuse the loop condition to finish when we seepassed string pointer here. Code that does unnecessary string indexing looks like it's written by a null characterFORTRAN or Java programmer who is still uncomfortable with pointers.

And don't forget to examine its return value before relying on string containing anything valid.

Well, that's not going to work, because str2num_C() will error on the newline character that gets() read.

if(err>0){
    printf("ERROR: %s",str2numErrText(err));
}
else{
    printf("Number + 2 is: %f",num);

Please write full lines of output (ending in \n). It's very bad behaviour to terminate with a partial line written.

And error messages ought to be printed to stderr, not stdout.

My overall assessment is that this reinvention of the bufferwheel is pointless anyway. It's much simpler to wrap standard parsers such as strtof() to fit the interface you need than to write this stuff from scratch. It's likely to be more efficient, too - although you'll want to run some actual benchmarks to determine that.

Use the right type - strlen() returns a size_t. But we don't need to measure the string anyway, as we're walking it character by character - just change the loop condition to finish when we see a null character.

Well, that's not going to work, because str2num_C() will error on the newline character in the buffer.

Use the right type - strlen() returns a size_t. But we don't need to measure the string anyway, as we're walking it character by character - we should loop like a native with for (const char *s = string; *s; ++) (or just use the passed string pointer here. Code that does unnecessary string indexing looks like it's written by a FORTRAN or Java programmer who is still uncomfortable with pointers.

And don't forget to examine its return value before relying on string containing anything valid.

Well, that's not going to work, because str2num_C() will error on the newline character that gets() read.

if(err>0){
    printf("ERROR: %s",str2numErrText(err));
}
else{
    printf("Number + 2 is: %f",num);

Please write full lines of output (ending in \n). It's very bad behaviour to terminate with a partial line written.

And error messages ought to be printed to stderr, not stdout.

My overall assessment is that this reinvention of the wheel is pointless anyway. It's much simpler to wrap standard parsers such as strtof() to fit the interface you need than to write this stuff from scratch. It's likely to be more efficient, too - although you'll want to run some actual benchmarks to determine that.

added 106 characters in body
Source Link
Toby Speight
Toby Speight
  • 81.7k
  • 14
  • 101
  • 308

Firstly, this code doesn't do what is described. It converts a string to float, not integer.

Your header file should contain only the public declarations. Put the implementation into a normal source file.

Your header file should contain only the public declarations. Put the implementation into a normal source file.

Firstly, this code doesn't do what is described. It converts a string to float, not integer.

Your header file should contain only the public declarations. Put the implementation into a normal source file.

Source Link
Toby Speight
Toby Speight
  • 81.7k
  • 14
  • 101
  • 308

Your header file should contain only the public declarations. Put the implementation into a normal source file.

int StrNumErr = 0;

Avoid global variables - they make it hard to reason about code, and can inhibit concurrent use of the functions.

int str2numErrCheck(){

Should be declared as taking no arguments, i.e. int str2numErrCheck(void). And we're not allowed to give it a name beginning with str - it needs to be changed to one that's not reserved for <string.h>.

    if(StrNumErr>0){
        int ab = StrNumErr;
        StrNumErr = 0;
        return ab;
    }else{
        return 0;
    }
}

As we never set StrNumErr to a negative value, and both branches have the same observable behaviour for StrNumErr==0, then we don't need the test. We could do with a better variable name than ab, too.

char * __restrict__ str2numErrText(int errnum){

The __restrict__ doesn't provide any value here.

if(errnum==22){
    return "Alphabets or Symbols present in the String";
}

What's the significance of 22? Why aren't we using named (enum) constants?

We shouldn't be returning const char* from a function declared to return char *. I suggest changing the function to return const char* instead.

void invalid_argument(const char * __restrict__ _err,int errcode)
{
    StrNumErr = errcode;
}

Again, no benefit from __restrict__ here. Avoid declaring identifiers beginning with _ (since that's easier than remembering exactly which ones are safe, and where) - and why are we ignoring _err anyway? That leads to inconsistency between what the caller provides and what we return from str2numErrText().

float power(float a,int b)

This function should be declared with static linkage, unless you really intend it to be part of the public interface.

The usual floating-point type is double. Calculating with float sacrifices a lot of precision.

And calculating powers by repeated multiplication is inefficient compared to binary exponentiation.

I don't see any benefit to using this function rather than the standard pow() from <math.h>.

    int _strlen = strlen(_string);

Use the right type - strlen() returns a size_t. But we don't need to measure the string anyway, as we're walking it character by character - just change the loop condition to finish when we see a null character.

float str2num_C(const char * __restrict__ _string)

Another reserved identifier, and another pointless __restrict__.

    else if(_curchar=='0'){
        //if char is 0!
        if(!(_numfound==1)){
            _numfound=1;
            _return_num = 0;
        }
        else if(_point==1){
            _return_num+= 0/(power(10,_pointplace));
            _pointplace+=1;
        }
        else{
            _return_num*=10;
            _return_num+=0;
        }
    }
    else if(_curchar=='1'){
        //if char is 1!
        if(!(_numfound==1)){
            _numfound=1;
            _return_num = 1;
        }
        else if(_point==1){
            _return_num+= 1/(power(10,_pointplace));
            _pointplace+=1;
        }
        else{
            _return_num*=10;
            _return_num+=1;
        }
    }

These branches can all be combined, using the fact that C guarantees that the encoding of digits 0...9 are contiguous (so curchar - '0' gives the integer value of the digit).

Instead of computing power() each time around, it might be worth keeping that value in a variable, and scaling it by 10 each time it's used:

double place_value = 1;
⋮

for (…) {
    ⋮
    if (curchar >= '0' && curchar <= '9') {
        int digit = curchar - '0';
        if (!numfound) { numfound = 1; }
        if (point) {
            place_value /= 10;
        }
        return_num = return_num * 10 + digit;
    }
    ⋮
}
return return_num * minus * place_value;

            invalid_argument("The string contains alphabetic charachters or symbol",22);

Check your spelling (characters). We might have spotted this if we'd actually used the value.

    //declaring variables
    char string[20];
    gets(string);

That's probably the most useless comment I've seen today.

What if 20 characters or more are entered? Eschew gets() - that's the most dangerous function in the Standard Library (it's impossible to use it safely). Use fgets():

fgets(string, sizeof string, stdin);

    num = str2num_C(string);

Well, that's not going to work, because str2num_C() will error on the newline character in the buffer.