You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
It seems that there is room for improvement on this implementation in terms of readability. Specifically the parameter epoch_height seems slightly confusing, as we tend to use the term height when applied to other parameters interchangeably with the position of that element. For example, we use "block height" to refer to the same thing that this function calls "block number". As a result, the term epoch_height would tend to similarly indicate the current position of the epoch. However, since the point of this function is to compute the epoch based on the block number / height, a more indicative parameter name that would better capture the intent of this parameter might be blocks_per_epoch.
In terms of the implementation itself, we have 3 separate branches of return path for this single function that seem a little confusing upon first glance, until we realize what's meant to be happening. Essentially we want to return block_number / epoch_height, which seems plane. However, we want to ensure that for block_number 0, we want to return 0, and as a result for every whole number division of the epoch_height, we want to return the previous epoch number.
Realizing this, It's clear that we could actually implement this without the conditional branches:
(block_height + epoch_height - 1) / epoch_height
This achieves the same result without the conditionals, and still passes all of the same tests.
The text was updated successfully, but these errors were encountered:
In inspecting the implementation of
epoch_from_block_number
as it currently exists:HotShot/crates/types/src/utils.rs
Lines 243 to 253 in 46e1c0a
It seems that there is room for improvement on this implementation in terms of readability. Specifically the parameter
epoch_height
seems slightly confusing, as we tend to use the term height when applied to other parameters interchangeably with the position of that element. For example, we use "block height" to refer to the same thing that this function calls "block number". As a result, the termepoch_height
would tend to similarly indicate the current position of the epoch. However, since the point of this function is to compute the epoch based on the block number / height, a more indicative parameter name that would better capture the intent of this parameter might beblocks_per_epoch
.In terms of the implementation itself, we have 3 separate branches of return path for this single function that seem a little confusing upon first glance, until we realize what's meant to be happening. Essentially we want to return
block_number / epoch_height
, which seems plane. However, we want to ensure that forblock_number
0, we want to return0
, and as a result for every whole number division of theepoch_height
, we want to return the previous epoch number.Realizing this, It's clear that we could actually implement this without the conditional branches:
This achieves the same result without the conditionals, and still passes all of the same tests.
The text was updated successfully, but these errors were encountered: