Monitoring params via JMX

Question

A task for hiring on a new job (I failed it with reason "very bad code").

Write a program, which will be monitoring the params via JMX. Param should have name, type and observabale value.

Why my code is bad? And how write this task more eleganty and clear?

---

main.java Starts my app

public class Main {
 public static String pluginFolder;
 public static Integer port = 9999;
 public static void main( String args[] )
 {
 //directory with plugins
 if (args.length>0) {
 pluginFolder = args[0];
 } else {
 System.out.println("Please specify folder with plugins! Type path to plugins after space");
 return;
 }
 //port number
 if (args.length>1) {
 try {
 Integer.parseInt(args[1]);
 } catch (NumberFormatException nfe) {
 Utils.showMessage("Wrong port. Using default port: " + port);
 }
 }
 MBeanContainer.startMBeanServer();
 }
}

MBeanContainer.java

There is MBeanServer, where controls observable params. We can start, stop MBeanServer and pass or get params from it.

public class MBeanContainer
{
 static MBeanServer mBeanServer;
 private static HashMap<ParamInterface, BaseParam> mbeansList = new HashMap<ParamInterface, BaseParam>();
 /**
 * Start Mbean server
 */
 public static void startMBeanServer() {
 mBeanServer = ManagementFactory.getPlatformMBeanServer();
 try {
 LocateRegistry.createRegistry(Main.port);
 JMXServiceURL url = new JMXServiceURL("service:jmx:rmi:///jndi/rmi://localhost:"+Main.port+"/server");
 JMXConnectorServer cs = JMXConnectorServerFactory.newJMXConnectorServer(url, null, mBeanServer);
 cs.start();
 } catch (ExportException ee) { //hide trace exception and continue work when mbean server is already run.
 if (ee.getCause().getClass() == BindException.class)
 Utils.showMessage("Server is already started. \n " + ee.getCause().getMessage() + " \n continue...");
 } catch (Exception e) {
 Utils.processException(e);
 }
 }
 /**
 * Register mbean in server
 * @param baseParam
 */
 public static void registerMbean(ParamInterface baseParam) {
 BaseParam mbeanObj = new BaseParam(baseParam);
 try {
 ObjectName mbeanName = new ObjectName("JMXParam:name="+baseParam.getType());
 mBeanServer.registerMBean(mbeanObj, mbeanName);
 mBeanServer.addNotificationListener(mbeanName, new ParamListener(), null, null);
 mbeansList.put(baseParam, mbeanObj);
 } catch (Exception e) {
 Utils.processException(e);
 }
 }
 /**
 * Return Mbean object by type of plugin
 * @param type
 * @return
 */
 public static BaseParam getParamMBeanByType(String type){
 for(Map.Entry<ParamInterface, BaseParam> entry : mbeansList.entrySet()) {
 if (entry.getKey().getType().equals(type)) {
 return entry.getValue();
 }
 }
 Utils.log("getParamMBeanByType return null for param type: " + type);
 return null; //never unreached
 }
 public static void stopMonitoring(){
 for(BaseParam mbean: mbeansList.values()){
 mbean.stop();
 }
 }
 public static void startMonitoring(){
 for(BaseParam mbean: mbeansList.values()){
 mbean.start();
 }
 }
}

ParamInterface.java Interface which all params should be implement

public interface ParamInterface {
 public int getInterval();
 public String getName();
 public String getType();
 public String getValue();
}

BaseParamMBean

Interface of working with param.

public interface BaseParamMBean
{
 public void start();
 public void stop();
 public void setInterval( long time );
}

BaseParam.java Observable param. Superclass for params. Can generate notifications and send them to server. Take an instance of ParamInterface in constructor as specialazed version of the param. Can stop, start, and change interval of sending events.

 public class BaseParam extends NotificationBroadcasterSupport implements BaseParamMBean, Runnable
 {
 private boolean stop = true;
 private int index = 0;
 private long interval = 1000;
 private String paramName;
 private String paramType;
 private String oldValue;
 private ParamInterface o;
 public BaseParam(ParamInterface o)
 {
 this.paramName = o.getName();
 this.paramType = o.getType();
 this.interval = o.getInterval();
 this.o = o;
 this.start();
 }
 public void setInterval( long interval )
 {
 this.interval = interval;
 }
 public void start()
 {
 try
 {
 stop = false;
 Thread t = new Thread( this );
 t.start();
 }
 catch( Exception e )
 {
 Utils.processException(e);
 }
 }
 public void stop()
 {
 stop = true;
 }
 public void run()
 {
 while( !stop )
 {
 try
 {
 Thread.sleep(interval);
 }
 catch( Exception e )
 {
 Utils.processException(e);
 }
 String newValue = o.getValue();
 AttributeChangeNotification notif = new AttributeChangeNotification(
 this, //object
 index, //sequenceNumber
 System.currentTimeMillis(), //cur time
 "Attribute Change", //msg
 this.paramName, //attributeName
 this.paramType, //attributeType
 oldValue, //old value
 newValue //new value
 );
 sendNotification(notif);
 oldValue = newValue;
 }//while
 }
 public MBeanNotificationInfo[] getNotificationInfo()
 {
 String[] attChanges = { AttributeChangeNotification.ATTRIBUTE_CHANGE };
 MBeanNotificationInfo[] info = new MBeanNotificationInfo[1];
 info[ 0 ] = new MBeanNotificationInfo(attChanges, "javax.management.AttributeChangeNotification", this.paramName);
 return info;
 }
}

