Opened 4 years ago

Closed 4 years ago

#17824 closed enhancement (fixed)

switch on string instead of many if-else with String comparison

Reported by: iwakeh Owned by: karsten
Priority: Low Milestone:
Component: Metrics/Library Version:
Severity: Minor Keywords:
Cc: Actual Points:
Parent ID: #17822 Points:
Reviewer: Sponsor:

Description

example from BridgeNetworkStatusImpl (there are many more and longer ones):

  switch(keyword){
      case "published":
        this.parsePublishedLine(line, parts);
      case "flag-thresholds":
        this.parseFlagThresholdsLine(line, parts);
   } 
   if (this.failUnrecognizedDescriptorLines) {
      throw new DescriptorParseException("Unrecognized line '" + line
            + "' in bridge network status.");
   } else {
      if (this.unrecognizedLines == null) {
         this.unrecognizedLines = new ArrayList<String>();
     }
...

instead of

  if (keyword.equals("published")) {
        this.parsePublishedLine(line, parts);
      } else if (keyword.equals("flag-thresholds")) {
        this.parseFlagThresholdsLine(line, parts);
      } else if (this.failUnrecognizedDescriptorLines) {
        throw new DescriptorParseException("Unrecognized line '" + line
            + "' in bridge network status.");
      } else {
        if (this.unrecognizedLines == null) {
          this.unrecognizedLines = new ArrayList<String>();
        }

Child Tickets

Attachments (1)

0001-task-17824-addendum.patch (38.2 KB) - added by iwakeh 4 years ago.
more

Download all attachments as: .zip

Change History (9)

comment:1 Changed 4 years ago by karsten

Status: newaccepted

Sure, sounds like a good change.

I wonder how we would handle chained if-else statements with some if's checking for equals() and others checking for startsWith() or using other methods. But I don't know if there are such cases in the current code. Just a thought.

I guess the more important question is the same as for #17823: how can we best avoid making this change while we both have unmerged branches lying around? Shortly before the next release?

comment:2 Changed 4 years ago by iwakeh

{{{startsWith()}} if-else-chains should also be changed to switch. The startsWith() for parsing should not be necessary.

In DescriptorImpl it could be achieved by splitting the first line and use the String after @type (if there is any) for the switch-stmt.
I also would suggest moving the version handling to the parser classes.
Some (like TorPerfResultImpl) verify the version anyway and could if need arises handle several versions in future. Thus there is no need for DescriptorImpl to check the versions.

comment:3 Changed 4 years ago by karsten

Agreed. (The above was mostly a note to myself.) Let's do it.

comment:4 Changed 4 years ago by karsten

Status: acceptedneeds_review

comment:5 Changed 4 years ago by iwakeh

Thanks for making all these changes!

I found some more places where I find the switch-statement more readable and attached a patch based on your public repo.

Please review and if there is a problem it would be great to add a test that the patch fails.

To change DescriptorImpl could cause a bigger rewrite and it is better not to include it now.
But it might be a good idea to keep an issue around for cleaning it up next year?

Changed 4 years ago by iwakeh

more

comment:6 Changed 4 years ago by karsten

Looks good! Pushed your patch to my branch, made a few whitespaces fixes, and added a change log entry for the three tickets. I'll run some "integration" tests now by running Onionoo/Metrics instances with this version and will release this as 1.1.0 probably tomorrow. Thanks!

comment:7 Changed 4 years ago by karsten

I just pushed another commit to that branch which adds getHostname() methods to DirSourceEntry and RelayNetworkStatusVote. It's a trivial change and probably doesn't require much review, but I thought I'd mention it here before merging it together with the other changes. Let me know if it breaks something I didn't think of.

comment:8 Changed 4 years ago by karsten

Resolution: fixed
Status: needs_reviewclosed

I created #17934 for the getHostname() part and merged the other changes into master. Resolving. Thanks!

Note: See TracTickets for help on using tickets.