Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve code quality and legibility #734

Closed

Conversation

kamushadenes
Copy link

@kamushadenes kamushadenes commented Jul 26, 2024

This PR intends to improve Golang code quality and legibility to facilitate future changes.

A non-exhaustive list of changes is:

  • Remove a ton of dead code / unused functions and vars in order to simplify review and auditing
  • Simplify some nested ifs that made the code hard to read (there are many left though)
  • Where applicable, replace if-else with switch-case
  • Replace many instances of strings.Index() checks with strings.Contains()
  • Replace strings.HasSuffix() + Trim with non-conditional strings.TrimSuffix()
  • Use var <name> <type> instead of <name> := <type>(zero value)
  • Replace var names that conflicted with reserved keywords (len, close, etc...)
  • Properly handle ineffectual assignments (like errs that weren't checked etc)
  • Remove unnecessary fmt.Sprintf() on non-formatting strings
  • Remove a bunch of redundant returns
  • Remove a bunch of unnecessary / redundant conversions
  • Simplify expressions such as x = x + y => x += y
  • Replace string(r.Bytes()) with r.String() in *bytes.Buffer instances.
  • Format files using go fmt
  • Replace some strings with consts, such as POST => http.MethodPost and 200 => http.StatusOK (there are still many repeated strings that could become consts)
  • Use append(slice, newSlice...) instead of a for loop
  • Use raw strings in regex.MustCompile
  • Fix instances of == true and if cond == true ... return true ... else return false
  • And others

There are still many improvements to be made, but this PR was already too big so I'll make separate PRs after this one (hopefully) gets merged.

@esimkowitz I need help testing this. It shouldn't change anything (besides perhaps throwing some errors that were previously incorrectly ignored), but better safe than sorry.

@esimkowitz
Copy link
Member

Closing as we've made significant changes to Wave with v0.8 that make these changes no longer relevant

@esimkowitz esimkowitz closed this Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants