Opened 20 months ago

Closed 6 months ago

#22674 closed enhancement (wontfix)

Consider changing instance methods to static methods

Reported by: karsten Owned by: metrics-team
Priority: Medium Milestone:
Component: Metrics/Library Version:
Severity: Normal Keywords: metrics-2018
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

There's a patch that we didn't merge yet that makes methods without instance references static. I'm including that patch here in bulk, just in case it gets lost when the branch gets deleted:

From fc04e540b86c7e9d375084c879c02280f8fd60ee Mon Sep 17 00:00:00 2001
From: iwakeh <iwakeh@torproject.org>
Date: Mon, 19 Jun 2017 15:08:45 +0000
Subject: Make methods without instance references 'static'.


diff --git a/src/main/java/org/torproject/descriptor/impl/DescriptorParserImpl.java b/src/main/java/org/torproject/descriptor/impl/DescriptorParserImpl.java
index a0be85c..ec04901 100644
--- a/src/main/java/org/torproject/descriptor/impl/DescriptorParserImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/DescriptorParserImpl.java
@@ -69,7 +69,7 @@ public class DescriptorParserImpl implements DescriptorParser {
         NL + Key.NETWORK_STATUS_VERSION.keyword + SP + "3"))
         && firstLines.contains(
         NL + Key.VOTE_STATUS.keyword + SP + "consensus" + NL))) {
-      return this.parseDescriptors(rawDescriptorBytes, descriptorFile,
+      return parseDescriptors(rawDescriptorBytes, descriptorFile,
           Key.NETWORK_STATUS_VERSION, RelayNetworkStatusConsensusImpl.class,
           this.failUnrecognizedDescriptorLines, includeUnparseableDescriptors);
     } else if (firstLines.startsWith("@type network-status-vote-3 1.")
@@ -79,7 +79,7 @@ public class DescriptorParserImpl implements DescriptorParser {
         NL + Key.NETWORK_STATUS_VERSION.keyword + SP + "3" + NL))
         && firstLines.contains(
         NL + Key.VOTE_STATUS.keyword + SP + "vote" + NL))) {
-      return this.parseDescriptors(rawDescriptorBytes, descriptorFile,
+      return parseDescriptors(rawDescriptorBytes, descriptorFile,
           Key.NETWORK_STATUS_VERSION, RelayNetworkStatusVoteImpl.class,
           this.failUnrecognizedDescriptorLines, includeUnparseableDescriptors);
     } else if (firstLines.startsWith("@type bridge-network-status 1.")
@@ -90,42 +90,42 @@ public class DescriptorParserImpl implements DescriptorParser {
           descriptorFile, fileName, this.failUnrecognizedDescriptorLines));
       return parsedDescriptors;
     } else if (firstLines.startsWith("@type bridge-server-descriptor 1.")) {
-      return this.parseDescriptors(rawDescriptorBytes, descriptorFile,
+      return parseDescriptors(rawDescriptorBytes, descriptorFile,
           Key.ROUTER, BridgeServerDescriptorImpl.class,
           this.failUnrecognizedDescriptorLines, includeUnparseableDescriptors);
     } else if (firstLines.startsWith("@type server-descriptor 1.")
         || firstLines.startsWith(Key.ROUTER.keyword + SP)
         || firstLines.contains(NL + Key.ROUTER.keyword + SP)) {
-      return this.parseDescriptors(rawDescriptorBytes, descriptorFile,
+      return parseDescriptors(rawDescriptorBytes, descriptorFile,
           Key.ROUTER, RelayServerDescriptorImpl.class,
           this.failUnrecognizedDescriptorLines, includeUnparseableDescriptors);
     } else if (firstLines.startsWith("@type bridge-extra-info 1.")) {
-      return this.parseDescriptors(rawDescriptorBytes, descriptorFile,
+      return parseDescriptors(rawDescriptorBytes, descriptorFile,
           Key.EXTRA_INFO, BridgeExtraInfoDescriptorImpl.class,
           this.failUnrecognizedDescriptorLines, includeUnparseableDescriptors);
     } else if (firstLines.startsWith("@type extra-info 1.")
         || firstLines.startsWith(Key.EXTRA_INFO.keyword + SP)
         || firstLines.contains(NL + Key.EXTRA_INFO.keyword + SP)) {
-      return this.parseDescriptors(rawDescriptorBytes, descriptorFile,
+      return parseDescriptors(rawDescriptorBytes, descriptorFile,
           Key.EXTRA_INFO, RelayExtraInfoDescriptorImpl.class,
           this.failUnrecognizedDescriptorLines, includeUnparseableDescriptors);
     } else if (firstLines.startsWith("@type microdescriptor 1.")
         || firstLines.startsWith(Key.ONION_KEY.keyword + NL)
         || firstLines.contains(NL + Key.ONION_KEY.keyword + NL)) {
-      return this.parseDescriptors(rawDescriptorBytes, descriptorFile,
+      return parseDescriptors(rawDescriptorBytes, descriptorFile,
           Key.ONION_KEY, MicrodescriptorImpl.class,
           this.failUnrecognizedDescriptorLines, includeUnparseableDescriptors);
     } else if (firstLines.startsWith("@type bridge-pool-assignment 1.")
         || firstLines.startsWith(Key.BRIDGE_POOL_ASSIGNMENT.keyword + SP)
         || firstLines.contains(NL + Key.BRIDGE_POOL_ASSIGNMENT.keyword + SP)) {
-      return this.parseDescriptors(rawDescriptorBytes, descriptorFile,
+      return parseDescriptors(rawDescriptorBytes, descriptorFile,
           Key.BRIDGE_POOL_ASSIGNMENT, BridgePoolAssignmentImpl.class,
           this.failUnrecognizedDescriptorLines, includeUnparseableDescriptors);
     } else if (firstLines.startsWith("@type dir-key-certificate-3 1.")
         || firstLines.startsWith(Key.DIR_KEY_CERTIFICATE_VERSION.keyword + SP)
         || firstLines.contains(
         NL + Key.DIR_KEY_CERTIFICATE_VERSION.keyword + SP)) {
-      return this.parseDescriptors(rawDescriptorBytes, descriptorFile,
+      return parseDescriptors(rawDescriptorBytes, descriptorFile,
           Key.DIR_KEY_CERTIFICATE_VERSION, DirectoryKeyCertificateImpl.class,
           this.failUnrecognizedDescriptorLines, includeUnparseableDescriptors);
     } else if (firstLines.startsWith("@type tordnsel 1.")
@@ -140,13 +140,13 @@ public class DescriptorParserImpl implements DescriptorParser {
         Key.NETWORK_STATUS_VERSION.keyword + SP + "2" + NL)
         || firstLines.contains(
         NL + Key.NETWORK_STATUS_VERSION.keyword + SP + "2" + NL)) {
-      return this.parseDescriptors(rawDescriptorBytes, descriptorFile,
+      return parseDescriptors(rawDescriptorBytes, descriptorFile,
           Key.NETWORK_STATUS_VERSION, RelayNetworkStatusImpl.class,
           this.failUnrecognizedDescriptorLines, includeUnparseableDescriptors);
     } else if (firstLines.startsWith("@type directory 1.")
         || firstLines.startsWith(Key.SIGNED_DIRECTORY.keyword + NL)
         || firstLines.contains(NL + Key.SIGNED_DIRECTORY.keyword + NL)) {
-      return this.parseDescriptors(rawDescriptorBytes, descriptorFile,
+      return parseDescriptors(rawDescriptorBytes, descriptorFile,
           Key.SIGNED_DIRECTORY, RelayDirectoryImpl.class,
           this.failUnrecognizedDescriptorLines, includeUnparseableDescriptors);
     } else if (firstLines.startsWith("@type torperf 1.")) {
@@ -158,7 +158,7 @@ public class DescriptorParserImpl implements DescriptorParser {
     }
   }
 
-  private List<Descriptor> parseDescriptors(byte[] rawDescriptorBytes,
+  private static List<Descriptor> parseDescriptors(byte[] rawDescriptorBytes,
       File descriptorFile, Key key,
       Class<? extends DescriptorImpl> descriptorClass,
       boolean failUnrecognizedDescriptorLines,
@@ -212,7 +212,7 @@ public class DescriptorParserImpl implements DescriptorParser {
       int[] offsetAndLength = new int[] { startAnnotations,
           endDescriptor - startAnnotations };
       try {
-        parsedDescriptors.add(this.parseDescriptor(rawDescriptorBytes,
+        parsedDescriptors.add(parseDescriptor(rawDescriptorBytes,
             offsetAndLength, descriptorFile, constructor,
             failUnrecognizedDescriptorLines));
       } catch (DescriptorParseException e) {
@@ -229,7 +229,7 @@ public class DescriptorParserImpl implements DescriptorParser {
     return parsedDescriptors;
   }
 
-  Descriptor parseDescriptor(byte[] rawDescriptorBytes,
+  static Descriptor parseDescriptor(byte[] rawDescriptorBytes,
       int[] offsetAndLength, File descriptorFile,
       Constructor<? extends DescriptorImpl> constructor,
       boolean failUnrecognizedDescriptorLines) throws DescriptorParseException {
diff --git a/src/test/java/org/torproject/descriptor/impl/DescriptorParserImplTest.java b/src/test/java/org/torproject/descriptor/impl/DescriptorParserImplTest.java
index 558a395..be86313 100644
--- a/src/test/java/org/torproject/descriptor/impl/DescriptorParserImplTest.java
+++ b/src/test/java/org/torproject/descriptor/impl/DescriptorParserImplTest.java
@@ -55,8 +55,7 @@ public class DescriptorParserImplTest {
     this.thrown.expect(DescriptorParseException.class);
     this.thrown.expectMessage("'176x.158.53.63' in line 'router UbuntuCore169 "
         + "176x.158.53.63 44583 0 0' is not a valid IPv4 address.");
-    DescriptorParserImpl dpi = new DescriptorParserImpl();
-    dpi.parseDescriptor(DEFECT.getBytes(),
+    DescriptorParserImpl.parseDescriptor(DEFECT.getBytes(),
         new int[]{0, DEFECT.getBytes().length}, null, constructor, false);
   }
 
-- 
cgit v0.10.2

I'm not yet sure whether that commit makes things better or worse. Let's discuss that here, not limited to this specific case.

Child Tickets

Change History (6)

comment:1 Changed 20 months ago by iwakeh

I think the entire structure needs some improvement.
This small change is useful to mark methods that don't rely on instance variables/methods as 'static'.
Anyway, parseDescriptors used to be static before and that was just changed (I missed the reason).

comment:2 in reply to:  1 Changed 20 months ago by karsten

Replying to iwakeh:

I think the entire structure needs some improvement.

Agreed.

This small change is useful to mark methods that don't rely on instance variables/methods as 'static'.
Anyway, parseDescriptors used to be static before and that was just changed (I missed the reason).

Ah, heh, mainly because I thought it was an oversight that it was static before. Happy to reconsider (without the time constraint of wanting/needing to put out two releases within 10 days)!

comment:3 Changed 17 months ago by karsten

Keywords: metrics-2018 added

comment:4 Changed 17 months ago by karsten

Keywords: metrics-2017 added; metrics-2018 removed

comment:5 Changed 14 months ago by iwakeh

Keywords: metrics-2018 added; metrics-2017 removed

Will be completed in 2018.

comment:6 Changed 6 months ago by karsten

Resolution: wontfix
Status: newclosed

So, I looked a bit closer at this ticket in the context of #27234. I do agree that the structure needs improvement. But that applies to many other places in the code. We don't need this particular ticket to remind us of that. Closing.

Note: See TracTickets for help on using tickets.