ParamListener

Listener shows event from params to console.

public class ParamListener implements NotificationListener {
 public void handleNotification( Notification not, Object obj )
 {
 AttributeChangeNotification notif = (AttributeChangeNotification) not;
 System.out.println(notif.getAttributeName()+": "+notif.getNewValue());
 CustomTable.updateCell(notif.getAttributeType(), notif.getNewValue().toString());
 }
}

Utils

public class Utils {
 public static void processException(Exception e){
 e.printStackTrace();
 }
 public static void showMessage(String s) {
 System.out.println(s);
 }
 public static void log(String s) {
 System.out.println("!" + s);
 }
}

CpuParam

Implementation of concrete param

public class CpuParam implements ParamInterface {
 private int maximum = 100;
 private int minimum = 0;
 private final String name = "CPU load";
 private final String type = "cpu.attribute.check";
 private int interval = 5000;
 @Override
 public int getInterval() {
 return interval;
 }
 @Override
 public String getName() {
 return name;
 }
 @Override
 public String getType() {
 return type;
 }
 @Override
 public String getValue() {
 Random rn = new Random();
 int range = maximum - minimum + 1;
 int randomNum = rn.nextInt(range) + minimum;
 return String.valueOf(randomNum);
 }
} 

Answer 1

Stylistic

• (Always assuming this is not garbled by the pasting) there is inconsistent indentation in some places, parameter lists sometimes have spaces, sometimes they don't and the names aren't that great (e.g. not for notification, o, cs are bad).
• The comments aren't very useful, except for the one in MBeanContainer, //hide trace..., because it actually clarifies the intent. I'd also either remove incomplete docstrings, or fill them in. IMO if they don't add to the understanding of the code, they just add clutter, but you could argue differently if you actually used the docstrings in the IDE or docs generation.
• Catching Exception at every turn suggests that the error handling isn't well structured. If you can't deal with the error, let it propagate and don't just print a stack trace (or at least re-throw so the caller can deal with it).
• In MBeanContainer the type check for BindException should probably be changed to use instanceof. Also, what's with the alternative case?
• Better use the base interface instead of specific classes when you write out the types, e.g. Map<ParamInterface, BaseParam>. Again, I'm pretty sure you don't rely on the hashing part. With one of the latest changes in Java you can also omit a lot of the generic arguments.
• If you use final once, either do it always, or not at all. Same for @Override.
• BaseParam copies the values from ParamInterface and stores the object itself. Again, choose one or the other.

Structural

• getInterval returns an int, setInterval accepts long. The parameter is named time in the interface, interval in the implementation. Renaming to milliseconds would be good, showing off JodaTime or a cron library would be great for an interview exercise I guess, although for millisecond resolution it's overkill.
• MBeanContainer is all global and static. Changing that to regular methods and maybe providing a singleton will make this way more reusable.
• BaseParam::run does too much, I'd split the notification creation into a method so the control flow is more obvious.

Okay, maybe others find more. Providing tests would also be something nice to show off.

Even though this is an interview exercise, I think at least these points would need to be addressed before considering using it in production and later maintaining it.

Thread safety is probably okay, but is something to look out for. It would be nice if stopMonitoring actually waited for threads to shutdown.

For the design itself, well, without a bit more restrictions it's hard to see what should be changed; apart from what I wrote it looks okay to me. JMX probably isn't the best thing to exercise anyway.

Answer 2

First impressions can make a huge impact when assessing someone's code. Let's look at your Main class:

public class Main {
 public static String pluginFolder;
 public static Integer port = 9999;
 public static void main( String args[] )
 {
 //directory with plugins
 if (args.length>0) {
 pluginFolder = args[0];
 } else {
 System.out.println("Please specify folder with plugins! Type path to plugins after space");
 return;
 }
 //port number
 if (args.length>1) {
 try {
 Integer.parseInt(args[1]);
 } catch (NumberFormatException nfe) {
 Utils.showMessage("Wrong port. Using default port: " + port);
 }
 }
 MBeanContainer.startMBeanServer();
 }
}

