Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
3  
The main issue in this program is the program design. Most importantly, you need to use the most fundamental concept in object-oriented design: private encapsulation. C++ is no different from Java in this. Since you are a beginner programmer, you should learn object-oriented design as early as possible, this is far more important than learning all the dirty details of a particular language. I'd actually go back to Java and study OO design with that language as foundation, since Java is far cleaner (though less powerful) than C++. –  Lundin Jan 30 '13 at 15:46
1  
Comment your code. What's the purpose of the code? What's the main algorithm? What are the important lines of code and why do they exist? –  Dave Jarvis Jan 30 '13 at 17:19
    
Thanks guys. I dont think i need to go back and relearn OOP because im sure i understand it, althought this code doesnt show it. I wrote this in about 15 minutes so i didnt clean it at all. Im going to repost it once i clean it in 2ish hours. (Also, i dont understand pointers, if anyone has advice) hopefully it will look alot nicer when cleaned –  Tips48 Jan 30 '13 at 18:23
    
Updated the code, attempted to clean it up and use some of the changes that were suggested –  Tips48 Jan 30 '13 at 21:24
    
My code has been cleaned up greatly, thank you guys. –  Tips48 Jan 31 '13 at 1:01

1 Answer 1

up vote 10 down vote accepted

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.

share|improve this answer
    
Thanks, lots of little things i didnt think of. As for printhelp() ill be using it when i start expanding the program. Matrix[][] is unused but i will be using it later. The reason i have the equation class is because ill be expanding it to do multiple things later. I just wrote some (messy) code to try to get the splitting. Your saying instead of #include <string> use std::string? O.o –  Tips48 Jan 30 '13 at 11:52
    
I understand putting that loop in a function since its used multiple times. My last question: is that char -> string conversion correct? I couldnt find any way to do it faster –  Tips48 Jan 30 '13 at 11:57
1  
You need #include <string> anyway. But use std::string rather than using std and string. –  Glenn Rogers Jan 30 '13 at 12:14
    
re the conversion, I don't want to even look at it!! For your loops, the first loop is doing find_last_of, and the second is doing substr. Use these instead of making your own. –  Glenn Rogers Jan 30 '13 at 12:20
    
I get how you can use substring and wish i remembered earlier, oops! But how would you use find_last_of? I dont really get how i would use it to replace the first loop. –  Tips48 Jan 30 '13 at 15:04

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.