Suggest using lastIndex instead of length - 1 for strings #1221

Arrgentum · 2022-03-02T12:31:47Z

UnsafeUseLastIndex
Added warn and fix rule that changes A.length -1 to A.lastIndex

This pull request closes #1140

Actions checklist

Implemented Rule, added Warnings
Added tests on checks
Added tests on fixers

What's done: - Added new rule to replace .length - 1 to .lastIndex - Added warn tests - Added fix tests (#1140)

### What's done: - Added new rule to replace ".length - 1" to ".lastIndex" - Added warn tests - Added fix tests - Updated Readme (rule 6.2.4) (#1140)

…lastIndex # Conflicts: # diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter6/UnsafeUseLastIndex.kt

### What's done: - Added new rule to replace ".length - 1" to ".lastIndex" - Added warn tests - Added fix tests - Updated Readme (rule 6.2.4) (#1140)

codecov · 2022-03-02T13:07:47Z

Codecov Report

Merging #1221 (a405812) into master (970f316) will increase coverage by 0.02%.
The diff coverage is 92.00%.

@@             Coverage Diff              @@
##             master    #1221      +/-   ##
============================================
+ Coverage     83.84%   83.87%   +0.02%     
- Complexity     2536     2543       +7     
============================================
  Files           103      104       +1     
  Lines          7136     7161      +25     
  Branches       1936     1939       +3     
============================================
+ Hits           5983     6006      +23     
  Misses          352      352              
- Partials        801      803       +2

Flag	Coverage Δ
unittests	`83.87% <92.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...cqfn/diktat/ruleset/rules/chapter6/UseLastIndex.kt	`91.30% <91.30%> (ø)`
...tlin/org/cqfn/diktat/ruleset/constants/Warnings.kt	`98.03% <100.00%> (+0.01%)`	⬆️
...cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt	`96.63% <100.00%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 970f316...a405812. Read the comment docs.

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter6/UnsafeUseLastIndex.kt

diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter6/UnsafeUseLastIndexFixTest.kt

info/guide/diktat-coding-convention.md

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt

petertrr · 2022-03-02T13:25:01Z

info/guide/diktat-coding-convention.md

@@ -2713,6 +2713,16 @@ fun SomeClass.deleteAllSpaces() {
 }
 ```

+#### <a name="r6.2.4"></a> 6.2.4 Don't use property length with operation - 1


You should add description in a specific chapter; the complete guide is then generated from them

You can remove new text from diktat-coding-convention.md completely and only add it into guide-chapter-6.md. After that, from info directory you can run ./gradlew updateMarkdownDocumentation to update guide and other files automatically. We don't do it regularly, so there might be a lot of changes...

info/guide/diktat-coding-convention.md

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter6/UnsafeUseLastIndex.kt

### What's done: - Added new rule to replace ".length - 1" to ".lastIndex" - Added warn tests - Added fix tests - Updated Readme (rule 6.2.4) (#1140)

…lastIndex

### What's done: - Added new rule to replace ".length - 1" to ".lastIndex" - Added warn tests - Added fix tests - Updated Readme (rule 6.2.4) (#1140)

orchestr7 · 2022-03-03T12:38:24Z

info/guide/guide-chapter-6.md

@@ -428,6 +428,22 @@ fun SomeClass.deleteAllSpaces() {
 }
 ```

+#### <a name="r6.2.4"></a> 6.2.4 Don't use property length with operation - 1


"Use lastIndex in case you need to get latest element of a collection"

orchestr7 · 2022-03-03T12:39:13Z

info/guide/guide-chapter-6.md

@@ -428,6 +428,22 @@ fun SomeClass.deleteAllSpaces() {
 }
 ```

+#### <a name="r6.2.4"></a> 6.2.4 Don't use property length with operation - 1
+You should not use property length with operation - 1, you can change this to lastIndex


Instead of using .length - 1 use built-in Kotlin method lastIndex, it makes the code more readable.

kgevorkyan · 2022-03-03T12:41:39Z

info/available-rules.md

Why full file is changed? Some changes in end lines?

Probably \n was replaced with \r\n, yes

Please configure your git to use only LF in commits How to change line-ending settings

orchestr7 · 2022-03-03T12:46:16Z

diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter6/UnsafeUseLastIndexWarnTest.kt

+        lintMethod(
+            """
+                    |val A = "AAAAAAAA"
+                    |val D = A.B.C.length - 1


could you please also add test for .length - 2 and .length, .length + 1

orchestr7 · 2022-03-03T12:47:58Z

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter6/UnsafeUseLastIndex.kt

+        val parent = node.treeParent
+        val textParent = parent.text.replace(node.text, text)
+        val newParent = KotlinParser().createNode(textParent)
+        val grand = parent.treeParent


grand is used only once - no need to create a variable for it

orchestr7 · 2022-03-03T12:53:53Z

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter6/UnsafeUseLastIndex.kt

+        }
+    }
+
+    private fun fixup(node: ASTNode) {


please move this method after it's usage changeRight

orchestr7 · 2022-03-03T13:00:41Z

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter6/UnsafeUseLastIndex.kt

+        }
+    }
+
+    private fun fixup(node: ASTNode) {


please rename to fix

orchestr7 · 2022-03-03T13:01:56Z

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter6/UnsafeUseLastIndex.kt

+class UnsafeUseLastIndex(configRules: List<RulesConfig>) : DiktatRule(
+    "last-index",
+    configRules,
+    listOf(Warnings.UNSAFE_USE_LAST_INDEX)


UNSAFE_USE_LAST_INDEX please rename it to something more accurate: USE_LAST_INDEX

orchestr7 · 2022-03-03T13:03:39Z

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/constants/Warnings.kt

@@ -178,6 +178,7 @@ enum class Warnings(
    INLINE_CLASS_CAN_BE_USED(true, "6.1.12", "inline class can be used"),
    EXTENSION_FUNCTION_WITH_CLASS(false, "6.2.3", "do not use extension functions for the class defined in the same file"),
    RUN_IN_SCRIPT(true, "6.5.1", "wrap blocks of code in top-level scope functions like `run`"),
+    UNSAFE_USE_LAST_INDEX(true, "6.2.4", "length - 1 should be changed to lastIndex"),


Instead of "length - 1" need to use built-in "lastIndex" operation

orchestr7 · 2022-03-03T13:11:34Z

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter6/UnsafeUseLastIndex.kt

+    }
+
+    private fun fixup(node: ASTNode) {
+        val text = node.firstChildNode.text.removeSuffix("length") + "lastIndex"


why do you change child here first? (getting the text)
Why not to replace it directly in parent?

orchestr7 · 2022-03-03T13:12:36Z

diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter6/UnsafeUseLastIndex.kt

+        val text = node.firstChildNode.text.removeSuffix("length") + "lastIndex"
+        val parent = node.treeParent
+        val textParent = parent.text.replace(node.text, text)
+        val newParent = KotlinParser().createNode(textParent)


To create a new node you simply need string with text.
Why do you need to make several changes (first for child, then for parent)?

### What's done: - Added new rule to replace ".length - 1" to ".lastIndex" - Added warn tests - Added fix tests - Updated Readme (rule 6.2.4) (#1140)

petertrr

LGTM

orchestr7

Looks good, but we need to keep in mind #1222

Alexey Votintsev added 6 commits March 1, 2022 17:15

Change .length - 1 to .lastIndex

cddcb5e

What's done: - Added new rule to replace .length - 1 to .lastIndex - Added warn tests - Added fix tests (#1140)

Change .length - 1 to .lastIndex

4fa104b

What's done: - Added new rule to replace .length - 1 to .lastIndex - Added warn tests - Added fix tests (#1140)

Change property length with operation - 1 to property lastIndex

8bbe9ff

### What's done: - Added new rule to replace ".length - 1" to ".lastIndex" - Added warn tests - Added fix tests - Updated Readme (rule 6.2.4) (#1140)

Merge remote-tracking branch 'origin/feature/lastIndex' into feature/…

9cb8ec2

…lastIndex # Conflicts: # diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/chapter6/UnsafeUseLastIndex.kt

Change property length with operation - 1 to property lastIndex

643b693

### What's done: - Added new rule to replace ".length - 1" to ".lastIndex" - Added warn tests - Added fix tests - Updated Readme (rule 6.2.4) (#1140)

Change property length with operation - 1 to property lastIndex

1893720

### What's done: - Added new rule to replace ".length - 1" to ".lastIndex" - Added warn tests - Added fix tests - Updated Readme (rule 6.2.4) (#1140)

Arrgentum requested review from petertrr and orchestr7 March 2, 2022 13:01

Merge branch 'master' into feature/lastIndex

2071764

sanyavertolet reviewed Mar 2, 2022

View reviewed changes

petertrr requested changes Mar 2, 2022

View reviewed changes

Alexey Votintsev added 3 commits March 2, 2022 17:32

Change property length with operation - 1 to property lastIndex

c46c481

### What's done: - Added new rule to replace ".length - 1" to ".lastIndex" - Added warn tests - Added fix tests - Updated Readme (rule 6.2.4) (#1140)

Merge remote-tracking branch 'origin/feature/lastIndex' into feature/…

ddf9542

…lastIndex

Change property length with operation - 1 to property lastIndex

3f4afe6

### What's done: - Added new rule to replace ".length - 1" to ".lastIndex" - Added warn tests - Added fix tests - Updated Readme (rule 6.2.4) (#1140)

orchestr7 reviewed Mar 3, 2022

View reviewed changes

kgevorkyan reviewed Mar 3, 2022

View reviewed changes

orchestr7 reviewed Mar 3, 2022

View reviewed changes

Alexey Votintsev added 3 commits March 3, 2022 21:34

Change property length with operation - 1 to property lastIndex

efe4a0d

### What's done: - Added new rule to replace ".length - 1" to ".lastIndex" - Added warn tests - Added fix tests - Updated Readme (rule 6.2.4) (#1140)

Change property length with operation - 1 to property lastIndex

7ab9210

### What's done: - Added new rule to replace ".length - 1" to ".lastIndex" - Added warn tests - Added fix tests - Updated Readme (rule 6.2.4) (#1140)

Change property length with operation - 1 to property lastIndex

8c4a93e

### What's done: - Added new rule to replace ".length - 1" to ".lastIndex" - Added warn tests - Added fix tests - Updated Readme (rule 6.2.4) (#1140)

Arrgentum requested a review from petertrr March 4, 2022 12:43

Arrgentum requested a review from orchestr7 March 4, 2022 12:43

petertrr approved these changes Mar 4, 2022

View reviewed changes

orchestr7 changed the title ~~Feature/last index~~ Suggest using lastIndex instead of length - 1 for strings Mar 4, 2022

orchestr7 approved these changes Mar 6, 2022

View reviewed changes

Merge branch 'master' into feature/lastIndex

a405812

Arrgentum merged commit aa3c22b into master Mar 11, 2022

Arrgentum deleted the feature/lastIndex branch March 11, 2022 09:53

Arrgentum self-assigned this Jun 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest using lastIndex instead of length - 1 for strings #1221

Suggest using lastIndex instead of length - 1 for strings #1221

Arrgentum commented Mar 2, 2022 •

edited

Loading

codecov bot commented Mar 2, 2022 •

edited

Loading

petertrr Mar 2, 2022

Arrgentum Mar 2, 2022

petertrr Mar 3, 2022

orchestr7 Mar 3, 2022

Arrgentum Mar 4, 2022

orchestr7 Mar 3, 2022

Arrgentum Mar 4, 2022

kgevorkyan Mar 3, 2022

sanyavertolet Mar 3, 2022 •

edited

Loading

kgevorkyan Mar 4, 2022

orchestr7 Mar 3, 2022 •

edited

Loading

Arrgentum Mar 4, 2022

orchestr7 Mar 3, 2022

Arrgentum Mar 4, 2022

orchestr7 Mar 3, 2022

Arrgentum Mar 4, 2022

orchestr7 Mar 3, 2022

Arrgentum Mar 4, 2022

orchestr7 Mar 3, 2022

Arrgentum Mar 4, 2022

orchestr7 Mar 3, 2022

Arrgentum Mar 4, 2022

orchestr7 Mar 3, 2022

orchestr7 Mar 3, 2022

petertrr left a comment

orchestr7 left a comment

Suggest using lastIndex instead of length - 1 for strings #1221

Suggest using lastIndex instead of length - 1 for strings #1221

Conversation

Arrgentum commented Mar 2, 2022 • edited Loading

Actions checklist

codecov bot commented Mar 2, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanyavertolet Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orchestr7 Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertrr left a comment

Choose a reason for hiding this comment

orchestr7 left a comment

Choose a reason for hiding this comment

Arrgentum commented Mar 2, 2022 •

edited

Loading

codecov bot commented Mar 2, 2022 •

edited

Loading

sanyavertolet Mar 3, 2022 •

edited

Loading

orchestr7 Mar 3, 2022 •

edited

Loading