Parsing numbers from equations into strings

Question

This is my first code in C++. Since I'm new to the language, I'm just looking for pointers on what can be made better. I tried to cut out unnecessary stuff, but there are some comments in there. I know Java, so if you're trying to explain anything in Java, I can understand it.

Feel free to mention anything, whether it be code style (which I'm completely unsure what the conventions are in C++), performance, etc.

The goal of the program is to take an equation, such as 5x^2+48204x=7, and parse the 5 and 48204 into strings.

using namespace std;

void printHelp();

class Equation {
public: 
    string equation1;
    string equation2;

    int matrix [3][2];

    Equation(string one, string two) {
        one.erase(remove_if(one.begin(), one.end(), isspace), one.end());
        two.erase(remove_if(two.begin(), two.end(), isspace), two.end());
        equation1 = one;
        equation2 = two;

        cout << equation1 << endl;

        init();
    }

    void init() {
        size_t firstx = equation1.find_first_of("x");
        size_t secondx = equation1.find_first_of("x", firstx + 1);
        if (secondx == string::npos) {
            cout << "Secondx == 0" << endl;
        }

        //cout << firstx << endl;
        //cout << secondx << endl;

        int startloc = firstx - 1;

        while (true) {
            //cout << "starting with startloc = " << startloc << endl;
            if (startloc == -1) {
                break;
            }
            char c = equation1.at(startloc);
            if (c == ' ' || c == '=' || c == '+' || c == '-' || c == '*' || c == '/') {
                //cout << "Found something" << endl;
                break;
            }
            startloc--;
        }

        string s;
        unsigned int i = startloc + 1;
        //cout << i << endl;
        //cout << firstx << endl;
        while (i < firstx) {
            //cout << "Character at " << i << endl;
            //cout << equation1[i] << endl;
            stringstream ss;
            string temp;
            ss << equation1[i];
            ss >> temp;
            s.append(temp);
            i++;
        }
        cout << s << endl;

        startloc = secondx - 1;

        while (true) {
            //cout << "starting with startloc = " << startloc << endl;
            if (startloc == 0) { 
                cout << "Problem" << endl;
            }
            char c = equation1.at(startloc);
            if (c == ' ' || c == '=' || c == '+' || c == '-' || c == '*' || c == '/') {
                //cout << "Found something" << endl;
                break;
            }
            startloc--;
        }

        s = "";
        i = startloc + 1;
        while (i < secondx) {
            stringstream ss;
            string temp;
            ss << equation1[i];
            ss >> s;
            s.append(temp);
            i++;
        }

        cout << s << endl;
    }

};

int main(int argc, char *argv[])
{
    Equation equation(argv[1], "blank");
    //printHelp();
    string s;
    cin >> s;
    return 0;
}

void printHelp() 
{
    cout << "Welcome!" << endl;
    cout << "Stuff about commands" << endl;
}

Answer 1

Without changing things too much...

• You need some #includes.

• Don't use using namespace. One of the issues is knowing where a function comes from (which one is being used) - this hides it. Using a short std:: gains you a lot in understandability. You can do using std::string which is better, but imo even that isn't worth it.

• You might as well put printHelp before main rather than after, that way you don't have to declare it. And it's not used currently anyway.

• Matrix also isn't used.

• Having an Equation class doesn't make sense for what you're (currently) using it for. You might as well just have a naked function.

• You don't allow for the possibility that firstx may be invalid, ie there are no x in the equation. This may be a given, of course. And for that matter, that the x^2 term will be first!

• If secondx is invalid, skip over processing it.

• Multiple breaks in while loops should be avoided, for readability. This (first) loop probably should be split off into another function since it's used twice, together with the following section.

• I'm a little nervous of int startloc = firstx - 1;, since firstx is unsigned and startloc isn't. I'd cast firstx to an int first if I were using it like this.

• Consider using std::string::find_last_of and std::string::substr. They'll simplify the code somewhat.

• Check that you don't lose a negative coefficient.