I am learning Go and wrote a small application that queries the Skyscanner API as my first attempt at learning. I was hoping someone more expert than me in the language could look over it at broad strokes and let me know if I'm on the right track. I really appreciate any feedback I can get!
Also, I know this is a fair amount to read over. Feedback on even one or two of the files is appreciated.
main.go
package main
import (
"fmt"
"log"
"os"
"text/tabwriter"
"github.com/sdstolworthy/go-fly/skyscanner"
)
func main() {
fmt.Println()
params := skyscanner.Parameters{
Adults: 1,
Country: "US",
Currency: "USD",
Locale: "en-US",
OriginPlace: "SLC-sky",
DestinationPlace: "BNA-sky",
OutbandDate: "anytime",
InboundDate: "anytime",
}
for _, v := range DestinationAirports {
params.DestinationPlace = v
fmt.Println(v)
SkyscannerQuotes := skyscanner.BrowseQuotes(params)
w := tabwriter.NewWriter(os.Stdout, 0, 0, 3, ' ', 0)
quote, err := SkyscannerQuotes.LowestPrice()
if err != nil {
log.Printf("%v\n\n", err)
continue
}
fmt.Fprintf(w, "Price:\t$%v\nDeparture:\t%v\nReturn:\t%v\t\n\n", quote.Price, quote.DepartureDate, quote.InboundDate)
}
}
destinations.go
package main
// DestinationAirports is an array of all airports that should be queried
var DestinationAirports = [...]string{
"CDG-sky",
"BNA-sky",
"LHR-sky",
"ZAG-sky",
"SAN-sky",
"MEX-sky",
}
skyscanner/config.go
package skyscanner
import (
"io/ioutil"
"log"
"gopkg.in/yaml.v2"
)
// Config contains application secrets
type Config struct {
MashapeKey string `yaml:"mashape_key"`
MashapeHost string `yaml:"mashape_host"`
BaseURL string `yaml:"base_url"`
}
func (c *Config) getConfig() *Config {
yamlFile, err := ioutil.ReadFile("conf.yaml")
if err != nil {
log.Printf("yamlFile.Get err #%v ", err)
}
err = yaml.Unmarshal(yamlFile, c)
if err != nil {
log.Fatalf("Unmarshal: %v", err)
}
return c
}
skyscanner/network.go
package skyscanner
import (
"encoding/json"
"fmt"
"io/ioutil"
"log"
"net/http"
"time"
)
func prettyPrint(apiResponse Response) {
fmt.Printf("%+v\n", apiResponse.Quotes)
}
func parseResponse(response *http.Response) Response {
body, readErr := ioutil.ReadAll(response.Body)
if readErr != nil {
log.Fatal(readErr)
}
apiResponse := Response{}
jsonErr := json.Unmarshal(body, &apiResponse)
if jsonErr != nil {
log.Fatal(jsonErr)
}
return apiResponse
}
func getRequest(url string) *http.Request {
req, err := http.NewRequest(http.MethodGet, url, nil)
var config Config
config.getConfig()
req.Header.Set("X-Mashape-Key", config.MashapeKey)
req.Header.Set("X-Mashape-Host", config.MashapeHost)
if err != nil {
log.Fatal(err)
}
return req
}
func getClient() *http.Client {
return &http.Client{
Timeout: time.Second * 2,
}
}
func formatURL(path string) string {
var c Config
c.getConfig()
baseURL := c.getConfig().BaseURL
return fmt.Sprintf("%v%v", baseURL, path)
}
func get(url string) *http.Response {
res, getErr := getClient().Do(getRequest(url))
if getErr != nil {
log.Fatal(getErr)
}
return res
}
/*
BrowseQuotes stub
*/
func BrowseQuotes(parameters Parameters) Response {
browseQuotes := formatURL(fmt.Sprintf("browsequotes/v1.0/%v/%v/%v/%v/%v/%v/%v",
parameters.Country,
parameters.Currency,
parameters.Locale,
parameters.OriginPlace,
parameters.DestinationPlace,
parameters.OutbandDate,
parameters.InboundDate,
))
res := get(browseQuotes)
return parseResponse(res)
}
skyscanner/parameters.go
package skyscanner
// Parameters generic request type for skyscanner api
type Parameters struct {
Country string `json:"country"`
Currency string `json:"currency"`
Locale string `json:"locale"`
OriginPlace string `json:"originPlace"`
DestinationPlace string `json:"destinationPlace"`
OutbandDate string `json:"outboundDate"`
Adults int `json:"adults"`
InboundDate string `json:"inboundDate"`
}
skyscanner/response.go
package skyscanner
import "errors"
// Response generic response from skyscanner api
type Response struct {
Quotes []struct {
QuoteID int `json:"QuoteId"`
MinPrice float64 `json:"MinPrice"`
Direct bool `json:"Direct"`
OutboundLeg struct {
CarrierIds []int `json:"CarrierIds"`
OriginID int `json:"OriginId"`
DestinationID int `json:"DestinationId"`
DepartureDate string `json:"DepartureDate"`
} `json:"OutboundLeg"`
InboundLeg struct {
CarrierIds []int `json:"CarrierIds"`
OriginID int `json:"OriginId"`
DestinationID int `json:"DestinationId"`
DepartureDate string `json:"DepartureDate"`
} `json:"InboundLeg"`
QuoteDateTime string `json:"QuoteDateTime"`
} `json:"Quotes"`
Places []struct {
PlaceID int `json:"PlaceId"`
IataCode string `json:"IataCode"`
Name string `json:"Name"`
Type string `json:"Type"`
SkyscannerCode string `json:"SkyscannerCode"`
CityName string `json:"CityName"`
CityID string `json:"CityId"`
CountryName string `json:"CountryName"`
} `json:"Places"`
Carriers []struct {
CarrierID int `json:"CarrierId"`
Name string `json:"Name"`
} `json:"Carriers"`
Currencies []struct {
Code string `json:"Code"`
Symbol string `json:"Symbol"`
ThousandsSeparator string `json:"ThousandsSeparator"`
DecimalSeparator string `json:"DecimalSeparator"`
SymbolOnLeft bool `json:"SymbolOnLeft"`
SpaceBetweenAmountAndSymbol bool `json:"SpaceBetweenAmountAndSymbol"`
RoundingCoefficient int `json:"RoundingCoefficient"`
DecimalDigits int `json:"DecimalDigits"`
} `json:"Currencies"`
}
// Prices returns an array of the lowest prices for a route and date
func (r *Response) Prices() []float64 {
priceList := make([]float64, len(r.Quotes))
for i, v := range r.Quotes {
priceList[i] = v.MinPrice
}
return priceList
}
// A QuoteSummary is a summary of a outbound trip with its price and date
type QuoteSummary struct {
Price float64
DepartureDate string
InboundDate string
}
// LowestPrice prints a list of the lowest prices and their accompanying dates
func (r *Response) LowestPrice() (*QuoteSummary, error) {
quote := QuoteSummary{
99999999,
"",
"",
}
for _, v := range r.Quotes {
if v.MinPrice < quote.Price {
quote.Price = v.MinPrice
quote.DepartureDate = v.OutboundLeg.DepartureDate
quote.InboundDate = v.InboundLeg.DepartureDate
}
}
if quote.Price == 99999999 {
return nil, errors.New("No quote found")
}
return "e, nil
}
1 Answer 1
(I only looked at the GitHub repository ...)
Ah you've got go vet already on, that's why it's not finding anything,
great!
Dockerfile looks good too. You might want to specify an exact Go
version just in case.
main.go looks fine to me. Only the unused return value from
router.Run() would pop up depending on your linter settings, which you
could explicitly ignore with _ = router.Run(), but I see it's being
logged in case of errors already.
airports.go has a few obvious comments that therefore don't add much
information. Also f should be closed. Not sure what happens if you
can't open the file? Also the constructed path there might not always
work, I'd argue that a command-line flag specifying the file directly,
or perhaps just a relative path if it's always called from the main
directory might be a bit safer. Finally each struct is saved
individually - I'm guessing this is not a problem so far, but you might
want some batching and transaction safety at some point.
db.go imports SQLite and PostgreSQL, but the connection opening is
using "postgres" only, consider moving that into the configuration
object too, or otherwise get rid of SQLite?
The fmt.Println should probably be a "proper" logger before you've to
convert more of those calls later on.
airports.go has all methods that can return an error, but don't have
a single one that actually does in any situation. I'd get rid of the
error return value if it's not being used. Instead of
upper(...) LIKE upper(...) consider using ILIKE. Now that I think
about it, also consider adding some default LIMIT's to limit the
potential for mistakes/misuse.
airportsController.go etc. use a global variable environment -
that's usually not a great idea since it makes it a bit harder to work
with, plus, nothing prevents you from putting that into the controller
structure and making e.g. seachAirports a method instead.
I'm not familiar with gin, so context.JSON etc. look a bit weird to
me, but nothing too bad. Consider though logging the database error for
debug purposes! It might be that the connection got dropped, or any
other of the big pool of possible failures and just returning
"No results found" will not help you figuring that one out after the
fact. N.b. it looks like if the caller specifies both iataCode and
cityName that the HTTP response will be attempted to be written twice,
that looks wrong to me, even if the framework catches that for you.
destinations.go, I had to look up the syntax again, the [...]
doesn't really buy you anything, does it.
quoteController.go is a bit more interesting again, saveQuotes
should set up a transaction I think?
Alright, so the skyscanner package again uses a global configuration
and again I'd advise against that.
analytics.go looks okay as long as your floating point values don't
get too big I think. Otherwise you'd have to search for the proper
algorithms and/or use a library.
http.go, hmm, looks a bit more odd, the logging isn't consistent (more
fmt.Print...), also printing of values without indication where it's
coming from; I'd also expect that you'd get rate-limited at some point.
Maybe doing only a few requests at the same time plus handling errors
more gracefully (that is, retrying automatically) would improve this.
Also prettyPrint is unused.
response.go looks fine, except in LowestPrice I rally dislike the
placeholder value 99999999 - of course it's somewhat common practice,
but simply looking at len(r.Quotes) would immediately tell you whether
there were any quotes at all!
Overall looks good to me, you could then also add some tests maybe.