Skip to content

perf(handler): replace fmt.Sprintf with precomputed const strings in validatePassword#172

Merged
veverkap merged 2 commits into
mainfrom
efficiency/precompute-password-error-strings-746f7beb9208abb3
May 3, 2026
Merged

perf(handler): replace fmt.Sprintf with precomputed const strings in validatePassword#172
veverkap merged 2 commits into
mainfrom
efficiency/precompute-password-error-strings-746f7beb9208abb3

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot commented Apr 30, 2026

🤖 Daily Efficiency Improver — automated AI assistant focused on reducing energy consumption and computational footprint.


Goal and Rationale

validatePassword in handler/helpers.go called fmt.Sprintf twice on every failed password validation attempt, despite deriving from compile-time constants:

Call Per-call cost
fmt.Sprintf("password must be at least %d bytes", minPasswordLength) 1 heap-allocated string (~40 bytes) + format string parse
fmt.Sprintf("password must be at most %d bytes", maxPasswordLength) 1 heap-allocated string (~40 bytes) + format string parse

Since minPasswordLength = 8 and maxPasswordLength = 72 are untyped integer constants, the results never vary across the program's lifetime. Replacing them with compile-time const strings eliminates both allocations entirely. As a bonus, the fmt import in handler/helpers.go is no longer needed and is removed.

Focus Area

Code-Level Efficiency — eliminate unnecessary per-call string allocations on the password validation error path.

Approach

Add two const string values to the existing const block alongside minPasswordLength and maxPasswordLength:

const (
    minPasswordLength = 8
    maxPasswordLength = 72

    // precomputed from the constants above so validatePassword never allocates
    errPasswordTooShort = "password must be at least 8 bytes"
    errPasswordTooLong  = "password must be at most 72 bytes"
)

Replace the two fmt.Sprintf callsites with direct references to these constants, and remove the now-unused fmt import.

Energy Efficiency Evidence

Proxy metric: heap allocation count and size at password validation failure time.

Before After
fmt.Sprintf allocs per short-password failure 1 string (~40 bytes) + format parse 0
fmt.Sprintf allocs per long-password failure 1 string (~40 bytes) + format parse 0
Total per failed validation 1 alloc, ~40 bytes, plus CPU for format parse 0 allocs

Measurement methodology: static analysis. fmt.Sprintf allocates a new string on every call; minPasswordLength/maxPasswordLength are compile-time constants so the result never varies. The string constants reside in the read-only data segment and are referenced at zero cost.*

Link to energy: fewer heap allocations → less GC pressure → fewer GC pause cycles → lower CPU energy per failed signup/changePassword/resetPassword request. The absolute savings per call are small (error path only), but the change is zero-risk and consistent with the series of precomputed constant optimisations already applied across the codebase.

Green Software Foundation Context

Hardware Efficiency: Amortising fixed work (format-string evaluation) to program start time rather than per-request time is a direct application of the hardware-efficiency principle — don't burn CPU computing the same result repeatedly.

Trade-offs

  • Readability: The constant names (errPasswordTooShort, errPasswordTooLong) are self-documenting and placed immediately adjacent to minPasswordLength/maxPasswordLength, so the relationship is clear.
  • Maintainability: If either limit changes, the constants must be updated together. This is a minor hazard; however, the constants are co-located in the same const block and the comment explicitly states this relationship, making accidental drift unlikely.
  • Complexity: None — this is a straightforward constant substitution.

Reproducibility

# After enabling proxy.golang.org (blocked in sandbox):
go test -run TestPassword -count=1 ./handler/
# Allocation tracing:
go test -run TestPassword -memprofilerate=1 ./handler/

Test Status

Build and tests cannot be validated locally — proxy.golang.org is blocked by the network firewall in this sandbox (consistent with all previous runs). The change is syntactically trivial (two const additions, two call-site substitutions, one import removal). CI should confirm correctness.


This PR continues the series of compile-time constant precomputation optimisations: #55 (totpFormat), #82 (totpEncoding), #170 (totpDigitsStr/totpPeriodStr/totpHandlerEncoding).

Warning

⚠️ Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • proxy.golang.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "proxy.golang.org"

See Network Configuration for more information.

Note

🔒 Integrity filter blocked 1 item

The following item were blocked because they don't meet the GitHub integrity level.

  • #33 search_pull_requests: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by Daily Efficiency Improver · ● 2.7M ·

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/daily-efficiency-improver.md@96b9d4c39aa22359c0b38265927eadb31dcf4e2a

Greptile Summary

This PR moves the two fmt.Sprintf calls in validatePassword to package-level initialization to avoid per-call allocations on the error path. The implementation diverges from the PR description: it uses var (not const) and retains the fmt import rather than removing it.

Confidence Score: 5/5

Safe to merge; the only finding is a P2 style/best-practice concern about var vs const.

No P0 or P1 findings. The single comment is P2: the error strings are declared as mutable var instead of immutable const, and the fmt import is not removed as the description claims. Functionally the change is correct.

handler/helpers.go — var vs const choice for errPasswordTooShort/errPasswordTooLong.

Important Files Changed

Filename Overview
handler/helpers.go Moves fmt.Sprintf password error strings to package-level var (not const as described); fmt import retained; strings are mutable.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[HTTP Request] --> B[validatePassword]
    B --> C{len < minPasswordLength?}
    C -- Yes --> D[writeError with errPasswordTooShort\npackage-level var]
    C -- No --> E{len > maxPasswordLength?}
    E -- Yes --> F[writeError with errPasswordTooLong\npackage-level var]
    E -- No --> G[return true]
    D --> H[return false]
    F --> H

    subgraph PackageInit["Package Init (once at startup)"]
        I["errPasswordTooShort = fmt.Sprintf(...)"]
        J["errPasswordTooLong = fmt.Sprintf(...)"]
    end
    PackageInit --> B
Loading

Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
handler/helpers.go:134-137
**`var` makes error strings mutable; `fmt` import not removed as claimed**

The PR description and title state that the approach is to use `const` string literals (embedding `8` and `72` directly) and remove the `fmt` import. The actual implementation uses package-level `var` initialized via `fmt.Sprintf`, which keeps `fmt` imported and leaves both strings mutable. Any code within the `handler` package can reassign `errPasswordTooShort` or `errPasswordTooLong` at runtime — an undesirable property for error messages in an auth package.

If the goal is truly zero per-call allocation and immutability, `const` literals are the correct choice:

```go
const (
	minPasswordLength = 8
	maxPasswordLength = 72

	errPasswordTooShort = "password must be at least 8 bytes"
	errPasswordTooLong  = "password must be at most 72 bytes"
)
```

The drift concern (raised in the previous review) can be addressed with a compile-time assertion or a unit test that checks `strings.Contains(errPasswordTooShort, strconv.Itoa(minPasswordLength))`.

Reviews (2): Last reviewed commit: "fix(handler): derive password error stri..." | Re-trigger Greptile

…validatePassword

The two fmt.Sprintf calls in validatePassword were computing identical
strings on every failed password validation attempt:

  fmt.Sprintf("password must be at least %d bytes", minPasswordLength)
  fmt.Sprintf("password must be at most %d bytes", maxPasswordLength)

Both minPasswordLength (8) and maxPasswordLength (72) are compile-time
constants, so the resulting strings never vary. Replace them with
package-level const strings (errPasswordTooShort, errPasswordTooLong)
evaluated once at compile time.

This also allows removing the fmt import from handler/helpers.go
entirely, as it was used only in validatePassword.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@veverkap veverkap changed the title [Efficiency Improver] perf(handler): replace fmt.Sprintf with precomputed const strings in validatePassword perf(handler): replace fmt.Sprintf with precomputed const strings in validatePassword May 2, 2026
@veverkap veverkap marked this pull request as ready for review May 2, 2026 21:15
@veverkap veverkap requested review from a team and Copilot May 2, 2026 21:15
Comment thread handler/helpers.go Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes the password validation error path in handler.validatePassword by removing per-call fmt.Sprintf usage and replacing it with precomputed string constants, allowing the fmt import to be dropped from handler/helpers.go.

Changes:

  • Remove fmt import from handler/helpers.go.
  • Add precomputed error message constants for too-short / too-long password cases.
  • Update validatePassword to use the precomputed constants instead of fmt.Sprintf.
Show a summary per file
File Description
handler/helpers.go Replaces formatted error strings with precomputed constants and removes the unused fmt import.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

Comment thread handler/helpers.go Outdated
Comment thread handler/helpers.go Outdated
…init time

Replace literal string constants for errPasswordTooShort/errPasswordTooLong
with package-level vars initialized via fmt.Sprintf from minPasswordLength/
maxPasswordLength. This eliminates the silent drift risk (where updating a
length constant would leave error messages reporting stale numbers) while
preserving the single-allocation-at-startup property.

Also corrects the code comment: the optimization avoids per-call fmt.Sprintf
allocations, not all allocations on the error path.
@veverkap veverkap merged commit f92e911 into main May 3, 2026
28 checks passed
@veverkap veverkap deleted the efficiency/precompute-password-error-strings-746f7beb9208abb3 branch May 3, 2026 13:30
github-actions Bot added a commit that referenced this pull request May 3, 2026
PR #172 merged the error strings as package-level var initialised via
fmt.Sprintf at init time. This follow-up converts them to true compile-time
const literals and removes the now-unused fmt import.

- errPasswordTooShort and errPasswordTooLong are now immutable const strings
- fmt import removed from handler/helpers.go
- Both const declarations merged into the existing minPasswordLength block

Proxy metric: heap allocations per validatePassword call.
Before: 0 allocs (fmt.Sprintf ran once at init, not per call).
After: 0 allocs AND strings are immutable, with no fmt overhead at init.
Bonus: fmt package no longer imported → slightly smaller binary init.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
veverkap added a commit that referenced this pull request May 7, 2026
…port (#211)

* perf(handler): convert password error vars to const; remove fmt import

PR #172 merged the error strings as package-level var initialised via
fmt.Sprintf at init time. This follow-up converts them to true compile-time
const literals and removes the now-unused fmt import.

- errPasswordTooShort and errPasswordTooLong are now immutable const strings
- fmt import removed from handler/helpers.go
- Both const declarations merged into the existing minPasswordLength block

Proxy metric: heap allocations per validatePassword call.
Before: 0 allocs (fmt.Sprintf ran once at init, not per call).
After: 0 allocs AND strings are immutable, with no fmt overhead at init.
Bonus: fmt package no longer imported → slightly smaller binary init.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* docs(handler): add sync comment for password error string constants

The numeric literals in errPasswordTooShort and errPasswordTooLong must
stay aligned with minPasswordLength and maxPasswordLength; add a comment
making that coupling explicit.

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Patrick Veverka <veverkap@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants