12
\$\begingroup\$

The Go Playground was boring so I decided to make a small application that throws me into the language. It basically queries the chatroom every second for new messages and displays them in the output pane (minus the bots their messages).

It's all new to me so any insight is welcome. One thing I've been particularly unsure about are my return statements and whether I'm using them correctly. Likewise, there are a bazillion different ways to declare a variable; have I done it correctly so far?

package main

import (
    "encoding/json"
    "fmt"
    sanitize "github.com/kennygrant/sanitize"
    "html"
    "io/ioutil"
    "net/http"
    "net/url"
    "strconv"
    "time"
)

type ChatEvent struct {
    Event_type    int8
    Time_stamp    int64
    Content       string
    User_id       int64
    User_name     string
    Message_id    int64
    Parent_id     int64
    Show_parent   bool
    Message_Edits int8
    Message_Stars int8
}

type ChatEvents struct {
    Events []ChatEvent
    Time   int64
    Sync   int64
    Ms     int8
}

const (
    Mode         = "Messages"
    ChatUrl      = "http://chat.stackexchange.com/chats/8595/events"
    FKey         = "68f5a3ce293c47399c96ffcd85e0d280" // Go to the chat, F12, reload, look at the /events call and steal its fkey property
    MessageCount = 100                                // Never use this value directly: SE only uses it as a hint and might return a bit more when it feels like it
    Since        = 0
)

var (
    lastMessage int64
)

func main() {
    ticker := time.NewTicker(1 * time.Second)
    quit := make(chan struct{})

    for {
        select {
        case <-ticker.C:
            body := sendRequest()
            parsedJson := parseJson(body)
            newestMessages := getNewestMessages(parsedJson.Events)

            for i := 0; i < len(newestMessages); i++ {
                var currentMessage = newestMessages[i]
                if currentMessage.User_id == -263 || currentMessage.User_id == 125580 {
                    // Ignore Captain Obvious & Duga
                    continue
                }

                fmt.Printf("[%v] %s: %s\n",
                    time.Unix(currentMessage.Time_stamp, 0).Format("15:04:05"),
                    currentMessage.User_name,
                    currentMessage.Content)
            }
        case <-quit:
            ticker.Stop()
            return
        }
    }
}

func sendRequest() (content []byte) {
    response, err := http.PostForm(ChatUrl, url.Values{
        "mode":     {Mode},
        "msgCount": {strconv.Itoa(MessageCount)},
        "fkey":     {FKey},
        "since":    {strconv.Itoa(Since)}})

    if err != nil {
        fmt.Println("Error retrieving response")
        fmt.Println(err.Error())
        return nil
    }

    defer response.Body.Close()
    content, err = ioutil.ReadAll(response.Body)
    if err != nil {
        fmt.Println("Error reading response")
        fmt.Println(err.Error())
        return nil
    }

    return
}

func parseJson(body []byte) (parsedJson ChatEvents) {
    err := json.Unmarshal(body, &parsedJson)
    if err != nil {
        fmt.Println("Error parsing json")
        fmt.Println(err.Error())
        return
    }

    return
}

func getNewestMessages(events []ChatEvent) (output []ChatEvent) {
    output = make([]ChatEvent, 0)

    for index := 0; index < len(events); index++ {
        if events[index].Time_stamp > lastMessage {
            events[index].Content = html.UnescapeString(sanitize.HTML(events[index].Content))
            output = append(output, events[index])
            lastMessage = events[index].Time_stamp
        }
    }

    return
}
\$\endgroup\$
8
+50
\$\begingroup\$
import sanitize "github.com/kennygrant/sanitize"

is better written as just plain ol':

import "github.com/kennygrant/sanitize"

since the rename is redundant.

As already mentioned, it's more common to be a bit more explicit with error handling in Go and let utility functions (such as your sendRequest) return any errors for handling "higher up".

