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

Cleanup. #4

Merged
merged 2 commits into from
Feb 18, 2016
Merged

Cleanup. #4

merged 2 commits into from
Feb 18, 2016

Conversation

hinnerk
Copy link
Contributor

@hinnerk hinnerk commented Feb 17, 2016

Turns out that we actually need less than 25% of the legacy code.

def self.start(config_path, hostapd_path)
networks = network_config config_path
# TODO: Is this rubyish?
if not networks.any?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I prefer the "full-method wrap" over guard clauses, i.e.

if networks.any?
  all the code
end

This implies that you return nil instead of false, but since that's also falsy in a lot of situations it's better - but opinions on this differ, see this recent discussion I had with a bunch of my fellow contributors for simplecov

@colszowka
Copy link
Contributor

Very cool! Made a few stylistic remarks, but nothing really big. One thing that I'd personally prefer would be to now jump the extra hoop and make this all instance- instead of class-methods. This way you could do away with all the parametrization of the method calls since they'd just pull in from the config loaded in def initialize(config_path) - but that would basically be icing on the cake, just a suggestion :)

@colszowka colszowka assigned hinnerk and unassigned colszowka Feb 18, 2016
@hinnerk
Copy link
Contributor Author

hinnerk commented Feb 18, 2016

Thank you very much for the review and your suggestions! I've implemented most of them.

hinnerk added a commit that referenced this pull request Feb 18, 2016
@hinnerk hinnerk merged commit c93fe54 into development Feb 18, 2016
@hinnerk hinnerk deleted the hh-cleanup branch February 18, 2016 13:18
@colszowka
Copy link
Contributor

👏

@hinnerk
Copy link
Contributor Author

hinnerk commented Feb 18, 2016

💃

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