Modify

Opened 7 weeks ago

Closed 3 weeks 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 7 weeks ago.
NTV2GridShiftFile.patch (3.3 KB) - added by GerdP 7 weeks ago.
my proposal for NTV2GridShiftFile
enhanced_for_loops_v2.patch (55.1 KB) - added by gaben 7 weeks ago.
three more readability improvements not related to loops

Download all attachments as: .zip

Change History (18)

Changed 7 weeks ago by gaben

Attachment: enhanced_for_loops.patch added

comment:1 Changed 7 weeks ago by GerdP

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

comment:2 Changed 7 weeks ago by gaben

Yeah, these two where I felt the same.

Changed 7 weeks ago by GerdP

Attachment: NTV2GridShiftFile.patch added

my proposal for NTV2GridShiftFile

Changed 7 weeks ago by gaben

Attachment: enhanced_for_loops_v2.patch added

three more readability improvements not related to loops

comment:3 Changed 7 weeks ago by 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.

Last edited 7 weeks ago by gaben (previous) (diff)

comment:4 in reply to:  3 Changed 7 weeks ago by stoecker

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 7 weeks ago by stoecker (previous) (diff)

comment:5 Changed 7 weeks ago by GerdP

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 Changed 7 weeks ago by gaben

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

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

comment:7 in reply to:  6 Changed 7 weeks ago by GerdP

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 Changed 7 weeks ago by gaben

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 Changed 7 weeks ago by GerdP

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:10 Changed 7 weeks ago by Klumbumbus

comment:11 Changed 7 weeks ago by GerdP

Ah, thanks. Forgot about that

comment:12 Changed 7 weeks ago by Klumbumbus

--> r17381

comment:13 Changed 7 weeks ago by GerdP

see r17384 for ConnectivityRelations

comment:14 Changed 7 weeks ago by GerdP

Milestone: 20.12

comment:15 Changed 3 weeks ago by simon04

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.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.