In addition to Effective Go, I suggest looking at the Go project's Code Review Comments page. Although the later is only what the Go Authors use and isn't necessarily more widely applicable, I find it a good starting point.

Among other things, with respect to naming it suggests (again as previously mentioned) using EventType, UserID, parsedJSON, etc instead of what you use. Also, while on the subject of naming, I personally subscribe to Russ Cox's naming philosophy which suggests i vs index, msg versus currentMessage, and e vs events[index] for identifiers used within a short span (as opposed to file or project wide identifiers).

When it comes to marshalling to/from JSON, use of struct tags allows you to not only map Go style names to JSON style names but also apply other attributes (notable things like "omitempty": FieldName string `json:"field_name,omitempty"`). In particular, without this if you tried to marshal your stucture back into JSON I believe you'd end up with your JSON fields capitalized (as Go's encoding/json package can only deal with exported fields).

The code review comments page also recommends applying caution/restraint to the use of named result parameters. They can be useful for documentation via godoc but I try and avoid them just to save a variable initialization line in the body. Your tastes may vary, but if you use them be careful of shadowing the named returns (e.g. with a named err error and inside an if/for having something like x, err := func()).

Instead of using something like ioutil.ReadAll to pre-read all the data I find it useful to look for "streaming" io.Reader based alternatives. For programs that process large data inputs (e.g. reading from a large file) piece by piece this can make a big difference. Here, encoding/json provides a Decoder type for this purpose. In this specific case I don't know if it makes any memory/performance difference but I find the result of using it (and combining your send and parse functions) seems to simplify the code.

