After 24 hours I beg for debugging help.
I am making a game ( not in c++ ) and I am trying to use c++ to pre-generate puzzles to solve in said game. The game is simple, a riddle... You have two bottles, one a 5 gallon bottle, the other a 3, you need precisely 4 gallons. Passing water back in forth this is very achievable. I want 5 bottle puzzles, and have successfully made them in the past using c++, but alas, I can't find my code :-( Worse is, as much as I try I cant tell why my rewrite isn't working as intended. It should be tracking which moves its made to get the best speed solution. One of the points of this program is to find the fastest possible solution. But the best move set isn't true... meaning if I apply the moves to the puzzle itself they aren't actually a solution. Where am I going wrong? I am hesitant to release my code, but I know in this case... the whole picture may be needed. Thank you in advance for helping me lock it down!
Code is not sanitized, this is all code for the app.
Code is not sanitized, this is all code for the app.
|
|
So you can still compile and have intended function the other actions I removed for the content character limit are located below. Replace te comment above with the code below for the full code.
|
|
Let's start from the top.
1. Too many global variables.
2. No indentation and lack of whitespace make this code unreadable.
3. Using goto. You really need to learn to use one of the more accepted loop constructs.
4. Putting multiple statements on one line. This makes following the logic of the program impossible. Put each statement on a line of it's own.
Should be more like:
5. Lack of helper functions. Functions should be your friends.
6. Use of "magic numbers" (23) in the above snippet.
7. Variables with numbers added (b4ct, b5ct, b3ct, etc) suggest that you should consider std::vector or arrays.
8. Cryptic variable names. Using meaningful variable and function names will make you program much easier to read and understand.
Fixing the above issues should get you started in the right direction.
1. Too many global variables.
2. No indentation and lack of whitespace make this code unreadable.
3. Using goto. You really need to learn to use one of the more accepted loop constructs.
4. Putting multiple statements on one line. This makes following the logic of the program impossible. Put each statement on a line of it's own.
if(action==23){if(b1ct<b1mt){if(b4ct>0){if(b4ct<=(b1mt-b1ct)){b1ct=b4ct+b4ct; b4ct=0; actioncount=actioncount+1;movelist+="4 to 1\n";};if(b4ct>(b1mt-b1ct)){b4ct=b4ct-(b1mt-b1ct); b1ct=b1mt; actioncount=actioncount+1;movelist+="4 to 1\n";};}; };};// 23 4 to 1 Should be more like:
|
|
5. Lack of helper functions. Functions should be your friends.
6. Use of "magic numbers" (23) in the above snippet.
7. Variables with numbers added (b4ct, b5ct, b3ct, etc) suggest that you should consider std::vector or arrays.
8. Cryptic variable names. Using meaningful variable and function names will make you program much easier to read and understand.
Fixing the above issues should get you started in the right direction.
Last edited on
I agree with everything jlb said and I would add the following:
9) No useful comments in the code.
Your program is incomprehensible.
It's no wonder you're having problems debugging it.
9) No useful comments in the code.
Your program is incomprehensible.
It's no wonder you're having problems debugging it.
Last edited on
Thank you very much for the feedback. I will return in a few hours with a complete rewrite taking these ideas into account. I may need some direction with how to get away from so many global variables though, but Ill do my best at the rest and see if cleaner code gets us there! Thanks again. Sorry I suck, Completely autodidactic until this point when it comes to c++
Before rewriting the existing code, see if you can reduce it using functions. ANy time I see lots of code that looks nearly identical, I wonder if there's a way to factor out the comonality.
In the rewrite I have reduced a ton of it to user defined functions. Not done yet though, a preview though. I also by adding some cout's for debugging
Also cleaning up the way I am writing my conditions to make it more readable, and adding useful commenting.
I figure the debug text kinda satisfies the need for commenting there. I hope I am headed the direction you suggested. Once I am done with the rewrite Ill go through and remove the unneeded if semicolons. I wonder how many bugs have been caused by those over the years.
|
|
Also cleaning up the way I am writing my conditions to make it more readable, and adding useful commenting.
|
|
I figure the debug text kinda satisfies the need for commenting there. I hope I am headed the direction you suggested. Once I am done with the rewrite Ill go through and remove the unneeded if semicolons. I wonder how many bugs have been caused by those over the years.
10. You need to consistently use indentation. Did you just not post the closing brace for the while statement or did you miss it?
Also note the added whitespace. Make you program readable by putting whitespace in your calculations and to help highlight program logic.
Also the following comment doesn't make much sense to me:
I get the random part, but what is a viable action?
|
|
Also note the added whitespace. Make you program readable by putting whitespace in your calculations and to help highlight program logic.
Also the following comment doesn't make much sense to me:
//Generate a random action to commit if it is a viable action. I get the random part, but what is a viable action?
Last edited on
if(action == 2 && bottle2current > 0){This is where I make sure its viable, Bottle 2 current needs to have something to empty for it to be viable, and one of these ifs has to go off for the action to be tracked due to
actions = actions + 1; nested inside the ifs.The end of the while loop isnt there just because of a short copy paste, I was working between the braces still so unfinished code was there. I caught the lack of whitespace and have improved on it. Another snippet:
|
|
Hopefully I am getting closer. I recognize which semicolons need removed, im just waiting to the end of the rewrite then pulling em out so I dont miss any. Looking better code guru's?
Still no whitespace, and multiple statements on one line, and semicolons after the ending braces, which should be placed to match the indentation not all grouped together.
|
|
Okay after looking at the code a little more, you should use arrays. For example, Instead of
do
Then you can write a function to transfer contents from one bottle to another
And you can pour between random bottle with somethink like
|
|
do
|
|
Then you can write a function to transfer contents from one bottle to another
|
|
And you can pour between random bottle with somethink like
|
|
Last edited on
Its alive! Working as intended. Been hacking at this for 30 hours now, I need some sleep. Ill be back for a propper flogging to get my formatting etc in shape after I sleep. No clue what the error was, but I can say this, making the code readable, and using more modern looping mechanisms like while, and incorporating user defined functions all played a huge part in making it work. THANK YOU ALL SO MUCH for forcing me to try clean rather than lazy! Ill never go back!
Hi,
A couple of other things:
is better written as:
And this one is a pet hatred for me:
Statements like that are error prone, UGLY, and non scalable. With dhayden's suggestions you can avoid the whole thing.
Also, can you avoid really long comments - your code is 3 screens wide on my system. There is a bit of a guideline that code shouldn't be any wider than 80 char - just for readability.
With the formatting of your code, hopefully you can get your IDE to do it auto-magically for you.
A couple of other things:
actions = actions + 1;is better written as:
++actions; // increment operator And this one is a pet hatred for me:
while(actions<maxactions && actions<=bestactioncount && bottle1current!=goal && bottle2current!=goal .....Statements like that are error prone, UGLY, and non scalable. With dhayden's suggestions you can avoid the whole thing.
Also, can you avoid really long comments - your code is 3 screens wide on my system. There is a bit of a guideline that code shouldn't be any wider than 80 char - just for readability.
With the formatting of your code, hopefully you can get your IDE to do it auto-magically for you.
Topic archived. No new replies allowed.