trouble with cin.getline

Can someone please explain why the cin.getline instruction doesnt work when the program goes a second time around .It is completely ignored.

Code:

//// output the factors of a chosen number//////

#include<iostream>
using namespace std;

void OutputFactors(void);
int GetValidNum(void);
int GoAgain(void);

int main(void)
{
	OutputFactors();

	return(0);
}

void OutputFactors(void)
{
	for(;;)
	{
		int num = GetValidNum();

		for(int i = 1; i <= num; i++)
		{
			if(num % i == 0)
			{
				cout << i << '\t';
			}
		}
		cout << '\n';
		

		if(0 == GoAgain())
		{
			break;
		}
	}
}

int GetValidNum(void)
{
	const int MAX = 128;
	char chNum[128] = {0};
	int check = 0;
	
	do
	{
		check = 0;
		cout << "Enter a number and I will output its factors.\n";

		cin.getline(chNum, MAX, '\n');
		
		for(int i = 0; chNum[i] != '\0'; i++)
		{
			if(chNum[i] < '0' || chNum[i] > '9')
			{
				check = 1;
			}
		}

		if(check == 1)
		{
			cout << "That is not a valid number.\n"
				 << "Please try again.\n";
		}
	}while(check == 1 || chNum[0] == '\0');

	int num = atoi(chNum);

	return(num);
}

int GoAgain(void)
{
	char goAgain = '\0';
	int check = 0;

	cout << "Would you like another go.\n";
	cin >> goAgain;

	if(goAgain == 'y' || goAgain == 'Y')
	{
		check = 1;
	}

	return(check);
}

Thanks Mark S.

First off all, I can't understand for the life of me why you have the OutputFactors function. Why don't you just put all of that crap in main? Secondly, to use an infinite for loop with a break in it when you could easily use a do while loop is also bad technique.

THIS NEXT PART CENSORED -- FOR BEING WAY TO AWESOME

There...

>Why don't you just put all of that crap in main?
Because it could be the beginning of a more comprehensive program, perhaps?

>Your problem is that getline() doesn't dump the '\n' after it reads in the string
Wrong, getline reads and discards the delimiter. The problem is in GoAgain where cin reads a single character and fails to extract the newline. Then getline immediately returns on the next call because a newline is the delimiter. This is a classic example of the scanf/gets problem where formatted and unformatted input simply don't mix. A simple fix would be to change this:

Code:

cin >> goAgain;

To this:

Code:

cin >> goAgain;
cin.ignore();

The ignore is effectively placed before any subsequent calls to getline. Calling ignore after the getline call would be ineffective because the problem occurs after the first getline call but before the second. A much better fix would be to avoid mixing formatted and unformatted input to begin with.

You don't need to use so many temporary variables (although if it makes it easier to read for you, then by all means use them). This, for example,

Code:

	int num = atoi(chNum);

	return(num);
}

is the same as

Code:

	return(atoi(chNum));
}

Originally Posted by Prelude

Because it could be the beginning of a more comprehensive program, perhaps?

Perhaps, but wouldn't count on that being the case considering that it's sitting in an infinite loop that doesn't return anything to main when it finally breaks.

As for the rest of my reply, I blame the Edy's Fudge Tracks I was concentrating on. I should have been looking at the GoAgain() call and not mixing get() up with getline().

Thanks for the solution.

Where is the best place to put the cin.ignore() function. I have put it after cin >> goAgain. It seems to work there.

