Searching and printing valid emails

[gizmo7008]

I'm doing a program, and trying to find the @, going back to the beginning to search for valid characters, and when I find an invalid one I move it forward to valid one. Same goes for the end, except just leaving it at the invalid character. My program compiles, however after it takes the input and output files, it just sits there. Nothing appears after it tells me my input and output files. I can't figure out why.

#include <fstream>
#include <iostream>
#include <string>
using namespace std;

class toLower{public: char operator() (char c) const {return tolower(c) ;}};

bool isValidCharacter(char c)
{
  bool validCheck = false;
  if ((c>='A' && c<= 'Z') || (c>='a' && c<='z') || (c>='0' && c<='9') || (c=='.') || (c=='-') || (c=='+'))
    validCheck = true;
  return validCheck;
}

int main()
{
  ifstream fin;

  string inputFile;
  string outputFile;
  string defaultInput = "fileContainingEmails.txt";
  string defaultOutput = "copyPasteMyEmails.txt";

  cout << "Enter input filename[default: fileContainingEmails.txt]: ";
  getline(cin, inputFile);

  if (inputFile.length() == 0)
  {
    inputFile = defaultInput;
    outputFile = defaultOutput;
    cout << "Enter output filename[default: copyPasteMyEmails.txt]: ";
    getline(cin,outputFile);

    if (outputFile.length() == 0)
      outputFile = defaultOutput;
  }
  else
  {
    string c;
    cout << "Enter output filename[default: " << inputFile << "]: ";
    getline(cin, c);

    if (c.length() == 0)
      outputFile = inputFile;
    else
      outputFile = c;
  }

  cout << "Input file: " << inputFile << endl;
  cout << "Output file: " << outputFile << endl;

  fin.open(inputFile.c_str());
  if (!fin.good()) throw "I/O error";

  string emails;
  int i;
  while(getline(fin,emails))
  {
    for(i = 0; i < emails.length(); i++)
    {
      if(emails[i] == '@')
      {
        int s = i;
        for(s; s <= emails.length(); s--)
        {
        if (s < 0) break;
        if (isValidCharacter(emails[s]) == false)
    {
      s++;
          if(isValidCharacter(emails[s]) == true) break;
    }
    }
        bool hasDot = false;
        int e = i;
    for(e; e <= emails.length(); e++)
    {
        if(e = emails.length()) break;
        if(isValidCharacter(emails[e]) == false) break;
        if(e == '.')
      hasDot = true;
    }
    if((s<i) && (e>i) && (hasDot == true))
      cout << emails << endl;

       } 
      }
    }

  cout << endl;

  return 0;
}  

[WaltP]

First off, you should use WHILE statements instead of FOR statements. You are not iterating through the string a set number of times, only until a specific condition is met. This will also let you process the string in 3 distinct segments:
1) find @ and end this loop
2) find beginning and end this loop
3) find end and end this loop
Each loop should be only about 2-3 statements

You have the loops interleaved making it harder to follow (also your indentation is inconsitent also making it hard)

[Labdabeta]

There are a number of issues with your code. The first one is merely for clarity and speed. Basically the isValidCharacter function can be 1 line:
state

return ((c>='A' && c<= 'Z') || (c>='a' && c<='z') || (c>='0' && c<='9') || (c=='.') || (c=='-') || (c=='+'));

Thus removing the if statement.

Your problem though lies in your loops. For example the following loop:

for (s; s<=emails.length; s--)

Unless s is greater than the emails.length this loop will not work as expected. Instead of counting down to 0 (for (s;s>=0;s--) 543210) it counts down to a number greater than it: 543210-1-2-3-4-5...-n+n... basically, until the int type wraps around to be greater than emails.length your loop will not terminate! You correct this using a break statement, but that is considered very poor programming practice!

I have no idea what you are getting at in lines 68 to 72, but I would definately suggest that it is your problem. You should usually not change the iterator character in a loop. If needed make a temporary copy of it instead.

line 78 is redundant, and another bad usage of the break statement. Put it this way, the break statement is only truly valid is switch statements (at least as far as I can think of off the top of my head, I love to be proven wrong).

I would suggest making your code confirm to proper programming pratices and then reposting if you can't solve the problem yourself. You may be surprised by how much easier it is to find a problem in a well written program.

[DeanMSands3]

Yeah, what WaltP said. Edit: And Labdabeta.
Ouch. Ouch. Ouch. This code really hurt me. I kept thinking that a finite state machine version would be a better approach. Ask if you're interested.
Back to your code, I debugged far enough to get it sort of working. It took a lot longer than I'd have liked it to.

int s=i; should be int s=i-1;

                    if (isValidCharacter(emails[s]) == false)
                    {
                        s++;
                        if(isValidCharacter(emails[s]) == true) break;
                    }

should be

                    if (isValidCharacter(emails[s]) == false)
                    {
                        s++;
                        if(isValidCharacter(emails[s]) == true) break;
                        else if(s==i) break;
                    }

int e = i; should be int e = i+1;

if(e = emails.length()) break; should be if(e == emails.length()) break;

if(e == '.') should be if(emails[e] == '.')

There's a few other changes I made.

Here's a full listing of my code with lines added for debugging. It's not %100, but it's better than nothing.

#include <fstream>
#include <iostream>
#include <string>
using namespace std;

class toLower{public: char operator() (char c) const {return tolower(c) ;}};

bool isValidCharacter(char c)
{
    bool validCheck = false;
    if ((c>='A' && c<= 'Z') || (c>='a' && c<='z') || (c>='0' && c<='9') || (c=='.') || (c=='-') || (c=='+'))
    validCheck = true;
    return validCheck;
}

int main()
{
    ifstream fin;

    string inputFile;
    string outputFile;
    string defaultInput = "fileContainingEmails.txt";
    string defaultOutput = "copyPasteMyEmails.txt";

    cout << "Enter input filename[default: fileContainingEmails.txt]: ";
    getline(cin, inputFile);

    if (inputFile.length() == 0)
    {
        inputFile = defaultInput;
        outputFile = defaultOutput;
        cout << "Enter output filename[default: copyPasteMyEmails.txt]: ";
        getline(cin,outputFile);

        if (outputFile.length() == 0)
        outputFile = defaultOutput;
    }
    else
    {
        string c;
        cout << "Enter output filename[default: " << inputFile << "]: ";
        getline(cin, c);

        if (c.length() == 0)
        outputFile = inputFile;
        else
        outputFile = c;
    }

    cout << "Input file: " << inputFile << endl;
    cout << "Output file: " << outputFile << endl;

    fin.open(inputFile.c_str());
    if (!fin.good()) throw "I/O error";

    string emails;
    int i;
    while(getline(fin,emails))
    {
        cout<<"Line: "<<emails<<endl;
        cout<<"Searching for '@'"<<endl;
        for(i = 0; i < emails.length(); i++)
        {
            cout<<i<<":"<<emails[i]<<"\t";
            if(emails[i] == '@')
            {
                cout<<"@ found at index: "<<i<<endl;
                int s = i-1;
                for(s; s <= emails.length(); s--)
                {
                    cout<<s<<"\t";
                    if (s < 0){
                        s++;break;
                    }
                    if (isValidCharacter(emails[s]) == false)
                    {
                        s++;
                        if(isValidCharacter(emails[s]) == true) break;
                        else if(s==i) break;
                    }
                }
                if(s==i) break;
                cout<<"Searching for '.'"<<endl;
                bool hasDot = false;
                int e = i+1;
                for(e; e <= emails.length(); e++)
                {
                    cout<<e<<":"<<emails[e]<<"\t";
                    if(e == emails.length()) break;
                    if(isValidCharacter(emails[e]) == false) break;
                    if(emails[e] == '.'){
                        hasDot = true;
                        cout<<"'.' found at index: "<<e<<endl;
                    }
                }
                if((s<i) && (e>i) && (hasDot == true)){
                    cout << emails << endl;
                }else{
                    cout<<"s:"<<s<<endl;
                    cout<<"i:"<<i<<endl;
                    cout<<"e:"<<e<<endl;
                }
                break;
            } 
        }
        cout<<endl;
    }

    cout << endl;

    return 0;
}  

[WaltP]

There are a number of issues with your code. The first one is merely for clarity and speed. Basically the isValidCharacter function can be 1 line:

For speed, yes. Clarity, no. IMO, your 1-liner is not as clear as the original. That's not to say the original is clear, either. It really isn't.

In my years of programming, I've found you program for clarity or speed. Rarely can you program for both.

Here's a full listing of my code with lines added for debugging.

Why? It's going to take longer for the OP to compare your code with his to get any real understanding (he's new, remember) so it would be easier just to replace his with yours. That's not what this board is about.

