Code Doesn't Work Properly

[MasterKay]

My code doesn't show results after pressing 1 or 2. It just become blank. Why is this?

#include <iostream>
#include <string>
#include <fstream>

using namespace std;

size_t readNames(string filename, string surnames[], string firstnames[]);
void sortSurnameFirst (string [], string[], size_t);
void displayNames (string [], string[], size_t);
void sortFirstNames (string [], string [], size_t);

int main()
{
int const SIZE = 1000;
size_t count = 0;
int choice;

string names[SIZE];
string surnames[SIZE];

ifstream inputFile;
string filename;
int number;

cout << "Enter the file name: ";
cin >> filename;

inputFile.open(filename.c_str());

if(inputFile)
{

while(count <SIZE && inputFile>>surnames[count] >> names[count])
{
count ++;
}

inputFile.close();

cout << " 1. Sort names by surnames then first names "<<endl;
cout << " 2. Sort names by first names then by surnames "<<endl;
cout<< "Enter your sorting choice: " ;
cin >> choice;

if (choice == 1)
{
cout<< "The names are sorted by surnames then first names"<<endl;
sortSurnameFirst (surnames, names,count);
displayNames(surnames, names,count);
}

if (choice == 2)
{
cout<< "The names are sorted by first names then surnames"<<endl;
sortSurnameFirst (surnames, names,count);
displayNames(surnames, names,count);
}

}

else
{
cout << "The following file doesnt exist" << endl;
}



return 0;
}






void sortSurnameFirst (string surnames[], string names[], size_t size)
{
bool swap;
string sort ;
string temp;
do
{
swap = false;
for (size_t i =0; i < size ; i++)
{
if (surnames[i] > surnames[i++])
{
sort = surnames[i];
surnames[i] = surnames[i++];
surnames[i++] = sort ;
swap = true;


}


if ( surnames[i] == surnames[i++] )
{
if (names[i] > names[i++] )
{
temp= names[i];
names[i] = names[i++];
names[i++] = temp ;
sort = surnames[i];
surnames[i] = surnames[i++];
surnames[i++] = sort ;

swap = true;
}
}



}

}
while (swap);
}


void displayNames( string surnames[], string names[], size_t size )
{

for ( size_t i =0; i < (size - 1) ; i++)
{
cout << surnames[i]<<" " << names[i] << endl;
}
}


void sortFirstNames (string surnames[], string names[], size_t size)
{
bool swap;
string sort1, temp1;
do
{
swap = false;

for ( size_t i =0; i < size ; i++)
{
if ( names[i] >names[i++] )
{
sort1 = names[i];
names[i] = names[i++];
names[i++] = sort1 ;
swap = true;

}
if ( names[i] == names[i++] )
{
if (surnames[i] > surnames[i++] )
{
temp1 = surnames[i];
surnames[i] = surnames[i++];
surnames[i++] = temp1 ;
sort1 = names[i];
names[i] = names[i++];
names[i++] = sort1 ;
swap = true;
}
}



}

}while (swap);
}

[newbieg]

This is a difficult kind of error. It isn't a syntax error so it has slipped past the compiler. This is going to require a bit of patience to find the problem.

When your program just hangs like that it usually means that there is an infinite loop somewhere in your program. I generally start poking around with a line of code like cout << "made it here".
I start in main, dropping it bellow a couple of function calls at a time. If it stops showing up then I can usually say that the problem function is above this cout statement. I then drop it into the function that I suspect to be the problem. (Make sure to cut and paste instead of copy/paste since you need to know exactly where this line is located.)

If I can get it into the infinite loop where it just continually re-displays, then I've located the problem and I know at least where the logic error is in general.

This is what I did with your code, and I can tell you that the problem is with your void sortSurnameFirst (string surnames[], string names[], size_t size) function. (you also appear to be calling this same function whether the user chooses 1 or 2).

Put the cout statement inside the outermost loop first. You'll see it loops forever. This means that your sentry variable "swap" is never false. This means that the next loop in never returns with swap equal to false.

So the final answer that I'm going to give is that the logic inside your for loop in sortSurnameFirst() is the key issue.
if (surnames[i] > surnames[i++]) is bad logic. The ++ operator actually changes the value of i, and worse, it doesn't change it until after the comparison so you are testing element[0] against element[0], setting element[1] = element[1] and the outer loop just makes it all happen over again. The if statement should be written if (surnames[i] > surnames[i+1]). This error is repeated several times in this one loop. This is not the end to the problems with the current version of your code, but I hope it's a good starting point for finding the rest.

[ne555]

Not an error, but...

