3
\$\begingroup\$

Recent take-home challenge I was assigned from a recruiter prior to technical interviews with the team. The team wanted it written in GOlang (I had no experience in the language) but told the recruiter I was confident I could complete the task. He mentioned he would let the team know of my inexperience with the language.

The only feedback I received back was there were too many "errors" and they didn't want to move forward. Looking back through and running my code I couldn't find anything glaring. Any critiques or suggestions would be appreciated.

What is "wrong" with my code?

Instructions

The goal of the exercise is to implement a chat server. 
The chat server will run on http://localhost:8081 and will support the following REST API: 

1. GET /messages

    list 100 most recent messages, sorted by 'timestamp' posted to the chat server.

    example:
    ========

    ```
    curl -H "Content-Type: application/json" http://localhost:8081/messages

    {
    "messages: [
        {"timestamp": 1491345710.18, "user": "superman", "text": "hello"},
        {"timestamp": 1491345713.18, "user": "batman", "text": "hello"}
    ]
    }

    ```

2. POST /message 

    a request to post the given message. 
    when the message is processed by the server a unix timestamp is recorded with each message.

    example:
    ========

    ```
    curl -X POST -H "Content-Type: application/json" --data '{"user":"superman", "text":"hello"}' http://localhost:8081/message

    {
    "ok": true
    }
    ```

3. GET /users

    a request to return a set of users that have posted messages so far.

    example:
    ========

    ```
    curl -H "Content-Type: application/json" http://localhost:8081/users

    {
    "users": [
        "superman", "batman"
    ]
    }
    ```

the server should respond with 404 to all other requests not listed above

1. Preferred programming language: GoLang
2. Please provide a *single implementation* with no external dependencies.
3. The submitted implementation will be tested by an automated script 
     that will build the chat app with: `make build` 
     and execute it with: `make run`
   (see attached Makefile)
4. Please describe what metrics you would monitor to track the performance of the chat server.
5. Bonus Points: please describe how you would improve the chat server API.

chatserver.go

package main

import (
    "net/http"
    "time"
    "encoding/json"
    "sort"
    "strings"
    "fmt"
    "strconv"
)

type PostedUsers struct {
    Users     []string `json:"users"`
}

type ValidPostResp struct {
    Ok     bool `json:"ok"`
}

type Message struct {
    User      string   `json:"user"`    
    Text      string   `json:"text"`
    TimeStamp float64      `json:"timestamp"`
}

// Create type for messages with custom sort functions that use the timestamp field 
type MessageSorter []Message
func (a MessageSorter) Len() int {
    return len(a)
}
func (a MessageSorter) Swap(i, j int) {
    a[i], a[j] = a[j], a[i]
}
func (a MessageSorter) Less(i, j int) bool {
    return a[i].TimeStamp < a[j].TimeStamp
}

// Globals to keep track of messages and usernames posted
var messages []Message
var usersPosted PostedUsers

// The number of messages to return on get request
const maxMessages = 100

// Endpoint handlers
func GetStatusEndpoint(c http.ResponseWriter, req *http.Request) {
    if req.Method != "GET" {
        respondWithError(c, http.StatusNotFound, "404 not found")
        return
    }

    c.Write([]byte("alive"))
}

func GetUsersEndpoint(c http.ResponseWriter, req *http.Request) {
    if req.Method != "GET" {
        respondWithError(c, http.StatusNotFound, "404 not found")
        return
    }
    respondWithJson(c, http.StatusOK, usersPosted)
}

func GetMessagesEndpoint(c http.ResponseWriter, req *http.Request) {
    if req.Method != "GET" {
        respondWithError(c, http.StatusNotFound, "404 not found")
        return
    }

    recentMessages := GetMostRecentMessagesSorted(maxMessages, messages)
    respondWithJson(c, http.StatusOK, recentMessages)
}

func PostMessageEndpoint(c http.ResponseWriter, req *http.Request) {
    if req.Method != "POST" {
        respondWithError(c, http.StatusNotFound, "404 not found")
        return
    }

    var mesg Message
    mesg.TimeStamp = GetCurrMessageTimeStamp()
    err := json.NewDecoder(req.Body).Decode(&mesg)
    if err != nil || mesg.User == "" || mesg.Text == "" {
        respondWithError(c, http.StatusBadRequest, "400 invalid request")
        return
    }
    messages = append(messages, mesg)

    if !hasUserPosted(mesg.User, usersPosted.Users) {
        usersPosted.Users = append(usersPosted.Users, mesg.User)
    }

    valid := ValidPostResp{true}
    respondWithJson(c, http.StatusOK, valid)
}

