mark2b / wpa-connect

wpa-connect
MIT License
61 stars 19 forks source link

Code refactor and optimization #10

Open Baldomo opened 2 years ago

Baldomo commented 2 years ago

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:

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.