Function locks into an infinite loop: why?

[The Fifth Horseman]

Okay, I have a program that moves across a bunch of records stored into memory and reports information about each (number, adress, some initial values).
#include <iostream>
#include <conio.h>
#include <fstream>
#include <math.h>
#include <iomanip>

using namespace std;

ifstream::pos_type size;
char *cache;

unsigned char unsign(char in)
{
if (in>=0) return in;
else return in+256;
}

int readshint(unsigned __int64 o=0)
{
unsigned int out=0;
for (int i=0; i<=1; i++)
{ out+=int(unsign(cache[i+o])*pow(256, i));}
return out;
}

void report (unsigned long int record, unsigned long int offset)
{
cout << "\nRecord " << setfill('0') << setw(2) << dec << record
<< " at " << setw(4) << hex << offset << "h : "
<< dec << readshint(offset) << " x " << readshint(offset+2)
<< " pixels.\n";
}

unsigned char countrecords (unsigned short int offset=0)
{
if (readshint(offset+4)==0)
{ return 1; }
else
{ return countrecords(offset+readshint(offset+4))+1; }
}

unsigned short int taketarget (unsigned short int records)
{
int target=0;
cout << "\nEnter target record number:\n";
while (target<1 || target>records)
{ cin >> target;
/*if (target<1 || target>records)
{ cout << "Enter a number between 1 and " << records << ".\n"; }*/
}
return target;
}

void save (unsigned short int offset)
{
cout << "\nNOT IMPLEMENTED YET!!!\n";
return;
}

main () {
ifstream infile ("in.chp", ios::in|ios::binary|ios::ate);
if (infile.is_open())
{
size = infile.tellg();
cache = new unsigned (unsign(char [size]));
infile.seekg (0, ios::beg);
infile.read (cache, size);
infile.close();
cout << "CHP read to memory.\n";
unsigned short int records=countrecords(), offset=0, target=1;
cout << dec << records << " records found in file.\n";
char trigger;
do {
/*cout << "Enter target record number...\n";
do {
cin >> target;
if (target>records || target==0)
cout << "Enter a value between 1 and " << records << ".\n";
} while (target>records || target<1);*/
for (int recind=1; recind<target; recind++)
{ offset+=readshint(offset+4); }
report(target, offset);
cout << "[N]ext, [P]revious, [I]ndex, [S]ave, [E]xit?\n";
do {
trigger=tolower(getch());
} while (trigger!='n' && trigger!='p' && trigger!='i' && trigger!='s' && trigger!='e');
switch (tolower(trigger))
{
case 'n': if (target<records-1) target++; else target=records; goto end;
case 'p': if (target>1) target--; goto end;
case 'i': target=taketarget(records); goto end;
case 's': save(offset); goto end;
end:;
}
} while (tolower(trigger)!='e');
delete[] cache;
}
else cout << "Unable to open input file\n";
cout << "\n";
system("PAUSE");
}

It (mostly) works, but the following function doesn't work as intended.
unsigned short int taketarget (unsigned short int records)
{
int target=0;
cout << "\nEnter target record number:\n";
while (target<1 || target>records)
{ cin >> target;
if (target<1 || target>records)
{ cout << "Enter a number between 1 and " << records << ".\n"; }
}
return target;
}
It's supposed to keep prompting the user until valid input is entered.
It does just fine if the user enters numeric values - but if a character is entered, the function loops infinitely without giving the user any way to enter new input.

What may be the cause of this behavior?

[El Quia]

I haven't really read all the code, and I am feeling somewhat sleepy and I am about to hit the sack, so maybe I am talking nonsense, but I think I see which is the problem. But, I can also be messing things up because of my ignorance about c++ :p

I think the problem is that you are putting the user input into an int. And then evaluating that input (an int) and pretending that the program realized when it is really a number and when it is another thing. Each character the user is inputing goes to the int as a group of chars, so I think that messes up everything. In these cases I think it is better to take the user input and put it into a string and then parse the string first to check if it is a numeric value or if it is anything else. And only if it is a numeric value, I would check if it falls between the values required. I don't know how many parsing functions the string object has in C++, so maybe it is something of a mess to do that. But assuming that the user will input a number is always a dangerous presumption. ALWAYS try the user input as typed by a moronic child hitting the keyboard in anger. Most of the time, you will be right :p

I got some curiosity at you checking twice in each loop at the condition (target<1 || target>records). Maybe it is a necessity here, maybe there is another way to express that without redundancy. But I can't decide now, as I am really too sleepy.

Well, I hope I could help you, this time :D. And if I wasn't of help, then blame my sleep deprivation! (which is odd, because I want' really sleep deprived, today...)

[Acero]

EL Quia is correct in their response. Validating the user input is always a requirement.

To expand upon the answer, when the user of your program enters a character into the iostream object(cin) of type int, the int is filled with garbage data and the failbit of the iostream object is set to true. Until the iostream object failbit is cleared and the data in the cin stream is dicarded, your cin is unusable and will not get user input again, hence your infinite loop.

Here is an example of something similar to what you might want to try:


unsigned short int taketarget (unsigned short int records)
{
int target=0;

while (target<1 || target>records)
{
cout << "\nEnter target record number:\n";
cin >> target;
if (cin.fail())
{
cout << "Invalid Input." << endl;
cin.clear();
cin.ignore();
cout << "Enter a number between 1 and " << records << ".\n";
}
}
return target;
}


I hope this helps you

[The Fifth Horseman]

Thanks, Acero. This solved the problem perfectly. :)

[Acero]

I'm quite happy to have been of help. May I ask what kind of project this code was for? School work? Working on a game?

Thanks, Acero. This solved the problem perfectly. :)

[The Fifth Horseman]

A tool to convert sprites from Space Hulk's non-standard format to PCX.