Severity: Info (DRY / maintainability — two copies of the same escape-aware tokenizer in two files; bugs found in one will need to be fixed in the other separately)
Files:
src/Servy.Core/EnvironmentVariables/EnvironmentVariablesValidator.cs
src/Servy.Core/EnvironmentVariables/EnvironmentVariableParser.cs
Code: Both files implement the same backslash-aware splitter / index-finder, with byte-for-byte identical logic and minor cosmetic differences:
| Helper |
Validator |
Parser |
SplitByUnescapedDelimiters |
EnvironmentVariablesValidator.cs:76-109 |
EnvironmentVariableParser.cs:71-104 |
IndexOfUnescapedChar |
EnvironmentVariablesValidator.cs:155-180 |
EnvironmentVariableParser.cs:112-137 |
CountUnescapedChar |
EnvironmentVariablesValidator.cs:117-144 |
(not present — only the parser doesn't use it) |
Unescape |
(not present) |
EnvironmentVariableParser.cs:144-181 |
What's wrong:
-
Two copies of the splitter. The two classes are even in the same Servy.Core.EnvironmentVariables namespace and live next to each other in the source tree. There is no reason for two private/public copies of the exact same algorithm — both are pure functions over (string, char[])/(string, char) with no state.
-
Inconsistent visibility for the same thing. EnvironmentVariablesValidator.IndexOfUnescapedChar is public (line 155), while EnvironmentVariableParser.IndexOfUnescapedChar is private (line 112). Future callers can't tell which one to consume.
-
Cosmetic drift already starting. Comment wording differs ("If even number of backslashes -> delimiter is unescaped" vs the parser's variant), method-level XML doc differs, the parser's variant says "The backslash is preserved so later Unescape can handle sequences correctly" — accurate only in that file. If someone fixes a bug in the validator (say, a unicode-edge issue in counting backslashes) they will silently leave the parser broken, or vice versa.
This is the same class of duplication as #872 (Helper.EscapeArgs ↔ ProcessHelper.EscapeProcessArgument), just in a different domain (env-var parsing instead of Win32 argv escaping).
Suggested fix: Hoist the three helpers (SplitByUnescapedDelimiters, IndexOfUnescapedChar, CountUnescapedChar, plus the parser's Unescape) into a single internal static class EscapedTokenizer (or EnvironmentVariableTokenHelper) in Servy.Core.EnvironmentVariables. Both EnvironmentVariablesValidator and EnvironmentVariableParser then become thin facades over the tokenizer:
namespace Servy.Core.EnvironmentVariables
{
internal static class EscapedTokenizer
{
public static string[] SplitByUnescapedDelimiters(string input, char[] delimiters) { ... }
public static int IndexOfUnescapedChar(string str, char ch) { ... }
public static int CountUnescapedChar(string str, char ch) { ... }
public static string Unescape(string input) { ... }
}
}
Then Validator.Validate and Parser.Parse each shrink by ~40 lines and a future bug fix automatically applies to both flows. Existing public surface stays compatible if you keep EnvironmentVariablesValidator.IndexOfUnescapedChar as a forwarding wrapper.
Severity: Info (DRY / maintainability — two copies of the same escape-aware tokenizer in two files; bugs found in one will need to be fixed in the other separately)
Files:
src/Servy.Core/EnvironmentVariables/EnvironmentVariablesValidator.cssrc/Servy.Core/EnvironmentVariables/EnvironmentVariableParser.csCode: Both files implement the same backslash-aware splitter / index-finder, with byte-for-byte identical logic and minor cosmetic differences:
SplitByUnescapedDelimitersEnvironmentVariablesValidator.cs:76-109EnvironmentVariableParser.cs:71-104IndexOfUnescapedCharEnvironmentVariablesValidator.cs:155-180EnvironmentVariableParser.cs:112-137CountUnescapedCharEnvironmentVariablesValidator.cs:117-144UnescapeEnvironmentVariableParser.cs:144-181What's wrong:
Two copies of the splitter. The two classes are even in the same
Servy.Core.EnvironmentVariablesnamespace and live next to each other in the source tree. There is no reason for two private/public copies of the exact same algorithm — both are pure functions over(string, char[])/(string, char)with no state.Inconsistent visibility for the same thing.
EnvironmentVariablesValidator.IndexOfUnescapedCharis public (line 155), whileEnvironmentVariableParser.IndexOfUnescapedCharis private (line 112). Future callers can't tell which one to consume.Cosmetic drift already starting. Comment wording differs ("If even number of backslashes -> delimiter is unescaped" vs the parser's variant), method-level XML doc differs, the parser's variant says "The backslash is preserved so later Unescape can handle sequences correctly" — accurate only in that file. If someone fixes a bug in the validator (say, a unicode-edge issue in counting backslashes) they will silently leave the parser broken, or vice versa.
This is the same class of duplication as #872 (
Helper.EscapeArgs↔ProcessHelper.EscapeProcessArgument), just in a different domain (env-var parsing instead of Win32 argv escaping).Suggested fix: Hoist the three helpers (
SplitByUnescapedDelimiters,IndexOfUnescapedChar,CountUnescapedChar, plus the parser'sUnescape) into a singleinternal static class EscapedTokenizer(orEnvironmentVariableTokenHelper) inServy.Core.EnvironmentVariables. BothEnvironmentVariablesValidatorandEnvironmentVariableParserthen become thin facades over the tokenizer:Then
Validator.ValidateandParser.Parseeach shrink by ~40 lines and a future bug fix automatically applies to both flows. Existing public surface stays compatible if you keepEnvironmentVariablesValidator.IndexOfUnescapedCharas a forwarding wrapper.