Skip to main content
added 853 characters in body
Source Link
Adriano Repetti
  • 10.5k
  • 1
  • 23
  • 47

For WHERE clause you'd better use the same approach of EF and go with Expression, it's a big step in complexity but much easier to use at calling point and much more safe. You may like to read Building LINQ Queries at Runtime in C# for inspiration. After this first step I'd split function into multiple parts, each one with a single responsibility. If you like to give names you may call it chain of responsibilities. It may be as easy as multiple private functions invoked through Reflection or a more complex mechanism. For example:

Note that _query is an instance field (the parameter of Translate() method). Now I'd like to address another problem: you're roaming around with a SQL string and errors will be catched only at run-time. Multiple places with string concatenation means that you have to test every single possible code path with all possible relevant values. Nice to have but I'd like to make it little bit more safe! You may introduce a command builder class, it's pretty handy especially for JOIN ANDand SELECT:

var command = new SqlCommandBuilderCommandBuilder();

if (query.Operators.AnyOfType<DqlSelectOperator>())
{
    command.AddSelect(ops.FirstOfType<DqlSelectOperator>().Fields,
        query.TargetEntityType.Name);
}
else
{
    command.AddSelect(query.TargetEntityType.Name);
}
private const string SqlCommandSelect = "SELECT";
private const string SqlFromClause = "FROM";

public SqlCommandBuilderCommandBuilder AddSelect(IEnumerable<string> fields, string tableName)
{
    _sql.AppendLine(SqlCommandSelect)
        .AppendLine(String.Join(",\n", fields.Select(x => QuoteSqlIdentifierQuoteIdentifier(x)))
        .AppendLine($"{SqlFromClause} {QuoteSqlIdentifierQuoteIdentifier(tableName)}"); 

    return this;
}

Of course code it'sis not complete but I guess you get the point. Note the call to QuoteSqlIdentifierQuoteIdentifier() (to be written), identifiers must be properly quoted! The two overloaded versions may even be merged (if (fields?.Count()) ?? 0 == 0 then just add *). Fields may be prefixed with table alias (useful in case of complex joins to avoid ambiguity.)

Now that you have a SqlCommandBuilderCommandBuilder class you can move SQL parameters inside it and your Translate() function will simply return that object. You may add handy helper methods to transform this command into a SqlCommand (SQL command text + parameters) or to a plain SQL string (for debugging purposes). If you know your building a SqlCommand (or similar) you can directly add parameters there, no need for an ExpandObject or a Dictionary<string, object>. Something similar, but with completely different purpose, can be seen in SqlCommandBuilder class.

I think you're almost done but there is still a thing that hurts me. All those AnyOfType() and FirstOfType(). Can you specify more than one DqlSelectOperator operator? Isn't it an error? Yes, it is then caller must be warned of this. You introduced your own FirstOfType<T>() extension method then also add SingleOfType<T>() (or use plain syntax .OfType<T>().Single()): if query is broken then it won't silently do something (probably unexpected).


Last note: things may be more complicate if you're using this interface to abstract queries to different repositories (let's say you have a query language for relational databases, NoSQL databases, XML databases/files, JSON files, etc...) In this caseyou need to return ICommandBuilder and you will need few specializations (one for each target format):

public interface IQueryTranslator
{
    Type SupportedType { get; }
    ICommandBuilder Translate(Query query);
}

Don't be tempted to always return a string, parameters validation/sanitization isn't an easy thing.

For WHERE clause you'd better use the same approach of EF and go with Expression, it's a big step in complexity but much easier to use at calling point and much more safe. After this first step I'd split function into multiple parts, each one with a single responsibility. If you like to give names you may call it chain of responsibilities. It may be as easy as multiple private functions invoked through Reflection or a more complex mechanism. For example:

Note that _query is an instance field (the parameter of Translate() method). Now I'd like to address another problem: you're roaming around with a SQL string and errors will be catched only at run-time. Multiple places with string concatenation means that you have to test every single possible code path with all possible relevant values. Nice to have but I'd like to make it little bit more safe! You may introduce a command builder class, it's pretty handy especially for JOIN AND SELECT:

var command = new SqlCommandBuilder();

if (query.Operators.AnyOfType<DqlSelectOperator>())
{
    command.AddSelect(ops.FirstOfType<DqlSelectOperator>().Fields,
        query.TargetEntityType.Name);
}
else
{
    command.AddSelect(query.TargetEntityType.Name);
}
private const string SqlCommandSelect = "SELECT";
private const string SqlFromClause = "FROM";

public SqlCommandBuilder AddSelect(IEnumerable<string> fields, string tableName)
{
    _sql.AppendLine(SqlCommandSelect)
        .AppendLine(String.Join(",\n", fields.Select(x => QuoteSqlIdentifier(x)))
        .AppendLine($"{SqlFromClause} {QuoteSqlIdentifier(tableName)}");
}

Of course code it's not complete but I guess you get the point. Note the call to QuoteSqlIdentifier() (to be written), identifiers must be properly quoted! The two overloaded versions may even be merged (if (fields?.Count()) ?? 0 == 0 then just add *). Fields may be prefixed with table alias (useful in case of complex joins to avoid ambiguity.)

Now that you have a SqlCommandBuilder class you can move SQL parameters inside it and your Translate() function will simply return that object. You may add handy helper methods to transform this command into a SqlCommand (SQL command text + parameters) or to a plain SQL string (for debugging purposes). If you know your building a SqlCommand (or similar) you can directly add parameters there, no need for an ExpandObject or a Dictionary<string, object>.

I think you're almost done but there is still a thing that hurts me. All those AnyOfType() and FirstOfType(). Can you specify more than one DqlSelectOperator operator? Isn't it an error? Yes, it is then caller must be warned of this. You introduced your own FirstOfType<T>() extension method then also add SingleOfType<T>() (or use plain syntax .OfType<T>().Single()): if query is broken then it won't silently do something (probably unexpected).

For WHERE clause you'd better use the same approach of EF and go with Expression, it's a big step in complexity but much easier to use at calling point and much more safe. You may like to read Building LINQ Queries at Runtime in C# for inspiration. After this first step I'd split function into multiple parts, each one with a single responsibility. If you like to give names you may call it chain of responsibilities. It may be as easy as multiple private functions invoked through Reflection or a more complex mechanism. For example:

Note that _query is an instance field (the parameter of Translate() method). Now I'd like to address another problem: you're roaming around with a SQL string and errors will be catched only at run-time. Multiple places with string concatenation means that you have to test every single possible code path with all possible relevant values. Nice to have but I'd like to make it little bit more safe! You may introduce a command builder class, it's pretty handy especially for JOIN and SELECT:

var command = new CommandBuilder();

if (query.Operators.AnyOfType<DqlSelectOperator>())
{
    command.AddSelect(ops.FirstOfType<DqlSelectOperator>().Fields,
        query.TargetEntityType.Name);
}
else
{
    command.AddSelect(query.TargetEntityType.Name);
}
private const string SqlCommandSelect = "SELECT";
private const string SqlFromClause = "FROM";

public CommandBuilder AddSelect(IEnumerable<string> fields, string tableName)
{
    _sql.AppendLine(SqlCommandSelect)
        .AppendLine(String.Join(",\n", fields.Select(x => QuoteIdentifier(x)))
        .AppendLine($"{SqlFromClause} {QuoteIdentifier(tableName)}"); 

    return this;
}

Of course code is not complete but I guess you get the point. Note the call to QuoteIdentifier() (to be written), identifiers must be properly quoted! The two overloaded versions may even be merged (if (fields?.Count()) ?? 0 == 0 then just add *). Fields may be prefixed with table alias (useful in case of complex joins to avoid ambiguity.)

Now that you have a CommandBuilder class you can move SQL parameters inside it and your Translate() function will simply return that object. You may add handy helper methods to transform this command into a SqlCommand (SQL command text + parameters) or to a plain SQL string (for debugging purposes). If you know your building a SqlCommand (or similar) you can directly add parameters there, no need for an ExpandObject or a Dictionary<string, object>. Something similar, but with completely different purpose, can be seen in SqlCommandBuilder class.

I think you're almost done but there is still a thing that hurts me. All those AnyOfType() and FirstOfType(). Can you specify more than one DqlSelectOperator operator? Isn't it an error? Yes, it is then caller must be warned of this. You introduced your own FirstOfType<T>() extension method then also add SingleOfType<T>() (or use plain syntax .OfType<T>().Single()): if query is broken then it won't silently do something (probably unexpected).


Last note: things may be more complicate if you're using this interface to abstract queries to different repositories (let's say you have a query language for relational databases, NoSQL databases, XML databases/files, JSON files, etc...) In this caseyou need to return ICommandBuilder and you will need few specializations (one for each target format):

public interface IQueryTranslator
{
    Type SupportedType { get; }
    ICommandBuilder Translate(Query query);
}

Don't be tempted to always return a string, parameters validation/sanitization isn't an easy thing.

added 176 characters in body
Source Link
Adriano Repetti
  • 10.5k
  • 1
  • 23
  • 47

For WHERE clause you'd better use the same approach of EF and go with Expression, it's a big step in complexity but much easier to use at calling point and much more safe. After this first step I'd split function into multiple parts, each one with a single responsibility. If you like to give names you may call it chain of responsibilities. It may be as easy as multiple private functions invoked through Reflection or a more complex mechanism. For example:

After this first step I'd split function into multiple parts, each one with a single responsibility. If you like to give names you may call it chain of responsibilities. It may be as easy as multiple private functions invoked through Reflection or a more complex mechanism. For example:

For WHERE clause you'd better use the same approach of EF and go with Expression, it's a big step in complexity but much easier to use at calling point and much more safe. After this first step I'd split function into multiple parts, each one with a single responsibility. If you like to give names you may call it chain of responsibilities. It may be as easy as multiple private functions invoked through Reflection or a more complex mechanism. For example:

added 186 characters in body
Source Link
Adriano Repetti
  • 10.5k
  • 1
  • 23
  • 47

A proper code review would need more details about your design, especially about DqlSelectQuery (especially purpose and intended use), then I'd limit myself to few things in your code. It will be more about design than aboutvisible code...

 

First things I'd consider to change is string interpolation. In this moment you're, in my opinion, abusing this feature. It's very handy for short interpolated expressions but you're extensively using this language feature with long expressions and it's killing readability. Don't go to refactor this now...

Now that you have a SqlCommandBuilder class you can move SQL parameters inside it and your Translate() function will simply return that object. You may add handy helper methods to transform this command into a SqlCommand (SQL command text + parameters) or to a plain SQL string (for debugging purposes). If you know your building a SqlCommand (or similar) you can directly add parameters there, no need for an ExpandObject or a Dictionary<string, object>.

A proper code review would need more details about your design, especially about DqlSelectQuery, then I'd limit myself to few things in your code. It will be more about design than about code...

First things I'd consider to change is string interpolation. In this moment you're, in my opinion, abusing this feature. It's very handy for short interpolated expressions but you're extensively using this language feature and it's killing readability. Don't go to refactor this now...

Now that you have a SqlCommandBuilder class you can move SQL parameters inside it and your Translate() function will simply return that object. You may add handy helper methods to transform this command into a SqlCommand (SQL command text + parameters) or to a plain SQL string (for debugging purposes).

A proper code review would need more details about your design, especially about DqlSelectQuery (especially purpose and intended use), then I'd limit myself to few things in your visible code.

 

First things I'd consider to change is string interpolation. In this moment you're, in my opinion, abusing this feature. It's very handy for short interpolated expressions but you're extensively using this language feature with long expressions and it's killing readability. Don't go to refactor this now...

Now that you have a SqlCommandBuilder class you can move SQL parameters inside it and your Translate() function will simply return that object. You may add handy helper methods to transform this command into a SqlCommand (SQL command text + parameters) or to a plain SQL string (for debugging purposes). If you know your building a SqlCommand (or similar) you can directly add parameters there, no need for an ExpandObject or a Dictionary<string, object>.

Source Link
Adriano Repetti
  • 10.5k
  • 1
  • 23
  • 47
Loading