Extracting and cleaning up duplicate emails

Question

Right now I am using a regex to extract emails from a text document, and filter the duplicates out of this using the Distinct function from linq. After which I run the output file through a clean up of duplicates as well, as the program can be run several times over several different files.

Now my question, would there be a more optimal way of checking the output file for duplicates? Because as it is right now, the bigger the file, the longer it will take.

And I have the feeling there should be a less intense way to check this.

// Extractor
public void Mail(string file)
{
    string strRegex = @"[A-Za-z0-9_\-\+]+@[A-Za-z0-9\-]+\.([A-Za-z]{2,3})(?:\.[a-z]{2})?";
    var myRegex = new Regex(strRegex, RegexOptions.None);

    var matches = new List<string>();
    foreach (Match myMatch in myRegex.Matches(file))
    {
        if (myMatch.Success)
        {
            matches.Add(myMatch.Value);
        }
    }

    var cleanMatch = matches.Distinct().ToList();

    for (var i = 0; i < cleanMatch.Count; i++)
    {
        Log.Mail(cleanMatch[i]);
    }

    CleanDuplicates();
}

// Duplicate Cleaner
private void CleanDuplicates()
{
    var lines = new List<string>(System.IO.File.ReadAllLines(System.Environment.GetFolderPath(System.Environment.SpecialFolder.DesktopDirectory) + "\\EmailList.txt"));

    lines = lines.Distinct().ToList();
    string target = System.Environment.GetFolderPath(System.Environment.SpecialFolder.DesktopDirectory) + "\\EmailList.txt";
    using (System.IO.StreamWriter newTask = new System.IO.StreamWriter(target, false))
    {
        for (var i = 0; i < lines.Count; i++)
        {
            newTask.WriteLine(lines[i]);
        }
    }
}

Answer 1

You'll be able to simplify this a lot if you use a better in memory datastructure than a List<T>. You only want distinct items, HashSet<T> will generally be better here as it has O(1) lookups.

public void Mail(string file)
{
    var strRegex = @"[A-Za-z0-9_\-\+]+@[A-Za-z0-9\-]+\.([A-Za-z]{2,3})(?:\.[a-z]{2})?";
    var myRegex = new Regex(strRegex, RegexOptions.None);
    var matches = new HashSet<string>();
    foreach (Match matchedValue in myRegex.Matches(file))
    {
        // Changed per Mjolka's comment
        if (matches.Add(matchedValue.Value))
        {
            Log.Mail(matchedValue.Value);
        }
    }
    CleanDuplicates();
}

Note that I've removed the uneccessary check for Success on the Match object.

Your naming could do with a bit of work myRegex doesn't really mean anything.

You could also store the Regex as a field on the class to declutter this method.

You can leverage a HashSet<string> in your CleanDuplicates method too:

private void CleanDuplicates()
{
    var fileLocation = System.Environment.GetFolderPath(System.Environment.SpecialFolder.DesktopDirectory) + @"\EmailList.txt";

    var lines = new HashSet<string>(File.ReadAllLines(fileLocation));

    File.WriteAllLines(fileLocation, lines.ToArray());
}

Definitely add a using System.IO it removes a lot of clutter.

Prefer built in methods e.g. File.WriteAllLines overwrites the file or creates a new one - no need to whip out a StreamWriter.

Also note that if your file becomes really large this will start getting pretty slow.

Final(?) Edit

Can't use var in the foreach loop as MatchCollection implements IEnumerable only - not the generic version. Sorry about that.

In order to use a Linq method (such as Select) we have to supply the type by calling Cast<T>:

foreach (var matchedValue in myRegex.Matches(file).Cast<Match>().Select(m => m.Value))
{
    if (matches.Add(matchedValue))
    {
        Log.Mail(matchedValue);
    }
}

Answer 2

var lines = new List<string>(System.IO.File.ReadAllLines(System.Environment.GetFolderPath(System.Environment.SpecialFolder.DesktopDirectory) + "\\EmailList.txt"));

lines = lines.Distinct().ToList();
string target = System.Environment.GetFolderPath(System.Environment.SpecialFolder.DesktopDirectory) + "\\EmailList.txt";
using (System.IO.StreamWriter newTask = new System.IO.StreamWriter(target, false))
{
    for (var i = 0; i < lines.Count; i++)
    {
        newTask.WriteLine(lines[i]);
    }
}

Calling .ToList() here is a pointless operation. ToList produces an indexable collection, but also evaluates everything in one go. Since all you're doing is iterating over the collection, stick with IEnumerable and use a foreach loop, as it makes your intentions clearer.

Secondly, you should be using var in your using statement, because the type of newTask is clear from the assignment.

You're duplicating your concatanation of the desktop directory and "EmailList.txt", as RobH says, put that in a string variable, and you'll only have to change that once. Better yet, refactor the magic string "EmailList.txt" into a const.

Why not use Path.Combine instead of string concatenation anyway?

Lastly, I'm seeing a lot of fully-qualified types. Not just System.IO, but System too.

//Outside of method:
const EmailListFileName = "EmailList.txt";

//Inside of method:
var emailListPath = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.DesktopDirectory), EmailListFileName);

var lines = File.ReadAllLines(emailListPath).Distinct();

using (var newTask = new StreamWriter(emailListPath, false))
{
    foreach(var line in lines)
    {
        newTask.WriteLine(line);
    }
}

Answer 3

Couple of things.

var matches = new HashSet<string>();
foreach (var myMatch in myRegex.Matches(file))
{
    if (myMatch.Success)
    {
        if(matches.Add(myMatch.Value))
        {
            Log.Mail(myMatch);
        }
    }
}

Use a hash set. If you add an item that is already in the list, it will ignore it. If the add is successful (it retuns a bool) then you can log it immediately.

You have written out to disk the discovered emails across multiple files. This is slow. I would suggest doing it in memory for the batch of files.

Answer 4

It's unfortunate that MatchCollection only implements the non-generic ICollection (though that's being fixed), but we can use Cast<T> to get around that. This is how I would suggest writing the main part of the first method:

var emails = myRegex.Matches(file)
    .Cast<Match>()
    .Select(match => match.Value)
    .Distinct();
foreach (var email in emails)
{
    Log.Mail(email);
}

Distinct is implemented using a hash set, so performance will be good.