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;
}

• Lundin: 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++.
• Dave Jarvis: 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?
• Tips48: 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: Updated the code, attempted to clean it up and use some of the changes that were suggested
• Tips48: My code has been cleaned up greatly, thank you guys.

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.

• Tips48: 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: 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
• Glenn Rogers: You need #include <string> anyway. But use std::string rather than using std and string.
• Glenn Rogers: 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.
• Tips48: 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.