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

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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