From d4b66838b529f33507ab49546e503d729775a89d Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Wed, 27 May 2026 16:22:45 -0500 Subject: [PATCH] refactor(dedup): extract self-duplicated helpers in three src/ files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tackles the in-file copy-paste duplications SonarQube flagged on `sql-utilities`, narrowing the dedup target to the cases where the extraction is a clear readability win. - `LinqToSql.Converters.ReverseConverterExtensions`: the three `ToLinqQueryBreakdown` overloads (SqlServer / PostgreSql / Snowflake) had identical 26-line bodies. Routes all three through a single `BuildLinqBreakdownFrom(QueryBreakdown)` private helper — works because Snowflake/PostgreSql `QueryBreakdown` derive from the SqlServer one, so the parameter type accepts all three. Public API preserved. - `Markdown.Expressions.ExpressionGenerator`: `VisitInExpression` and `VisitNotInExpression` had identical 18-line bodies differing only in the "IN"/"NOT IN" label. Both now delegate to a new private `RenderInList(label, searchExpression, values)`. - `PostgreSql.Statements.StatementExpressionParser`: the qualified- column-name building loop and the column-id switch were duplicated across `HandleStringToken` (qualified-column branch) and `GrabColumnExpression`. Extracted to a shared `BuildQualifiedColumnExpression(seededBuilder, reader)` private helper. Deliberately *not* refactored: `PostgreSql.Statements.StatementReader`'s `<` / `>` operator handlers, which Sonar also flags as duplicate. The shared pattern there is a structural sequence of "MovePosition; character check; emit Token; return" repeated across single-/two-char operator variants; folding it into a helper would replace four short, self-explanatory inline checks with `TryMatchTwoCharOperator('=', ...)` indirection that obscures what each branch actually emits. The dedup isn't worth the readability tax. All 1180 tests stay green. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Converters/ReverseConverterExtensions.cs | 54 +++---------------- .../Expressions/ExpressionGenerator.cs | 28 +++------- .../Statements/StatementExpressionParser.cs | 45 +++------------- 3 files changed, 23 insertions(+), 104 deletions(-) diff --git a/src/Strata.SqlTools.LinqToSql/Converters/ReverseConverterExtensions.cs b/src/Strata.SqlTools.LinqToSql/Converters/ReverseConverterExtensions.cs index 9f5cc55..dc3a214 100644 --- a/src/Strata.SqlTools.LinqToSql/Converters/ReverseConverterExtensions.cs +++ b/src/Strata.SqlTools.LinqToSql/Converters/ReverseConverterExtensions.cs @@ -19,29 +19,7 @@ public static class ReverseConverterExtensions public static LinqQueryBreakdown ToLinqQueryBreakdown(this QueryBreakdown breakdown) { ArgumentNullException.ThrowIfNull(breakdown); - - var linq = new LinqQueryBreakdown( - breakdown.SelectClause?.Clause ?? "*", - breakdown.FromClause?.Clause ?? string.Empty, - breakdown.WhereClause?.Clause ?? string.Empty - ); - - if (!string.IsNullOrWhiteSpace(breakdown.GroupByClause?.Clause)) - { - linq.GroupByClause.Clause = breakdown.GroupByClause.Clause; - } - - if (!string.IsNullOrWhiteSpace(breakdown.HavingClause?.Clause)) - { - linq.HavingClause.Clause = breakdown.HavingClause.Clause; - } - - if (!string.IsNullOrWhiteSpace(breakdown.OrderByClause?.Clause)) - { - linq.OrderByClause.Clause = breakdown.OrderByClause.Clause; - } - - return linq; + return BuildLinqBreakdownFrom(breakdown); } /// @@ -52,29 +30,7 @@ public static class ReverseConverterExtensions public static LinqQueryBreakdown ToLinqQueryBreakdown(this PostgreSqlBreakdown breakdown) { ArgumentNullException.ThrowIfNull(breakdown); - - var linq = new LinqQueryBreakdown( - breakdown.SelectClause?.Clause ?? "*", - breakdown.FromClause?.Clause ?? string.Empty, - breakdown.WhereClause?.Clause ?? string.Empty - ); - - if (!string.IsNullOrWhiteSpace(breakdown.GroupByClause?.Clause)) - { - linq.GroupByClause.Clause = breakdown.GroupByClause.Clause; - } - - if (!string.IsNullOrWhiteSpace(breakdown.HavingClause?.Clause)) - { - linq.HavingClause.Clause = breakdown.HavingClause.Clause; - } - - if (!string.IsNullOrWhiteSpace(breakdown.OrderByClause?.Clause)) - { - linq.OrderByClause.Clause = breakdown.OrderByClause.Clause; - } - - return linq; + return BuildLinqBreakdownFrom(breakdown); } /// @@ -85,7 +41,13 @@ public static class ReverseConverterExtensions public static LinqQueryBreakdown ToLinqQueryBreakdown(this SnowflakeBreakdown breakdown) { ArgumentNullException.ThrowIfNull(breakdown); + return BuildLinqBreakdownFrom(breakdown); + } + // Shared body — PostgreSql/Snowflake QueryBreakdown derive from SqlServer.QueryBreakdown, + // so all three public overloads can flow through this single helper. + private static LinqQueryBreakdown BuildLinqBreakdownFrom(QueryBreakdown breakdown) + { var linq = new LinqQueryBreakdown( breakdown.SelectClause?.Clause ?? "*", breakdown.FromClause?.Clause ?? string.Empty, diff --git a/src/Strata.SqlTools.Markdown/Expressions/ExpressionGenerator.cs b/src/Strata.SqlTools.Markdown/Expressions/ExpressionGenerator.cs index 8f19153..99b384a 100644 --- a/src/Strata.SqlTools.Markdown/Expressions/ExpressionGenerator.cs +++ b/src/Strata.SqlTools.Markdown/Expressions/ExpressionGenerator.cs @@ -485,37 +485,23 @@ public class ExpressionGenerator : IVisitor } public string VisitInExpression(InExpression inExpression) - { - var sb = new StringBuilder(); - sb.AppendLine($"{Indent()}IN:"); - _indentLevel++; - sb.AppendLine($"{Indent()}Search Expression:"); - _indentLevel++; - sb.AppendLine(inExpression.SearchExpression.Accept(this)); - _indentLevel--; - sb.AppendLine($"{Indent()}Values:"); - _indentLevel++; - foreach (var value in inExpression.ValuesToCompare) - { - sb.AppendLine(value.Accept(this)); - } - _indentLevel--; - _indentLevel--; - return sb.ToString(); - } + => RenderInList("IN", inExpression.SearchExpression, inExpression.ValuesToCompare); public string VisitNotInExpression(NotInExpression inExpression) + => RenderInList("NOT IN", inExpression.SearchExpression, inExpression.ValuesToCompare); + + private string RenderInList(string label, Expression searchExpression, IEnumerable values) { var sb = new StringBuilder(); - sb.AppendLine($"{Indent()}NOT IN:"); + sb.AppendLine($"{Indent()}{label}:"); _indentLevel++; sb.AppendLine($"{Indent()}Search Expression:"); _indentLevel++; - sb.AppendLine(inExpression.SearchExpression.Accept(this)); + sb.AppendLine(searchExpression.Accept(this)); _indentLevel--; sb.AppendLine($"{Indent()}Values:"); _indentLevel++; - foreach (var value in inExpression.ValuesToCompare) + foreach (var value in values) { sb.AppendLine(value.Accept(this)); } diff --git a/src/Strata.SqlTools.PostgreSql/Statements/StatementExpressionParser.cs b/src/Strata.SqlTools.PostgreSql/Statements/StatementExpressionParser.cs index fc3cf9e..d55642f 100644 --- a/src/Strata.SqlTools.PostgreSql/Statements/StatementExpressionParser.cs +++ b/src/Strata.SqlTools.PostgreSql/Statements/StatementExpressionParser.cs @@ -121,39 +121,7 @@ public class StatementExpressionParser : SqlServerStatementExpressionParser // Check if this is a qualified column name (e.g., users.id) if (reader.TokenType == TokenType.Operator && reader.TokenValue == ".") { - // Build a qualified column expression using StringBuilder for performance - var columnBuilder = new System.Text.StringBuilder(startingToken); - while (reader.TokenType == TokenType.Operator && reader.TokenValue == ".") - { - reader.Read(); // Skip the dot - - if (reader.TokenType == TokenType.String || reader.TokenType == TokenType.ColumnIdentifier) - { - columnBuilder.Append('.').Append(reader.TokenValue); - reader.Read(); - } - else - { - throw new InvalidSyntaxException( - $"Invalid syntax at position {reader.Position}. Expected column identifier after dot."); - } - } - - var columnToken = columnBuilder.ToString(); - - // Return a column expression for the qualified name - var dataColumnId = GetColumnIdFromToken(columnToken); - var tableSource = new RegisteredTableSource(1001, "FW", "DEPARTMENT", "DEPT"); - return dataColumnId switch - { - 1 => new RegisteredTableColumnExpression(dataColumnId, "DEPARTMENT_ID", tableSource), - 2 => new RegisteredTableColumnExpression(dataColumnId, "NAME", tableSource), - 3 => new RegisteredTableColumnExpression(dataColumnId, "REVENUE", tableSource), - 4 => new RegisteredTableColumnExpression(dataColumnId, "DISCHARGE_DATE", tableSource), - 586883 => new RegisteredTableColumnExpression(dataColumnId, "FIXED_COST", tableSource), - 586664 => new RegisteredTableColumnExpression(dataColumnId, "VARIABLE_COST", tableSource), - _ => new RegisteredTableColumnExpression(dataColumnId, GetDefaultColumnName(columnToken), tableSource) - }; + return BuildQualifiedColumnExpression(new System.Text.StringBuilder(startingToken), reader); } // Not a qualified column, treat as a string expression @@ -182,9 +150,14 @@ public class StatementExpressionParser : SqlServerStatementExpressionParser { var columnBuilder = new System.Text.StringBuilder(reader.TokenValue); reader.Read(); + return BuildQualifiedColumnExpression(columnBuilder, reader); + } - // Handle qualified names: table.column, "Table"."Column", etc. - // Keep reading while we see dot-separated identifiers + // Consumes dot-separated identifier segments from the reader, appending each to the seeded builder, + // then maps the resulting qualified name to a RegisteredTableColumnExpression. Shared between + // HandleStringToken (qualified column path) and GrabColumnExpression (entry-point path). + private RegisteredTableColumnExpression BuildQualifiedColumnExpression(System.Text.StringBuilder columnBuilder, IStatementReader reader) + { while (reader.TokenType == TokenType.Operator && reader.TokenValue == ".") { reader.Read(); // Skip the dot @@ -202,8 +175,6 @@ public class StatementExpressionParser : SqlServerStatementExpressionParser } var columnToken = columnBuilder.ToString(); - - // Use base implementation to get the column expression var dataColumnId = GetColumnIdFromToken(columnToken); var tableSource = new RegisteredTableSource(1001, "FW", "DEPARTMENT", "DEPT"); return dataColumnId switch