Skip to main content
added 368 characters in body
Source Link
rose
  • 325
  • 1
  • 6

Your code is very clear and readable in general. I came up with a very nitpicky list of style suggestions :)

A few comments on main()

  • time.Duration(1000) * time.Millisecond can be written as just 1000 * time.Millisecond
  • Call the cancel function from context.WithTimeout, otherwise your program has a resource leak (not that it matters from main, but still).
    ctx, cancel := context.WithTimeout(...)
    defer cancel()
    
  • Use ctx consistently, there are some places in main where it refers to context.Background() instead.
  • Handle the error from os.Create, or your code will be impossible to debug when it fails.
  • Similarly, you probably want to print an error if urlHolder.WriteTo fails.
  • I would also suggest making a helper function for creating the XML file and writing to it, in particular if you ever change the file naming pattern you would need to change it in two places.
  • I think there's no need for a goroutine in main? You can just run it directly unless I'm missing something.

A few things on urlholder.go

  • Time time.Time type is almost always used as a value and not as a pointer. The URL.LastMod field should be a time.Time, not a *time.Time.
  • This is subjective, but UrlMap should be spelled as URLMap.
  • No need to make a slice of size zero, just use nil.
  • UrlMap.Add should return an error instead of panicing.
  • The UrlMap.Add method uses a different method of serialization than WriteTo, so the byte count may not be correct. I would suggest calling WriteTo from inside Add, as follows
    func (m *UrlMap) Add(u *URL) int {
        s.URLs = append(s.URLs, u)
        var out strings.Builder
        m.WriteTo(&out)
        return len(out.String())
    }
    

I'm not sure what the point of CounterWriter is when the result of urlHolder.WriteTo is ignored anyway.

And since you asked about interfaces, there's no need to use an interface here. In general you should only refactor to use interfaces when you already have two implementations in mind. Using an interface preemptively for a "cleaner" design is an example of YAGNI: you ain't gonna need it.

Your code is very clear and readable in general. I came up with a very nitpicky list of style suggestions :)

A few comments on main()

  • time.Duration(1000) * time.Millisecond can be written as just 1000 * time.Millisecond
  • Call the cancel function from context.WithTimeout, otherwise your program has a resource leak (not that it matters from main, but still).
    ctx, cancel := context.WithTimeout(...)
    defer cancel()
    
  • Use ctx consistently, there are some places in main where it refers to context.Background() instead.
  • Handle the error from os.Create, or your code will be impossible to debug when it fails.
  • Similarly, you probably want to print an error if urlHolder.WriteTo fails.
  • I would also suggest making a helper function for creating the XML file and writing to it, in particular if you ever change the file naming pattern you would need to change it in two places.
  • I think there's no need for a goroutine in main? You can just run it directly unless I'm missing something.

A few things on urlholder.go

  • Time time.Time type is almost always used as a value and not as a pointer. The URL.LastMod field should be a time.Time, not a *time.Time.
  • This is subjective, but UrlMap should be spelled as URLMap.
  • No need to make a slice of size zero, just use nil.
  • UrlMap.Add should return an error instead of panicing.

I'm not sure what the point of CounterWriter is when the result of urlHolder.WriteTo is ignored anyway.

And since you asked about interfaces, there's no need to use an interface here. In general you should only refactor to use interfaces when you already have two implementations in mind. Using an interface preemptively for a "cleaner" design is an example of YAGNI: you ain't gonna need it.

Your code is very clear and readable in general. I came up with a very nitpicky list of style suggestions :)

A few comments on main()

  • time.Duration(1000) * time.Millisecond can be written as just 1000 * time.Millisecond
  • Call the cancel function from context.WithTimeout, otherwise your program has a resource leak (not that it matters from main, but still).
    ctx, cancel := context.WithTimeout(...)
    defer cancel()
    
  • Use ctx consistently, there are some places in main where it refers to context.Background() instead.
  • Handle the error from os.Create, or your code will be impossible to debug when it fails.
  • Similarly, you probably want to print an error if urlHolder.WriteTo fails.
  • I would also suggest making a helper function for creating the XML file and writing to it, in particular if you ever change the file naming pattern you would need to change it in two places.
  • I think there's no need for a goroutine in main? You can just run it directly unless I'm missing something.

A few things on urlholder.go

  • Time time.Time type is almost always used as a value and not as a pointer. The URL.LastMod field should be a time.Time, not a *time.Time.
  • This is subjective, but UrlMap should be spelled as URLMap.
  • No need to make a slice of size zero, just use nil.
  • UrlMap.Add should return an error instead of panicing.
  • The UrlMap.Add method uses a different method of serialization than WriteTo, so the byte count may not be correct. I would suggest calling WriteTo from inside Add, as follows
    func (m *UrlMap) Add(u *URL) int {
        s.URLs = append(s.URLs, u)
        var out strings.Builder
        m.WriteTo(&out)
        return len(out.String())
    }
    

I'm not sure what the point of CounterWriter is when the result of urlHolder.WriteTo is ignored anyway.

And since you asked about interfaces, there's no need to use an interface here. In general you should only refactor to use interfaces when you already have two implementations in mind. Using an interface preemptively for a "cleaner" design is an example of YAGNI: you ain't gonna need it.

Source Link
rose
  • 325
  • 1
  • 6

Your code is very clear and readable in general. I came up with a very nitpicky list of style suggestions :)

A few comments on main()

  • time.Duration(1000) * time.Millisecond can be written as just 1000 * time.Millisecond
  • Call the cancel function from context.WithTimeout, otherwise your program has a resource leak (not that it matters from main, but still).
    ctx, cancel := context.WithTimeout(...)
    defer cancel()
    
  • Use ctx consistently, there are some places in main where it refers to context.Background() instead.
  • Handle the error from os.Create, or your code will be impossible to debug when it fails.
  • Similarly, you probably want to print an error if urlHolder.WriteTo fails.
  • I would also suggest making a helper function for creating the XML file and writing to it, in particular if you ever change the file naming pattern you would need to change it in two places.
  • I think there's no need for a goroutine in main? You can just run it directly unless I'm missing something.

A few things on urlholder.go

  • Time time.Time type is almost always used as a value and not as a pointer. The URL.LastMod field should be a time.Time, not a *time.Time.
  • This is subjective, but UrlMap should be spelled as URLMap.
  • No need to make a slice of size zero, just use nil.
  • UrlMap.Add should return an error instead of panicing.

I'm not sure what the point of CounterWriter is when the result of urlHolder.WriteTo is ignored anyway.

And since you asked about interfaces, there's no need to use an interface here. In general you should only refactor to use interfaces when you already have two implementations in mind. Using an interface preemptively for a "cleaner" design is an example of YAGNI: you ain't gonna need it.