7
\$\begingroup\$

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]);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Does the order of the items in the file matter? \$\endgroup\$ – RobH May 14 '15 at 8:12
  • \$\begingroup\$ @RobH Order is unimportant \$\endgroup\$ – MX D May 14 '15 at 8:12
8
\$\begingroup\$

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);
    }
}
\$\endgroup\$
  • \$\begingroup\$ HashSet.Add returns a bool indicating if the item was added, so you can use that instead of the call to Contains. \$\endgroup\$ – mjolka May 14 '15 at 11:41
  • \$\begingroup\$ Great point mjolka - I thought there was something that did that... Couldn't remember, should have looked at the docs. \$\endgroup\$ – RobH May 14 '15 at 12:16
  • \$\begingroup\$ matchedValue.Value doesn't work. With your edit, matchedValue returns as a object, make it a string matchedValue, or converting it to string in the loop fixes it though. \$\endgroup\$ – MX D May 14 '15 at 13:15
  • \$\begingroup\$ @MXD - I've actually put it through a compiler now so will edit with more details :) \$\endgroup\$ – RobH May 14 '15 at 13:19
3
\$\begingroup\$
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);
    }
}
\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. As I did not include the actual writer, it appends onto a existing file. Making 1 file grow bigger over time. As for the files it gets them from, they go one by one. So I am not sure what you are trying to say here. \$\endgroup\$ – MX D May 14 '15 at 8:01
  • 1
    \$\begingroup\$ All of this could be magnitudes more efficient if you'd use a HashSet<T>. \$\endgroup\$ – Jeroen Vannevel May 14 '15 at 11:56
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.