Aiming to improve readability of PArSEC (EVM) transaction flow #239
Conversation
There was a problem hiding this comment.
UT-ACK, I find these changes to be largely helpful for readability, but left some comments to consider. Overall I would say these are welcome updates.
I would also say that some of the value added by changing function names could be achieved by adding doc comments for our private methods like we do for public methods, so maybe we should consider that as well.
| static_cast<int>(it.second), | ||
| "for", | ||
|
|
||
| if(std::holds_alternative<broker::value_type>(res_variant)) { |
There was a problem hiding this comment.
I personally find this easier to read with std::visit, as it makes clear that all possible variant types are covered
| @@ -620,7 +628,7 @@ namespace cbdc::parsec::agent::runner { | |||
| } | |||
| } | |||
|
|
|||
| void evm_runner::handle_lock_from_account( | |||
| void evm_runner::handle_lockfromaccount_and_continue_exec( | |||
There was a problem hiding this comment.
For some of these longer names, it might be easier to just specify in a descriptive comment that this function ex. handles lock and then continues execution. I am of the opinion that we should do this anyway even for private methods but curious what others think
There was a problem hiding this comment.
My bias is towards making the code itself readable and descriptive - with minimal comments if possible. Not sure if this is successful, but the hope is that when the reader looks at the block of code, the attention is drawn to this line which follows the main flow of execution.
| @@ -34,7 +34,9 @@ namespace cbdc::parsec::broker { | |||
| /// \param result_callback function to call with the begin result. | |||
| /// \return true if the request to the ticket machine was initiated | |||
| /// successfully. | |||
| auto begin(begin_callback_type result_callback) -> bool override; | |||
| auto get_new_ticket_number( | |||
| std::function<void(ticketnum_or_errcode_type)> result_callback) | |||
There was a problem hiding this comment.
The nice thing about using an alias like begin_callback_type here is that it's easy to redefine what the type is later. Not sure if there's any reason to pass a function with a different return or argument type besides (void, ticketnum_or_errcode_type) but just something to consider.
That being said, it is much easier to determine what the argument actually is when presented this way.
Signed-off-by: Alexander Jung <[email protected]>
Signed-off-by: Alexander Jung <[email protected]>
The "begin" operations and states of both Agent and Broker are explicitly acquisition of a new ticket number. Rename to clarify this. Signed-off-by: Alexander Jung <[email protected]>
Note that std::visit/overloaded construct is used throughout opencbdc. Why change only this instance? The length of the std::visit block and the fact that the variant is passed as the last argument at the end of that block may place significant cognitive load on the reader. Signed-off-by: Alexander Jung <[email protected]>
…SEC)
agent::impl
do_start() --> do_start_function()
handle_run() —> handle_run_result()
evm_runner
run_execute_transaction() —> start_execute_transaction()
handle_lock_from_account() —> handle_lockfromacct_and_continue_exec()
lock_ticket_number_key() —> lock_ticket_number_key_and_continue_exec()
Signed-off-by: Alexander Jung <[email protected]>
Signed-off-by: Alexander Jung <[email protected]>
AlexRamRam
force-pushed
the
readability
branch
from
cbf8884
to
9b303f3
Compare
Human readability, ease of access and maintainability are subjective to a large degree. An iterative approach of modest-scale changes with potential for discussions and feedback may be the way to go, particularly in a large open source community such as ours.
Here is one iteration of proposed changes from the perspective of following a EVM transaction flow in PArSEC.
Relates to issue #236 (does not solve it)