HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Facebook-esque User Search

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
esquesearchfacebookuser

Problem

I'm looking for some opinions on improvement/technique of code in C# calling to a Neo4j database to perform a multi-faceted query looking for users. My goals were to lookup by first/last name, phone number, birthday, and e-mail address. I wanted case insensitivity, and I want the query to respect certain privacy settings that users can set (i.e. can users look me up via phone number?), and a block list.

I use regexes and some CultureInfo code to put things into a standard format, but I feel that my code is a little bloated and could be made smaller or more optimized.

```
// Social Function, Non-Throwing //
// Searches for any and all users matching full name, first name, last name, e-mail, birth date, or phone number in the given search query.
private static Regex _emailRegex = new Regex("[-0-9a-zA-Z.+_]+@[-0-9a-zA-Z.+_]+\\.[a-zA-Z]{2,4}");
private static Regex _dateRegex = new Regex(@"^((0?[13578]|10|12)(-|\/)(([1-9])|(0[1-9])|([12])([0-9]?)|(3[01]?))(-|\/)((19)([2-9])(\d{1})|(20)([01])(\d{1})|([8901])(\d{1}))|(0?[2469]|11)(-|\/)(([1-9])|(0[1-9])|([12])([0-9]?)|(3[0]?))(-|\/)((19)([2-9])(\d{1})|(20)([01])(\d{1})|([8901])(\d{1})))$");
private static Regex _phoneRegex = new Regex("[0-9]+");
public List Find(string query)
{
// internal search query
List results = doFind(query);

// return empty results if we found no one
if (results.Count == 0)
return results;

// remove users from results if we blocked them or they blocked us
results.RemoveAll(x => x.IsBlocked(this.Id));

return results;
}

private static List doFind(string query)
{
query = query.Trim();

if (_emailRegex.IsMatch(query)) // input was an e-mail address
{
try
{
return Graph.Instance.Cypher
.Match("(user:User)")
.Where((User user) => user.Email == query)
.AndWhere("user.Privacy_AllowEMailLookup = true")
.Return(user => user.As())

Solution

doFind should be PascalCase.

I would make each of those cases their own method, e.g. GetUserByEmail etc. That way your 100+ lines long doFind becomes more manageable, especially if there are new filters added in the future.

I see a lot of repeated code:

Graph.Instance.Cypher
.Match("(user:User)")


And:

.Return(user => user.As())
.Results.ToList();


Isn't it possible to keep this logic, as well as the try...catch in the doFind and have the various checks return an Expression or a Query that you then can use, so you'd end up with something like this:

private static List doFind(string query)
{      
    ICypherFluentQuery whereExpression;

    if (_emailRegex.IsMatch(query)) // input was an e-mail address
    {
        whereExpression = GetUsersByEmail();
    }
    else if (_dateRegex.IsMatch(query.Replace(".","/").Replace("-","/"))) // input was a birthday
    {
        whereExpression = GetUsersByBirthDay();
    }
    else if (_phoneRegex.IsMatch(query.Replace("(","").Replace(")","").Replace("-","").Trim()))
    {
        whereExpression = GetUsersByPhone();
    }
    else 
    {
        whereExpression = GetUsers();
    }

    try 
    {
        return Graph.Instance.Cypher
            .Match("(user:User)")
            .Apply(whereExpression)
            .ReturnDistinct(user => user.As())
            .Results.ToList();
    }
    catch { return new List(); }
}


This is example code -- I don't know neo4j, so I don't know if this is possible.

Also: is that try...catch really necessary? It certainly looks dangerous: an exception is thrown and it is just ignored and an empty list is return.

.Replace(".","/").Replace("-","/") and .Replace("(","").Replace(")","").Replace("-","") are repeated, and thus they should be moved to methods.

Is this the proper way to do this: user.DateOfBirth.ToString() == dto.Date.ToString()? Can't you just compare these dates directly?

This looks odd:

catch (Exception ex)
{
    Logger.Error(ex);
    return new List();
}


At least this time you're logging the exception (unlike above), but again you're returning an empty list. So unless someone checks the error log, no one is any the wiser that an exception has occurred.

Code Snippets

Graph.Instance.Cypher
.Match("(user:User)")
.Return(user => user.As<User>())
.Results.ToList();
private static List<User> doFind(string query)
{      
    ICypherFluentQuery whereExpression;

    if (_emailRegex.IsMatch(query)) // input was an e-mail address
    {
        whereExpression = GetUsersByEmail();
    }
    else if (_dateRegex.IsMatch(query.Replace(".","/").Replace("-","/"))) // input was a birthday
    {
        whereExpression = GetUsersByBirthDay();
    }
    else if (_phoneRegex.IsMatch(query.Replace("(","").Replace(")","").Replace("-","").Trim()))
    {
        whereExpression = GetUsersByPhone();
    }
    else 
    {
        whereExpression = GetUsers();
    }

    try 
    {
        return Graph.Instance.Cypher
            .Match("(user:User)")
            .Apply(whereExpression)
            .ReturnDistinct(user => user.As<User>())
            .Results.ToList();
    }
    catch { return new List<User>(); }
}
catch (Exception ex)
{
    Logger.Error(ex);
    return new List<User>();
}

Context

StackExchange Code Review Q#93214, answer score: 2

Revisions (0)

No revisions yet.