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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed array to range #2720

Merged
merged 3 commits into from Mar 8, 2019

Conversation

Projects
None yet
4 participants
@StevenArmandLee
Copy link
Contributor

StevenArmandLee commented Feb 15, 2019

Issue Link 馃敆

status code validation inefficient #2719

Goals 鈿斤笍

increase the efficiency of the code, contain in array is O(n) where as contain in Range is comparison
https://github.com/apple/swift/blob/3b6c6cc0a5424fbac81213376969432b0b16ad5e/stdlib/public/core/Range.swift#L165-L168

Testing Details 馃攳

Ran the existing unit test and all passed

@StevenArmandLee

This comment has been minimized.

Copy link
Contributor Author

StevenArmandLee commented Feb 15, 2019

tested with measure, using String giving us 0.00223s where Range gives us 0.0000106s with test code of acceptableStatusCodes.contains(299)

@agnosticdev

This comment has been minimized.

Copy link

agnosticdev commented Feb 16, 2019

Very interesting @StevenArmandLee. Are the time measurements the elapsed times measured from the call to acceptableStatusCodes to validate the response code?

For example:

let dateStart = Date()
...
let elapsedTime = dateStart.timeIntervalSinceNow
print("\(abs(elapsedTime))")
@StevenArmandLee

This comment has been minimized.

Copy link
Contributor Author

StevenArmandLee commented Feb 16, 2019

@agnosticdev I did not test the full function, I just the contains method with measure block provided by XCTest
here is my test code

func testSpeed() {
        var acceptableStatusCodes: [Int] { return Array(200..<300) }
//        var acceptableStatusCodes: Range<Int> { return 200..<300 }
        self.measure {
            acceptableStatusCodes.contains(299)
            acceptableStatusCodes.contains(299)
            acceptableStatusCodes.contains(299)
            acceptableStatusCodes.contains(299)
            acceptableStatusCodes.contains(299)
            acceptableStatusCodes.contains(299)
            acceptableStatusCodes.contains(299)
        }
    }

the reason I use 299 is to present the worst case scenario.

@agnosticdev

This comment has been minimized.

Copy link

agnosticdev commented Feb 16, 2019

Excellent, thank you for providing that testing information @StevenArmandLee

@StevenArmandLee

This comment has been minimized.

Copy link
Contributor Author

StevenArmandLee commented Feb 19, 2019

Will this PR being reviewed?

@cnoon

This comment has been minimized.

Copy link
Member

cnoon commented Feb 19, 2019

Sure @StevenArmandLee, it's just not our highest priority right now. Thanks for understanding.

@jshier

This comment has been minimized.

Copy link
Contributor

jshier commented Mar 8, 2019

This is a relatively minor improvement but an improvement nonetheless. Thanks for the PR!

@jshier jshier merged commit 5fe7897 into Alamofire:master Mar 8, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@jshier jshier modified the milestones: 5.X, 5.0.0-beta.3 Mar 8, 2019

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