• We’re currently investigating an issue related to the forum theme and styling that is impacting page layout and visual formatting. The problem has been identified, and we are actively working on a resolution. There is no impact to user data or functionality, this is strictly a front-end display issue. We’ll post an update once the fix has been deployed. Thanks for your patience while we get this sorted.

Logic Bug in a small C++ function...

DarkThinker

Platinum Member
Hey everyone, I have a little bug in a function and I have been examining it and trying to make it work, nothing so far.

The program is supposed to ask the user for an array's length, then ask him to populate it with values, after that a function count(number, length) is called, it's a recursive function. count()'s purpose is to look for repeated entries and output them back the user. Sounds simple but the bug is making it harder than it is.

I would appreciate any help towards fixing this 🙂

Working Code,

#include <iostream>
#include <string>

using namespace std;
class project9_3{

public:
//Methods
void count(int number, int length);
void createArray();

//Variables
int *array;
int length_max;
int arrayIndex;
int tempCountMatch;
string junkString;
};

/*
createArrray is a method that initializes the
array's contents and size according to the user's inputs
*/
void project9_3::createArray(){

cout << "Enter the length of the array > ";
cin >> length_max;
getline(cin, junkString);
array = new int[length_max];

for(int i=0;i<length_max;i++){
cout << "Enter value for position " << i << "\n";
cin >> array[ i ];
getline(cin, junkString);
// cout << "array[" << i << "]=" << array[ i ] << "\n";
}

arrayIndex=0;
count(array[0], length_max);
}


void project9_3::count(int number, int length){
// cout << "Entering count\n";
tempCountMatch=0;
for(int i=0;i<length;i++){
// cout <<"number= " << number << " for i= " << i << " for tempCountMatch= " << tempCountMatch << "\n";
if(number==array[ i ]){
tempCountMatch++;
}
if(tempCountMatch==2){
cout << number << " has repeated more than once\n";
break; //make the for loop exit
}
}
if(arrayIndex<length){
arrayIndex++;
// cout << "arrayIndex= " << arrayIndex << "\n";
number=array[arrayIndex];//increment index so we check next number
//cout << "Recursive count call\n";
count(number, length);
}else{
delete [] array;
return;//exits the program
}
}


int main()
{
project9_3 test;
test.createArray();
}

int main()
{
project9_3 test;
test.createArray();
}
 
Originally posted by: HigherGround
ever heard of the 'Enter' key?

I pasted the code form VIM and that's how it came out to be, I didn't notice. As soon as I get home I will update the code. Sorry
 
Originally posted by: Cooler
Nice to see im not the only coder out there that uses "we" in comments.

:laugh: it's a bad habit., I tend to think of myself having to explain my code to a group of people when I am coding, it's absurd I know.
 
Originally posted by: DarkThinker
OK, I have repasted the code in the OP 🙂

Ok, now tell us what it's doing that you don't expect. I read it through, and nothing glaringly obvious jumped out except for the fact that you aren't bounds checking the user input. Of course, if it were obvious you'd have found it. 🙂
 
When you allocate memory for "array", you are redefining the variable. So your only really allocating space for the variable "array" inside of createArray()'s scope.

If you wish to allocate memory for the global variable "array", change the line to:

array = new int[length_max];

instead of

int *array = new int[length_max];


also you are leaking memory by not calling an explicit delete on the array after you are done with it.

Good Luck.
 
Your bug is a scoping issue.

int foo = 0;
void Foo() {
int foo = 5;
}

void bar() {
cout << "foo is " << foo << endl;
Foo();
cout << "foo is " << foo << endl;
}
 
Originally posted by: HalflifeDivided
When you allocate memory for "array", you are redefining the variable. So your only really allocating space for the variable "array" inside of createArray()'s scope.

If you wish to allocate memory for the global variable "array", change the line to:

array = new int[length_max];

instead of

int *array = new int[length_max];


also you are leaking memory by not calling an explicit delete on the array after you are done with it.

Good Luck.

His array pointer is declared as a member of the class, so I don't think it's a scoping issue.
 
Originally posted by: Markbnj
Originally posted by: HalflifeDivided
When you allocate memory for "array", you are redefining the variable. So your only really allocating space for the variable "array" inside of createArray()'s scope.

If you wish to allocate memory for the global variable "array", change the line to:

array = new int[length_max];

instead of

int *array = new int[length_max];


also you are leaking memory by not calling an explicit delete on the array after you are done with it.

Good Luck.

His array pointer is declared as a member of the class, so I don't think it's a scoping issue.

His code has been changed since we first discussed it. Are you still running into issues OP?

 
Originally posted by: Markbnj
Originally posted by: DarkThinker
OK, I have repasted the code in the OP 🙂

Ok, now tell us what it's doing that you don't expect. I read it through, and nothing glaringly obvious jumped out except for the fact that you aren't bounds checking the user input. Of course, if it were obvious you'd have found it. 🙂

