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

Code refactor and optimization #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Baldomo
Copy link

@Baldomo Baldomo commented Apr 3, 2022

First things first: this PR does not introduce any change in functionality. I just found the code hard to read because of the many nested if's and other non-idiomatic code choices, so I simplified the following snippets of code (these are examples). I also cleaned up go.mod with go mod tidy because it contained some unneeded dependencies and the code has been linted and formatted with gofumpt and golangci-lint.

Some notes before the code:

  • I use Go 1.18 but I deliberately avoided using newer features of the language because I wanted this to be a "simple" code refactoring/optimization job
  • The example programs in examples/ were also formatted, linted and modified per the rules listed below
  • The examples still work the same
    • The debug log is shorter because of the optimizations I got by making the code simpler (especially by avoiding iteration over maps)
  • The dependencies were not updated with go get -u or anything
  • connectContext.error was renamed to connectContext.err (in wpa-connect.go) because the linter didn't like it

Top-level if check for self.Error

The function can just return early if self.Error is not nil, instead of having a big code block inside a huge if.

func (self *BSSWPA) ReadWPS() *BSSWPA {
    if self.Error == nil {
        // do stuff
    }
    return self
}

becomes

func (self *BSSWPA) ReadWPS() *BSSWPA {
    if self.Error != nil {
        return self
    }
    
    // do stuff
    
    return self
}

if call := ...; call.Err = nil with empty if block

Since self.Error will be nil anyways and we care only about the value of call.Err, we don't need to check wether it's nil or not and just assign it straight to self.Error.

if call := self.Interface.WPA.Connection.BusObject().Call("org.freedesktop.DBus.AddMatch", 0, match); call.Err == nil {
} else {
    self.Error = call.Err
}
return self

becomes

call := self.Interface.WPA.Connection.BusObject().Call("org.freedesktop.DBus.AddMatch", 0, match)
self.Error = call.Err

return self

Function named return simplification

In several cases, using unnamed return values can improve readability of a function, because it requires all the returned values be explicitly on a single line, nils included.
(This is more of a personal opinion, though).

func (self *WPA) get(name string, target dbus.BusObject) (value interface{}, e error) {
    if variant, err := target.GetProperty(name); err == nil {
        value = variant.Value()
    } else {
        e = err
    }
    return
}

can be simplified into

func (self *WPA) get(name string, target dbus.BusObject) (interface{}, error) {
    variant, err := target.GetProperty(name)
    if err != nil {
        return nil, err
    }

    return variant.Value(), nil
}

Removed iteration over map retrieved from DBus

Self explanatory. Since the code iterates over all the keys of a map with a nested if, and maps can only have a single value bound to a specific key, the code can be simplified to a straight map-value-ok check. (Variables have also been renamed to proper names instead of value and key)

for key, variant := range value {
    if key == "Type" {
        self.WPS = variant.Value().(string)
    }
}

becomes

if wpsType, found := value["Type"]; found {
    self.WPS = wpsType.String()
}

Use defer for closing connections and signal waiting

Using defer is very good practice when handling connection cleanup and similar boilerplate: a deferred function will always run, even on early return. This prevents memory leaks and also makes the code somewhat cleaner.

wpa.WaitForSignals(self.onScanSignal)
// ...
iface.AddSignalsObserver()

// do stuff

iface.RemoveSignalsObserver()
wpa.StopWaitForSignals()
return

becomes

wpa.WaitForSignals(self.onScanSignal)
defer wpa.StopWaitForSignals()
// ...
iface.AddSignalsObserver()
defer iface.RemoveSignalsObserver()

// do stuff

return

Types and New* function moved to beginning of the file

Self explanatory. Makes type definition easier to find just by opening the file.

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.

1 participant