Remove various IUOs, force unwraps, and force casts #89
Conversation
The code as it was would not have resulted in crashes, but this syntax is better because it remove the need for force unwrapping altogether.
The value the consuming code will return is the same, but removing the `as!` prevents runtime crashes if the value accessed cannot cast to `TimeInterval?`
This DRYs the consuming code.
| func testEndpoint() { | ||
| let endpoint = RequestBuilder.buildPixelEndpoint() | ||
| XCTAssert(endpoint != "", "buildPixelEndpoint should return a non-empty string") | ||
| } |
There was a problem hiding this comment.
This test was redundant given the next one performed a more accurate check that implied non-emptyness.
| var expected: String = "https://p1.parsely.com/mobileproxy" | ||
| var actual = RequestBuilder.buildPixelEndpoint() | ||
| XCTAssert(actual == expected, "buildPixelEndpoint should return the correct URL for the given date") | ||
| expected = "https://p1.parsely.com/mobileproxy" | ||
| actual = RequestBuilder.buildPixelEndpoint() | ||
| XCTAssert(actual == expected, "buildPixelEndpoint should return the correct URL for the given date") |
There was a problem hiding this comment.
As far as I could see, these were duplicated checks.
There was a problem hiding this comment.
Our pixel server used to have today's date in the hostname. I think the multiple checks should have been removed when we made it a static hostname.
mokagio
changed the title
| var expected: String = "https://p1.parsely.com/mobileproxy" | ||
| var actual = RequestBuilder.buildPixelEndpoint() | ||
| XCTAssert(actual == expected, "buildPixelEndpoint should return the correct URL for the given date") | ||
| expected = "https://p1.parsely.com/mobileproxy" | ||
| actual = RequestBuilder.buildPixelEndpoint() | ||
| XCTAssert(actual == expected, "buildPixelEndpoint should return the correct URL for the given date") |
There was a problem hiding this comment.
Our pixel server used to have today's date in the hostname. I think the multiple checks should have been removed when we made it a static hostname.
| urlref: urlref, | ||
| metadata: metadata, | ||
| extra_data: extra_data, | ||
| // empty string seems a weird default, but is what we had in the code before this comment was written |
There was a problem hiding this comment.
The idsite aka apikey is the primary identifier of which customer traffic is coming from. It's normally their domain, eg arstechnica.com. Making it default to blank means that any pixels coming from a misconfigured SDK will be filtered out of our pipeline very early in the process so that we don't waste CPU and storage with bad data.
In most instances, the code was safe enough to ensure these maneuvers would never crash. Still, not having them in the first place makes the code even safer 🤓
I recommend reviewing commit by commit.