Made methods in CFSocketStream consistent in style

[ghost]

In CFSocketStream, there are two methods, _CFStreamErrorFromError and _CFErrorFromStreamError. Both methods are similar, except one has multiple return statements while the other only has one. I changed both methods to have multiple return statements while keeping the original structure the same.

[xwu]

I don't think this is an improvement. The purpose of having return result at the end is to allow the reader to see at a glance that a result is returned regardless of which branches are taken.

Moreover, you have taken code that's bracketed by a lock and moved it outside, which does not appear to be correct.

I think it's fine if you want to change the two lines that return a result to read result = ..., but I really don't see the point.

[ghost]

Fixed!

[ghost]

@swift-ci please test

[xwu]

You don't have permissions to trigger testing. For any changes you submit, you should test them locally on your own computer. See the readme for more details.

[ghost]

I do test them. They pass. The exception was the one rebase where the extra parenthesis got in there and resulted in failed compilation

[ghost]

I disagree with your statement, because a return statement is a return statement. Developers know when a return statement is hit, nothing else is done; it is the end. At a glance, developers need to check the blocks to see if there will be any changes even in class where that does not happen; only in the else block is a variable changed as it goes along. In the other branches, result is set and only returned at the end.

As for the result variable, only declaring it when there needs to be a variable to be changed is more practical and I doubt any developer would know this. I would understand if the functions are over 30 lines long, but when a function is under 20 lines and fit entirely on screen, the entire function can be seen and developers know each branch has an end.

Those are my two cents on this. You can disagree with them.

[ghost]

Perfect!

[xwu]

Yes, early return can be an improvement, but for such consistency the code would have had to be refactored differently (and more extensively).

In the absence of a compelling reason, we try not to make stylistic changes to existing code, because that can only introduce additional bugs. We'll see if others find this change worthwhile as it is for the sake of consistency, but honestly, I think the existing code is fine.

Outdated

[ghost]

I agree with your proposal. Maybe others will find it worthwhile, maybe other's won't.

[xwu]

Did you change the PR again? If you want to use early return, then this refactoring will have to be more extensive. Specifically, there’s no point in using else if the other branch always returns. That would improve the clarity to the reader.

But this is a more extensive change, and as I stated, in general, we don’t make stylistic changes like this to long-established code just for the sake of style, because it can introduce new bugs.

[xwu]

Yes, good, that would be the more extensive refactoring.

You can see how it's a lot of change--not sure if we want to have that churn, and the potential for errors, when the existing code is essentially fine; at best, this will compile down to the same thing.

[ghost]

Fixed!

[ghost]

Closing this for now