7
\$\begingroup\$

I have a class that spawns threads to process data. I am working on instrumentation code. When the process is started, time = System.currentTimeMillis(); When it completes, time = System.currentTimeMillis() - time; I have a method to retrive this time:

/**
 * Get the time taken for process to complete
 * 
 * @return Time this process has run if running; time it took to complete if not
 */
public long getRunTime() {
   return processRunning? System.currentTimeMillis() - time : time;
}

Is this a clear use of the ternary operator? Is my javadoc comment clear?

\$\endgroup\$
  • 3
    \$\begingroup\$ should document your units \$\endgroup\$ – Ron Feb 3 '11 at 3:03
5
\$\begingroup\$

Since the method has a return value even if the code is not finished processing the first line of your javadoc ought not read "Get the time taken for process to complete" but perhaps rather "Get the current or total processing time" or something of that nature? I also agree with Bert F's comment, but if there were clear comments explaining the behavior and you don't intend on exposing start and end times individually then there's no reason to waste an extra variable; you can either keep the code as it is and add comments or optionally get rid of the boolean "processRunning" and instead use your completedTime/runTime variable to establish the fact that it's still running. All minor gripes, all in all.

\$\endgroup\$
6
\$\begingroup\$

I have no problem the ternary operator.

But it is hard to tell that the result is what the comments is saying. Without having a context on what time is (which is a bad variable name) it is hard to understand the result of the function.

time: Bad variable name. Time of what?

\$\endgroup\$
4
\$\begingroup\$

It appears that the method sometimes returns a timestamp (time) and sometimes it returns a duration (difference between timestamps) (assuming that time is always a timestamp - if not, then this issue simply moves to time having a dual definition).

\$\endgroup\$
  • 3
    \$\begingroup\$ What about processRunning? System.currentTimeMillis() - startTime : completedTime - startTime; \$\endgroup\$ – Michael K Feb 2 '11 at 19:26
  • 1
    \$\begingroup\$ Works for me, as does processRunning ? System.currentTimeMillis() - startTime : runTime; \$\endgroup\$ – Bert F Feb 2 '11 at 19:30
2
\$\begingroup\$

It is a matter of style and taste but I'd rather go with

public long getRunTime() {
   if(processRunning)
     return System.currentTimeMillis() - time; 

     return time;
}

I just think this reads easier, and its easier to comment this. Now set me on fire for multiple return statements :D

\$\endgroup\$
  • \$\begingroup\$ Actually, I don't have a problem with this style. I do have a problem with multiple exit points spread throughout the function. \$\endgroup\$ – Michael K Feb 21 '11 at 21:14
  • \$\begingroup\$ Looks a little odd without the else, IMHO. \$\endgroup\$ – Todd Lehman Jan 22 '12 at 8:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.