hashgraph / hedera-services

Crypto, token, consensus, file, and smart contract services for the Hedera public ledger
Apache License 2.0
292 stars 124 forks source link

FCHashMap code readability and documentation improvement #5370

Open swirlds-automation opened 1 year ago

swirlds-automation commented 1 year ago

I recently spent time reviewing PR: https://github.com/swirlds/swirlds-platform/pull/6447 Without diagrams and documentation it was very hard to read to make sense of.

Anybody who needs to understand that code would have a very hard time and either will have to reach out to the author or as most developers do just be brave and spend a lot of time reading it.

Based on personal experience

  1. A few simple diagrams preferably embedded to the javadoc
  2. Naming variables as close to the problem they solve, though this is always difficult to get it right
  3. Add succinct (tricky) documentation at the right(tricky) places
swirlds-automation commented 1 year ago

This might be a good starting point: https://lucid.app/lucidchart/5b8f3bdd-47d5-481d-8460-7460e74a3a77/edit?viewport_loc=-1335%2C-243%2C4779%2C2583%2C0_0&invitationId=inv_ba5e7456-8519-47c3-a709-6e04e5401cdd author:cody-littley, createdAt:2023-01-18T14:54:04Z, updatedAt=2023-01-18T14:54:04Z

swirlds-automation commented 1 year ago

Having good documentation for this class is important, even if we end up fully replacing all MerkleMaps with VirtualMaps. There are intended use cases for FCHashMap that won't involve MerkleMap. author:cody-littley, createdAt:2023-01-18T14:55:05Z, updatedAt=2023-01-18T14:55:05Z

swirlds-automation commented 1 year ago

migrated from: url=https://github.com/swirlds/swirlds-platform/issues/6551 author:deepak-swirlds, #:6551, createdAt:2023-01-17T16:39:55Z, updatedAt=2023-02-17T22:55:39Z labels=Migration:Data