Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite
2

It's an interview question, I was asked to compare 2 strings, and the number in the string are evaluated to integers when comparing.

I can use C/C++ and can not use stl or stdlib. Some test cases are showed in the code.

I submitted the code below but failed. The interviewer said that the code is not clean enough. Can anybody review the code?

#include <cstdio>
#include <cmath>
bool is_num(char x) {return (x>='0' && x<='9');}

int get_num(const char* s, int len)
{
    int j = 0;
    int ret = 0;
    while (len > 0)
    {
        ret += (s[j++] - '0') * pow(10, len-1);
        len --;
    }
    return ret;
}


int my_compare(const char* s1, const char* s2)
{
    while((*s1)&&(*s2))
    {
        int value1 = 0;
        int value2 = 0;
        bool is_t1_num = false;
        bool is_t2_num = false;
        int len_t1 = 0;
        int len_t2 = 0;
        const char*  t1;
        const char*  t2;
        if (is_num(*s1))
        {
            t1 = s1;

            while(is_num(*t1)&&(*t1))
            {
                len_t1 ++;
                t1++;
            }
            value1 = get_num(s1, len_t1);
            is_t1_num = true;
        }
        if (is_num(*s2))
        {
            t2 = s2;
            while(is_num(*t2)&&(*t2))
            {
                len_t2 ++;
                t2++;
            }
            value2 = get_num(s2, len_t2);
            is_t2_num = true;            
        }

        if ((!is_t1_num) && (!is_t2_num))
        {
            if ((*s1) < (*s2)) return -1;
            if ((*s1) > (*s2)) return 1;
            s1++;
            s2++;
        }

        if ((is_t1_num) && (is_t2_num))
        {
            if (value1 < value2) return -1;
            if (value1 > value2) return 1;
            s1 += len_t1;
            s2 += len_t2;
        }

        if ((is_t1_num) && (!is_t2_num))
        {
            if (value1>0) return 1;
            s1 += len_t1;
        }

        if ((!is_t1_num) && (is_t2_num))
        {
            if (value2>0) return -1;
            s2 += len_t2;
        }
    }//end while
    if (!(*s1) && !(*s2)) return 0;
    if (!(*s1)) return -1;
    if (!(*s2)) return 1;
}


int main()
{
    //TEST CASE 0
    printf("%d\n", my_compare("ab", "1"));
    printf("%d\n", my_compare("123", "456"));
    printf("%d\n", my_compare("abc", "def"));
    printf("%d\n", my_compare("abc123def", "abc000def"));
    printf("%d\n", my_compare("ab", "abc"));

    //TEST CASE 1
    printf("%d\n", my_compare("abc000def", "abcdef"));
    printf("%d\n", my_compare("abc101def", "abcdef"));

    //TEST CASE 2
    printf("%d\n", my_compare("abc101def", "abceef"));
}
share|improve this question
The requirements are not really clear: from the code it seems that the return value of the function is the value stored in the strings if both strings are the same. What about when they differ, what should be the returned value? Additionally, the requirements don't mention whether the number must be positive, what if the input is "-1"? – David Rodríguez - dribeas Aug 15 '12 at 18:41

1 Answer

up vote 3 down vote accepted

First pick a language.

Either use C or use C++. Do NOT use a mixture of the two (its sloppy and can introduce bugs). So first off you should have picked a language. You seem to have picked both:

 #include <cmath>                         // C++
 printf("%d\n", my_compare("ab", "1"));   // Why use printf in C++
                                          // It is obtuse dangerous (not type safe).

The two languages have a basic similar syntax but they are different languages treat them as such.

Know what is available from the standard libraries:

bool is_num(char x) {return (x>='0' && x<='9');}

This exists in the standard library as std::isdigit()

If this is C++ why are you passing C-Strings around.

int get_num(const char* s, int len)

Much better to write:

int get_num(std::string const& s)

Also there is no point in writing this function (especially since it is error prone to do so.

  • In c++ use operator>> to stream to an int.
  • In C use sscanf(s, "%d%n", &value, $length).

Also your variable names are bad. Pick names that make it easy to read the code and understand what you are doing.

    int j = 0;
    int ret = 0;
    while (len > 0)
    {
        // Why add the extra complexity of `j++` here.
        // You have made the expression hard enough to read as it is.
        // You don;t get points for being clever in an interview its readability
        // we want to see.
        ret += (s[j++] - '0') * pow(10, len-1);
        len --;
    }
    return ret;
}

Yes in C it is traditional to put all the variables at the top of the function (so fine for C). But in C++ this is considered bad practice and sloppy. Declare variables as close to the point you want to use them (preferably just before you use them). That way you can see the type at the point you are using them.

        int value1 = 0;
        int value2 = 0;
        bool is_t1_num = false;
        bool is_t2_num = false;
        int len_t1 = 0;
        int len_t2 = 0;
        const char*  t1;
        const char*  t2;

You are repeating the same processes twice.
First you scan over the string finding the length of the number.
Second you scan over the string calculating the value of the number.
There is no need to do that. Use one pass.

            while(is_num(*t1)&&(*t1))
            {
                len_t1 ++;
                t1++;
            }
            value1 = get_num(s1, len_t1);

Also you have the same boilerplate code for both t1 and t2. If you have code that does the same thing factor it out into a function.

share|improve this answer
1  
He said he could not use stl nor stdlib. If I'm right, std::string is part of the stl. I don't see the point in not being allowed to use them though since you'll have to use them when working. And saying that stdlib is not allowed, is it stdlib.h or the whole C standard library? – Morwenn Aug 16 '12 at 8:30

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.