1
\$\begingroup\$

What is does

I'm reading The Go Programming Language. Below is my code for the last exercise in the first chapter, which is to create a webserver, generate an image, and serve the image via the server, allowing modifications of the image via query string parameters, eg: localhost:8000/?cycles=25&bgcolor=0,255,255.

My Concerns

I'm open to any and all suggestions, but any advice specifically concerning these points would be especially helpful.

Duplicated code. There's just enough duplication to trigger my OCD, but each one is slightly different so I'm not sure how I could delegate to a function without creating a bunch of very similar functions. In particular, the two for loops where I'm parsing colors really bother me.

Type Juggling. strconv.ParseInt has a third argument to specify the bit size but always returns int64 anyway, so I still have to explicitly cast then to unsigned 8 bit integers. Take a look at my img.SetColorIndex call where I'm doing all sorts of casting just for some simple arithmetic.

Concision. I am baffled at the fact that I can write a simple server using the standard library with about 4 lines of code, but turning a string into an array of numbers took me 19 lines of code. And I had to use basically the same 19 lines twice because I couldn't figure out what to specify as the type when passing a color.RGBA as a function parameter.

package main

import (
    "log"
    "net/http"
    "io"

    "image"
    "image/color"
    "image/gif"
    "math"
    "math/rand"

    "strconv"
    "strings"
)

var bgcolor = color.RGBA{0, 0, 0, 255}
var fgcolor = color.RGBA{255, 255, 255, 255}

func main() {
    http.HandleFunc("/", serveImage)
    log.Fatal(http.ListenAndServe("localhost:8000", nil))
}

func serveImage(w http.ResponseWriter, r* http.Request) {
    if err := r.ParseForm(); err != nil {
        log.Print(err)
    }
    cycles := 10.0
    res := 0.001
    size := 100
    frames := 64
    delay := 8
    if value, exists := r.Form["cycles"]; exists {
        if v, err := strconv.ParseFloat(value[0], 64); err == nil {
            cycles = v
        }
    }
    if value, exists := r.Form["res"]; exists {
        if v, err := strconv.ParseFloat(value[0], 64); err == nil {
            res = v
        }
    }
    if value, exists := r.Form["size"]; exists {
        if v, err := strconv.ParseFloat(value[0], 64); err == nil {
            size = int((v-1)/2)
        }
    }
    if value, exists := r.Form["frames"]; exists {
        if v, err := strconv.ParseInt(value[0], 10, 0); err == nil {
            frames = int(v)
        }
    }
    if value, exists := r.Form["delay"]; exists {
        if v, err := strconv.ParseInt(value[0], 10, 0); err == nil {
            delay = int(v)
        }
    }
    if value, exists := r.Form["bgcolor"]; exists {
        BGColorLoop: 
            for {
                parts := strings.Split(value[0], ",")
                if len(parts) != 3 {
                    break BGColorLoop
                }
                for _, val := range parts {
                    if v, err := strconv.ParseInt(val, 10, 0); err != nil || int(v) > 255 || int(v) < 0 {
                        break BGColorLoop
                    }
                }
                r, _ := strconv.ParseInt(parts[0], 10, 8)
                g, _ := strconv.ParseInt(parts[1], 10, 8)
                b, _ := strconv.ParseInt(parts[2], 10, 8)
                bgcolor = color.RGBA{uint8(r), uint8(g), uint8(b), 255}
                break BGColorLoop
            }
    }
    if value, exists := r.Form["fgcolor"]; exists {
        FGColorLoop: 
            for {
                parts := strings.Split(value[0], ",")
                if len(parts) != 3 {
                    break FGColorLoop
                }
                for _, val := range parts {
                    if v, err := strconv.ParseInt(val, 10, 0); err != nil || int(v) > 255 || int(v) < 0 {
                        break FGColorLoop
                    }
                }
                r, _ := strconv.ParseInt(parts[0], 10, 8)
                g, _ := strconv.ParseInt(parts[1], 10, 8)
                b, _ := strconv.ParseInt(parts[2], 10, 8)
                fgcolor = color.RGBA{uint8(r), uint8(g), uint8(b), 255}
                break FGColorLoop
            }
    }
    lissajous(w, cycles, res, size, frames, delay)
}

