Refactor threshold method #412
Conversation
The `threshold` function is a candidate for refactoring because it contains code at multiple layers of abstraction as well as duplicate code. Refactor out the sorting closures into three, very similar, functions. Refactor only, no logic changes.
Currently we are using a mutable variable to manually implement the logic that the `fold` combinator provides.
| // costy satisfactions are satisfied, the most costy dissatisfactions are dissatisfied). | ||
| // | ||
| // Args are of form: (<count_sat>, <count_dissat>) | ||
| fn sat_minus_dissat<'r, 's>( |
There was a problem hiding this comment.
In cb4c037:
I think all these functions should be defined at the top of the function that they're used in.
There was a problem hiding this comment.
I'm glad you brought this up, I've wanted to discuss it with you for ages now :) When I started writing Rust I had come from C and used to do as you suggest. I was then introduced to the idea of laying out a source file with the most important things at the top, this implies putting functions below where they are called. Another data point; I also read the idea somewhere of writing functions at a single level of abstraction the writing next, and below that, all the functions at the next layer of abstraction and so on.
Related; we have a bunch of places where the error struct and implementation is at the top of the file, I'd prefer to see that code buried way down lower so I don't see it very often.
Is there a reason you favour putting functions above where they were first called (apart from habit from writing/reading C code)?
There was a problem hiding this comment.
I would like these functions' visibility to match the only scope where they're used, since they are essentially helpers for the code in that scope.
There was a problem hiding this comment.
This isn't about being C-like -- you can't even do what I'm suggesting in C.
There was a problem hiding this comment.
I have no strong preferences here.
If the name is descriptive and is being used multiple times in different methods(I think it is the case for some of the functions here). It makes sense to have it at the bottom.
On the other hand, if it is just a helper function, it makes sense to have it a function near the body or even a sub-function inside the main function. The readers can cleanly separate these things.
| fn opt_add(a: Option<usize>, b: Option<usize>) -> Option<usize> { | ||
| a.and_then(|x| b.map(|y| x + y)) | ||
| } | ||
|
|
||
| /// Returns Some((x0+y0, x1+y1)) is both x and y are Some. Otherwise, returns `None`. | ||
| fn opt_tuple_add(a: Option<(usize, usize)>, b: Option<(usize, usize)>) -> Option<(usize, usize)> { |
There was a problem hiding this comment.
In 28662d2:
Same here. I think this'd be clearer defined close to its use.
Do you mean the combinator stuff or the 'sort_by' functions? I'm not 100% sure the sort_by functions are better than inline. |
|
I mean the combinator stuff -- I've gotten pushback in rust-bitcoin for being too functional. (But in this library maybe it's ok..) |
lolz, we should get some less-functional devs around here. |
|
More seriously, this is a valid concern. I get that functional style combinators are hard to understand if one is not familiar with them. On the other hand, it took me a while to work this code out, when I first read it I couldn't work out why we were setting the variable continuously, then I had to check exactly how the |
I'm yet to work out whats the separation between the various crates in our stack, i.e., different people, different ethos, different coding expectations, different review expectations, different coding styles? I'm defaulting to treating them all the same, is this wrong? |
|
This library is maintained by programming language people (Sanket and me, mainly), so generally you can get away with more obscure CS idioms than you could in rust-bitcoin. (It also, by nature, requires much more CS-heavy code.) Similarly rust-secp is maintained by cryptographers so you can get away with being more crypto-heavy. |
There was a problem hiding this comment.
ACK 28662d2. IMO, these are clean improvements.
I have a very weak preference towards @tcharding's suggestion of hiding the methods away particularly because the names are descriptive.
Reading opt_add, sat_minus_dissat_cost the reader can guess what the functions might do. However, for a generic helper function or something similar. I would side with @apoelstra.
| // costy satisfactions are satisfied, the most costy dissatisfactions are dissatisfied). | ||
| // | ||
| // Args are of form: (<count_sat>, <count_dissat>) | ||
| fn sat_minus_dissat<'r, 's>( |
There was a problem hiding this comment.
I have no strong preferences here.
If the name is descriptive and is being used multiple times in different methods(I think it is the case for some of the functions here). It makes sense to have it at the bottom.
On the other hand, if it is just a helper function, it makes sense to have it a function near the body or even a sub-function inside the main function. The readers can cleanly separate these things.
|
We can continue this discussion but merging this based on Andrew's ACK. |
|
Every now and again I play with defining functions inside of functions but I'm never that happy with the result, because of all the types Rust has on functions its never that clean. Now I write this, perhaps defining name closures would have been nice in this case. I'll have a play with that. |
|
I wouldn't recommend naming closures -- I agree, that quickly seems to go bad in surprising ways (like, call a closure once and it'll seem to work, then years later add a second call and watch the compiler blow up). But if you define a function inside a function with the |
Do a few refactorings in an attempt to make the
thresholdmethod easier to read.