JPPF Issue Tracker
star_faded.png
Please log in to bookmark issues
enhancement_small.png
CLOSED  Enhancement JPPF-346  -  Make JPPFManagementInfo immutable
Posted Oct 30, 2014 - updated Aug 15, 2018
action_vote_minus_faded.png
0
Votes
 action_vote_plus_faded.png
icon_info.png This issue has been closed with status "Closed" and resolution "RESOLVED".
Issue details
  • Type of issue
    Enhancement
  • Status
     
    		 Closed
  • Assigned to
    lolo4j
  • Type of bug
    Not triaged
  • Likelihood
    Not triaged
  • Effect
    Not triaged
  • Posted by
    Daniel Widdis
  • Owned by
    Not owned by anyone
  • Category
    None/Irrelevant
  • Resolution
    RESOLVED
  • Priority
    Low
  • Reproducability
    Always
  • Severity
    Low
  • Targetted for
    icon_milestones.png JPPF 4.2.x
Issue description
I'm debating whether to call this a bug, but the other options didn't seem to fit.

JPPFManagementInfo contains an attribute "active" and a public setter method for it: 
/**
   * Specify whether the node is active or inactive.
   * @param active <code>true</code> if the node is active, <code>false</code> if it is inactve.
   * @exclude
   */
  public void setActive(final boolean active)
  {
    this.active = active;
  }

Because this method is not documented in the API on the website, it's clearly not intended for public consumption. However it is public and is therefore available to IDEs (such as Eclipse) that auto-complete method names on classes, and has internal Javadocs accessible to IDEs, causing potential confusion.
Steps to reproduce this issue
  1. Write code in an IDE such as Eclipse that auto-completes method names.
  2. Create a local variable JPPFManagementInfo node
  3. Write a conditional if (node.isActive()) { ... } intending to toggle the node's state
  4. Observe with pleasant surprise that the IDE helpfully nodes the presence of a node.setActive() method and try to use it
  5. Observe with disappointment that it does not change the node's active state, because a node's active state is actually kept track of in the AbstractNodeContext class. It may, however, incorrectly change the result of a later node.isActive() call.



#2
Comment posted by
Daniel Widdis
Oct 30, 01:39
After posting, I noted the @exclude tag, confirming my suspicion that it's public for internal use but not intended for API usage. Clearly my Eclipse installation doesn't respect that tag.

I might suggest using a method name that is less likely to create confusion, such as setIsActive().
#4
Comment posted by
lolo4j
Oct 30, 06:37
Yes the @exclude tag is used to exclude elements (attributes, methods, classes, packages) from the javadoc only. I know I should make JPPFManagement immutable (and probably a few other classes as well), which probably means changing the constrctuor(s) to take all existing attributes. Depending on how much code needs to be changed, I may transfer this work to milestone 5.0, but it is not a high prirority right now.
#5
Comment posted by
Daniel Widdis
icon_reply.pngOct 30, 07:31, in reply to comment #4
lolo4j wrote:
Depending on how much code needs to be changed, I may transfer this work to
milestone 5.0, but it is not a high prirority right now.


Yes, it's certainly low priority, probably near the bottom of the list. I had a suspicion it wasn't going to work when I stumbled on the method and I was right. The method name is just so enticing that it makes you hope for long enough to run a test and confirm that the docs you read were correct after all.

In other projects I've seen such methods in the javadocs with clear instructions "don't use this". Not sure this needs that much (or any) reworking. My intent was to either make an easy fix (method rename or javadoc note not to use it) or just say "won't fix".
#7
Comment posted by
lolo4j
Nov 06, 22:27
Here's what I intend to do with this:
  • since I'm not very keen on making API changes in a maintenance release, in the 4.2 branch I will remove the @exclude tag and instead add a paragraph at the start of the javadoc for the method, saying "This method is for internal use only. Invoking it simply sets an internal flag and does not affect the active state of the node"
  • in the trunk (for 5.0) I will either change the method name or find a way to not have to use it at all
#8
Comment posted by
lolo4j
Nov 12, 23:53
implemented in