Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make lvProducerIndex() and lvConsumerIndex() accessible from outside #57

Closed
ChristianWulf opened this issue Mar 27, 2015 · 11 comments
Closed

Comments

@ChristianWulf
Copy link

I need to monitor the SpScArrayQueue. Unfortunately, I can neither extends from it nor I can access the getter methods mentioned in the subject.

Hence, could you please remove the final modifier of the class and make its getter methods protected?

@ChristianWulf ChristianWulf changed the title Make lvProducerIndex() and lvConsumerIndex() public Make lvProducerIndex() and lvConsumerIndex() accessible from outside Mar 27, 2015
@nitsanw
Copy link
Contributor

nitsanw commented Mar 27, 2015

So... do you want to make the counters visible or make the class not final?
Also, what are you trying to monitor?

@ChristianWulf
Copy link
Author

I want to access the counters. But there are two ways to achieve this:

  1. make the counters protected and the queue non-final
  2. make the counters public

I like the second solution.

I want to monitor the number of pushes and pulls to get the number itself and to compute the throughput for a given time interval.

@ChristianWulf
Copy link
Author

Thank you very much! Just one additional note: please adhere to the Java naming conventions. Rename currentProducerIndex to getCurrentProducerIndex and currentConsumerIndex to getCurrentConsumerIndex.

@mjpt777
Copy link
Contributor

mjpt777 commented Apr 15, 2015

Just because a lot of people name things a certain way in Java does not mean it is right ;-)

I'd recommend Nitsan sticks with non get/set options. I'd recommend everyone watches this great presentation by Kevlin Henney who provides great analysis on the subject. As a dyslexic I find the dogma that goes with bad naming and brackets so annoying and hard to read.

http://www.infoq.com/presentations/7-ineffective-coding-habits

@ChristianWulf
Copy link
Author

Oh, a discussion about clean code. Great! ...Let me comment on your statement with a few questions:

What do you mean by "right"? Let us assume it is right, why should "getXXX" be wrong? If you mean "it is three letters shorter" then I agree. If you conclude "it is-for this reason-faster to understand", then I ask you why? As far as I know, our eyes recognize and interpret words, not letters.

Anyway, let us now consider the following: in general, a method comprises a number of statements and thus does some computation on an object. To tell a human what a method does, we can, e.g., give it a meaningful name. Such a meaningful name must always contain a verb. Otherwise, it does not express what is done, but only "who", "what", "where", "when" etc.

I agree with Henney. Programmers should avoid superfluous text. But why are "get" and "set" superfluous within accessor method names? I claim "get" (or let it be "read" or "return") and "set" (or "write") are absolutely necessary in this context since they express what is done, i.e., they represent the verbs.

If a programming language provides the concept of properties, then programmers should use them in the way Henny tells in this talk. However, if a programming language does not support properties (as, for example, Java), programmers should not try to represent them by means of methods in the same way. And if so, then ask yourself: why did M$ introduce properties at all?

In summary, a method represents an action which can only be described exactly by using a verb.

@mjpt777
Copy link
Contributor

mjpt777 commented Apr 16, 2015

To me properties are things and not directives to the object to do something. They do not mutate state or perform a computation so they do not require a verb. If you take a step further with a fluent API then the "set" prefix makes for very strange reading.

Now in this specific case get means nothing. What is important is the memory ordering semantics of the load from memory. Nitsan indicates this with an "lv" prefix which I believe stands for load volatile. I would personally expand this or make it explicit in another way, such as acquireXXX as used in memory models. Get does not convey this important semantic.

@ChristianWulf
Copy link
Author

Yes. Properties represent a subset of the object's state. I agree. But Java does not provide properties! If you use methods for properties, you cannot distinguish an ordinary method and a property.

Anyway, a property may perform a computation. For example, consider a property that represents the full name of a person. This person object stores its first name and its last name as attributes, but does not own a full name attribute. The full name property can be computed from the first and the last name. That's a legal and reasonable use case for a property.

And concerning Nitsan's code style, I don't mind how he organizes the code internally. But I do mind how the public API looks like. So I talk about currentProducerIndex, not about lvProducerIndex.

@mjpt777
Copy link
Contributor

mjpt777 commented Apr 16, 2015

What does "index" really mean in this case? Index of the array, index as a offset in memory, index over time.

How about the counters are simply renamed to be elementsAdded() and elementsRemoved() to be more explicit?

@nitsanw
Copy link
Contributor

nitsanw commented Apr 16, 2015

ooooh, everyone has been having such fun while I was asleep.
So... naming is a sensitive topic, and public APIs require thought.
I'm not of the get/set religion. I like the clarity of properties personally.
I picked current* because I want the API to express the temporal nature on these indexes. I also left it as *Index because I'm only exposing these on queues which have them and have no plan currently to add such API to all queues. I'll give the elementsAdded/Removed option some thought, but I'm reluctant to add semantics to offer/poll.
I will add some javadoc to also express the fact that they guarantee no concurrency related semantics and the long value returned may be negative. In fact they may not reflect elementsAdded/Removed if we chose to implement sparse elements by changing the index increment.
To directly answer @ChristianWulf original comment: "please adhere to the Java naming conventions"
"No, thank you"

@ChristianWulf
Copy link
Author

Sounds better. But does this method represents a property, or a listener, or does it set a flag, or does it even triggers an event that is called elements[has been]Added? What ever you try, you cannot represent a property by a method. You always need to append the brackets () that indicate a method call and thus an action requiring a verb.

@nitsanw
Copy link
Contributor

nitsanw commented Apr 16, 2015

This is the wrong place for Java code conventions flame fights, please take this up on some relevant mailing list. Fill free to express your interest in enforcing some checkstyle rules in another ticket.

@JCTools JCTools locked and limited conversation to collaborators Apr 16, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants