HiveBrain v1.2.0
Get Started
← Back to all entries
patterngoModerate

Golang HTTP status checker

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
golanghttpstatuschecker

Problem

I'm writing my first somewhat production ready (or not) Go program and could do with some feedback from someone more experienced with go.

The code reads a list of URLs from a JSON file and then makes a GET request to each URL in a loop to check that they are returning a 200 OK response. I have attempted to do this concurrently using go routines but I'm concerned about my synchronization step.

monitor.go

```
package main

import (
"encoding/json"
"fmt"
"github.com/bradfitz/gomemcache/memcache"
"os"
"time"
)

// Configuration encapsulates the result of reading the JSON configuration
// file.
type Configuration struct {
URLs []string
Memcached string
}

// loadConfig loads a configuration file in JSON format and returns a
// Configuration instance.
func loadConfig(path string) (Configuration, error) {
file, _ := os.Open(path)
defer file.Close()

decoder := json.NewDecoder(file)
configuration := Configuration{}
err := decoder.Decode(&configuration)

return configuration, err
}

func main() {
type Result struct {
url string
status Status
}

rc := make(chan Result)

configuration, err := loadConfig("config.json")

if err != nil {
fmt.Println("Error :", err)
return
}

sites := make(map[string]*Site, len(configuration.URLs))

for _, url := range configuration.URLs {
sites[url] = &Site{url, UNCHECKED}
}

mc := memcache.New(configuration.Memcached)

sites_output := make(map[string]bool)

for {
for _, site := range sites {
go func(site *Site, rc chan Result) {
status, _ := site.Status()
rc <- Result{site.url, status}
}(site, rc)
}

for i := 0; i < len(sites); i++ {
res := <-rc
site := sites[res.url]
if site.last_status != res.status {
sites[res.url].last_status = res.status
}
}

Solution

Style

-
Use CamelCase only. last_status -> lastStatus, site_json -> siteJSON, etc.

-
Even in constants. UP -> Up or StatusUp, etc.

-
Imports are usually split into groups by source, so separate memcache from stdlib with a newline.

-
Stuttering. E.g. configuration := Configuration{} could be c := Configuration{}.

-
People would search for main() in main.go. So I would rename monitor.go to main.go.

Code

file, _ := os.Open(path)


I see you skip error handling in some places. I'm not sure whether this is just a brief version for this site, or an actual code, but this is a very bad practice.

v.last_status == 2

if (err == nil) && (resp.StatusCode == 200) {


Avoid magic numbers. Use Up/StatusUp and http.StatusOK respectively.

for i := 0; i < len(sites); i++ {


I'd use range. In Go 1.4 it'll be legal to write for range sites as well.

site := sites[res.url]
if site.last_status != res.status {
    sites[res.url].last_status = res.status
}


You index the map twice and make an unnecessary comparison. Wouldn't sites[res.url] = res.status do?

Architecture

If you can change the output JSON format, I'd suggest saving the status code returned by http.Get rather than a simple bool. That will give you more info on why the site is down (4XX and 5XX error) or that the site have moved (3XX).


I have attempted to do this concurrently using go routines but I'm concerned about my synchronization step.

Your synchronisation seems mostly fine to me. (Don't forget to test with -race though). An alternative would be to use sync.WaitGroup and a map with a sync.Mutex.

Code Snippets

file, _ := os.Open(path)
v.last_status == 2

if (err == nil) && (resp.StatusCode == 200) {
for i := 0; i < len(sites); i++ {
site := sites[res.url]
if site.last_status != res.status {
    sites[res.url].last_status = res.status
}

Context

StackExchange Code Review Q#64254, answer score: 15

Revisions (0)

No revisions yet.