Back to marshalling, it can sometimes be helpful/useful to create your own thin wrapper types with custom (un)marshalling rather than limiting yourself to basic types. For example, in the code below I've added a setime type that unmarshals from the way StackExchange API is documented to do all time/dates. You could easily add a MarshalJSON method if marshalling was also required. The difference isn't that important here, but for example, it allows easier use of all the time.Time methods/functions (e.g. time.Since(msg.Timestamp.Time), changing the output timezone, etc.

In several places you do things like:

    for i := 0; i < len(newestMessages); i++ {
            var currentMessage = newestMessages[i]
    …
    for index := 0; index < len(events); index++ {
            // use events[index]

It's more idomatic to do:

    for _, msg := range newestMessages {
            // use msg
    …
    for i := range events {
            // use events[i] or
            e := &events[i]
            // and use e.Content, &e, etc

or even:

    for _, e := range events {
            // use e

In this specific case, this last form would make an extra copy (and make the e.Content = fn(e.Content) line only modify the copy). I wouldn't worry about the extra copy here, but you could avoid it by using e := &events[i] as shown above or by changing the type to []*Events (although at the expense of producing more garbage).

Your main routine has a quit channel that doesn't get used. If this was for future expansion, or for use if/when main was turned into a cancel-able function then that's fine. (Although, perhaps using defer ticker.Stop() immediately after creation would be more idiomatic in that case.) However, as-is main can be greatly simplified by removing the unused channel.

Instead of checking for a pair of explicit user ids in the middle of your code, it's cleaner to move those into either a list or a map. In particular, using a map[userIDType]bool allows the check to be just if ignoredUsers[msg.UserID] {continue}.

Finally, for things like FKey, I prefer to put any authorization/API tokens anywhere but in hard-coded constants. Alternatives are on the command line, in an environment variable, in a config file, or some combination of those.

Oh, and it's unclear to me what the API is, but perhaps you should be setting the "since" field in your request to either the latest timestamp or something like time.Since(latest) + fudgeDuration or somesuch.

Edit: I made this change but forgot to mention it. You rarely need/want to call an error's Error() method. In particular, fmt.Println(err.Error()) is a long winded way of saying fmt.Println(err) (fmt handles both fmt.Stringer, aka String(), as well as error interfaces). I've seen others do things like panic(err.Error()) which is worse then panic(err) since although the message is the same it throws away any extra information/context (lost to someone trying to recover for example).


Here is what I came up with after applying the above. [Also available as a git clone-able gist with changes made a few at a time so you can see some intermediate alternatives.]

package main

import (
    "encoding/json"
    "flag"
    "fmt"
    "html"
    "log"
    "net/http"
    "net/url"
    "os"
    "strconv"
    "time"

    "github.com/kennygrant/sanitize"
)

type ChatEvent struct {
    EventType    int8   `json:"event_type"`
    Timestamp    setime `json:"time_stamp"`
    Content      string `json:"content"`
    UserID       int64  `json:"user_id"`
    Username     string `json:"user_name"`
    MessageID    int64  `json:"message_id"`
    ParentID     int64  `json:"parent_id"`
    ShowParent   bool   `json:"show_parent"`
    MessageEdits int8   `json:"message_edits"`
    MessageStars int8   `json:"message_stars"`
}

type ChatEvents struct {
    Events []ChatEvent
    Time   int64
    Sync   int64
    Ms     int8
}

const (
    mode    = "Messages"
    chatURL = "http://chat.stackexchange.com/chats/8595/events"
    // Never use this value directly: SE only uses it as a hint
    // and might return a bit more when it feels like it
    messageCount = 100
    since        = 0
)

var (
    lastMessage time.Time
    fkey        = flag.String("fkey", "", `StackExchange chat "fkey"`)
    ignoreUsers = map[int64]bool{
        -263:   true, // "Captain Obvious"
        125580: true, // "Duga"
    }
)

func main() {
    flag.Parse()
    if *fkey == "" {
        fmt.Fprintln(os.Stderr, `missing required -fkey argument

Go to the chat, F12, reload, look at the /events call
and steal its fkey property`)
        os.Exit(2) // 2 to match flag's exit code
    }

    for range time.Tick(time.Second) {
        parsedJSON, err := fetchMessages()
        if err != nil {
            log.Println("fetching messages:", err)
            // TODO, abort or backoff on repeated failures?
            continue
        }

        newestMessages := getNewestMessages(parsedJSON.Events)
        for _, msg := range newestMessages {
            if ignoreUsers[msg.UserID] {
                continue
            }

            fmt.Printf("[%v] %s: %s\n",
                msg.Timestamp.Format("15:04:05"),
                msg.Username,
                msg.Content)
        }
    }
}

func fetchMessages() (ChatEvents, error) {
    var parsedJSON ChatEvents
    resp, err := http.PostForm(chatURL, url.Values{
        "mode":     {mode},
        "msgCount": {strconv.Itoa(messageCount)},
        "fkey":     {*fkey},
        "since":    {strconv.Itoa(since)},
    })
    if err != nil {
        return parsedJSON, err
    }
    defer resp.Body.Close()
    dec := json.NewDecoder(resp.Body)
    err = dec.Decode(&parsedJSON)
    return parsedJSON, err
}

func getNewestMessages(events []ChatEvent) []ChatEvent {
    output := make([]ChatEvent, 0)
    for _, e := range events {
        if e.Timestamp.After(lastMessage) {
            e.Content = html.UnescapeString(sanitize.HTML(e.Content))
            output = append(output, e)
            lastMessage = e.Timestamp.Time
        }
    }
    return output
}

// setime is an unmarshable StackExchange timestamp or date.
// See https://api.stackexchange.com/docs/dates
type setime struct{ time.Time }

func (t *setime) UnmarshalJSON(b []byte) error {
    var seconds int64
    if err := json.Unmarshal(b, &seconds); err != nil {
        return err
    }
    t.Time = time.Unix(seconds, 0)
    return nil
}
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Note, that before Go 1.4 (I think) the above for range … loop isn't valid and won't compile. If using an old Go either upgrade or change it to for _ = range …. \$\endgroup\$ – Dave C Apr 25 '15 at 18:33
  • \$\begingroup\$ This is perfect and exactly what I was hoping for! I'll create a bounty tomorrow as an extra token of my appreciation. \$\endgroup\$ – Jeroen Vannevel Apr 25 '15 at 19:01
  • \$\begingroup\$ @JeroenVannevel, thanks but there is no need for a bounty. I'm just glad you found it useful. \$\endgroup\$ – Dave C Apr 25 '15 at 19:06
  • \$\begingroup\$ BTW, if you were to want to marshal ChatEvent you would likely want to add ",omitempty" to a few of the fields' json tags, like MessageEdits and MessageStars since they appear to normally be absent. Also perhaps some other types might be more appropriate in a few cases instead of int8/int64 (i.e. uint64 maybe, you'd need to review official documentation to know for sure). \$\endgroup\$ – Dave C Apr 25 '15 at 19:10
8
\$\begingroup\$

Disclaimer: I don't know Go.

Error handling

The sendRequest method may fail, returning nil, but the caller in main continues happily as if nothing happened. In fact, looking at the context of the call in main, my first impression was that sendRequest magically always succeeds, which is not the case, and so a bit misleading.

Likewise, parseJson may also fail, parsing nothing, but from where you call it, this is not obvious, which is again a bit misleading.

Naming

It seems [the common convention][1] is to use camelCase or PascalCase for names, no underscores. So I suggest to rename the fields of the ChatEvent struct. I'd go as far as to say use PascalCase for types and camelCase for variables, but this is not so clear in the docs.

The variable parsedJson says nothing about what kind of object it is. So I'd rename it to chatEvents.

Exceptional users

The list of such users would be better in a global set:

if currentMessage.User_id == -263 || currentMessage.User_id == 125580 {
    // Ignore Captain Obvious & Duga
    continue
}

Too wide lines

The line-end comments in the constant definitions make me scroll to the far right to read them. Even worse, while reading the code further down, when I see lines that seem to end near the right end of the text, the presence of the horizontal scrollbar makes me wonder if there's something more if I scroll right. And usually there's nothing, as expected, but this is really annoying. Please avoid line-end comments if they make the line wider than the longest real line of code. Or better yet, avoid line-end comments if they will make the line exceed 70 characters.

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ I'll look into the error handling, that's wrong indeed. The naming is stuck: the json responses have underscores and I haven't come across a way to change how it's interpreted. The wide lines are a little annoying in a webview but splitting it up in multiple lines causes the Go formatter to make it ugly. I'll see if I can change something there. \$\endgroup\$ – Jeroen Vannevel Apr 25 '15 at 11:27
  • 1
    \$\begingroup\$ Instead of changing Go identifier names to match a different language style (JSON), it's usually better to name the Go identifiers in "the Go way" and if required used struct tags, e.g. UserID int64 `json:"user_id"` . In particular, if you want to marshal into JSON that's the only way to get JSON field names to start with a lower case. \$\endgroup\$ – Dave C Apr 25 '15 at 15:06
  • 1
    \$\begingroup\$ @janos, camelCase and PascalCase (as you call them) are both idomatic Go identifier styles but with different meanings! The first is a local non-exported identifier (local to the package) whereas the second is exported from the package (unless it's a local variable or something, in which case it normally wouldn't start with an upper case letter). \$\endgroup\$ – Dave C Apr 25 '15 at 15:09
  • 1
    \$\begingroup\$ WRT "too wide" line comments; if you have a longer comment, move it to just above the line being commented and gofmt should mostly leave it be and godoc should still attach it to the item that follows. \$\endgroup\$ – Dave C Apr 25 '15 at 15:12
  • 1
    \$\begingroup\$ @DaveC: that struct tag thing looks very promising! I'll admit that when I saw it pass by earlier, I just thought it was documentation. I'll definitely change that then. Feel free to put those remarks in an answer, they're valuable. \$\endgroup\$ – Jeroen Vannevel Apr 25 '15 at 17:39

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.