Code review? | Discord Gophers | Page 1

sleek stone · 2024-04-16T21:19:39.741Z

some quick observations - constants should be not prefixed with _, use either upper camel case or lower depending if you want to export them or not https://github.com/KasimKaizer/SweetClover/blob/main/internal/music/music.go#L15 https://github.com/KasimKaizer/SweetClover/blob/main/cmd/tui/styles.go#L18 - you should add tests to cover the application logic, namely `music.go`. the bubbletea TUI part might be harder to test but I am sure there are solutions for that too - you probably don't need an `internal` directory, just flatten the repository and have your main in `cmd` - as an extra you might want to set up a github action for example to build a release, this way it's easier to distribute the application to end users I didn't compile or look deeper into the logic with channels and wait groups but writing tests would be a good start to be sure that things ||probably|| work the way you want them to

having go packages with upper case letters is generally against convention. nothing inherently wrong with it though
TextWidth or textWidth
-you have your processDir thing that uses sync.Once. I would personally just use a slice and errs.Join -> https://pkg.go.dev/errors#Join
over just directory flattening, i would only have a main package.
having the music stuff in its own package is probably unnecessary
instead of having the Music struct have a FilePath string, i would instead make PopulateMusicMeta take a io.Reader (ideal) or just a filepath itself and populate based on that. feels like filepath is an implementation leak as to be part of the struct.
based on the implementation of CoverArtASCII, i would personally just change around the music function. i would make the NewMusic take a path (or io.Reader), parse the tag, and store the contents of the tag in the struct as well, instead of having the ASCII function re-read it.

type Music struct {
  Name, Artist, Album string
  Year int

  tag tag.Tag // not sure if this is the right type
}

func NewMusicFromPath(path string) (*Music, error) {
  f, err := os.Open(path)
  if err != nil { ... }
  defer f.Close()
  return NewMusic(f)
}

func NewMusic(r io.Reader) (*Music, error) {
  m := &Music{..}
  var err error
  m.tag, err = tag.Readfrom(r)
  // ... rest of populateMusicMeta can go here
}

func (m *Music) CoverArtASCII(w, h int) {
  // use internal `tag` instead of re-reading from file
}

#Code review?