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<User> Find(string query)
{
// internal search query
List<User> 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<User> 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<User>())
.Results.ToList();
}
catch { return new List<User>(); }
}
else if (_dateRegex.IsMatch(query.Replace(".","/").Replace("-","/"))) // input was a birthday
{
query = query.Replace(".","/").Replace("-","/");
DateTimeOffset dto = DateTimeOffset.Parse(query);
try
{
return Graph.Instance.Cypher
.Match("(user:User)")
.Where((User user) => user.DateOfBirth.ToString() == dto.Date.ToString())
.Return(user => user.As<User>())
.Results.ToList();
}
catch { return new List<User>(); }
}
else if (_phoneRegex.IsMatch(query.Replace("(","").Replace(")","").Replace("-","").Trim()))
{
try
{
query = query.Replace("(", "").Replace(")", "").Replace("-", "").Trim();
return Graph.Instance.Cypher
.Match("(user:User)")
.Where((User user) => user.PhoneNumber == query)
.AndWhere("user.Privacy_AllowPhoneLookup = true")
.Return(user => user.As<User>())
.Results.ToList();
}
catch { return new List<User>(); }
}
else
{
try
{
query = WebUtility.HtmlDecode(query);
query = System.Globalization.CultureInfo.CurrentCulture.TextInfo.ToTitleCase(query);
ICypherFluentQuery q = Graph.Instance.Cypher
.Match("(user:User)")
.Where((User user) => user.FullName == query)
.OrWhere((User user) => user.FirstName == query)
.OrWhere((User user) => user.LastName == query)
.OrWhere((User user) => user.Handle == query.ToLower())
.OrWhere((User user) => user.Id.ToString() == query);
List<string> tokens = query.Split(new Char[] { ' ', ',' }, StringSplitOptions.RemoveEmptyEntries).ToList();
if (tokens != null && tokens.Count > 0)
{
foreach (string token in tokens)
{
string t = System.Globalization.CultureInfo.CurrentCulture.TextInfo.ToTitleCase(token);
q = q.OrWhere((User user) => user.FullName == t)
.OrWhere((User user) => user.FirstName == t)
.OrWhere((User user) => user.LastName == t)
.OrWhere((User user) => user.Handle == t)
.OrWhere((User user) => user.Id.ToString() == t.ToLower());
}
}
return q
.ReturnDistinct(user => user.As<User>())
.Results.ToList();
}
catch (Exception ex)
{
Logger.Error(ex);
return new List<User>();
}
}
}
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<User>())
.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<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>(); }
}
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<User>();
}
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.