3
\$\begingroup\$

For a new project, I've started learning Golang. This is one of my very first steps in Go.

The objective that I've given myself is to convert int to a String representing the roman numeral of that int. The supported range shall start with 0, negative numbers can be ignored, and it's okay for now that for negative numbers, an empty String is returned.

File roman_test.go

package main

import "testing"

func TestRoman(t *testing.T) {
    testcases := []struct {
        number int
        roman string
    }{
        { 0, "" },
        { 1, "I" },
        { 2, "II" },
        { 4, "IV" },
        { 5, "V" },
        { 1993, "MCMXCIII" },
        { 2018, "MMXVIII" },
        { 1111, "MCXI" },
        { 2222, "MMCCXXII" },
        { 444, "CDXLIV" },
        { 555, "DLV" },
        { 666, "DCLXVI" },
        { 999, "CMXCIX" },
    }

    for _, testcase := range testcases {
        roman := Roman(testcase.number)
        if roman != testcase.roman {
            t.Errorf("%d expected to convert to %s, got: %s.", testcase.number, testcase.roman, roman)
        }
    }
}

File roman.go

package main

func Roman(number int) string {
    conversions := []struct{
        value int
        digit string
    }{
        {1000, "M"},
        {900, "CM"},
        {500, "D"},
        {400, "CD"},
        {100, "C"},
        {90, "XC"},
        {50, "L"},
        {40, "XL"},
        {10, "X"},
        {9, "IX"},
        {5, "V"},
        {4, "IV"},
        {1, "I"},
    }

    roman := ""
    for _, conversion := range conversions {
        for number >= conversion.value {
            roman += conversion.digit
            number -= conversion.value
        }
    }
    return roman
}

I'm mainly interested in feedback on the following aspects:

  • Go code style
  • Idiomatic go
  • Better structure
\$\endgroup\$
2
\$\begingroup\$

This is some fantastic first Go code!

Great use of anonymous structs for test fixtures. I disagree with @sineemore, these are incredibly idiomatic. Nice job writing tests, as well!

There isn't much to be said for the code itself. It's simple and reads clearly. Perhaps you'd want to document it (go doc is fantastic).

I guess strings.Builder is the idiomatic way to do string concatentation in Go >=1.10. However, this is largely for performance reasons.

var roman strings.Builder
for _, conversion := range conversions {
    for number >= conversion.value {
        roman.WriteString(conversion.digit)
        number -= conversion.value
    }
}
return roman.String()

Your next steps are learning about packaing and naming. I'd suggest that instead of calling the field digit you call it something more descriptive like numeral (conversion.numeral reads better than conversion.digit to me). Perhaps you can also come up with better names for roman and number?

I'd also give the function a better name. And to do so, I'd place it in a package called roman. Then call it FromInt. In this way, you use it like roman.FromInt(42), which reads much better. Package naming is really important in Go and you'll find a lot of the standard libraries follow this pattern of making the package name part of the "sentence" instead of just some extra stuff you have to prepend.

In doing this, I'd also recommend putting your tests in package roman_test so they can't access any private functions or variables. This is convention.

\$\endgroup\$
0
\$\begingroup\$

Your code is well formatted and structured.

Nice usage of anonymous struct at Roman function, but don't overuse them: your test cases may be written as a map and it's preferable way.

testcases := map[int]string{
    0:    "",
    1:    "I",
    2:    "II",
    4:    "IV",
    5:    "V",
    444:  "CDXLIV",
    555:  "DLV",
    666:  "DCLXVI",
    999:  "CMXCIX",
    1111: "MCXI",
    1993: "MCMXCIII",
    2018: "MMXVIII",
    2222: "MMCCXXII",
}
\$\endgroup\$

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.