Skip to content

[Code Quality] EnvironmentVariablesValidator and EnvironmentVariableParser — duplicated 'SplitByUnescapedDelimiters' and 'IndexOfUnescapedChar' implementations (DRY) #901

@Christophe-Rogiers

Description

@Christophe-Rogiers

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:

  1. 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.

  2. 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.

  3. 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.EscapeArgsProcessHelper.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.

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions