Skip to content

Prefix-penalty scoring is a permanent no-op, and its test can't catch that #66

Description

@matt-edmondson

What's wrong

In FuzzySearch/Fuzzy.cs, PenalizeNonPatternCharacters:

internal const int unmatchedPrefixLetterPenalty = 0;
internal const int maxPrefixPenalty = 0;
...
int penalty = Math.Max(strIdx * unmatchedPrefixLetterPenalty, maxPrefixPenalty);

With unmatchedPrefixLetterPenalty = 0, strIdx * 0 is always 0, and Math.Max(0, 0) is always 0 — so no penalty is ever applied for characters preceding the first match, regardless of how far into the subject the match starts.

Why it matters / concrete failure scenario

Contains("xxxxxxxxxxfoo", "foo") and Contains("foo", "foo") receive identical prefix treatment even though a documented purpose of this code path is to reward matches nearer the start of the string — matches starting deep into a long subject score exactly the same as matches at position 0 on this dimension, degrading result ranking quality for typical search-as-you-type use.

This is compounded by FuzzySearch.Test/FuzzyTests.cs → PenalizeNonPatternCharacters_FirstPatternChar_AppliesPrefixPenalty, which computes its expected value using the same constants (Math.Max(strIdx * Fuzzy.unmatchedPrefixLetterPenalty, Fuzzy.maxPrefixPenalty)). Because the test derives its expectation from the same zero constants rather than an independent expected value, it passes trivially and can never catch a regression or confirm the feature does anything at all.

Related, same file: CalculateScore builds a List<int> matchedIndices (via two Add calls) that is never read, returned, or exposed anywhere — since Contains calls CalculateScore on every invocation, this is a pure-waste allocation on every scored match, worth cleaning up alongside this.

Suggested fix

Decide deliberately whether the prefix penalty is a real feature or dead weight:

  • If intended: restore non-zero values for unmatchedPrefixLetterPenalty/maxPrefixPenalty (tuned via real test cases), and rewrite the test to assert a fixed expected score rather than recomputing it from the same constants under test.
  • If not intended: delete PenalizeNonPatternCharacters and the now-pointless test.

Either way, remove the dead matchedIndices list and its two Add calls in CalculateScore.

Acceptance criteria

  • A match starting later in the subject string scores lower (or the feature is deliberately removed with the misleading test deleted) — the outcome is a documented, intentional decision, not silent dead code.
  • The prefix-penalty test computes its expected value independently of the constants under test, so it can actually catch a regression.

Metadata

Metadata

Labels

bugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions