3
\$\begingroup\$

I have created a program that turns a decimal into a fraction:

package main

import (
        "fmt"
        "github.com/retep-mathwizard/utils/convert"
        "github.com/retep-mathwizard/utils/input"
        "math"
        "strings"
)

func main() {
        fmt.Println("Input your decimal")
        num := input.StringInput(" > ")
        digits := num[strings.Index(num, ".")+1:]
        decimal := num[strings.Index(num, "."):]
        numbers := num[:strings.Index(num, ".")]
        denom := convert.FloatToInt(math.Pow(10, convert.IntToFloat(len(digits))))
        numer := convert.FloatToInt(convert.StringToFloat(decimal) * convert.IntToFloat(denom))
        var gcf int
        for i := numer/2 + 1; i >= 0; i-- {
                if (numer%i == 0) && (denom%i == 0) {
                        gcf = i
                        break
                }

        }
        newnum := numer / gcf
        newdum := denom / gcf
        fmt.Println(numbers+" and ", newnum)
        fmt.Println("    -------")
        fmt.Println("      ", newdum)
}

I need to implement repeating fractions, and a maximum decimal length. Over 13 digits, and it takes a while. Feel free to butcher my code!

\$\endgroup\$
3
\$\begingroup\$

Note: Your code is not useful. Rational numbers are available in the Go math/big standard library. For example,

package main

import (
    "fmt"
    "math/big"
)

func main() {
    fmt.Println("Enter a number:")
    var r = new(big.Rat)
    fmt.Scanln(r)
    fmt.Println(r)
}

I read your code, the code you imported from github.com/retep-mathwizard, and the code from rolfl's answer. I see problems.

Let's start with something simple, the calculation of the greatest common divisor or factor.

You embedded this code in a slab of code. It should be a function. For example,

func gcf(numer, denom int) int {
    var gcf int
    for i := numer/2 + 1; i >= 0; i-- {
        if (numer%i == 0) && (denom%i == 0) {
            gcf = i
            break
        }
    }
    return gcf
}

It's very inefficient. For well over two thousand years we have known of a much better way to do this.

Rolfl used a much better algorithm.

// Euclidian algorithm
func gcf(a, b int64) int64 {

    if a < b {
        return gcf(b, a)
    }

    if b == 0 {
        return a
    }

    a = a % b
    return gcf(b, a)
}

If you put blank lines between each sentence you can't see very much on a single screen. It's essential that code be readable. Think of blank lines as more like paragraph separators.

The algorithm is implemented using recursion. Any recursive algorithm can be written as an iterative algorithm. Go doesn't have tail recursion. Limit the use of recursion to the few cases where recursion is much easier to understand than iteration.

Here's an idiomatic Go GCD function. It's simple, direct, and fast.

func gcd(x, y int64) int64 {
    for y != 0 {
        x, y = y, x%y
    }
    return x
}

Here are some Go benchmarks which show how slow your GCF algorithm is.

BenchmarkGCFXXX        30000         54564 ns/op    
BenchmarkGCFPeter   10000000           156 ns/op

Go types play an essential role in writing idiomatic Go. Rational is an obvious type:

// A rational number r is expressed as the fraction p/q of two integers:
// r = p/q = (d*i+n)/d.
type Rational struct {
    i int64 // integer
    n int64 // fraction numerator
    d int64 // fraction denominator
}

Like many types, Rational should have a formatted string function. Your formatted output is ugly

7182818284590 and  141592653
    -------
       1000000000

I think this looks much better

7182818284590 + 141592653/1000000000

Here's a Go String method for the Rational type. Note the attention to pretty-printing details.

func (r Rational) String() string {
    var s string
    if r.i != 0 {
        s += strconv.FormatInt(r.i, 10)
    }
    if r.n != 0 {
        if r.i != 0 {
            s += " + "
        }
        if r.d < 0 {
            r.n *= -1
            r.d *= -1
        }
        s += strconv.FormatInt(r.n, 10) + "/" + strconv.FormatInt(r.d, 10)
    }
    if len(s) == 0 {
        s += "0"
    }
    return s
}

An obvious indication of problems is the difficulty in testing. Your program only handles one input value. Rolfl hardcoded a single fixed value: num := "12345.6785".

For a problem like this, which is driven by user input, the Go main function should be an input loop. For example,

func main() {
    snr := bufio.NewScanner(os.Stdin)
    enter := "Enter a decimal number:"
    for fmt.Println(enter); snr.Scan(); fmt.Println(enter) {
        d := snr.Text()
        if len(d) == 0 {
            break
        }
        r, err := ParseDecimal(d)
        if err != nil {
            fmt.Fprintln(os.Stderr, "Input error:", err)
            continue
        }
        fmt.Println(r)
    }
    if err := snr.Err(); err != nil {
        if err != io.EOF {
            fmt.Fprintln(os.Stderr, err)
        }
    }
}

Now we have an easy way to test. Let's make sure that we can handle all reasonable values. Here are some values that you don't handle.

0; 0.; -0; 1; -1; -1.1; 1.; and so on.

You should always expect the worst from user input and handle errors gracefully. For example,'

func ParseDecimal(s string) (r Rational, err error) {
    sign := int64(1)
    if strings.HasPrefix(s, "-") {
        sign = -1
    }
    p := strings.IndexByte(s, '.')
    if p < 0 {
        p = len(s)
    }
    if i := s[:p]; len(i) > 0 {
        if i != "+" && i != "-" {
            r.i, err = strconv.ParseInt(i, 10, 64)
            if err != nil {
                return Rational{}, err
            }
        }
    }
    if p >= len(s) {
        p = len(s) - 1
    }
    if f := s[p+1:]; len(f) > 0 {
        n, err := strconv.ParseUint(f, 10, 64)
        if err != nil {
            return Rational{}, err
        }
        d := math.Pow10(len(f))
        if math.Log2(d) > 63 {
            err = fmt.Errorf(
                "ParseDecimal: parsing %q: value out of range", f,
            )
            return Rational{}, err
        }
        r.n = int64(n)
        r.d = int64(d)
        if g := gcd(r.n, r.d); g != 0 {
            r.n /= g
            r.d /= g
        }
        r.n *= sign
    }
    return r, nil
}

Even if you write impeccable code, you will still be judged on the quality of the external packages that you use. The packages from github.com/retep-mathwizard/ are of very low quality.

Here's my complete program. It's a first draft.

package main

import (
    "bufio"
    "fmt"
    "io"
    "math"
    "os"
    "strconv"
    "strings"
)

// A rational number r is expressed as the fraction p/q of two integers:
// r = p/q = (d*i+n)/d.
type Rational struct {
    i int64 // integer
    n int64 // fraction numerator
    d int64 // fraction denominator
}

func (r Rational) String() string {
    var s string
    if r.i != 0 {
        s += strconv.FormatInt(r.i, 10)
    }
    if r.n != 0 {
        if r.i != 0 {
            s += " + "
        }
        if r.d < 0 {
            r.n *= -1
            r.d *= -1
        }
        s += strconv.FormatInt(r.n, 10) + "/" + strconv.FormatInt(r.d, 10)
    }
    if len(s) == 0 {
        s += "0"
    }
    return s
}

func gcd(x, y int64) int64 {
    for y != 0 {
        x, y = y, x%y
    }
    return x
}

func ParseDecimal(s string) (r Rational, err error) {
    sign := int64(1)
    if strings.HasPrefix(s, "-") {
        sign = -1
    }
    p := strings.IndexByte(s, '.')
    if p < 0 {
        p = len(s)
    }
    if i := s[:p]; len(i) > 0 {
        if i != "+" && i != "-" {
            r.i, err = strconv.ParseInt(i, 10, 64)
            if err != nil {
                return Rational{}, err
            }
        }
    }
    if p >= len(s) {
        p = len(s) - 1
    }
    if f := s[p+1:]; len(f) > 0 {
        n, err := strconv.ParseUint(f, 10, 64)
        if err != nil {
            return Rational{}, err
        }
        d := math.Pow10(len(f))
        if math.Log2(d) > 63 {
            err = fmt.Errorf(
                "ParseDecimal: parsing %q: value out of range", f,
            )
            return Rational{}, err
        }
        r.n = int64(n)
        r.d = int64(d)
        if g := gcd(r.n, r.d); g != 0 {
            r.n /= g
            r.d /= g
        }
        r.n *= sign
    }
    return r, nil
}

func main() {
    snr := bufio.NewScanner(os.Stdin)
    enter := "Enter a decimal number:"
    for fmt.Println(enter); snr.Scan(); fmt.Println(enter) {
        d := snr.Text()
        if len(d) == 0 {
            break
        }
        r, err := ParseDecimal(d)
        if err != nil {
            fmt.Fprintln(os.Stderr, "Input error:", err)
            continue
        }
        fmt.Println(r)
    }
    if err := snr.Err(); err != nil {
        if err != io.EOF {
            fmt.Fprintln(os.Stderr, err)
        }
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Ouch! I did a lot of bad things.... \$\endgroup\$ – XXX Mar 24 '16 at 21:12
2
\$\begingroup\$

The use of the retep-mathwizard code makes things hard to follow, and they appear to be completely gratuitous. The conversions from to and from Float values are easily done with regular casting of values, and the input code is unnecessary too.

The repeated use of strings.Index(num, ".") makes the code hard to read, and, worse, you cheat a whole lot with this line: digits := num[strings.Index(num, ".")+1:].... that's just really, really horrible. You don't actually use digits except to get their length in convert.IntToFloat(len(digits)) You should just do some subtraction instead of getting the length of the string later.

But, what's all moot, is that if someone enters a value that's a whole number, everything will break. If the user enters the value 100, with no decimal point, what then?

As for the power function, consider using math.Pow10(e).

Then, use strconv package for conversions.

I would consider taking the input value as an argument to the program instead of trying to read standard input. Just use os.Args[1] (if it exists).

So, really, what you should be doing is something along the lines of:

pointPos := strings.Index(num, ".")
if pointPos < 0 {
    // no fractions.
    fmt.Println(num)
    return
}

whole, err := strconv.ParseInt(num[:pointPos], 10, 64)
if err != nil {
    log.Fatalf("Unable to parse %v: %v", num[:pointPos], err)
}

numerator, err := strconv.ParseInt(num[pointPos+1:], 10, 64)
if err != nil {
    log.Fatalf("Unable to parse %v: %v", num[pointPos+1:], err)
}

denominator := int64(math.Pow10(len(num) - pointPos - 1))

Your GCF function is also an area of improvement. The Euclidian algorithm is really easy to apply in go (it's recursive, but that's OK):

// Euclidian algorithm
func gcf(a, b int64) int64 {

    if a < b {
        return gcf(b, a)
    }

    if b == 0 {
        return a
    }

    a = a % b
    return gcf(b, a)
}

So, in summary, use the common libraries where you can. A smarter GCF function makes that part simpler, and all the cobversions to/from Float, etc. were making it hard to read.

See this all up in the Playground

| improve this answer | |
\$\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.