HackerNews news fetcher

The following code is part of a big application. I am new to Go, and appreciate feedback on the best practices and improvements. Please consider styling and comments as well.

Every 30 minutes, this will fetch news from HackerNews and store it in a database:

What fetches the news:

```
package services

import (
"fmt"
"net/http"
"log"
"io/ioutil"
"github.com/demas/cowl-go/pkg/quzx-crawler"
"encoding/json"
"github.com/demas/cowl-go/pkg/postgres"
"time"
"strconv"
)

// represent an implementation of quzx_crawler.HackerNewsService
type HackerNewsService struct {
}

const idsUrl = "https://hacker-news.firebaseio.com/v0/topstories.json?print=pretty"
const newsUrl = "https://hacker-news.firebaseio.com/v0/item/%d.json?print=pretty"
const syncInterval = 60 * 30 // 30 minutes

func (s *HackerNewsService) Fetch() {

var lastSyncTime int64
var err error

lastSyncTimeStr := (&postgres.SettingsRepository{}).GetSettings("lastHackerNewsSyncTime")
if lastSyncTimeStr == "" {
lastSyncTime = time.Now().Unix() - syncInterval - 1
} else {
lastSyncTime, err = strconv.ParseInt(lastSyncTimeStr, 10, 64)
if err != nil {
log.Fatal(err)
}
}

currentTime := time.Now().Unix()

if lastSyncTime + syncInterval > currentTime {
return
}

// get list of messages
res, err := http.Get(idsUrl)
if err != nil {
log.Fatal(err)
}

jsn, err := ioutil.ReadAll(res.Body)
res.Body.Close()
if err != nil {
log.Fatal(err)
}

// decode
var ids []int64
err = json.Unmarshal(jsn, &ids)

if err != nil {
log.Fatal(err)
} else {

for _, id := range ids {

if !(&postgres.HackerNewsRepository{}).NewsExists(id) {

// fetch each news
log.Println("hacker news: " + fmt.Sprintf(newsUrl, id))
res, err := http.Get(fmt.Sprintf(newsUrl, id))
if err != ni

• newsUrl → newsURL, same for idsUrl.

• const syncInterval = 30 * time.Minute is more readable. You can also convert everything that represents a time into a time.Duration or a time.Time, rather than deal with Unix timestamps. There are functions in the time package that do what you want to do (time.Date, time.Since, etc.).

• About that, why does Fetch need to do complicated things with times, checking when was the last time it was called, etc.? Shouldn't the caller take care of this with a simple time.Ticker instead? It feels like this complexity shouldn't be there.

• HackerNewsService is a weird name for an implementation. HackerNewsFetcher maybe?

• Your postgres package isn't very idiomatic, which makes the callers do weird things to use it. For example, why do you have a SettingsRepository struct that doesn't contain anything? Why not simply have GetSettings as a package-level function? Same for the other empty structs of this package (HackerNewsRepository…).

• It's more idiomatic to test for len(myString) == 0 rather than myString == "".

• I'm not exactly sure what you're doing with lastSyncTime (this is pretty complex). Maybe add a comment for the line lastSyncTime = time.Now().Unix() - syncInterval - 1 or refactor this part?

• Talking about refactor, Fetch() is way too long. Split it in sub-functions.

• No need for an else after log.Fatal.

-
Rather than:

// fetch each news
log.Println("hacker news: " + fmt.Sprintf(newsUrl, id))

why not:
log.Println("fetching from hacker news", fmt.Sprintf(newsUrl, i))

• Using log.Fatal everywhere makes your code not very resilient. Maybe it's worth looking at "what can fail for transient reasons" (typically, database transactions…), and re-try the thing that failed a few times with exponential backoff.

• There's lots of whitespace in the "what stores the data" part, which isn't very idiomatic. Empty lines in functions aren't really frequent.

StackExchange Code Review Q#158132, answer score: 3