The first thing I notice is that you have two static fields, and that they are public, and not final.

Static fields should be uncommon, and, when they exist they should be final. If they are not final, they should at least be private....

In your check to find the pluginFolder, that's almost OK. The return from the main method is not what I would expect, though. If there's a problem, I would expect a non-zero exit code.. System.exit(1), or perhaps an exception.

The Port number check is more concerning:

 if (args.length>1) {
 try {
 Integer.parseInt(args[1]);
 } catch (NumberFormatException nfe) {
 Utils.showMessage("Wrong port. Using default port: " + port);
 }
 }

You use a Utils class here for this message, but not if there's no args[0]?

Additionally, if the args[1] here successfully parses as an int, you don not actually save the value to port... you just throw it away. So, even though you say you support changing the port number, you never actually change the port number.

Finally, what should you have done in the main method?

At minimum, I would have preferred to see the code:

private static final int DEFAULT_PORT = 9999;
public static void main (String args[]) {
 if (args.length == 0) {
 Utils.showMessage("Usage: prog plugin_folder {port}");
 System.exit(1);
 }
 int port = DEFAULT_PORT;
 if (args.length > 1) {
 try {
 port = Integer.parseInt(args[1]);
 } catch (NumberFormatException nfe) {
 Utils.showMessage("Wrong port. Using default port: " + port);
 }
 }
 MBeanContainer.startMBeanServer(args[0], port);
}

Then, your startMBeanServer(...) method does not need to access the public static fields on the Main class.

Answer 3

I suppose this was a homework assignment. I do this too when looking for talent. Instead of the extreme pressure of writing code on the spot at the interview, I give the opportunity to do your best in your private time, in your most comfortable environment. And I expect nothing less than a piece of beauty in return.

There are a couple things that you can do almost effortlessly that will make a big difference:

• Apply standard formatting using the built in functions of your IDE. For example Control-Shift-f in Eclipse, Control-Alt-l in IntelliJ. Apply to all your files.
• Eliminate all warnings your IDE gives you.
• Eliminate even more issues: install the FindBugs plugin in Eclipse. In IntelliJ (I'm talking about the free community edition here) the default warnings are already pretty strict.
• Add a couple of unit tests. And not just any unit tests, make them good.

Except for the unit tests, the above items are actually pretty mechanical: any monkey can do them. Yet, you haven't. This seriously crippled your chances from the start.

Here are a couple more things on top of the other excellent reviews by @ferada and @rolfl. But you should also follow the above suggestions, as they will uncover even more.

---

private static HashMap<ParamInterface, BaseParam> mbeansList = new HashMap<ParamInterface, BaseParam>();

Use interface types in variable and method parameter declarations whenever possible (most of the time). And, when submitting for a job application, choose a current Java version. Today, that should be at least Java 7, in which case your IDE would tell you to use the diamond operator when creating instances.

private static Map<ParamInterface, BaseParam> mbeansList = new HashMap<>();

---

Reduce the number of connections between your classes. For example, Main depends on MBeanContainer, but MBeanContainer also depends on Main (for the port setting). In this example Main should have passed all the necessary parameters to MBeanContainer. This is a particularly bad example, because a class like Main is effectively just a throw-away launcher: its sole purpose is to start up the "machinery" of a program / framework, and nothing should depend on it.

---

Avoid unused stuff. Every single line of code in a program may potentially introduce a bug, and it inevitably adds to the complexity. Look for unused elements and remove them. Especially for a job interview, there's no place for speculation about potential future uses. Leave in what has purpose, leave out everything that doesn't.

For example in Main, you save args[0] in pluginFolder, but then you never use it. Similarly, you parse args[1], but you never use it.

See, that Main class gave you a lot more problems than it solved. It's kind of crazy, but honestly, this trivial implementation would have been a level of magnitude better:

public class Main {
 public static void main(String args[]) {
 MBeanContainer.startMBeanServer(9999);
 }
}

With so little code, this is a lot harder to pick on. This also effectively eliminates the possibility of another class referencing Main.blah. As a general rule: the less code you have, the better. (Less code meaning less logic, less unused stuff. Not "clever" syntax that hurts readability.)

• Dmitry1405: janos, thanks a lot for your usefull answer!!! I also am confused of my approach with exceptions. Should I generate catch exceptions as I did, or some better way exists?
• janos: At the minimum, stop catching Exceptions. Catch the specific sub-types of Exception that are being thrown. Also consider what is the best way to handle them rather than just logging / printing, and if it makes sense to catch them or let them bubble up in the call stack.