Yeah really. When asking for help it would be to your advantage to give as much info as possible. If it's a compilation error, give the line that has the problem, and the associated error. If it's a run time error, describe the symptom.

Not trying to give you a hard time but I see this a lot when people ask for programming help. Not giving enough info only reduces the likelihood of you getting proper help.
 
Originally posted by: HalflifeDivided
Originally posted by: Markbnj
Originally posted by: HalflifeDivided
When you allocate memory for "array", you are redefining the variable. So your only really allocating space for the variable "array" inside of createArray()'s scope.

If you wish to allocate memory for the global variable "array", change the line to:

array = new int[length_max];

instead of

int *array = new int[length_max];


also you are leaking memory by not calling an explicit delete on the array after you are done with it.

Good Luck.

His array pointer is declared as a member of the class, so I don't think it's a scoping issue.

His code has been changed since we first discussed it. Are you still running into issues OP?

Thanks to you guys the issues have been resolved I have updated the code in the OP and it's all good after that.(eventhough the repeated number ends up getting printed as many times as it was repeated, that's fine for now, it's not an issue ATM).


Originally posted by: dighn
Originally posted by: Markbnj
Originally posted by: DarkThinker
OK, I have repasted the code in the OP 🙂

Ok, now tell us what it's doing that you don't expect. I read it through, and nothing glaringly obvious jumped out except for the fact that you aren't bounds checking the user input. Of course, if it were obvious you'd have found it. 🙂

Yeah really. When asking for help it would be to your advantage to give as much info as possible. If it's a compilation error, give the line that has the problem, and the associated error. If it's a run time error, describe the symptom.

Not trying to give you a hard time but I see this a lot when people ask for programming help. Not giving enough info only reduces the likelihood of you getting proper help.

Well I am sorry, I just thought it was pretty obvious that I was talking about the recursive function Count throughout my OP .
 
Just a comment. In the count() function, are you sure you want this?

if(tempCountMatch==2)

Wouldn't you want:

if(tempCountMatch > 1)

Otherwise, if the user puts in a value 3 times, it doesn't look like your code will pick it up. I could be missing something though.
 
Originally posted by: DarkThinker
Well I am sorry, I just thought it was pretty obvious that I was talking about the recursive function Count throughout my OP .

But you didn't say how it was not working. e.g. did it crash? gave you the wrong count? Information like that can make it a lot quicker for us to diagnose the problem.
 
Originally posted by: engineereeyore
Just a comment. In the count() function, are you sure you want this?

if(tempCountMatch==2)

Wouldn't you want:

if(tempCountMatch > 1)

Otherwise, if the user puts in a value 3 times, it doesn't look like your code will pick it up. I could be missing something though.

the lack of formatting makes it harder to read, but that line is inside the loop so it goes to 2 first.
 
Originally posted by: DarkThinker
Originally posted by: Cooler
Nice to see im not the only coder out there that uses "we" in comments.

:laugh: it's a bad habit., I tend to think of myself having to explain my code to a group of people when I am coding, it's absurd I know.

// We do it too 😛
 
Originally posted by: DarkThinker
Originally posted by: HalflifeDivided
Originally posted by: Markbnj
Originally posted by: HalflifeDivided
When you allocate memory for "array", you are redefining the variable. So your only really allocating space for the variable "array" inside of createArray()'s scope.

If you wish to allocate memory for the global variable "array", change the line to:

array = new int[length_max];

instead of

int *array = new int[length_max];


also you are leaking memory by not calling an explicit delete on the array after you are done with it.

Good Luck.

His array pointer is declared as a member of the class, so I don't think it's a scoping issue.

His code has been changed since we first discussed it. Are you still running into issues OP?

Thanks to you guys the issues have been resolved I have updated the code in the OP and it's all good after that.(eventhough the repeated number ends up getting printed as many times as it was repeated, that's fine for now, it's not an issue ATM).


The reason why the program is repeating the output is because you are checking the entire array again... (you are doing too much work.)

Watch... Sample array = 1 2 3 2

(1) 2 3 2
1 doesnt have a match, so move on.

1 (2) 3 2
2 has a match, output & move on.

1 2 (3) 2
3 doesnt have a match, so move on.

1 2 3 (2)
2 has a match, output & move on.

reached the max length = end

Generally when processing an array recursively, you only pass on a sub portion of the array. The correct way to perform this is...

(1) 2 3 2
1 doesnt have a match, so move on.

(2) 3 2
2 has a match, output & move on.

(3) 2
3 doesnt have a match, so move on.

(2)
2 doesnt have a match, output & move on.

Array empty = stop



Rather than passing the entire array, perhaps you want to pass on a counter which could indicate which position in the array would be the one to process first on this iteration, and ignore everything before it.
 
Out of curiosity, did it have to be done recursively (i.e. for a project focusing on learning recursion)? One of my favorite recursion problems is translating math functions (i.e. basic ones) into recursive functions. Those seem to stump people pretty well, because they think of the basic math functions as... well basic. But doing them recursively makes you simplify them even more.
 
Back
Top