func InvalidEndpoint(c http.ResponseWriter, req *http.Request) {
    respondWithError(c, http.StatusNotFound, "404 not found")
    return
}

// Gets the current time in seconds as float with precision to 2 decimal points
func GetCurrMessageTimeStamp() float64 {
    // Get current time in seconds 
    // ** Could've used time.Now().Unix() but noticed in the instructions the timestamps had precision to 2 decimal places
    secsFloat := float64(time.Now().UnixNano()) / float64(1000000000)
    secsString := fmt.Sprintf("%.2f", secsFloat)
    timestampSec, convErr := strconv.ParseFloat(secsString, 64)
    if (convErr != nil) {
        // Something went wrong with conversion falling back to seconds
        timestampSec = float64(time.Now().Unix())
    }

    return timestampSec
}

// This function sorts allMessages from latest to most recent
// and returns an array with the most recent total messages
func GetMostRecentMessagesSorted(total int, allMessages []Message) []Message {

    // If we could assume that allMessages will always have the most recent messages 
    // with the highest index, then we wouldn't need this sort 
    // (currently that assumption is correct but could change later)
    sort.Sort(MessageSorter(allMessages))

    // If the length of allMessages is greater than the total requested 
    // calculate the first index we should start capturing recentMessages
    startIndex := 0
    if len(allMessages) > total {
        startIndex = len(allMessages) - total
    }

    var recentMessages []Message
    for i := 0; i < len(allMessages); i++ {
        if i >= startIndex {
            recentMessages = append(recentMessages, allMessages[i])
        }
    }
    return recentMessages
}

// This function checks for the occurence of the checkUser string in the 
// usersPosted array. Assuming usernames that post would be case insentive
func hasUserPosted(checkUser string, usersPosted []string) bool {
    for _, user := range usersPosted {
        if strings.ToLower(user) == strings.ToLower(checkUser) {
            return true
        }
    }
    return false
}

func respondWithError(c http.ResponseWriter, code int, msg string) {
    respondWithJson(c, code, map[string]string{"error": msg})
}

func respondWithJson(c http.ResponseWriter, code int, payload interface{}) {
    response, _ := json.Marshal(payload)
    c.Header().Set("Content-Type", "application/json")
    c.WriteHeader(code)
    c.Write(response)
}


func main() {
    http.HandleFunc("/status", GetStatusEndpoint)
    http.HandleFunc("/users", GetUsersEndpoint)
    http.HandleFunc("/messages", GetMessagesEndpoint)
    http.HandleFunc("/message", PostMessageEndpoint)
    http.HandleFunc("/", InvalidEndpoint)
    err := http.ListenAndServe(":8081", nil)
    panic(err)
}

Makefile

build:
    go build -o chatserver

test:
    go test

run:
    ./chatserver

alive:
    curl "http://localhost:8081/status"
    curl -X POST -d "{\"user\": \"batman\", \"text\":\"hello\"}" "http://localhost:8081/message"
    curl "http://localhost:8081/messages"
    curl "http://localhost:8081/users"

api_test.go

package main

import (
    "testing"
    "net/http"
    "net/http/httptest"
    "bytes"
    "encoding/json"
    "strconv"
)

type PostMessage struct {
    User      string   `json:"user"`    
    Text      string   `json:"text"`
}

type Header struct {
    Key          string
    Value        string
}

type EndPoint struct {
    Path          string
    HandlerFunc   func(http.ResponseWriter, *http.Request)
    CheckMethods  []CheckMethod
}

type CheckMethod struct {
    Type                         string
    Body                         []byte
    Headers                      []Header
    ExpectedStatusCode           int
    ValidateRespFn               func(*testing.T, *bytes.Buffer)
    SetupFn                      func()
    TearDownFn                   func()
}

