Consensys / vscode-solidity-metrics

Generate Solidity Source Code Metrics, Complexity and Risk profile reports for your project.
https://marketplace.visualstudio.com/items?itemName=tintinweb.solidity-metrics
46 stars 6 forks source link

Bloated nSLOC numbers due to formatting for readability #21

Open webthethird opened 11 months ago

webthethird commented 11 months ago

Hello,

When I started using this extension (which is mostly great), I reviewed this definition of nSLOC by maurelian. It does seem like a useful metric, and I appreciated the following rationale for flattening multiline function declarations:

We do this because listing arguments on multiple lines actually increases readability, and does not increase complexity.

However, I've noticed other scenarios where the same logic should apply but currently does not, resulting in bloated nSLOC numbers in the report when using a Solidity code formatter. To list just some instances where adding a new line for readability increases nSLOC:

  1. Contract declarations which use a new line between inherited contracts
  2. Calling functions with multiple arguments separated by new lines (the function-specific rationale is only applied to declarations, not calls)
  3. Emitting events and custom errors with multiple arguments separated by new lines
  4. Event and custom error declarations with multiple arguments separated by new lines (even if emitting should be counted as multiple lines, event and error declarations should be a direct extension of the function declaration rule)
  5. Instantiating a new struct with multiple properties separated by new lines
  6. An if condition followed by a single statement without brackets, like a revert with a custom error, when the statement is dropped to the next line and indented
  7. A nested mapping declaration, such as mapping(address => mapping(address => uint256)) public multiplierPerTokenPerUser when the name and visibility are dropped to the next line and indented
  8. Other instances where part of a statement is dropped to the next line to keep from exceeding the line character length limit, often due to longer variable names which also improve readability by making it clear what they mean

Aside from these issues stemming from new lines, I would argue that the closing curly bracket at the end of a code block should not be counted, since there's no logic there, but I suppose that might be debatable. Perhaps when it's the end of an if or for block, it might be more warranted than at the end of a function or contract, which really can't be avoided. But in either case, I could currently cut down on a bunch of nSLOC by simply moving that bracket to the previous line, after the last semicolon, even though that would be a total eyesore. In fact, ultimately that seems to stem from new lines too.

webthethird commented 11 months ago

For what it's worth, this is more of a concern because some auditors have stipulations in their contracts about the maximum nSLOC.

tintinweb commented 5 months ago

happy to take a PR 🙌