icerpc / icerpc-csharp

A C# RPC framework built for QUIC, with bidirectional streaming, first-class async/await, and Protobuf support.
https://docs.icerpc.dev
Apache License 2.0
107 stars 13 forks source link

slicec-cs Cleanup Pass 2 #3888

Closed InsertCreativityHere closed 9 months ago

InsertCreativityHere commented 9 months ago

This PR performs some more cleanup in slicec-cs. No logic or function is altered by this PR, it just cleans and refactors. It removes 54 lines of code (for a running total of 140 lines).

I'm only opening a PR to make sure CI passes. But if you want to review it, go for it!

InsertCreativityHere commented 9 months ago

I guess I merged just as you finished reviewing :)

It is somewhat of a Rust convention, because Rust lacks a ternary operator, and matching like this is more compact that a full if/else statement.

InsertCreativityHere commented 9 months ago

Neither is more correct than the other. For me, I only use this match convention if the branches are very simple, ie. if I would normally use a ternary operator.

if field.is_tagged() {
    encode_tagged_type
} else {
    encode_type
}

vs

match field.is_tagged() {
    true => encode_tagged_type,
    false => encode_type
}
pepone commented 9 months ago

, because Rust lacks a ternary operator

Rust if is an expression so there is no need for a ternary operator, in C the ternary operator is required because if is an statement.

I very much prefer if for checking true/false because it is more specific, when you see an if you know that is all it is doing.

Whatever we do we should be consistent, a few lines below in the PR we have:

let parameters = if return_type {
        operation.non_streamed_return_members()
    } else {
        operation.non_streamed_parameters()
    };
InsertCreativityHere commented 9 months ago

when you see an if you know that is all it is doing.

I used to be of the same opinion! But over time I've come to see it the other way :)

match bool can only have 2 possibilities. Right underneath it you'll have an exhaustive list of the two paths it can take. There's no other sane outcome here. And I can see the single condition being tested, up front, right after the match.

An if statement can invite intermediate checks: if foo {} else if unrelated_thing {} else {}, so there could be more than 2 possible paths. And the conditions being checked can be completely unrelated.

With match, all the branches are indented to the same level, with no break in between. This makes it easy to see where the check starts and ends:

match thing {
    true => something,
    false => something_else,
}

With if, there's a dedented line in-between, breaking it up. In the middle of lots of code (like in slicec-cs), this makes it just a little harder to see what's going on.

if thing {
    something
} else {
    something_else
}

You can grep through our repos, we use it in a handful of places in slicec, the language server, and obviously slicec-cs:

true =>|false =>

Or, searching on GitHub: https://github.com/search?q=true+%3D%3E+language%3ARust&type=code&l=Rust There's no definitive rule on when to use it, it's just style and preference.

I only use it when the true/false are very short. If the cases need more than one line, I use if/else instead. I don't have a reason for this. It's just what I've done shrugs.

InsertCreativityHere commented 9 months ago

Despite my small novel:

I don't actually care!

I just wanted to convey what eventually made me switch sides. If you want to update everything to use if/else, that's genuinely fine by me! Because I think the most important thing, is that we're consistent.

There's all kinds of things like this in slicec-cs, hence why I've started cleaning it. I'm just doing the larger refactorings first before going through and looking at what looks weird or inconsistent!