5
\$\begingroup\$

I always try to port the roman numeral conversion algorithm to learn new languages. Reading the Go manual as a beginner, this is the most idiomatic way I've been able to write it.

I would love it if you could give me comments/advice on the following code, and also maybe on some questions I have regarding it:

  • About the found := false flag, is there a better way of doing this? And by that I mean: breaking from a loop while storing a value? Because I cannot assign the variable matchingGlyph to nil to display not-found-ness, the only way I could think of was to have a separate flag.
  • At what spots am I under-doing it or over-doing it?
  • How does it look from a modern Go perspective?
  • How is my error management?

What I know could be improved, but chose not to implement: roman glyphs order validation. Right now the code implements error handling for invalid glyphs, but doesn't handle invalid ordering like XIVMM (in lieu of MMXIV). Both would produce 2014, but the ordering of the former is invalid.

Thank you!

roman.go

package main

import (
    "fmt"
    "strings"
)

// RomanGlyph contains a roman numeral and its base 10 integer equivalent
type RomanGlyph struct {
    numeral string
    value   int
}

// RomanGlyphsByNumeral is a dictionary of roman numeral glyphs to their base 10 integer equivalents
// Ordered for efficient matching in RomanNumberToInt
var RomanGlyphsByNumeral = []RomanGlyph{
    {"CM", 900}, {"M", 1000}, {"CD", 400}, {"D", 500},
    {"XC", 90}, {"C", 100}, {"XL", 40}, {"L", 50},
    {"IX", 9}, {"X", 10}, {"IV", 4}, {"V", 5},
    {"I", 1},
}

// RomanGlyphsByValue is a dictionary of roman numeral glyphs to their base 10 integer equivalents
// Ordered for efficient matching in IntToRomanNumber
var RomanGlyphsByValue = []RomanGlyph{
    {"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400},
    {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40},
    {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4},
    {"I", 1},
}

// RomanNumberToInt converts a roman number to its base 10 integer equivalent
func RomanNumberToInt(romanNumber string) (result int, err error) {
    count := len(romanNumber)
    runes := []rune(romanNumber)

    i := 0
    for i < count {
        substring := string(runes[i:count])

        found := false
        var matchingGlyph RomanGlyph

        for _, glyph := range RomanGlyphsByNumeral {
            if strings.HasPrefix(substring, glyph.numeral) {
                found = true
                matchingGlyph = glyph
                break
            }
        }

        if !found {
            return 0, fmt.Errorf("Cannot find valid roman numeral at %v", substring)
        }

        result += matchingGlyph.value
        i += len(matchingGlyph.numeral)
    }

    return result, nil
}

// IntToRomanNumber converts a base 10 integer to its roman number equivalent
func IntToRomanNumber(number int) (result string, err error) {
    if number < 1 {
        return "", fmt.Errorf("Integer argument `number` should be greater than 1. Received %v", number)
    }

    for _, romanGlyph := range RomanGlyphsByValue {
        for number >= romanGlyph.value {
            result += romanGlyph.numeral
            number -= romanGlyph.value
        }
    }

    return result, nil
}

roman_test.go

package main

import "testing"

var samples = []struct {
    value   int
    numeral string
}{
    {2014, "MMXIV"},
    {1993, "MCMXCIII"},
    {1111, "MCXI"},
    {444, "CDXLIV"},
}

func TestRoman(t *testing.T) {
    // Test failure for converting integer 0 to a roman numeral
    _, zeroErr := IntToRomanNumber(0)
    if zeroErr == nil {
        t.Error("Converting int 0 to roman should have failed")
    }

    // Test failure for converting invalid roman numerals to int
    _, invalidNumeralErr := RomanNumberToInt("XAV")
    if invalidNumeralErr == nil {
        t.Error("Converting roman 'XAV' to int should have failed because A is not a valid roman numeral")
    }

    // Test samples
    for _, sample := range samples {
        result, err := IntToRomanNumber(sample.value)

        if err != nil {
            t.Errorf("Sample int %v should have produced numeral %v. Failed with %v", sample.value, sample.numeral, err)
        } else if result != sample.numeral {
            t.Errorf("Sample int %v should have produced numeral %v. Got %v", sample.value, sample.numeral, result)
        }
    }

    // Test roundtrip conversion for numbers 1..2000
    for i := 1; i <= 2000; i++ {
        roman, err := IntToRomanNumber(i)
        if err != nil {
            t.Fatal(err)
        }

        backToI, err := RomanNumberToInt(roman)
        if err != nil {
            t.Fatal(err)
        }

        if i != backToI {
            t.Errorf("Number %v failed roundtrip: %v > %v > %v", i, i, roman, backToI)
        }
    }
}
```
\$\endgroup\$
3
+50
\$\begingroup\$

How is my error management?

Error strings should not be capitalized (unless beginning with proper nouns or acronyms) or end with punctuation, since they are usually printed following other context. That is, use fmt.Errorf("something bad") not fmt.Errorf("Something bad"), so that log.Printf("Reading %s: %v", filename, err) formats without a spurious capital letter mid-message. This does not apply to logging, which is implicitly line-oriented and not combined inside other messages.

And see standard library src/io/io.go examples:

// ErrShortBuffer means that a read required a longer buffer than was provided.
var ErrShortBuffer = errors.New("short buffer")

// EOF is the error returned by Read when no more input is available.
// Functions should return EOF only to signal a graceful end of input.
// If the EOF occurs unexpectedly in a structured data stream,
// the appropriate error is either ErrUnexpectedEOF or some other error
// giving more detail.
var EOF = errors.New("EOF")

You don't need found := false flag, see 5, and for the final polished code see 8:


Code review:
1. Inside func IntToRomanNumber(number int), the:

return "", fmt.Errorf("Integer argument `number` should be greater than 1. Received %v", number)

Should be greater than zero:

return "", fmt.Errorf("integer argument `number` should be greater than 0, received %v", number)
  1. Using strings.Builder is a good habit.
  2. Use (string, error) instead of (result string, err error) for function return values.
  3. See standard library for sub-string s = strings.TrimPrefix(s, glyph.numeral):
// TrimPrefix returns s without the provided leading prefix string.
// If s doesn't start with prefix, s is returned unchanged.
func TrimPrefix(s, prefix string) string {
    if HasPrefix(s, prefix) {
        return s[len(prefix):]
    }
    return s
}
  1. About the found := false flag:
func RomanNumberToInt(s string) (int, error) {
    result := 0
ScanningGlyphs:
    for len(s) > 0 {
        for _, glyph := range RomanGlyphsByNumeral {
            if strings.HasPrefix(s, glyph.numeral) {
                s = s[len(glyph.numeral):] // s = strings.TrimPrefix(s, glyph.numeral)
                result += glyph.value
                continue ScanningGlyphs
            }
        }
        return 0, fmt.Errorf("cannot find valid roman numeral at %v", s)
    }
    return result, nil
}
  1. The Classic Roman Numeral Mistake:

    On the face of it, IL and IC appear to follow the same subtractive principle as IV and IX, i.e. IL = L (50) - I (1) = 49.
    This is actually not valid.
    The subtractive principle for Roman numbers has these restrictions:
    You can only subtract a power of ten, and only from the next two higher "digits", where the digits are {I, V, X, L, C, D, M}.
    That is, only I, X and C can be subtracted, AND I can be subtracted only from V and X; X can be subtracted only from L and C; C can be subtracted only from D and M.
    By these rules, the Roman numerals IL for 49 and IC for 99 do not work.
    The correct representation for 49 is XLIX, for 99 is XCIX.

So the RomanNumberToInt("IC") should return error not 101:

// 0 value means invalid glyph.
var RomanGlyphsByNumeral = []RomanGlyph{
    {"IM", 0}, {"VM", 0}, {"XM", 0}, {"LM", 0}, {"CM", 900}, {"DM", 0}, {"M", 1000},
    {"ID", 0}, {"VD", 0}, {"XD", 0}, {"LD", 0}, {"CD", 400}, {"D", 500},
    {"IC", 0}, {"VC", 0}, {"XC", 90}, {"LC", 0}, {"C", 100},
    {"IL", 0}, {"VL", 0}, {"XL", 40}, {"L", 50},
    {"IX", 9}, {"VX", 0}, {"X", 10},
    {"IV", 4}, {"V", 5},
    {"I", 1},
}
func RomanNumberToInt(s string) (int, error) {
    result := 0
ScanningGlyphs:
    for len(s) > 0 {
        for _, glyph := range RomanGlyphsByNumeral {
            if strings.HasPrefix(s, glyph.numeral) {
                if glyph.value == 0 {
                    return 0, fmt.Errorf("invalid roman glyph %q", glyph.numeral)
                }
                s = s[len(glyph.numeral):] // s = strings.TrimPrefix(s, glyph.numeral)
                result += glyph.value
                continue ScanningGlyphs
            }
        }
        return 0, fmt.Errorf("cannot find valid roman numeral at %v", s)
    }
    return result, nil
}
  1. In typography, a glyph is an elemental symbol within an agreed set of symbols, so naming the RomanGlyph struct:
type RomanGlyph struct {
    numeral string
    value   int
}

Is more readable this way:

// Roman struct contains a roman glyph and its base 10 integer equivalent.
type Roman struct {
    glyph string
    value int
}

  1. If you are in the main you don't need to export (Capitalize) anything, otherwise only export intended items, and name the package roman then function name will be used as roman.RomanGlyphToInt by other packages, and that stutters; consider calling this GlyphToInt:

roman/roman.go file:

// Package roman implements simple functions to convert roman glyphs to and from int.
// See example.
package roman

import (
    "fmt"
    "strings"
)

// roman struct contains a roman glyph and its base 10 integer equivalent.
type roman struct {
    glyph string
    value int
}

// romanGlyphs is a list of roman glyphs and their base 10 integer equivalents.
// Ordered for efficient matching in RomanGlyphToInt.
// 0 value means invalid glyph.
var romanGlyphs = []roman{
    {"IM", 0}, {"VM", 0}, {"XM", 0}, {"LM", 0}, {"CM", 900}, {"DM", 0}, {"M", 1000},
    {"ID", 0}, {"VD", 0}, {"XD", 0}, {"LD", 0}, {"CD", 400}, {"D", 500},
    {"IC", 0}, {"VC", 0}, {"XC", 90}, {"LC", 0}, {"C", 100},
    {"IL", 0}, {"VL", 0}, {"XL", 40}, {"L", 50},
    {"IX", 9}, {"VX", 0}, {"X", 10},
    {"IV", 4}, {"V", 5},
    {"I", 1},
}

// GlyphToInt returns an equivalent integer number in base 10 with error value.
// You can only subtract a power of ten, and only from the next two higher "digits",
// where the digits are {I, V, X, L, C, D, M}.
// only I, X and C can be subtracted, AND I can be subtracted only from V and X;
// X can be subtracted only from L and C; C can be subtracted only from D and M.
func GlyphToInt(s string) (int, error) {
    s = strings.ToUpper(s)
    result := 0
ScanningGlyphs:
    for len(s) > 0 {
        for _, glyph := range romanGlyphs {
            if strings.HasPrefix(s, glyph.glyph) {
                if glyph.value == 0 {
                    return 0, fmt.Errorf("invalid roman glyph %q", glyph.glyph)
                }
                s = s[len(glyph.glyph):] // s = strings.TrimPrefix(s, glyph.glyph)
                result += glyph.value
                continue ScanningGlyphs
            }
        }
        return 0, fmt.Errorf("cannot find valid roman glyph at %q", s)
    }
    return result, nil
}

// romanGlyphsByValue is a list of roman glyphs to their base 10 integer equivalents.
// Ordered for efficient matching in IntToGlyph.
var romanGlyphsByValue = []roman{
    {"M", 1000}, {"CM", 900}, {"D", 500}, {"CD", 400},
    {"C", 100}, {"XC", 90}, {"L", 50}, {"XL", 40},
    {"X", 10}, {"IX", 9}, {"V", 5}, {"IV", 4},
    {"I", 1},
}

// IntToGlyph converts a base 10 integer to its roman equivalent glyph.
func IntToGlyph(n int) (string, error) {
    if n <= 0 {
        return "", fmt.Errorf("integer number should be greater than 0, received %d", n)
    }
    var result strings.Builder
    for _, roman := range romanGlyphsByValue {
        for n >= roman.value {
            n -= roman.value
            result.WriteString(roman.glyph)
        }
    }
    // if n != 0 || result.Len() == 0 {
    //  panic("needs developer attention: romanGlyphsByValue list values must be > 1") // or test all values and remove this
    // }
    return result.String(), nil
}

roman/roman_test.go file:

package roman

import "testing"

var samples = []struct {
    value int
    glyph string
}{
    {2014, "MMXIV"},
    {1993, "MCMXCIII"},
    {1111, "MCXI"},
    {444, "CDXLIV"},
}

func TestRoman(t *testing.T) {
    // Test failure for converting integer 0 to a roman glyph
    _, zeroErr := IntToGlyph(0)
    if zeroErr == nil {
        t.Error("Converting int 0 to roman should have failed")
    }

    // Test failure for converting invalid roman glyphs to int
    _, invalidNumeralErr := GlyphToInt("XAV")
    if invalidNumeralErr == nil {
        t.Error("Converting roman 'XAV' to int should have failed because A is not a valid roman glyph")
    }

    // Test failure for converting invalid roman glyphs to int
    _, invalidNumeralErr = GlyphToInt("IC")
    if invalidNumeralErr == nil {
        t.Error("Converting roman 'IC' to int should have failed because A is not a valid roman glyph")
    }

    // Test samples
    for _, sample := range samples {
        result, err := IntToGlyph(sample.value)
        if err != nil {
            t.Errorf("Sample int %v should have produced glyph %v. Failed with %v", sample.value, sample.glyph, err)
        } else if result != sample.glyph {
            t.Errorf("Sample int %v should have produced glyph %v. Got %v", sample.value, sample.glyph, result)
        }
    }

    // Test roundtrip conversion for numbers 1..2000
    for i := 1; i <= 2000; i++ {
        roman, err := IntToGlyph(i)
        if err != nil {
            t.Fatal(err)
        }
        backToI, err := GlyphToInt(roman)
        if err != nil {
            t.Fatal(err)
        }
        if i != backToI {
            t.Errorf("Number %v failed roundtrip: %v > %v > %v", i, i, roman, backToI)
        }
    }
}

Test output:

ok roman    0.038s  coverage: 100.0% of statements
Success: Tests passed.
  1. Usage example (example/main.go file):
func main() {
    n, err := roman.GlyphToInt("CC")
    if err != nil {
        log.Fatal(err)
    }
    fmt.Println(n) // 200

    s, err := roman.IntToGlyph(2020)
    if err != nil {
        log.Fatal(err)
    }
    fmt.Println(s) // MMXX
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Additionally, error strings should not be capitalized or end with punctuation. \$\endgroup\$ – Dave C Apr 11 at 14:41
  • \$\begingroup\$ In IntToGlyph it might also be better to remove the initial check for n <= 0 and instead, after the loop check if n != 0 || result.Len() == 0 { return "", errors.New("some failure message here") }. Among other things it catches more failures (e.g. an algorithm or table error). \$\endgroup\$ – Dave C Apr 11 at 14:46
  • \$\begingroup\$ In GlyphToInt I'd make the first step be s = strings.ToUpper(s) to support arbitrarily cased input. \$\endgroup\$ – Dave C Apr 11 at 14:47
  • \$\begingroup\$ @DaveC: First and last comments applied, for the second comment putting if n<= 0 at top is performance wise and lean, so I would keep it, then the option is to add another if n != 0 || result.Len() == 0 { return "", errors.New("some failure message here") } after the loop which makes coverage: below 100.0%, since coverage tests this problem so it is redundant. and makes coverage below 100.0%. Thank you for the comment. \$\endgroup\$ – wasmup Apr 11 at 16:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.