Conversation
There was a problem hiding this comment.
Thanks for this! :)
I'll do a proper review later this weekend...
Functionality looks all good, might be room to improve the structure of the code, implement some best practices and make it a bit more efficient. Looks like tests are missing too.
Error handling too, there are instances where errors aren't getting caught
handlers/archives.go
Outdated
| func getWaybackData(url string) (map[string]interface{}, error) { | ||
| cdxUrl := fmt.Sprintf("%s?url=%s&output=json&fl=timestamp,statuscode,digest,length,offset", archiveAPIURL, url) | ||
|
|
||
| resp, err := http.Get(cdxUrl) |
There was a problem hiding this comment.
Sometimes requests just hang, so we should include a timeout to prevent that
handlers/archives.go
Outdated
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| urlParam := r.URL.Query().Get("url") | ||
| if urlParam == "" { | ||
| http.Error(w, "missing 'url' parameter", http.StatusBadRequest) |
There was a problem hiding this comment.
Don't forget, if there's an error anywhere, we need to still respond with JSON, like { error: "Missing 'url' parameter" }. Same goes for the other error responses below.
| // Remove the header row | ||
| data = data[1:] | ||
|
|
||
| firstScan, err := convertTimestampToDate(data[0][0]) |
There was a problem hiding this comment.
Do we need to do a length check, before accessing data[0] and data[len(data)-1] to avoid potential panics?
| func convertTimestampToDate(timestamp string) (time.Time, error) { | ||
| year, err := strconv.Atoi(timestamp[0:4]) | ||
| if err != nil { | ||
| return time.Time{}, err | ||
| } | ||
| month, err := strconv.Atoi(timestamp[4:6]) | ||
| if err != nil { | ||
| return time.Time{}, err | ||
| } | ||
| day, err := strconv.Atoi(timestamp[6:8]) | ||
| if err != nil { | ||
| return time.Time{}, err | ||
| } | ||
| hour, err := strconv.Atoi(timestamp[8:10]) | ||
| if err != nil { | ||
| return time.Time{}, err | ||
| } | ||
| minute, err := strconv.Atoi(timestamp[10:12]) | ||
| if err != nil { | ||
| return time.Time{}, err | ||
| } | ||
| second, err := strconv.Atoi(timestamp[12:14]) | ||
| if err != nil { | ||
| return time.Time{}, err | ||
| } | ||
| return time.Date(year, time.Month(month), day, hour, minute, second, 0, time.UTC), nil | ||
| } |
There was a problem hiding this comment.
It's a small thing, but I'd probably put the minuteIndex, hourIndex, dayIndex, etc as variables, to avoid magic numbers. Because, for example hour, err := strconv.Atoi(timestamp[8:10]) is a bit hard to read.
const (
yearIndex = 0
monthIndex = 4
dayIndex = 6
hourIndex = 8
minuteIndex = 10
secondIndex = 12
timestampLength = 14
averagePageSizeDiv = 100
)
There was a problem hiding this comment.
The golang way :)
mask := "20060102150405"
return time.Parse(mask, timestamp)
Codecov ReportAttention: Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
|
handlers/archives.go
Outdated
| } | ||
|
|
||
| func getScanFrequency(firstScan, lastScan time.Time, totalScans, changeCount int) map[string]float64 { | ||
| formatToTwoDecimal := func(num float64) float64 { |
There was a problem hiding this comment.
can use
fmt.Sprintf("%.2f", num)3 participants
No description provided.