func TestEndPoints(t *testing.T) {

    notAllowedGetMethod := CheckMethod {
        Type: "GET",
        Body: nil,
        Headers: nil,
        ExpectedStatusCode: http.StatusNotFound,
        ValidateRespFn: nil,
        SetupFn: nil,
        TearDownFn: nil,
    }
    notAllowedPostMethod := CheckMethod {
        Type: "POST",
        Body: nil,
        Headers: nil,
        ExpectedStatusCode: http.StatusNotFound,
        ValidateRespFn: nil,
        SetupFn: nil,
        TearDownFn: nil,
    }
    notAllowedPutMethod := CheckMethod {
        Type: "PUT",
        Body: nil,
        Headers: nil,
        ExpectedStatusCode: http.StatusNotFound,
        ValidateRespFn: nil,
        SetupFn: nil,
        TearDownFn: nil,
    }
    notAllowedDeleteMethod := CheckMethod {
        Type: "DELETE",
        Body: nil,
        Headers: nil,
        ExpectedStatusCode: http.StatusNotFound,
        ValidateRespFn: nil,
        SetupFn: nil,
        TearDownFn: nil,
    }

    jsonStr, _ := json.Marshal(PostMessage{"Elon", "Hello!"})
    endPoints := []EndPoint {
        EndPoint {
            Path: "/status",
            HandlerFunc: GetStatusEndpoint,
            CheckMethods: []CheckMethod {
                CheckMethod {
                    Type: "GET",
                    Body: nil,
                    Headers: nil,
                    ExpectedStatusCode: http.StatusOK,
                    ValidateRespFn: validateGetStatusResponse,
                    SetupFn: nil,
                    TearDownFn: nil,
                },
                notAllowedPostMethod,
                notAllowedPutMethod,
                notAllowedDeleteMethod,
            },
        },
        EndPoint {
            Path: "/message",
            HandlerFunc: PostMessageEndpoint,
            CheckMethods: []CheckMethod {
                CheckMethod {
                    Type: "POST",
                    Body: jsonStr,
                    Headers: []Header{
                        Header{"Content-Type", "application/json"},
                    },
                    ExpectedStatusCode: http.StatusOK,
                    ValidateRespFn: validatePostMessageResponse,
                    SetupFn: nil,
                    TearDownFn: nil,
                },
                notAllowedGetMethod,
                notAllowedPutMethod,
                notAllowedDeleteMethod,

            },
        },
        EndPoint {
            Path: "/messages",
            HandlerFunc: GetMessagesEndpoint,
            CheckMethods: []CheckMethod {
                CheckMethod {
                    Type: "GET",
                    Body: nil,
                    Headers: nil,
                    ExpectedStatusCode: http.StatusOK,
                    ValidateRespFn: validateGetMessagesResponse,
                    SetupFn: getMessagesSetup,
                    TearDownFn: getMessagesTearDown,
                },
                notAllowedPostMethod,
                notAllowedPutMethod,
                notAllowedDeleteMethod,
            },
        },
        EndPoint {
            Path: "/users",
            HandlerFunc: GetUsersEndpoint,
            CheckMethods: []CheckMethod {
                CheckMethod {
                    Type: "GET",
                    Body: nil,
                    Headers: nil,
                    ExpectedStatusCode: http.StatusOK,
                    ValidateRespFn: validateGetUsersResponse,
                    SetupFn: getUsersSetup,
                    TearDownFn: getUsersTearDown,
                },
                notAllowedPostMethod,
                notAllowedPutMethod,
                notAllowedDeleteMethod,
            },
        },
        EndPoint {
            Path: "/",
            HandlerFunc: InvalidEndpoint,
            CheckMethods: []CheckMethod {
                {
                    Type: "GET",
                    Body: nil,
                    Headers: nil,
                    ExpectedStatusCode: http.StatusNotFound,
                    ValidateRespFn: nil,
                    SetupFn: nil,
                    TearDownFn: nil,
                },
                {
                    Type: "POST",
                    Body: nil,
                    Headers: nil,
                    ExpectedStatusCode: http.StatusNotFound,
                    ValidateRespFn: nil,
                    SetupFn: nil,
                    TearDownFn: nil,
                },
                {
                    Type: "PUT",
                    Body: nil,
                    Headers: nil,
                    ExpectedStatusCode: http.StatusNotFound,
                    ValidateRespFn: nil,
                    SetupFn: nil,
                    TearDownFn: nil,
                },
                {
                    Type: "DELETE",
                    Body: nil,
                    Headers: nil,
                    ExpectedStatusCode: http.StatusNotFound,
                    ValidateRespFn: nil,
                    SetupFn: nil,
                    TearDownFn: nil,
                },
            },
        },
    }

    for _, endPoint := range endPoints {
        for _, method := range endPoint.CheckMethods {

            if method.SetupFn != nil {
                method.SetupFn()
            }

            req, err := http.NewRequest(method.Type, endPoint.Path, bytes.NewBuffer(method.Body))
            if err != nil {
                t.Fatal(err)
            }

            respRecorder := executeRequest(req, endPoint.HandlerFunc)
            checkResponseCode(t, method.ExpectedStatusCode, respRecorder.Code, endPoint.Path)

            if (method.ValidateRespFn != nil) {
                method.ValidateRespFn(t, respRecorder.Body)
            }

            if method.TearDownFn != nil {
                method.TearDownFn()
            }
        }
    }
}

func executeRequest (req *http.Request, endPointFunc func(http.ResponseWriter, *http.Request)) *httptest.ResponseRecorder {
    respRecorder := httptest.NewRecorder()
    handler := http.HandlerFunc(endPointFunc)
    handler.ServeHTTP(respRecorder, req)
    return respRecorder
}

func checkResponseCode(t *testing.T, expected, actual int, endPoint string) {
    if expected != actual {
        t.Errorf("%s handler returned wrong status code: got %v want %v", endPoint, actual, expected)
        return
    }
}


func validateGetStatusResponse(t *testing.T, body *bytes.Buffer) {
    if body.String() != "alive" {
        t.Errorf("get status handler returned incorrect response: got %v want %v", body.String(), "alive")
    }
}

func validatePostMessageResponse(t *testing.T, body *bytes.Buffer) {
    var validResp ValidPostResp
    decodeErr := json.NewDecoder(body).Decode(&validResp)
    if decodeErr != nil {
        t.Errorf("post message handler returned incorrect response: got %v want %v", body.String(), `{"ok":true}`)
        return
    }
}

func getMessagesSetup() {
    // Fill the global messages array with test messages
    messages = nil
    now := GetCurrMessageTimeStamp()
    for i:=0; i < 200; i++ {
        user := "elon-" + strconv.Itoa(i)
        text := "the master plan " + strconv.Itoa(i)
        now = now + 1
        messages = append(messages, Message{user, text, now})
    }
}

func getMessagesTearDown() {
    messages = nil
}

func validateGetMessagesResponse(t *testing.T, body *bytes.Buffer) {
    var getMessages []Message
    decodeErr := json.NewDecoder(body).Decode(&getMessages)
    if decodeErr != nil {
        t.Errorf("get messages handler couldn't be parsed to valid messages: got %v", body.String())
        return
    }

    // Check that the messages array returned is not greater than the max allowed
    if len(getMessages) > maxMessages {
        t.Errorf("get messages handler returned too many messages: got %v want %v", len(getMessages), maxMessages)
        return
    }


    // Check the messages are in the right order
    for i, getMesg := range getMessages {
        if (i > 0 && getMesg.TimeStamp < getMessages[i - 1].TimeStamp) {
            getMesgJsonStr, _ := json.Marshal(getMesg)
            prevMesgJsonStr, _ := json.Marshal(getMessages[i - 1])
            t.Errorf("get messages handler returned invalid message order at index %v: got %v prev %v", i, getMesgJsonStr, prevMesgJsonStr)
        }
    }
}

func getUsersSetup() {
    // Fill the global users array with test users
    usersPosted.Users = nil
    for i:=0; i < 200; i++ {
        user := "elon-" + strconv.Itoa(i)
        usersPosted.Users = append(usersPosted.Users, user)
    }
}

func getUsersTearDown() {
    usersPosted.Users = nil
}