|| foo.cpp: In function ‘int main()’:
foo.cpp|22 col 6| warning: unused variable ‘number’ [-Wunused-variable]
||   int number;
||       ^~~~~~
|| foo.cpp: In function ‘void sortSurnameFirst(std::__cxx11::string*, std::__cxx11::string*, size_t)’:
foo.cpp|70 col 31| warning: operation on ‘i’ may be undefined [-Wsequence-point]
||     if(surnames[i] > surnames[i++]) {
||                               ~^~
foo.cpp|72 col 29| warning: operation on ‘i’ may be undefined [-Wsequence-point]
||      surnames[i] = surnames[i++];
||                             ~^~
foo.cpp|77 col 32| warning: operation on ‘i’ may be undefined [-Wsequence-point]
||     if(surnames[i] == surnames[i++]) {
||                                ~^~
foo.cpp|78 col 26| warning: operation on ‘i’ may be undefined [-Wsequence-point]
||      if(names[i] > names[i++]) {
||                          ~^~
foo.cpp|80 col 24| warning: operation on ‘i’ may be undefined [-Wsequence-point]
||       names[i] = names[i++];
||                        ~^~
foo.cpp|83 col 30| warning: operation on ‘i’ may be undefined [-Wsequence-point]
||       surnames[i] = surnames[i++];
||                              ~^~
|| foo.cpp: In function ‘void sortFirstNames(std::__cxx11::string*, std::__cxx11::string*, size_t)’:
foo.cpp|108 col 25| warning: operation on ‘i’ may be undefined [-Wsequence-point]
||     if(names[i] > names[i++]) {
||                         ~^~
foo.cpp|110 col 23| warning: operation on ‘i’ may be undefined [-Wsequence-point]
||      names[i] = names[i++];
||                       ~^~
foo.cpp|114 col 26| warning: operation on ‘i’ may be undefined [-Wsequence-point]
||     if(names[i] == names[i++]) {
||                          ~^~
foo.cpp|115 col 32| warning: operation on ‘i’ may be undefined [-Wsequence-point]
||      if(surnames[i] > surnames[i++]) {
||                                ~^~
foo.cpp|117 col 30| warning: operation on ‘i’ may be undefined [-Wsequence-point]
||       surnames[i] = surnames[i++];
||                              ~^~
foo.cpp|120 col 24| warning: operation on ‘i’ may be undefined [-Wsequence-point]
||       names[i] = names[i++];
||                        ~^~

> When your program just hangs like that it usually means that there is an
> infinite loop somewhere in your program. I generally start poking around with
> a line of code like cout << "made it here".
that's horrible.
You could simply run through a debugger, when the program freezes, send an interruption and the debugger will show the last line to be executed.
five seconds.


> The ++ operator actually changes the value of i, and worse, it doesn't change
> it until after the comparison
I'll bet it's undefined behaviour.
I think that clang is more explicit

warning: unsequenced modification and access to 'i' [-Wunsequenced]

[newbieg]

@ne555
I usually have a good Idea where my code breaks since I like to test-compile as often as I can. I'm usually able to find the error without much use of the above method, but it was helpful when I was first starting out. I agree it is not the most efficient method to find one error, but it gives me time to get associated with the code and coding style that I'm looking at.

I usually stick to the terminal/console rather than IDE's, so I would be stuck trying to figure out how to use gdb which just does not look fun.

What debugger or IDE do you tend to use?

I do like that you've got all warnings turned on, and I'm now thinking of aliasing g++ with the -Wall -Wextra commands.

[newbieg]

OK, the alias is set in my .bashrc

alias g++="g++ -Wall -Wextra"

and it's working nicely.

[ne555]

gdb is not that hard to use.
also, valgrind helps to detect invalid access.

[TheIdeasMan]

@newbieg

One should also specify the standard to compile to:

g++ -std=c++14 -Wall -Wextra -pedantic-errors

With later versions of g++, the default is gnu++14, which is not quite the same thing.

I routinely compile with these options as well:

http://www.cplusplus.com/forum/general/183731/#msg899203

Version 7.1 of g++ is now out, it has support for c++17.

[MasterKay]

You guys didn't help me... :(

[newbieg]

@MaterKay
You are spamming the forums with these posts. People have shown you how to fix the code in all the posts that you made two days ago. You spammed again today with exactly the same code, no effort on your part to correct the errors. We are talking about proper debugging on a linux/unix system since that's what you and I need to learn how to do and this is posted in the *nix forums.

@TheIdeasMan
Thanks. I'll try pedantic-errors as well. I did read that there was one option that turned absolutely everything on, but that it was so verbose that it rendered the thing useless, and it starts warning about perfectly acceptable c++ code. (Probably because it turned on -Wtraditional) I don't want to get to that point.
That -Wfloat-equal looks like something I definitely want as well (I had to test it myself, I am really surprised that -Wall -Wextra -pedantic-errors didn't actually turn on -Wfloat-equal. Thank you again.

OK, there's a lot of information about common coding issues (things to avoid) in the man pages for g++. I'm going to spend today reading those. [I almost forgot that I also need to look up how to actually use gdb, thanks ne555]

[Edit: G++ man pages:

-pedantic
Issue all the mandatory diagnostics listed in the C standard. Some of them are left out by default, since they trigger frequently on harmless code.

-pedantic-errors
Issue all the mandatory diagnostics, and make all mandatory diagnostics into errors. This includes mandatory diagnostics that GCC issues without
-pedantic but treats as warnings.

Ah, so actually it's pedantic that I want to avoid at this point... maybe I'll alias a version with pedantic and call it pg++ or something... I agree that it's a good idea to be as compliant as possible, but I don't want to put it in my g++ alias in case I forget it's there and it starts causing issues.]

[newbieg]

A good starting tutorial for gdb was found here: https://www.cs.umd.edu/~srhuang/teaching/cmsc212/gdb-tutorial-handout.pdf

Note that they too have -pedantic-errors turned on in their examples... Hmm. Maybe it's not as likely to cause problems as I was thinking...

[ Edit: Sorry Ideasman, you wouldn't have suggested -pedantic-errors if it didn't work for you.
I just realized that the -W[tag] that the error is instanced in is shown in the error information, so that would indicate to a forgetful me in the future that the alias exists.]

My current alias is

alias g++="g++ -std=c++14 -Wall -Wextra -pedantic-errors -Wfloat-equal -Wuninitialized -Wconversion -Wfloat-conversion"

[benhart]

I think the problem is somewhere in this loop, It might goes out of bounds. Better check that, and detect the error as fast as possible. I also recommend detecting those as errors just before they to hard to find. If your'e having problems doing that alone, you can get some help from program like checkmarx and others. I, anyways, recommend learning to do it also on your own.
Good luck!
Ben.

[Cubbi]

newbieg wrote:

so actually it's pedantic that I want to avoid at this point... maybe I'll alias a version with pedantic and call it pg++ or something.

the most important effect of -pedantic-errors is that it turns off non-portable language extensions, turning g++ into a standard C++ compiler The standard selection option like -std=c++14 is not enough: it makes sure g++ will accept valid C++14 programs, but it won't reject invalid C++14 programs if they happen to use a GNU language extension.

[Thomas1965]

Actually the worst spammer here is this guy(?) called benhart. Just mentioning the name of a particular program so that the search engines will increase it's ranking. Does anybody know why he/she gets away with it?

@OP,
I am not sure where you saw sth. like this if (surnames[i] > surnames[i++]) but in general it's bad practice to modify the loop counter inside a foor loop.
What you should try is:if (surnames[i] > surnames[i+1]) in all places.
Also you need to change the for loop to for ( size_t i =0; i < size -1 ; i++)

[gunnerfunner]

change the for loop to for ( size_t i = 0; i < size -1; i++)

the most kosher way to do this, of course:

for (std::string::size_type i = 0; i < size - 1; ++i)

but I'm as guilty as anyone else of relapsing to size_t most of the time admittedly

[Thomas1965]

but I'm as guilty as anyone else of relapsing to size_t most of the time admittedly

@gunnerfunner,
if you use Visual Studio this is fine.

For type string, it is equivalent to size_t.

https://msdn.microsoft.com/en-us/library/5ddehwe8(v=vs.110).aspx

[gunnerfunner]

if you use Visual Studio this is fine.

bit too platform-dependent wouldn't you say Thomas?

It is conceivable that someone might give our program a string so long that an int is insufficient to contain its length.

- Accelerated C++ by Koenig.

[Thomas1965]

Personally I don't care that much about platform independence. I am a hobby programmer, writing programs for myself or friends and they all use Windows. Only once I wrote a little app for a Mac and then I used Java.

It is conceivable that someone might give our program a string so long that an int is insufficient to contain its length.
- Accelerated C++ by Koenig.

Difficult to imagine. On 32bit systems a size_t has a length of ~4GB - more that a 32bit CPU can handle.

[mbozzi]

The container's size_type depends on the allocator.

See, e.g., this Stack Overflow answer
http://stackoverflow.com/a/12499353

The implication is that you're probably fine using size_t in a specific (i.e., non-generic) context. In contexts where the allocator is unknown it's incorrect to use it in favor of blah::size_type, although I'm certain I've written that bug many times.