Skip to content

LOC of individual functions #2377

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

Closed
fabric-and-ink opened this issue Jan 18, 2018 · 10 comments
Closed

LOC of individual functions #2377

fabric-and-ink opened this issue Jan 18, 2018 · 10 comments
Labels
E-hard Call for participation: This a hard problem and requires more experience or effort to work on good-first-issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST

Comments

@fabric-and-ink
Copy link

fabric-and-ink commented Jan 18, 2018

It would be great to have a lint for "long functions". This lint would count the number of lines (without comments) for each function and print a warning if it is larger than some value. The warnings may help to improve code readability in addition to the cyclomatic complexity lint.

@oli-obk oli-obk added good-first-issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST labels Jan 19, 2018
@snowsignal
Copy link

By 'lines', do you mean the number of newlines the function body contains (minus comment lines) or do you mean the total amount of statements the function body contains?

@fabric-and-ink
Copy link
Author

fabric-and-ink commented Jan 21, 2018

No, not the total number of statements. Just the number of non-empty lines that are not comments. They are a good simple measure for determining when a function body should be split up into separate functions. If I remember correctly, the maintainers of the linux kernel are not accepting functions that are longer than "two pages". I personally think when you reach more than 50 lines it is time to move individual parts out. Of course one could cheat and make very long lines but this would be a separate issue. In combination with rustfmt this would not work anyway.

Link to related rust project. Famous person's opinion on the topic.

@snowsignal
Copy link

So we should measure the number of newlines inside a function body, not counting them if there is no space between that and the previous newline, or if the start of the line is a comment tag or inside a comment body, and see if that number is less than 50, and if not we inform them that they should refactor the function? Good idea. What do you think should be the message that informs the programmer that their function is too long?

@fabric-and-ink
Copy link
Author

Yes, precisely. Hmm, maybe something like "The function {$fn_name} seems to be rather long. Try to refactor it into smaller functions to improve readability."?

@oli-obk oli-obk added the E-hard Call for participation: This a hard problem and requires more experience or effort to work on label Jan 22, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Jan 22, 2018

Bonus points for suggesting a place where to split the function. Somewhere where as few arguments as possible are needed, but the function is also sufficiently separated (like not suggesting to split off the last statement in a function or something).

Or splitting out subsections of a function. This will probably require some dataflow analysis on MIR.

@fabric-and-ink
Copy link
Author

fabric-and-ink commented Jan 22, 2018

Haha, in that case the E-hard label would really be appropriate. And you probably could write a nice paper about the algorithm. But this isn't necessary. There should be some creative work left for the developer.

@snowsignal
Copy link

I think that the suggestion for where to split the function would be really cool, but that would be rather hard to implement. I think that we should implement the lint function that does a LOF check first, and then maybe make the recommended-split suggestion a separate lint function.

@gentoid
Copy link

gentoid commented Apr 14, 2018

Hello. I'm new to Rust, and I'd like to solve the easy part of this - check LOF

@oli-obk
Copy link
Contributor

oli-obk commented Apr 15, 2018

@gentoid great! Feel free to ask here or just open an unfinished PR if you have any questions.

@gentoid
Copy link

gentoid commented Apr 15, 2018

thanks @oli-obk ! Currently, I'm digging into it 😃 And it looks like the https://llogiq.github.io/2015/06/04/workflows.html post is still useful, despite the fact it points/uses outdated stuff sometimes

bors added a commit that referenced this issue Feb 2, 2019
Adding lint test for excessive LOC.

This is a WIP for #2377. Just wanted to pull in because I had a few questions:

1. Is it okay that I'm approaching this via counting by looking at each line in the snippet instead of looking at the AST tree? If there's another way to do it, I want to make sure I'm doing the correct way, but I wasn't sure since the output AST JSON doesn't seem to contain whitespace.

2. My function is definitely going to trigger the lint, so also wanted to see if there was something obvious I could do to reduce it.

3. Are the two tests fine, or is there something obvious I'm missing?

4. Obviously bigger question - am I approaching the line count correctly. Current strategy is count a line if it contains some code, so skip if it's just comments or empty.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-hard Call for participation: This a hard problem and requires more experience or effort to work on good-first-issue These issues are a good way to get started with Clippy T-AST Type: Requires working with the AST
Projects
None yet
Development

No branches or pull requests

5 participants