func validateGetUsersResponse(t *testing.T, body *bytes.Buffer) {
    var getUsers PostedUsers
    decodeErr := json.NewDecoder(body).Decode(&getUsers)
    if decodeErr != nil {
        t.Errorf("get users handler couldn't be parsed to valid response: got %v should be an object with structure like %v", body.String(), "{users:['userexample1','userexample2']}")
    }

    if len(getUsers.Users) != 200 {
        t.Errorf("get users returned incorrect number of users: got %v want %v", len(getUsers.Users), 200)
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Have you tested whether it works as it should? \$\endgroup\$ – Mast Aug 17 '18 at 6:24
  • \$\begingroup\$ yes I have, running "make alive" there are no errors and all the data returned looks correct. I even have unit tests that I built out as well that I provided to them and can post on here as well \$\endgroup\$ – shreddish Aug 17 '18 at 14:03
4
\$\begingroup\$

The specs given to you read a little strangely. I don't know have having no external dependencies makes something a *single implementation* or for that matter what single implementation means (other than the obvious, only one implementation, not several). External dependencies in go aren't that uncommon. Specifically when creating an HTTP server, as you discovered the stdlib approach is quite verbose. There are minimal frameworks out there that can do a much better job at capturing function without needing so much boilerplate (and also provide nice testing utilities, something that you also could have benefited from). Further, as I understand the Go philosophy is pretty opposed to Makefiles. The entire point of all the care put into go build is so that you don't need to spend time crafting a Makefile and consumers of your code don't need to spend time reading it (or documentation) to understand how to use it. That request strikes me as odd.

As a nit, you should only return 404 for endpoints that actually don't exist. /messages exists, so DELETE /messages shouldn't 404, it should 405 Method Not Allowed (and send the header Allow: POST, GET).

As another nit, I just noticed they mix /message and /messages. As a rule, RESTful APIs should usually use the plural for everything. This way programmatic API access is easier. Without this rule given the endpoint /message, you need a pluralizer to derive the endpoint to list all messages.

The spec is also way underspecified. Is there any requirement for persistence? Is this server standalone or meant to be run in a distributed context (the latter complicates the persistence greatly, for one)? Should /users return all users who have ever posted or only those in the last 100 messages? Are we allowed to discard messages beyond the last 100?

Things I noticed scrolling through your code:

  • In general, for an executable prefer the name main.go. Then go build will do what you want (produce a binary with the same name as the parent dir)
  • Your Makefile doesn't really properly leverage make and instead could be a script that switches on the first arg. You should use dependencies and name targets as the files they produce, or otherwise mark as .PHONY. If you said that you had previous experience with a language that relies heavily on Makefiles, this would be a red flag to me as an interviewer.
GO:=go

build: chatserver

run: chatserver
    ./chatserver

chatserver: *.go
    $(GO) build

test:
    $(GO) test

.PHONY: build run test
  • Your code could do with some separation of concerns/Single responsibility principle. That fact that all of your logic is in one file should be a red flag. Separate this out. I'd pull out the business logic into its own interface and then provide an implementation to an HTTP server. In this way, when testing the HTTP server in isolation, you can pass mock of the business logic. You have somewhat implemented this already, but with global functions and state (and global state is bad).

chat/message.go:

package chat

import (
    "time"
)

// Whether you expose these is a design decision. You could choose not to,
// and then provide User(), Text(), and SentAt(), but for simplicity I chose
// not to.
// For simplicity of the HTTP server, you may also choose to add the JSON
// serialization here, but that does tie this abstract model to a concrete
// representation, which may be undesirable if there are alternative protocols
// that want to serialize messages (say you want to forward messages to a
// Slack API, for example)
type Message struct {
    User string
    Text string
    SentAt time.Time
}

func NewMessage(user, text string) Message {
    return Message{User: user, Text: text, SentAt: time.Now()}
}

chat/user.go:

package chat

type User struct {
    Name string
}

chat/state.go:

package chat

type State interface {
    RecentMessages() []Message
    AddMessage(Message)
    ActiveUsers() []User
}

chat/in_memory_state.go:

package chat

type InMemoryState struct {
    messages []Message
    users UserSet
}

// You may want to make this configurable by adding it to InMemoryState
const NumRecentMessages int = 100

func NewInMemoryState() *InMemoryState {
    return &InMemoryState{
        messages: nil,
        users: NewUserSet(),
    }
}

func (s *InMemoryState) RecentMessages() []Message {
    return s.messages
}

func (s *InMemoryState) AddMessage(message Message) {
    recentMessages = s.messages[max(0, len(s.messages) - NumRecentMessage - 1)]
    s.messages = append(recentMessages, message)
    s.users.Append(message.User)
}

func max(x, y int) int {
    if x > y {
        return x
    }

    return y
}

func (s *InMemoryState) ActiveUsers() []User {
    return s.users.Slice()
}

That is the extent of your business logic. Really. That's it. Simple and concise. Really easy to test. UserSet should just be a wrapper struct containing a map[User]bool and []User. The former acts as a set and the later is the list of all keys in the map (provided so we have quick access to a slice of all Users without needing to enumerate through the keys of the map).

With a design like this, your main should be a lot simpler:

package main

import (
    "flag"
    "github.com/you/chat" // admittedly a pain point of go
)

var address = flag.String("address", ":8081", "the address for the chat HTTP server to listen on")

func main() {
    state := chat.NewInMemoryState()
    server := chat.NewHTTPServer(state)
    log.Println(server.ListenAndServe(*address))
}

Note that the signature of NewHTTPServer is:

func NewHTTPServer(state State) *HTTPServer

Let's say that one of your comments for how to improve the service is you want to add persistence. You can now do that very easily. Just create a new implementation of State, perhaps DiskPersistedState. What if you decide you want to back with Postgres? Create a PostgresState. Separation like this allows you to individually configure the concerns where appropriate and allows the HTTP server to care about none of these implementation details. All it has to care about is that its given something that can return to it a []Message, append a Message, and return a []User. This is a lot easier to reason about than yours, which has to worry about implementing the UserSet logic, doing data validation, etc. alongside the HTTP-specific concerns.

  • Good use of sort.Sort. As a nitpick, we'd usually say ByTimestamp instead of MessageSorter. It reads a bit nicer this way: sort.Sort(ByTimestamp(messages)). But, above and in your code the messages slice is already sorted, so why sort again?
  • return alone is unnecessary and should be eliminated
  • As I did above, use time.Time for message timestamps instead of float64. The go community spent a lot of time perfecting that API. Use it! If you need to serialize it to some silly format to meet the spec, do that in HTTPServer.
  • Decent commenting. Look into go doc to see how you can easily get beautiful docs automatically.
  • Instead of your creation of a new slice in GetMostRecentMessagesSorted, why not just use a slice (see above for an example)? This will be way more efficient than allocating all that memory for each request!
  • Don't panic(http.ListenAndServe()). It always returns an error!

Your HTTP server stuff (if you refactored out the business logic) largely looks okay. Looking at your tests, the fact that you have a PostMessage suggests that perhaps in your refactored HTTPServer perhaps you should have a struct like that for reading input JSON and then copy the relevant fields into a real chat.Message (perhaps by using chat.NewMessage() to abstract away the time stuff).

Your tests:

Separating out the business logic will help keep these more concise. I had a hard time groking these as TestEndPoints is huge. When testing the server, pass in a mocked State. Then you don't have to deal with setting up state via API calls. You can test endpoints in isolation.

Your tests though long aren't very thorough. Instead, hand craft specific examples. Separate these out into different functions. Mocking out state makes this easy. Generate a state with no messages, a few messages, no users, and a few users. See if the /messages and /users endpoints return the proper thing (by hardcoding the result you expect into the test). Try calling the POST /messages and see if state.AddMessage() is called with the proper Message. Then exercise edge cases like improper request bodies, bad methods, and bad request paths.

All in all, this is a very good for your first time using Go. You picked up on a lot of the idioms fairly well. There are a few jagged spots, but I would argue that language itself also has a few warts that really clash with best practices that apply to every other language. So, well done and keep chugging with go! If anything, I would look to it to do anything requiring concurrency or any sort of networking. Go's standard library is the essence of elegance.

\$\endgroup\$
1
  • \$\begingroup\$ > "for an executable prefer the name main.go. Then go build will do what you want (produce a binary with the same name as the parent dir)". This is what go build always does, the name of the source file is irrelevant and if a more descriptive name makes sense (like chatserver.go here) then it's good to use that instead of main.go. \$\endgroup\$ – Dave C Sep 3 '18 at 19:06

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.