Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bloated nSLOC numbers due to formatting for readability #21

Open
webthethird opened this issue Nov 22, 2023 · 2 comments
Open

Bloated nSLOC numbers due to formatting for readability #21

webthethird opened this issue Nov 22, 2023 · 2 comments
Labels
help wanted Extra attention is needed

Comments

@webthethird
Copy link

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
Copy link
Author

webthethird commented Nov 22, 2023

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

@tintinweb
Copy link
Member

happy to take a PR 🙌

@tintinweb tintinweb added the help wanted Extra attention is needed label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants