Modify

Opened 4 years ago

Closed 3 years ago

#20167 closed enhancement (fixed)

[patch] Improve code readability by replacing indexed loops with foreach

Reported by: gaben Owned by: team
Priority: minor Milestone: 20.12
Component: Core Version:
Keywords: Cc:

Description

There is no code logic change, just readability improvements.

Not all indexed loops replaced, just where it makes the code more compact and readable.

Also found a file with inconsistent formatting (UrlValidatorTest.java), this also got fixed.

You may want to rename the loop variables in a few places if the ones I provided not good enough :)

Attachments (3)

enhanced_for_loops.patch (53.5 KB ) - added by gaben 4 years ago.
NTV2GridShiftFile.patch (3.3 KB ) - added by GerdP 4 years ago.
my proposal for NTV2GridShiftFile
enhanced_for_loops_v2.patch (55.1 KB ) - added by gaben 4 years ago.
three more readability improvements not related to loops

Download all attachments as: .zip

Change History (18)

by gaben, 4 years ago

Attachment: enhanced_for_loops.patch added

comment:1 by GerdP, 4 years ago

OK for most, but I think NTV2GridShiftFile and ConnectivityRelations are not really improved.

comment:2 by gaben, 4 years ago

Yeah, these two where I felt the same.

by GerdP, 4 years ago

Attachment: NTV2GridShiftFile.patch added

my proposal for NTV2GridShiftFile

by gaben, 4 years ago

Attachment: enhanced_for_loops_v2.patch added

three more readability improvements not related to loops

comment:3 by gaben, 4 years ago

@GerdP Thanks, much better :)

If anyone commits this, merge my last and GerdP's patch. I could merge it by myself, but don't know what the patch etiquette is, so left it as is.

Last edited 4 years ago by gaben (previous) (diff)

in reply to:  3 comment:4 by stoecker, 4 years ago

Replying to gaben:

@GerdP Thanks, much better :)

If anyone commits this, merge my last and GerdP's patch. I could merge it by myself, but don't know what the patch etiquette is, so left it as is.

Apply it with a note "patch by xxx" or "modified patch by xxx" depending on the fact if you changed something or not. NOTE: Do not use full email addresses in a comment (if only e-mail known the e.g. use only the name part of the mail).

Last edited 4 years ago by stoecker (previous) (diff)

comment:5 by GerdP, 4 years ago

In 17374/josm:

see #20167: [patch] Improve code readability by replacing indexed loops with foreach
Patch by gaben, slightly modified
I removed the changes for

  • GpxImageCorrelation.java, they introduce a TODO
  • ConnectivityRelations.java (no improvement in readability)

comment:6 by gaben, 4 years ago

@stoecker Thanks, I think wiki:DevelopersGuide/PatchGuide needs an update ;)

@GerdP Don't forget committing your improvements of the NTV2GridShiftFile.

in reply to:  6 comment:7 by GerdP, 4 years ago

Replying to gaben:

@GerdP Don't forget committing your improvements of the NTV2GridShiftFile.

sonar lint doesn't like them and suggests to merge the loops. I have no idea what this structure contains so I didn't want to replace one issue with another one.

comment:8 by gaben, 4 years ago

Oh, I see. And how does Sonar felt about my patch? I tried checking, but the browser just stalled. Maybe I need a local instance.

Btw JOSM's Sonar instance is 1 year old, the latest version is 8.5.1, but 8.0 deployed.

comment:9 by GerdP, 4 years ago

Your patch introduced a checkstyle issue in class UrlValidatorTest (linelength > 145)
Did not see that before committing and I am not sure what to do with that long line

                "http://l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.l.org"));

If the number of dots is relevant it might be better to compute this test string using the number.

comment:11 by GerdP, 4 years ago

Ah, thanks. Forgot about that

comment:12 by Klumbumbus, 4 years ago

--> r17381

comment:13 by GerdP, 4 years ago

see r17384 for ConnectivityRelations

comment:14 by GerdP, 4 years ago

Milestone: 20.12

comment:15 by simon04, 3 years ago

Resolution: fixed
Status: newclosed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.