Opened 5 months ago

Last modified 5 months ago

#28114 new enhancement

Improve getter names for boolean fields in metrics-lib

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

Description

This ticket is created from #24419. #24419 notes that not all Onionoo methods could be changed to the isXY() pattern for boolean getter methods due to interfaces such as org.torproject.descriptor.NetworkStatusEntry. This ticket is to update metrics-libs accordingly. From #24419:

  1. I did not change src/test/org/torproject/metrics/onionoo/updater/DummyStatusEntry's "getUnmeasured()" to "isUnmeasured()" because the interface org.torproject.descriptor.NetworkStatusEntry requires it, and I figured org.torproject.descriptor is outside the scope of this ticket.

Agreed. We'd first have to change metrics-lib interfaces, put out a new major metrics-lib release, and then update the tests in Onionoo. Worth doing, though we would ideally combine this with other changes that require a major metrics-lib version bump.

Child Tickets

Change History (7)

comment:1 Changed 5 months ago by efgyirfe784

Parent ID: #24419

comment:2 Changed 5 months ago by karsten

Parent ID: #24419

Removing the parent ID, or otherwise I cannot close #24419 which is now merged.

comment:3 Changed 5 months ago by irl

I do prefer the isXY() format for boolean getters, but I wonder what to do about these in the ServerDescriptor interface. Some suggestions:

public boolean getUsesEnhancedDnsLogic() -> isUsingEnhancedDnsLogic()
public boolean getCachesExtraInfo() -> isExtraInfoCache()
public boolean getAllowSingleHopExits() -> isSingleHopExit()
public boolean getTunnelledDirServer() -> isTunnelledDirServer()

A side note: there is a typo in the JavaDoc for getTunnelledDirServer() that could be fixed at the same time.

comment:4 Changed 5 months ago by efgyirfe784

Sounds good. What about

public boolean getAllowSingleHopExits() -> isSingleHopExitAllowed()

comment:5 Changed 5 months ago by karsten

One thing to keep in mind here is that current metrics-lib getter names follow the naming of Tor descriptor keywords. If we rename methods returning booleans to more meaningful English, it'll become a slightly bit harder for folks looking at dir-spec.txt to find the corresponding metrics-lib method. Not sure what to do about this.

comment:6 Changed 5 months ago by karsten

Oh, another thought from a couple months back: we could simply drop the "get" from all getters. There are no setters anyway. Just an idea that we could discuss here before changing any code.

comment:7 Changed 5 months ago by efgyirfe784

Good point about the keywords. Where can I find dir-spec.txt?

I do like the flow of dropping the getters as the client code sounds natural:

   if (o.usesEnhancedDnsLogic()) {}
   if (o.cachesExtraInfo()) {}
   if (o.allowSingleHopExits()) {}
   if (o.tunnelledDirServer()) {}

This could become confusing if setters are ever created in the future, but I'm guessing that's not too likely.

Keeping a simple rule like "prefix any keyword with 'get' and that's the method name to retrieve the keyword value" is probably more important than using more meaningful English-sounding method names or even more natural sounding client code, since it makes library users' lives easier. If determining the method to retrieve a keyword now depends on the type being returned, things become more complicated for library users.

It looks like some Descriptors already use an "is" prefix for boolean return types though:

   RelayNetworkStatusVote.isSharedRandParticipate()
   ServerDescriptor.isHibernating()
   ServerDescriptor.isHiddenServiceDir()

(some non-Descriptors also use "is" prefix for boolean return types)

Note: See TracTickets for help on using tickets.