patterncsharpMinor
Facebook-esque User Search
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
```
// 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())
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.