3
\$\begingroup\$

I need to extract some metrics from ManagedBeans and store the collected metrics in an object bean.

The object bean is defined with member variables to hold the metrics. In this case, is it right to extend the ObjectBean and write the extractor class?

For example:

class ThreadMetricsBean {
    private long liveCount, daemonCount;

    public void record(long liveCount, long daemonCount) {
        this.liveCount = liveCount;
        this.daemonCount = daemonCount;
    }
}

// Here to avoid code redundancy I feel it's correct to extend the  ThreadMetricsBean class
// Am I wrong ?? 

class ThreadMetricsExtractor {
    private long liveCount, daemonCount;
    private ThreadMetricsBean bean = new ThreadMetricsBean();

    public void run() throws Exception{
        extractMetrics();
        recordToBean();
    }

    private void extractMetrics() {
        ThreadMXBean threadMXBean = ManagementFactory.getThreadMXBean();
        this.liveCount = threadMXBean.getThreadCount();
        this.daemonCount = threadMXBean.getDaemonThreadCount();
    }

    private void recordToBean() {
        bean.record(liveCount, daemonCount);
    }
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ @palacsint that was a typo mistake. Now I have corrected. \$\endgroup\$
    – krishna
    Apr 10, 2014 at 4:42

1 Answer 1

1
\$\begingroup\$
  1. I would declare variables on separate lines.

    private long liveCount, daemonCount;
    

    From Code Complete, 2nd Edition, p761:

    Use only one data declaration per line

    [...]

    It’s easier to modify declarations because each declaration is self-contained.

    [...]

    It’s easier to find specific variables because you can scan a single column rather than reading each line. It’s easier to find and fix syntax errors because the line number the compiler gives you has only one declaration on it.

  2. The code has a temporal coupling here:

    public void run() throws Exception{
        extractMetrics();
        recordToBean();
    }
    

    A maintainer easily can change the order of the called methods which brokes it. See: Clean Code by Robert C. Martin, G31: Hidden Temporal Couplings, p302

  3. For this:

    // Here to avoid code redundancy I feel it's correct to extend 
    // the  ThreadMetricsBean class
    // Am I wrong ?? 
    

    It's hard to say that which would be better without knowledge about clients and typical/possible usages of these classes. You might be able to remove redundancy in the following way too:

    class ThreadMetricsExtractor {
        private final ThreadMetricsBean bean = new ThreadMetricsBean();
    
        public void run() throws Exception {
            storeMetrics();
        }
    
        private void storeMetrics() {
            final ThreadMXBean threadMXBean = ManagementFactory.getThreadMXBean();
            final int liveCount = threadMXBean.getThreadCount();
            final int daemonCount = threadMXBean.getDaemonThreadCount();
            bean.record(liveCount, daemonCount);
        }
    }
    

    I've found that composition usually leads better design, cleaner and testable code than inheritance. A reference: Effective Java, Second Edition, Item 16: Favor composition over inheritance.

  4. The throws Exception is unnecessary here, as far I see nothing throws any checked exception inside the run method:

    public void run() throws Exception{
        extractMetrics();
        recordToBean();
    }
    
  5. You could use integers here:

    this.liveCount = threadMXBean.getThreadCount();
    this.daemonCount = threadMXBean.getDaemonThreadCount();
    

    Both called methods returns int, so liveCount and daemonCount could be ints too.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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