I originaly had a do while loop but while trying to locate the problem switched to for(;.

Originally Posted by Mark S.

Thanks for the solution.

Where is the best place to put the cin.ignore() function. I have put it after cin >> goAgain. It seems to work there.

I originaly had a do while loop but while trying to locate the problem switched to for(;.

Right after the cin where you have it would be the best place.

>Perhaps, but wouldn't count on that being the case considering that it's sitting
>in an infinite loop that doesn't return anything to main when it finally breaks.
Why not? It's an output generator that encapsulates error handling. There's no need for a return value at this point. If errors are to be handled using exceptions, there may never be need for a return value. Now, riddle me this. What if the OP wants to add, say, three more functions like that one. And what if the OP wants to then add a general testing framework around those functions that can be extended for ever more functions. Throwing everything in main suggests a lack of foresight.

>I blame the Edy's Fudge Tracks I was concentrating on
Hmm...Now I want ice cream.

>> What if the OP wants to add, say, three more functions like that one. And what if the OP wants to then add a general testing framework around those functions that can be extended for ever more functions.

Mark S., are you planning to add a general testing framework?


>> Throwing everything in main suggests a lack of foresight.

Indeed, but when I make I program, I don't plan them! If I did I'd assume I may have lesser main-s, size wise, but size isn't everything! I once had a 1500 line main before I learnt how to use functions. It reduced the entire line count to 300 lines

Originally Posted by Prelude

Why not? It's an output generator that encapsulates error handling. There's no need for a return value at this point. If errors are to be handled using exceptions, there may never be need for a return value. Now, riddle me this. What if the OP wants to add, say, three more functions like that one. And what if the OP wants to then add a general testing framework around those functions that can be extended for ever more functions. Throwing everything in main suggests a lack of foresight.

I think you know as well as I do that's not what the OP was planning on doing. I was merely trying to make a point that main isn't just a place to throw a simpe function into that runs your main program. If you want to use it for error handling, that's fine, but I honestly saw no signs of the OP doing that and I believe your defending his/her action with things s/he might not understand, yet, could likely promote a misunderstanding of main and a bad habit.

Originally Posted by Prelude

Hmm...Now I want ice cream.

Yeah, well don't buy Edy's Fudge Tracks... it sucks. I should have went with the good ol' Cookies and Cream.

>> I was merely trying to make a point that main isn't just a place to throw a simpe function into that runs your main program.

Sure it is. The current design is at least as good (and is actually better IMO) than putting that code in main. It is not a bad habit, it is a good habit. Separating your code into functions that accomplish distinct tasks is a good habit. Writing code that allows for future enhancement is a good habit, regardless of whether that code is ever actually changed. Putting that code in main just because it is the main part of the code is the bad habit.

Originally Posted by Daved

Separating your code into functions that accomplish distinct tasks is a good habit.

That function isn't a distinct task, it's the whole task. Did you look at the function? It consists of everything the program is meant to and will ever mean to do. It tells the user what the program does, doing it's application and asking them if they want to do it again.... and again, and again... It's not a part of a much bigger program and it really couldn't be without changing what's currently there. If there was anything useful that could be made out of those three functions with the exception of creating an unnessasary shell program that calls the functions among others, then I would agree with you, but there really isn't. What the OP has done here is made a box (OutputFactors), that goes inside of a box (main), that goes inside of another box (the Operating System), but there really is no logical reason for the middle box, because everything that would go in that box, could easily go in the outermost box. Why not just throw another function in there that calls OutputFactors and call that from main? Then when we learn more we can use that function for error handling and main can be the shell for our own operating system.

It's just a silly argument. Any changes s/he would plan on making with that program in the future would be just as easy cause all s/he would have to do is copy everything in main and toss it into a new function. It would take an extra 20 seconds, at most. As it stands this program has an unnecessary level of indirection that makes it more difficult to read and in my opinion no better off for future advancement.


To the OP: If you really want that program to be useful in the future and modifiable to become a bigger program, and by the way, this is just my opinion, I can't speak for the other posters here... then I would redesign it so that all of the I/O (the cins and couts) are in main. That way, if you want to use it in something graphical, or perhaps something over a network, you have these nice little functions that do the math for you and you can discard the main with all the console I\O which will be useless to you when you look to change it. There is nothing wrong with a function that gets the factors, in fact it's a good idea, but I generally like to make my functions as portable as possible to any kind of medium.

some schools actutally teach that there should be NOTHING in main except function calls, some variable definitions, and minimal logic flow (small loops and such)... I dont' wholly agree with that, but main does make a nice outline of sorts for much bigger programs

Originally Posted by major_small

some schools actutally teach that there should be NOTHING in main except function calls, some variable definitions, and minimal logic flow (small loops and such)... I dont' wholly agree with that, but main does make a nice outline of sorts for much bigger programs

Exactly, there really shouldn't be much. I didn't say move the whole program to main, I said get rid of the unnecessary indirection and importability in that one function. Originally, I said that whole function should be in main, but then Prelude said that function is useful separate in the case of if the OP wanted to use main for something bigger. "Fine", I though, she's correct... you could do that, however, I don't think that's what the OP was thinking of doing and if that's the case, then the extra function is useless. Even if he wanted to make something bigger out of it, he'd have to rework it. Some point after that, the post went bizarro world on me...

Main should be small... it shouldn't be one function call small.

A whole task is still a distinct task. There might not be a use for the middle box, but at least it is packaged and ready with a label already applied.

Bottom line is separating tasks into functions is a much better habit than putting your code inside main. Whether it is applied perfectly in this case or not, the idea is still correct. Putting that code back inside main is a step backwards.

The point of programming for future enhancement is not about whether that enhancement will actually occur on the current program. It is about good practice so that when you do need to enhance a program, it is easier to do and do correctly.

BTW, most windows programs have a main that consists of a single function call.