From 67b7f94ef462a8708c6c0a1cb79fefff65142354 Mon Sep 17 00:00:00 2001 From: param108 Date: Thu, 8 Dec 2022 20:56:46 +0530 Subject: [PATCH] Preferring early loop exits to reduce indentation --- stylecheck/analysis.go | 5 ++ stylecheck/doc.go | 34 +++++++ stylecheck/lint.go | 27 ++++++ stylecheck/lint_test.go | 1 + .../CheckEarlyLoopReturns.go | 89 +++++++++++++++++++ 5 files changed, 156 insertions(+) create mode 100644 stylecheck/testdata/src/CheckEarlyLoopReturns/CheckEarlyLoopReturns.go diff --git a/stylecheck/analysis.go b/stylecheck/analysis.go index d560d972..08ea6e22 100644 --- a/stylecheck/analysis.go +++ b/stylecheck/analysis.go @@ -81,4 +81,9 @@ var Analyzers = lint.InitializeAnalyzers(Docs, map[string]*analysis.Analyzer{ Requires: []*analysis.Analyzer{generated.Analyzer, inspect.Analyzer}, }, "ST1023": sharedcheck.RedundantTypeInDeclarationChecker("should", false), + "ST1024": { + Run: CheckEarlyLoopReturns, + Requires: []*analysis.Analyzer{generated.Analyzer, inspect.Analyzer}, + }, + }) diff --git a/stylecheck/doc.go b/stylecheck/doc.go index 7ac707ee..5c16c5c8 100644 --- a/stylecheck/doc.go +++ b/stylecheck/doc.go @@ -259,4 +259,38 @@ information on how to write good documentation.`, NonDefault: true, MergeIf: lint.MergeIfAll, }, + + "ST1024": { + Title: "Prefer Early loop returns", + Since: "2022.12", + Text: `if a for loop has only one if condition in it and +it is longer than 10 statements then its better to have a simple +if with a break early on instead. + +Instead of + for { + if n > 5 { + ... + ... + ... + (more than 10 lines) + } + } +use + for { + if n <= 5 { + break + } + ... + ... + ... + (more than 10 lines) + } + } + +This reduces the indent which makes it more readable.`, + NonDefault: true, + MergeIf: lint.MergeIfAll, + }, + }) diff --git a/stylecheck/lint.go b/stylecheck/lint.go index e08e5f9b..7e9ec7b7 100644 --- a/stylecheck/lint.go +++ b/stylecheck/lint.go @@ -988,3 +988,30 @@ func CheckExportedVarDocs(pass *analysis.Pass) (interface{}, error) { pass.ResultOf[inspect.Analyzer].(*inspector.Inspector).Nodes([]ast.Node{(*ast.GenDecl)(nil), (*ast.ValueSpec)(nil), (*ast.FuncLit)(nil), (*ast.FuncDecl)(nil)}, fn) return nil, nil } + +func CheckEarlyLoopReturns(pass *analysis.Pass) (interface{}, error) { + fn := func(node ast.Node, push bool) bool { + switch node := node.(type) { + case *ast.ForStmt: + if push { + // If a for statement has only one statement in its body + // and it is an if statement + // and there are more than 10 statements inside the if stmt + // then we could return earlier by reversing the if condition + if len(node.Body.List) == 1 { + switch ifstmt := node.Body.List[0].(type) { + case *ast.IfStmt: + if len(ifstmt.Body.List) > 10 { + report.Report(pass, node, fmt.Sprintf(`for loop with a single if statement in the body is a candidate for early return`), report.FilterGenerated()) + return true + } + } + } + } + } + return true + } + pass.ResultOf[inspect.Analyzer].(*inspector.Inspector).Nodes([]ast.Node{(*ast.ForStmt)(nil)}, fn) + + return nil, nil +} diff --git a/stylecheck/lint_test.go b/stylecheck/lint_test.go index 4d61528e..8f2e44f9 100644 --- a/stylecheck/lint_test.go +++ b/stylecheck/lint_test.go @@ -26,6 +26,7 @@ func TestAll(t *testing.T) { "ST1021": {{Dir: "CheckExportedTypeDocs"}}, "ST1022": {{Dir: "CheckExportedVarDocs"}}, "ST1023": {{Dir: "CheckRedundantTypeInDeclaration"}, {Dir: "CheckRedundantTypeInDeclaration_syscall"}}, + "ST1024": {{Dir: "CheckEarlyLoopReturns"}}, } testutil.Run(t, Analyzers, checks) diff --git a/stylecheck/testdata/src/CheckEarlyLoopReturns/CheckEarlyLoopReturns.go b/stylecheck/testdata/src/CheckEarlyLoopReturns/CheckEarlyLoopReturns.go new file mode 100644 index 00000000..096b49a5 --- /dev/null +++ b/stylecheck/testdata/src/CheckEarlyLoopReturns/CheckEarlyLoopReturns.go @@ -0,0 +1,89 @@ +package pkg + +import ( + "math/rand" + "fmt" +) + +func fn() { + n := rand.Intn(10) + for { //@ diag(`for loop with a single if statement in the body is a candidate for early return`) + if n > 5 { + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + } + } + + // Comments should not affect the warning. + for { //@ diag(`for loop with a single if statement in the body is a candidate for early return`) + // Just an uninteresting comment. + if n > 5 { + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + } + } + + // The if statement has < 10 lines so no warning should be shown. + for { + if n > 5 { + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + } + } + for { + // This if condition should NOT be caught because there is another statement after + // the if condition within the for loop. + if n > 5 { + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + } + fmt.Println("Extra line") + } + for { + fmt.Println("Extra line") + // This if condition should NOT be caught because there is another statement before + // the if condition within the for loop. + if n > 5 { + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + fmt.Println("1") + } + } + +}