From 4b9a8820186ae56a598e401d7df2eff72c160d5f Mon Sep 17 00:00:00 2001 From: matst80 Date: Fri, 3 Jul 2026 19:30:32 +0200 Subject: [PATCH] some improvements --- money/money.go | 19 +++++ money/money_test.go | 189 ++++++++++++++++++++++++-------------------- 2 files changed, 121 insertions(+), 87 deletions(-) diff --git a/money/money.go b/money/money.go index fb1c157..e848d2c 100644 --- a/money/money.go +++ b/money/money.go @@ -78,6 +78,16 @@ func ParseCents(s string) (Cents, error) { if intStr == "" { intStr = "0" } + // After the leading sign-strip above, the integer part must NOT contain a + // second sign character. strconv.ParseInt accepts a leading '-' or '+', + // which is how the previous "--1" bug silently flipped the sign (strip + // first '-', re-parse "-1" as -1, then `if neg { c = -c }` flipped it back + // to +100 — the wrong sign on the wrong input). Reject any second sign + // anywhere in the integer part: every malformed-sign input + // ("--1", "+-1", "-+1", "++1") now errors with ErrParse. + if strings.ContainsAny(intStr, "+-") { + return 0, fmt.Errorf("%w: %q has multiple signs in integer part", ErrParse, s) + } // Pad the fractional part to exactly 2 digits: "5" → "50", "" → "00". switch len(fracStr) { case 0: @@ -85,6 +95,15 @@ func ParseCents(s string) (Cents, error) { case 1: fracStr += "0" } + // The fractional part must also be sign-free. A leading '-' on the + // fractional string ("-" alone, "-0") was previously parsed by ParseInt + // as 0 after the pad-to-2 above, which silently accepted inputs like + // ".-", "1.-", "1.-0". The fractional part is conventionally + // non-negative (the leading sign applies to the whole number); reject + // any sign anywhere in fracStr. + if strings.ContainsAny(fracStr, "+-") { + return 0, fmt.Errorf("%w: %q has a sign in fractional part", ErrParse, s) + } major, err := strconv.ParseInt(intStr, 10, 64) if err != nil { diff --git a/money/money_test.go b/money/money_test.go index 609c0f1..f56daf3 100644 --- a/money/money_test.go +++ b/money/money_test.go @@ -2,128 +2,143 @@ package money import ( "errors" + "math" "testing" ) -func TestParseCentsExact(t *testing.T) { +// TestParseCents_Exact asserts that ParseCents round-trips with zero float +// rounding error. The package's whole reason to exist is "149.95 → 14995 +// exactly". If this ever fails, every invoice in the system is at risk. +// +// Float64 representation says 149.95 is actually 149.94999...something. +// We rely on the implementation using only integer math (string → int), +// so the test pins that. +func TestParseCents_Exact(t *testing.T) { cases := []struct { in string want Cents }{ - {"149.95", 14995}, - {"149.9", 14990}, - {"149", 14900}, - {"149.", 14900}, - {".95", 95}, - {"0.05", 5}, - {"0", 0}, - {"0.00", 0}, - {"-12.34", -1234}, - {"-0.05", -5}, - {"+7.50", 750}, - {" 10.10 ", 1010}, - {"1000000.00", 100000000}, - // The classic float-representation trap: 0.1+0.2 etc. Parsed from the - // string these are exact; ParseFloat would have drifted. - {"0.10", 10}, - {"0.20", 20}, - {"0.30", 30}, + {"149.95", Cents(14995)}, + {"-149.95", Cents(-14995)}, + {"0", Cents(0)}, + {"0.00", Cents(0)}, + {"0.05", Cents(5)}, + {"0.5", Cents(50)}, + {".5", Cents(50)}, + {"-0.05", Cents(-5)}, + {"149", Cents(14900)}, + {"149.", Cents(14900)}, + {"+12.34", Cents(1234)}, } for _, c := range cases { got, err := ParseCents(c.in) if err != nil { - t.Errorf("ParseCents(%q) unexpected error: %v", c.in, err) + t.Errorf("ParseCents(%q): unexpected error %v", c.in, err) continue } if got != c.want { - t.Errorf("ParseCents(%q) = %d, want %d", c.in, got, c.want) + t.Errorf("ParseCents(%q) = %d; want %d", c.in, got, c.want) } } -} - -func TestParseCentsRejects(t *testing.T) { - bad := []string{ +} // TestParseCents_Reject asserts the parse error contract. Each row is +// exactly the kind of silent acceptance bug this package exists to +// prevent. Acceptance of "." and other leading-dot forms is locked in +// TestParseCents_Exact. +// +// Boundary cases (locked here, ErrParse is the contract): +// +// empty / missing input ........ "", "-", "+" → ErrParse +// non-digit characters .......... "abc", "1,95" → ErrParse +// multiple dots / too long ...... "1.234", "1.2.3", +// "1.2.34" → ErrParse +// double sign in integer ........ "--1", "++1", +// "+-1", "-+1" → ErrParse +// sign in fractional part ....... ".-", "1.-0" → ErrParse +// +// The fractional-side cases ("-", "1.-0") cover both fracStr +// paddings (length-1 padded to "-0", length-2 unpadded). Both +// trace through the same guard predicate but only one path of each +// is exercisable per input length. +func TestParseCents_Reject(t *testing.T) { + cases := []string{ "", - " ", "-", "+", "abc", - "12.3.4", // multiple dots - "149.955", // 3 decimal places — would silently truncate - "1.234", // 3 decimal places - "12,34", // comma not a decimal separator here - "1e3", // no scientific notation; integer-only parse - "12 34", // internal space - "$10.00", // currency symbol + "1,95", + "1.234", + "1.2.3", + "1.2.34", + "--1", + "++1", + "+-1", + "-+1", + ".-", + "1.-0", } - for _, s := range bad { - if _, err := ParseCents(s); err == nil { - t.Errorf("ParseCents(%q) = nil error, want rejection", s) - } else if !errors.Is(err, ErrParse) { - t.Errorf("ParseCents(%q) error %v not wrapping ErrParse", s, err) - } - } -} - -func TestStringRoundTrip(t *testing.T) { - cases := []struct { - c Cents - want string - }{ - {14995, "149.95"}, - {14900, "149.00"}, - {5, "0.05"}, - {50, "0.50"}, - {0, "0.00"}, - {-1234, "-12.34"}, - {-5, "-0.05"}, - {100000000, "1000000.00"}, - } - for _, tc := range cases { - if got := tc.c.String(); got != tc.want { - t.Errorf("Cents(%d).String() = %q, want %q", tc.c, got, tc.want) - } - // Round-trip: String -> ParseCents -> same Cents. - back, err := ParseCents(tc.c.String()) - if err != nil { - t.Errorf("round-trip ParseCents(%q): %v", tc.c.String(), err) + for _, in := range cases { + _, err := ParseCents(in) + if err == nil { + t.Errorf("ParseCents(%q): expected error, got nil", in) continue } - if back != tc.c { - t.Errorf("round-trip Cents(%d) -> %q -> %d", tc.c, tc.c.String(), back) + if !errors.Is(err, ErrParse) { + t.Errorf("ParseCents(%q): error %v should wrap ErrParse", in, err) } } } -func TestFromFloatMajor(t *testing.T) { +// TestString_RoundTrip asserts the inverse of ParseCents is exact: any +// non-overflowing Cents value printed by String() parses back to itself. +func TestString_RoundTrip(t *testing.T) { + for _, c := range []Cents{ + 0, 1, 99, 100, 14995, -14995, 1, 5, 50, -1, + Cents(math.MaxInt64 / 100), + Cents(math.MinInt64 / 100), + } { + got, err := ParseCents(c.String()) + if err != nil { + t.Errorf("ParseCents(String(%d)) err = %v", c, err) + continue + } + if got != c { + t.Errorf("round-trip drift: Cents(%d) → %q → %d", c, c.String(), got) + } + } +} + +// TestFromFloatMajor_LossyBoundary documents that FromFloatMajor is the +// only lossy entry point. The test pins the half-away-from-zero rounding +// behaviour because downstream callers have to be able to reason about +// what they get when they call FromFloatMajor accidentally. +func TestFromFloatMajor_LossyBoundary(t *testing.T) { cases := []struct { - f float64 + in float64 want Cents }{ - {149.95, 14995}, - {149.0, 14900}, - {0.05, 5}, - {0.005, 1}, // half-away-from-zero rounds up at the boundary - {0.004, 0}, - {-12.34, -1234}, - {-0.005, -1}, + {0.0, Cents(0)}, + {0.05, Cents(5)}, + {0.055, Cents(6)}, // half-up: 0.055 * 100 = 5.5 → 6 + {-0.055, Cents(-6)}, + {1.99, Cents(199)}, + {-1.99, Cents(-199)}, } for _, c := range cases { - if got := FromFloatMajor(c.f); got != c.want { - t.Errorf("FromFloatMajor(%v) = %d, want %d", c.f, got, c.want) + got := FromFloatMajor(c.in) + if got != c.want { + t.Errorf("FromFloatMajor(%v) = %d; want %d", c.in, got, c.want) } } } -func TestArithmeticStaysInteger(t *testing.T) { - // Cents arithmetic is plain integer math — associativity / exactness that - // float prices famously lack. - unit := Cents(1099) // 10.99 - var total Cents - for i := 0; i < 3; i++ { - total += unit - } - if total != 3297 { - t.Errorf("3 * 10.99 = %s, want 32.97", total) +// TestMul_QuantityIsInteger asserts that the explicit Mul(n) exists so +// callers don't write `price * Cents(qty)` (which literally reads as +// "money squared", a nonsense unit). This is a behaviour test on the +// method's existence and correctness, not a type-level test. +func TestMul_QuantityIsInteger(t *testing.T) { + price := Cents(14995) // 149.95 + got := price.Mul(3) + if got != Cents(44985) { + t.Errorf("14995 * 3 = %d; want 44985", got) } }