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

Just making removeAllkeys more generic #482

Merged
merged 6 commits into from May 22, 2018

Conversation

Projects
None yet
4 participants
@LucianoPAlmeida
Copy link
Member

commented May 19, 2018

Just making removeAllkeys more generic so we can pass any sequence.

Checklist

  • I checked the Contributing Guidelines before creating this request.
  • New extensions are written in Swift 4.
  • New extensions support iOS 8.0+ / tvOS 9.0+ / macOS 10.10+ / watchOS 2.0+.
  • I have added tests for new extensions, and they passed.
  • All extensions have a clear comments explaining their functionality, all parameters and return type in English.
  • All extensions are declared as public.
  • I have added a changelog entry describing my changes.

LucianoPAlmeida added some commits May 19, 2018

Fix
@codecov

This comment has been minimized.

Copy link

commented May 19, 2018

Codecov Report

Merging #482 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #482   +/-   ##
=======================================
  Coverage   91.66%   91.66%           
=======================================
  Files          61       61           
  Lines        2713     2713           
=======================================
  Hits         2487     2487           
  Misses        226      226
Flag Coverage Δ
#ios 91.66% <100%> (ø) ⬆️
#osx 91.66% <100%> (ø) ⬆️
#tvos 91.66% <100%> (ø) ⬆️
Impacted Files Coverage Δ
.../Extensions/SwiftStdlib/DictionaryExtensions.swift 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afd673f...927cf91. Read the comment docs.

@SwifterSwiftBot

This comment has been minimized.

Copy link

commented May 19, 2018

1 Warning
⚠️ Consider adding tests for new extensions or updating existing tests for a modified SwifterSwift extension
1 Message
📖 Thank you for submitting a pull request to SwifterSwift. The team will review your submission as soon as possible.

Generated by 🚫 Danger

public mutating func removeAll(keys: [Key]) {
keys.forEach({ removeValue(forKey: $0)})
public mutating func removeAll<S: Sequence>(keys: S) where S.Element == Key {
Set<Key>(keys).forEach({ removeValue(forKey: $0) })

This comment has been minimized.

Copy link
@guykogus

guykogus May 21, 2018

Contributor

Is it necessary to create the Set at all? I think this qualifies as premature optimisation.

Let's calculate:
n = size of self
k1 = original size of keys
k2 = size of Set(keys)
Creating the set costs O(k1) (at best).
According to the documentation, removeValue(forKey:) costs O(n)
Therefore, removing values costs O(k2 * n) (assuming no keys were actually removed, for arguments' sake)
So the total cost = O(k1 + (k2 * n))

If you don't create the set then it would simply be O(k1 * n). And if somebody finds in writing their app that this function takes a long time because of repeated keys then they could optimise it themselves by creating the set.

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida May 21, 2018

Author Member

I see the point of letting this decision to the user, I think it's o good option :))

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida May 21, 2018

Author Member

Btw nice article about premature optimisation, thank's for sharing 👍

This comment has been minimized.

Copy link
@guykogus

guykogus May 21, 2018

Contributor

My old boss used to always call us up on it. He successfully drilled it into my head 🤯

This comment has been minimized.

Copy link
@SD10

SD10 May 22, 2018

Member

Good thoughts

public mutating func removeAll(keys: [Key]) {
keys.forEach({ removeValue(forKey: $0)})
public mutating func removeAll<S: Sequence>(keys: S) where S.Element == Key {
Set<Key>(keys).forEach({ removeValue(forKey: $0) })

This comment has been minimized.

Copy link
@guykogus

guykogus May 21, 2018

Contributor

I think SwiftLint gives a warning when you can remove the parenthesis, e.g. in forEach. So it would just be
forEach { removeValue(forKey: $0) }

public mutating func removeAll(keys: [Key]) {
keys.forEach({ removeValue(forKey: $0)})
public mutating func removeAll<S: Sequence>(keys: S) where S.Element == Key {
keys.forEach({ removeValue(forKey: $0) })

This comment has been minimized.

Copy link
@guykogus

guykogus May 21, 2018

Contributor

My old comment seems to have disappeared, but I think it's better to remove the parenthesis for forEach. I.e. forEach { removeValue(forKey: $0) }. It's what autocomplete does and I think SwiftLint complains about it too.

This comment has been minimized.

Copy link
@LucianoPAlmeida

LucianoPAlmeida May 21, 2018

Author Member

Swiftlint doesn't complain, but no problem changing it

This comment has been minimized.

Copy link
@guykogus

guykogus May 21, 2018

Contributor

Not all SwiftLint rules are applied by default. You're welcome to add it: unneeded_parentheses_in_closure_argument ;)
https://github.com/realm/SwiftLint/blob/master/Source/SwiftLintFramework/Rules/UnneededParenthesesInClosureArgumentRule.swift

@SD10

This comment has been minimized.

Copy link
Member

commented May 22, 2018

I approve this pull request

Sent with GitHawk

@LucianoPAlmeida LucianoPAlmeida merged commit 3d31723 into master May 22, 2018

5 checks passed

codecov/changes No unexpected coverage changes found.
Details
codecov/patch 100% of diff hit (target 91.66%)
Details
codecov/project 91.66% (+0%) compared to afd673f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@LucianoPAlmeida LucianoPAlmeida deleted the generic-remove-all branch May 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.