func lissajous(out io.Writer, cycles float64, res float64, size int, nframes int, delay int) {
    freq := rand.Float64() * 3.0 
    anim := gif.GIF{LoopCount: nframes}
    phase := 0.0 
    palette := []color.Color{bgcolor, fgcolor}
    for i := 0; i<nframes; i++ {
        rect := image.Rect(0, 0, 2*size+1, 2*size+1)
        img := image.NewPaletted(rect, palette)
        for t:=0.0; t<cycles*2*math.Pi; t+=res {
            x := math.Sin(t)
            y := math.Sin(t*freq+phase)
            img.SetColorIndex(size+int(x*float64(size)+0.5), size+int(y*float64(size)+0.5), 1)
        }
        phase += 0.1
        anim.Delay = append(anim.Delay, delay)
        anim.Image = append(anim.Image, img)
    }
    gif.EncodeAll(out, &anim)
}
\$\endgroup\$
1
\$\begingroup\$

Code must be correct. To be sure that code is correct, code must be readable.


I took some of your baffling code:

var bgcolor = color.RGBA{0, 0, 0, 255}
var fgcolor = color.RGBA{255, 255, 255, 255}

if value, exists := r.Form["bgcolor"]; exists {
BGColorLoop:
    for {
        parts := strings.Split(value[0], ",")
        if len(parts) != 3 {
            break BGColorLoop
        }
        for _, val := range parts {
            if v, err := strconv.ParseInt(val, 10, 0); err != nil || int(v) > 255 || int(v) < 0 {
                break BGColorLoop
            }
        }
        r, _ := strconv.ParseInt(parts[0], 10, 8)
        g, _ := strconv.ParseInt(parts[1], 10, 8)
        b, _ := strconv.ParseInt(parts[2], 10, 8)
        bgcolor = color.RGBA{uint8(r), uint8(g), uint8(b), 255}
        break BGColorLoop
    }
}
if value, exists := r.Form["fgcolor"]; exists {
FGColorLoop:
    for {
        parts := strings.Split(value[0], ",")
        if len(parts) != 3 {
            break FGColorLoop
        }
        for _, val := range parts {
            if v, err := strconv.ParseInt(val, 10, 0); err != nil || int(v) > 255 || int(v) < 0 {
                break FGColorLoop
            }
        }
        r, _ := strconv.ParseInt(parts[0], 10, 8)
        g, _ := strconv.ParseInt(parts[1], 10, 8)
        b, _ := strconv.ParseInt(parts[2], 10, 8)
        fgcolor = color.RGBA{uint8(r), uint8(g), uint8(b), 255}
        break FGColorLoop
    }
}

I rewrote it (a first draft) in Go:

bgcolor, err := formColor(r, "bgcolor")
if err != nil {
    bgcolor = color.RGBA{0, 0, 0, 255}
}
fgcolor, err := formColor(r, "fgcolor")
if err != nil {
    fgcolor = color.RGBA{255, 255, 255, 255}
}

Where

var errFormColor = errors.New("invalid form color")

func formColor(r *http.Request, colorKey string) (color.RGBA, error) {
    var rgb [3]uint8

    value := r.Form[colorKey]
    if len(value) <= 0 {
        return color.RGBA{}, errFormColor
    }
    parts := strings.SplitN(value[0], ",", len(rgb)+1)
    if len(parts) != len(rgb) {
        return color.RGBA{}, errFormColor
    }
    for i, part := range parts {
        p, err := strconv.ParseUint(part, 10, 8)
        if err != nil {
            return color.RGBA{}, errFormColor
        }
        rgb[i] = uint8(p)
    }

    return color.RGBA{rgb[0], rgb[1], rgb[2], 255}, nil
}
\$\endgroup\$
  • \$\begingroup\$ After fixing a few minor errors your code compiled, which surprised me because originally I had tried to delegate the same block of code to a function but it would not allow me to return the color. The compiler kept saying "color.RGBA is not a type." Eventually I gave up. I will try to duplicate that error later to see if I can figure out where I went wrong. Thanks for your input! \$\endgroup\$ – I wrestled a bear once. Jan 6 at 21:52

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.