And a finite state machine??!?? He can't even parse an email! How is he supposed to program a finite state machine? And what would his instructor say about it? ;-)

[gizmo7008]

I changed my loops a bit to what I could understand from everyone. The program compiles still, and runs. It is still waiting after the files are entered. It lists my input and output file but no emails.

This is what I changed the loops to:

string emails;
int i;
  while(getline(fin,emails))
  {
    for(i = 0; i < emails.length(); i++)
    {
      if(emails[i] == '@')
      {
        int s = i;
        while(true)
        {
          if (s < 0) break;
          if (isValidCharacter(emails[s]) == false)
          {    
            s++;
            if(isValidCharacter(emails[s]) == true) break;
            else if(s == i) break;
          }
         }
        bool hasDot = false;
        int e = i;
        while(true)
       {
         if(e == emails.length()) break;
         if(isValidCharacter(emails[e]) == false) break;
         if(emails[e] == '.')
         hasDot = true;
       }
       if((s<i) && (e>i) && (hasDot == true))
         cout << emails << endl;

     } 
   } 
}

[WaltP]

1) You are still using a FOR loop
2) You are still nesting your loops

How about:

While Loop to find the @
  End of loop

From @ position:
While loop to find beginning
  End of loop

From @ position:
While loop to find end
  End of loop

Do NOT use while(true), that is not an appropriate condition in your case. You are looking for something specific.

[gizmo7008]

Like this? It is 3 separate loops now.

while(getline(fin,emails))
  {
      while(i < emails.length())
      {
        i = 0;
        i++;
        if(emails[i] == '@') break;
      }
      int s = i;
      while(s > 0)
      {
        s--;
        if (s < 0) break;
        if (isValidCharacter(emails[s]) == false)
        {
          s++;
          if(isValidCharacter(emails[s]) == true) break;
          else if(s == i) break;
        }
      }
      bool hasDot = false;
      int e = i;
      while(e != emails.length())
      {
        e++;
        if(e == emails.length()) break;
        if(isValidCharacter(emails[e]) == false) break;
        if(emails[e] == '.')
          hasDot = true;
      }
      if((s<i) && (e>i) && (hasDot == true))
        cout << emails << endl;

    }
  }

[WaltP]

Repeat#1: You are looking for something specific!!!

while(i < emails.length())
while(s > 0)
while(e != emails.length())

Not one of those while statements look for anything specific

Repeat #2: Each loop should be only about 2-3 statements

[gizmo7008]

I'm looking for valid characters, is that what I need to put in the while loop statement? Otherwise, I'm not sure I'm understanding completely.

[WaltP]

I'm looking for valid characters, is that what I need to put in the while loop statement?

BINGO!!!

Of course, you also have to correctly handle the cases when no valid character is found.

[gizmo7008]

Okay, here is what I have now. It doesn't wait now after entering the input/output files, but it doesn't put down the emails. It has a termination error.
"terminate called throwing an exceptionAbort trap: 6"

  while(getline(fin,emails))
  {
      while(atSymbol(emails[i]) == true)
      {
        i = 0;
        i++;
        if(emails[i] == '@') break;
      }
      int s = i;
      while(isValidCharacter(emails[s]) == true)
      {
        s--;
        if (s < 0) break;
        if (isValidCharacter(emails[s]) == false)
        {
          s++;
          if(isValidCharacter(emails[s]) == true) break;
          else if(s == i) break;
        }
      }
      bool hasDot = false;
      int e = i;
      while(isValidCharacter(emails[e]) == true)
      {
        e++;
        if(e == emails.length()) break;
        if(isValidCharacter(emails[e]) == false) break;
        if(emails[e] == '.')
          hasDot = true;
      }
      string anEmail = emails.substr(s,e-s);
      if((s<i) && (e>i) && (hasDot == true))
        cout << emails << endl;

    }

[WaltP]

Concentrate carefully on this loop:

while(atSymbol(emails[i]) == true)
{
    i = 0;
    i++;
    if(emails[i] == '@') break;
}

1) In the while, aren't you looking specifically for an @?
2) Is the break really necessary with the proper while statement?
3) Comment each line to explain the reason for the line.
4) Use pencil and paper and execute the loop to see if it does the correct thing.

[gizmo7008]

Okay, I've figured it out. My program is now working. Thanks!