Puppet – Vagrant : Smarter / Better / Stronger

Posted by mestachs in devops, ruby on March 5, 2013

After a few weeks of devops ‘light’ practice @8thcolor, I thought I should share my findings. The presentation of Nathen Harvey pushed me a bit further to share them. I will assume you just read deploying rails or a similar book. Started to play with vagrant/puppet/capistrano, and discovered that the real world is not like the sample in the book.

I’d like to live in theory
In theory everything works fine.

Vagrant

Create a single file for your project to describe the type of machine you want, the software that needs to be installed, and the way you want to access the machine (ssh, port forwarding, shared drive, …). Store this file with your project code.

Run a single command — “vagrant up” — and sit back as Vagrant puts together your complete development environment. Say goodbye to the “works on my machine” excuse as Vagrant creates identical development environments for everyone on your team.

If you are not using it yet … you should ! Developed by Mitchell Hashimoto, Vagrant is your new IDE. You won’t test on beta, pre-production, … simply on your workstation. With vagrant, you messed up your manifests ? No problem vagrant destroy && vagrant up, you are back on road.

Basebox with veewee : because time is money

Don’t waste your time with vagrant sample boxes… often not up to date, not sure about the content, older puppet version, obsolete Virtualbox Guest Additions… Build your own with veewee. 30 minutes download included, just too good to be true. Thanks to Patrick Debois. Don’t forget to adapt the mirror in the preseed.cfg file of your vm configuration.

In my case, I had to tweak a bit veewee to fix dns resolution conflicts with my ubuntu laptop :
– adapt the network settings in lib/veewee/provider/virtualbox/box/helper/create.rb
– add at the end of the bootorder line : --natdnsproxy1 on --natdnshostresolver1 on

Reuse : be lazy and clever

Reusing existing modules is always a good idea, people smarter than you (or at least me) already worked on the installation process for most open source components

git submodule add https://github.com/puppetlabs/puppet-postgresql.git modules/postgresql
git submodule add https://github.com/puppetlabs/puppetlabs-stdlib.git modules/stdlib
git submodule add https://github.com/blt04/puppet-rvm.git modules/rvm
git submodule add https://github.com/rodjek/puppet-logrotate modules/logrotate
git submodule add https://github.com/blt/puppet-ssh.git modules/ssh

Another (better) option is to use librarian-puppet to avoid the hassle of managing git submodules (specially rm). See Carlos Sanchez post for the benefits.

If you build, build to be maintainable

If you don’t find an existing module in the forge don’t hesitate to create one or improve an existing one. But don’t do it blindly, some patterns are emerging amongst puppeteers. A good layout of your modules, that allows parametrization/separation of concerns/optional concerns like monitoring, firewall, … is documented in the Example42 foo module. Quality matters, puppeteers push the adoption of rspec for unit testing modules specially if you plan to support multiple distributions.

You can also check your manifests with puppet-lint and see if they conforms to style guide :

sudo gem install puppet-lint
cd /tmp/vagrant-puppet/
puppet-lint --with-filename .

To fix some issues like tabs, trailing whitespaces, All strings that do not contain variables or escape characters like \n or \t should be enclosed in single quotes see some of the bash scripts from bashrc_puppet. Note that the next version of puppet-lint can fix some of them with –fix option. The next step is to enforce puppet-lint reviews with your continuous build : a jenkins-ci sample. During fosdem2013, Bryan Berry demonstrated how to use test-kitchen for multi-node integration tests. The demo is for chef but possible to port to puppet ;)

Performance Vagrant and puppet

If you are using vagrant and provisioning/destroying/reprovisioning/… you don’t want to waste your time so we can speed up the installation by sharing a folder for your package manager, here’s an example for debian

For puppet, adding –evaltrace will enable performance logs :

Info: /Stage[main]/Ruby/Rvm_system_ruby[ruby-1.9.3-p362]: Evaluated in 261.84 seconds
Info: /Stage[main]/Ruby/Rvm_gem[ruby-1.9.3-p362@rails3.2.11/passenger]: Evaluated in 18.14 seconds
Info: /Stage[main]/Rvm::Passenger::Apache::Ubuntu::Post/Exec[passenger-install-apache2-module]: Evaluated in 43.45 seconds
Info: /Stage[main]/Ruby/Rvm_gem[ruby-1.9.3-p362@rails3.2.11/rails]: Evaluated in 89.32 seconds

You can parse them with a script similar to puppet-profiler. Again a Tim Sharpe (rodjek) project, thanks Tim for puppet-lint and puppet-profiler.

In our case we can still spare a few minutes if we can speed up the rvm/ruby installation.
This can be done by skipping the ruby compilation with prebuilt binaries and a corporate repository.

Production environment

You perhaps don’t have the need (or the infrastructure) to install puppet-master, puppet-agent, puppet-dashboard, … so you can start with a master-less setup. Once you have received a new server (from your hosting provider), you can bootstrap the installation of puppet via capistrano and let librarian-puppet take care of the puppet manifest/modules and apply.

Debugging tricks

And finally some debugging tricks that can help you diagnose some issues.

Adding debugging notice

notice("Installing ruby      : ${ruby_rails_version} ")

Re-applying puppet manifests without vagrant reload or up/destroy

vagrant ssh
sudo su
cd /tmp/vagrant-puppet/manifests && puppet apply --pluginsync --verbose --modulepath '/tmp/vagrant-puppet/modules-0' /tmp/vagrant-puppet/manifests/default.pp

Debugging cycle or missing dependency

As you probably know, the order isn’t guaranteed (if you forgot a dependency) or you accidentally creates cycles in the puppet catalog. It’s possible to graph these dependencies to ease the debugging.

Add the graph options --graph and --graphdir '/tmp/vagrant-puppet/modules-0/graphs'

root@host:/tmp/vagrant-puppet/manifests# cd /tmp/vagrant-puppet/manifests && puppet apply --graph --graphdir '/tmp/vagrant-puppet/modules-0/graphs' --pluginsync --verbose --modulepath '/tmp/vagrant-puppet/modules-0' /tmp/vagrant-puppet/manifests/default.pp

Install
– download Gephi
– launch ./bin/gephi
– open the expanded_relationship.dot
– layout, navigate, filter, … and finally discover the cycle or the missing dependency

, , , , , , ,

6 Comments

Through the eyes of sonar : Collections.

Posted by mestachs in java, sonar on February 26, 2013

Collections usage

After Exception handling, Immutable objects, Complexity, Naming and comments, let’s see what sonar can teach you about Collections.

Use Arrays As List

Use asList instead of tight loops 

 final SWTBotTreeItem[] errorItemsFromNode =...
 for (int j = 0; j < errorItemsFromNode.length; j++) {		
    errorItems.add(errorItemsFromNode[j]);
 }

Can be rewritten as :

 errorItems = Arrays.asList(errorItemsFromNode)

Note that the returned list is a non modifiable list. So if another part of the code tries to modify it will receive a UnsupportedOperationException 

 java.util.List errorItems = new java.util.ArraysList(Arrays.asList(errorItemsFromNode));

Performance – Inefficient use of keySet iterator instead of entrySet iterator

The following code requires to do lookup (files.get(…)) and so evaluating hashCode on the key and while iterating on a Map and the doing a lookup the map implementation 

 Map<String, Map<DateTime, File>> files = ...

  for (Iterator<String> iterator = files.keySet().iterator(); iterator.hasNext();) {
      final String serverName = iterator.next();			
      final Map<DateTime, File> serverStats = files.get(serverName);
      ...
  }

You can prevent this, let’s use the entrySet() that will give you the key and associated value in a Map.Entry 

Map<String, Map<DateTime, File>> files = ...
  for (Map.Entry<String, List<MailServerStats>> entry : files.entrySet()) {
      final String serverName = entry.getKey();			
      final Map<DateTime, File> serverStats = entry.getValue();
      ...
  }

Loose coupling

public class AST
   {
       private HashMap<String, Object> properties = new HashMap<String, Object>();
       
       public HashMap<String, Object> getProperties() { 
              return properties
       }
   }

Please use collection interfaces (Map, Set, List) instead of concrete implementation (HashMap, HashSet, ArrayList, LinkedList,…) : 

 public class AST
   {
       private Map<String, Object> properties = new HashMap<String, Object>();
       
       public Map<String, Object> getProperties() { 
              return properties
       }
   }

This would allow you to easily switch implementation, in this case may be you want the map to be sorted.
You just need to change the new HashMap… by a new TreeMap.

Replace Vector With List

Consider replacing this Vector with the newer java.util.List
Vector synchronizes on each individual operation. That’s almost never what you want to do.
Note that Vector is synchronized… and ArrayList/LinkedList are not.
So if the synchronized nature is required in your code base
you should use Collections.synchronized

 List list = Collections.synchronizedList(new ArrayList());

Replace Hashtable With Map

Consider replacing this Hashtable with the newer java.util.Map
Hashtable synchronizes on each individual operation. That’s almost never what you want to do.

note that Hashtable is synchronized… and HashMap/TreeMap/… are not.
So if the synchronized nature is required in your code base
you should use Collections.synchronized

 Map list = Collections.synchronizedMap(new HashMap());

Class Cast Exception With To Array

This usage of the Collection.toArray() method will throw a ClassCastException.

  return (String[]) values.toArray();

the signature of the method : Object[] toArray()
the ‘java.lang.ClassCastException: [Ljava.lang.Object; incompatible with [Ljava.lang.String;'
but you have the other method T[] toArray(T[] a) 

  return (String[]) values.toArray(new String[values.size()]);

Note that if you use generics you can drop the cast :

  List<String> values = new ArrayList<String>();
  values.add("1");
  values.add("2");
 return values.toArray(new String[values.size()]);

Correctness - Impossible downcast of toArray() result

Same explanation as previous rules.

	private static Map<String, String> linksreasonidtogroupid = new HashMap<String, String>();
	public void testSample() throws Exception {
		getReasonsid();
	}
	
	public static String[] getReasonsid() throws Exception {
		Collection<String> values = linksreasonidtogroupid.values();
		return (String[]) values.toArray();

	}

You endup with : 

java.lang.ClassCastException: [Ljava.lang.Object;
	at CastTest.getReasonsid(CastTest.java:18)
	at CastTest.testSample(CastTest.java:11)

Correctness - Invocation of toString on an array

Suppose you want to logs the variables in the following program :

String[] vars = { "1", "2" };
System.out.println(vars.toString());

the output produced isn't really helpfull

[Ljava.lang.String;@10b4199

instead use conditionnal logging and org.apache.commons.lang.ArrayUtils.toString (nullsafe and performant)

String[] vars = { "1", "2" };
if (LOG.isDebugEnabled()) {
    LOG.debug(ArrayUtils.toString(vars));
}

[1, 2]

3 Comments

Through the eyes of sonar : Naming & Comments.

Posted by mestachs in java, sonar on January 7, 2013

Fourth “Through the eyes of sonar” article ( 1, 2, 3 ), let’s review various naming & comments rules.

Guidelines

English: Illustration of all caps .vs. mixed case.

English: Illustration of all caps .vs. mixed case. (Photo credit: Wikipedia)

The excellent work of Cagdas Basaraner provide us these guidelines :

  • Use short enough and long enough variable names in each scope of code. Generally length may be
    • 1 char for loop counters,
    • 1 word for condition/loop variables,
    • 1-2 words for methods,
    • 2-3 words for classes,
    • 3-4 words for globals.
  • Use specific names for variables, for example "value", "equals", "data", … are not valid names for any case.
  • Use meaningful names for variables. Variable name must define the exact explanation of its content.
  • Don’t start variables with o_, obj_, m_, etc. A variable does not need tags which states it is a variable.
  • Obey company naming standards and write variable names consistently in application: e.g. txtUserName, lblUserName, cmbSchoolType, … Otherwise readability will reduce and find/replace tools will be unusable.
  • Obey programming language standards and don’t use lowercase/uppercase characters inconsistently: e.g. userName, UserName, USER_NAME, m_userName, username, …
    • use Camel Case (aka Upper Camel Case) for classes: VelocityResponseWriter
    • use Lower Case for packages: com.company.project.ui
    • use Mixed Case (aka Lower Camel Case) for variables: studentName
    • use Upper Case for constants : MAX_PARAMETER_COUNT = 100
    • use Camel Case for enum class names and Upper Case for enum values.
    • don’t use ‘_’ anywhere except constants and enum values (which are constants).
  • Don’t reuse same variable name in the same class in different contexts: e.g. in method, constructor, class. So you can provide more simplicity for understandability and maintainability.
  • Don’t use same variable for different purposes in a method, conditional etc. Create a new and different named variable instead. This is also important for maintainability and readability.
  • Don’t use non-ASCII chars in variable names. Those may run on your platform but may not on others.
  • Don’t use too long variable names (e.g. 50 chars). Long names will bring ugly and hard-to-read code, also may not run on some compilers because of character limit.
  • Decide and use one natural language for naming, e.g. using mixed English and German names will be inconsistent and unreadable.
  • Use meaningful names for methods. The name must specify the exact action (ideally behavior) of the method and for most cases must start with a verb. (e.g. createPasswordHash)
  • Obey company naming standards and write method names consistently in application: e.g. getTxtUserName(), getLblUserName(), isStudentApproved(), … Otherwise readability will reduce and find/replace tools will be unusable.
  • Obey programming language standards and don’t use lowercase/uppercase characters inconsistently: e.g. getUserName, GetUserName, getusername, …
    • use Mixed Case for method names: getStudentSchoolType
    • use Mixed Case for method parameters: setSchoolName(String schoolName)
  • Use meaningful names for method parameters, so it can document itself in case of no documentation.

Sonar Naming Ruleset

Package Name

A package is lowercase and avoid strange character

Name ‘com.mestach.myBeans‘ must match pattern ‘^[a-z]+(\.[a-zA-Z_][a-zA-Z0-9_]*)*$‘.

 
package com.mestach.myBeans; //bad
package com.mestach.model;   //good

Either you repackage to com.mestach.mybeans or com.mestach.model
Note that windows isn’t case sensitive but linux is. So if you are using an old versioning system like CVS,
Renaming once committed to a linux from windows may be harder (packages end up as folder : com/mestach/mybeans vs com/mestach/myBeans)

Member Name

Member Name : Name ‘FIELD_FIRSTNAME‘ must match pattern ‘^[a-z][a-zA-Z0-9]*$‘.

 protected String FIELD_FIRSTNAME = "fieldfirstname";

Either
– it’s a constant not well defined → add static final
– it’s an attribute of the class and so it’s preferable to use the lowercase version : fieldFirstName

Local Variable Name and Parameter Name

Local Variable Name : Name ‘fromDate_db2TimeStamp‘ must match pattern ‘^[a-z][a-zA-Z0-9]*$‘

String fromDate_db2TimeStamp = DATEFORMAT_db2TimeStamp.format(fromDate);

Avoid underscore in variable name, java naming convention is more camelCase.

String fromDateDb2TimeStamp = DATEFORMAT_db2TimeStamp.format(fromDate);

Parameter and variable should starts by a lowercase :

public void setMember(Member Member) // BAD
 public void setMember(Member member) //GOOD

A more complex sample involving spring injection :

        //BAD
       @Autowired
       private MyService module_MyService;
      public void setModule_MyService(MyService module_MyService){
               this.module_MyService = module_MyService;
      }

Prefer

        //GOOD
        @Autowired
        @Qualifier("module_MyService")
        private MyService myService;
        public void setMyService(MyService myService){
            this.myService=myService;
       }

Static Variable Name

Sometime… it’s just a final which is missing :

private static Logger LOG = Logger.getLogger(BatchLogger.class);   // BAD
private static final Logger LOG = Logger.getLogger(BatchLogger.class); //GOOD

Often in the code base you have (open) enum like this classes

public class ObjectType {
   public static String SRVTYPE = "SERVER";
   public static String PRINTERTYPE = "PRINTER"
   public static ObjectType SERVER = new ObjectType(SRVTYPE );
   public static ObjectType PRINTER= new ObjectType(PRINTERTYPE );
   private String code;
   
   private ObjectType(String c) {
	code = c;
   }
   public String getCode() {
	return code;
    }
  }

Can be fixed by :

 public class ObjectType {
   private static final String SRVTYPE = "SERVER";
   private static final String PRINTERTYPE = "PRINTER"
   public static final ObjectType SERVER = new ObjectType(SRVTYPE );
   public static final ObjectType PRINTER= new ObjectType(PRINTERTYPE );


    ...

– Don’t publish you SERVER string “constant”, make them private
– The api using this ObjectType.SRVTYPE can be fixed by ObjectType.SERVER.getCode()

Malicious code vulnerability – Field isn’t final but should be

– Make your constant final ! It will prevent modifying the constant inadvertently

 
ObjectType.SERVER =ObjectType.PRINTER

Method Name

Starts with lower case and use camelCase

 //BAD
  public String ProductReasonForm(Model model) {

Prefer

 
//GOOD
  public String productReasonForm(Model model) {

“Name methods after what they are intended to do.” (rather than how they do it) Kent Beck

Bad practice – Class names shouldn’t shadow simple name of superclass

This would help other developper to import the “correct” implementation.

  public class DwrController extends org.directwebremoting.spring.DwrController

Just rename the class to avoid ambiguity.

  public class CustomDwrController extends org.directwebremoting.spring.DwrController

Bad practice – Confusing method names

Bad practice – Confusing method names : Confusing to have methods com.mestach.Person.getDeclarationWithincome() and com.mestach.Person.getDeclarationWithIncome()

Did you notice the “case changes in method name “: Withincome vs WithIncome

– either you wanted to override the method… and you mispelled it.
– either you should perhaps rename it with a less confusing name.

Use positive conditionals

Use positive conditionals. Readability of positive conditionals are better than negative ones.

Negated boolean variable names must be avoided.

 boolean isNotFound; // BAD
 boolean isNoError // BAD

Positive !

 boolean isError; // GOOD
 boolean isFound; // GOOD

The problem arise when the logical not operator is used and double negative arises.
It is not immediately apparent what !isNotError means.

Avoid boolean flags in method signature

Don’t use method alternatives with boolean flag variables (public void someMethod(bool flag)). Write more than one method for each flag condition.

Eg :

 Person loadPerson(Long id, boolean withContactInfos)

Prefer

 /** don't the load the ContactInfos by default */
 Person loadPerson(Long id )
 Person loadPersonWithContactInfos(Long id )

Or introduce an initializationMode

 enum PersonInitializationMode {
     WITHCONTACTINFOS, WITHACCOUNTNUMBERS
  }

    Person loadPerson(Long id ,PersonInitializationMode...  initMode)

More samples from Alex Ruiz’s boolean arguments can be evil

In summary, let’s not use boolean parameters. They don’t communicate their purpose. Instead, we can:
– in the case of methods, split them in two
– in the case of constructors, use static factory methods
– in the case of “three-state” booleans, use Java enums

Use understandable and long names for variables.

– Loop variable names can be i, j, k, index etc.,
– local variable names must be longer than loop variables, parameter names must be longer than local variables and static variable names must be longer than parameters; proportional with scope size

Prefer

 int currentLineNumber;
double amount;

over

 int cln;
 double a;

non-Static nested inner classes

If possible make them static :
– They are more loosely coupled to the parent class. In general, the weaker the coupling, the better.
– They do not hold a pointer to the parent instance. This means:
– They can exist without a parent instance.
– They have a slightly smaller memory footprint.
– They don’t prevent the parent from being GCed.
And ideally move them outside, this will make the code more readable.

A typical example where it causes issues : inner class that extends TimerTask gets registered with a Timer, and then the timer’s thread keeps both the TimerTask and the parent instance alive.

Avoid static methods

Think twice before defining a method as static and be sure if you really need to. Static methods are harder to manage :
– test (state, singleton) and
– evolve (methods overriding, use of beans, alternate implementation).

Comments

Prefer a good naming over comments

For example avoid abbreviation

  /** compute the average */
 double compAvg(double a ,double b)

The name of the method explain it’s purpose !

double computeAverage(double a ,double b)

A comment is often never maintained as the code is refactored.
Leaving the maintainer with comments that don’t match the reality anymore.

Bad comment is worse than no comment.

Don’t comment to delete code

Don’t use comment lines to delete code, just delete. Version controlling system will help you if deleted code is required.

Sonar can help you identifying this. In the comments widget you have the commented LOCs.

 Comments
 28.6%
 70,143 lines 
 74.6% docu. API
 4,604 undocu. API
1,770 commented LOCs
Enhanced by Zemanta

2 Comments

Jenkins as monitoring platform of the poor

Posted by mestachs in groovy, jenkins on December 10, 2012

The goal

The goal was to monitor some html page and wsdl availability. I don’t really have access to all the monitoring infrastructure and wanted to check my development servers. I was looking for a lightweight way of monitoring them. I’ve mixed jenkins and groovy and ended up to pretty and low-cost monitoring solution ;)

Install the necessary plugin and tools

Manage Jenkins > Manage Plugins :
– Groovy plugin : This plugin adds the ability to directly execute Groovy code.
– Green Balls : Changes Hudson to use green balls instead of blue for successful builds
– Groovy Postbuild Plugin : This plugin executes a groovy script in the Jenkins JVM. Typically, the script checks some conditions and changes accordingly the build result, puts badges next to the build in the build history and/or displays information on the build summary page.

Manage Jenkins > Configure System : Groovy > Groovy installations or Install automatically
For the groovy plugin you can use the built-in tool installer or just point it to a unzipped binary of groovy
GROOVY_HOME : /opt/groovy/

So let’s create a free-style jenkins job with the following settings

Discard Old Builds : Max # of builds to keep : 100
Build periodically : */10 * * * *
Execute Groovy Script : groovy command :

servers = ['ex1.server.com','ex2.server.com','ex3.server.com']

wdsls=[]
simpleurls=[]
servers.each() {host ->
   wdsls.add("http://${host}/ws/MyWebService?wsdl")
   simpleurls.add("http://${host}/ui/MyConsole.html")
}

def koCount=0;
def slowCount=0;
def checkUrl = { url, check ->
 def status ='KO'
 def host =''
   start= System.currentTimeMillis()
    try {
     myurl = new URL(url)
       host =myurl.getHost()
       def text = myurl.getText(connectTimeout: 10000, readTimeout: 10000)
       def ok = check(url,text)
      status = ok?'OK':'KO';
      if (!ok) {koCount++}
    } catch (Throwable t) {
       koCount++
       }
    end= System.currentTimeMillis()
    if ((end-start)>100)
       slowCount++
    println "$host\t"+status+'\t'+(end-start)+'\t'+' '+url
}
def checkAllUrl =  {urls, check -> urls.each() {url ->checkUrl(url,check)}}
def wsdlCheck = {url,content -> content.contains("wsdl:definitions")}
def pingCheck = {url,content -> content.contains("status=NORMAL")}
def contentCheck = {url,content -> content.contains("login")}

checkAllUrl (wdsls,wsdlCheck )
checkAllUrl (simpleurls,contentCheck)

println "ko.count="+koCount
println "slow.count="+slowCount

if (koCount>0 || slowCount >0) {
    System.exit(-1)
}

Build two lists of urls : wsdls, simpleurls based on a list of servers.
A first closure checkUrl get the content of an url and update counters ok, ko
, it’s also receiving another closure that will check the expected content of the url content.
Now depending on the kind of content call the checkAllUrl with matching check closure wsdlCheck ,contentCheck,….

Add a Groovy Postbuild : Groovy script:

def addShortTextSlow = { comp,shortcomp->
matcher = manager.getMatcher(manager.build.logFile, comp+".count=(.*)\$")
if(matcher?.matches()) {
    manager.addShortText(shortcomp+' '+matcher.group(1), "grey", "white", "0px", "white")
}
}
addShortTextSlow('slow','slow')
addShortTextSlow('ko','ko')

That’s it !

Subscribe to the jenkins “RSS for failures” feed or your preferred jenkins notification tool and benefit from the jenkins built-in ui !

You have an history of the checks :

jenkins-monitoring-history

And trending
jenkins-monitoring-trend

You can easily embed this graph or the green/red ball in jira our your wiki :


<a href="http://myjenkins.com/job/monitoring/lastBuild/consoleText">
    <img src="http://myjenkins.com/job/monitoring/buildTimeGraph/png" alt="200" title="200" border="0"/>
</a>

<a href="http://myjenkins.com/job/monitoring/lastBuild/consoleText">
    <img src="http://myjenkins.com/job/monitoring/lastBuild/buildStatus" border="0">
</a>

The sky is the limit !

Ok now you got the idea… let’s add some checks to gather

– check some open ports : 

try {
    s = new Socket(host, port);
    s.withStreams { input, output ->	}
    println "management port ok $host $port"
} catch (Exception e){
    koCount++
    println "management port KO for  $host $port : "+e.getMessage()
}

– access jmx beans

import javax.management.remote.*
def serverUrl = new JMXServiceURL('service:jmx:rmi:///jndi/rmi://ex1.server.com:9999/jmxrmi')
def server = JMXConnectorFactory.connect(serverUrl).MBeanServerConnection;
def memory = new GroovyMBean(server, 'java.lang:type=Memory')
println memory.listAttributeNames() 
println memory.listOperationNames()

– some jamon statistics : 

jamonurls=[]
jamonurlsuffix='/jamonadmin.jsp?sortCol=2&sortOrder=desc&displayTypeValue=RangeColumns&RangeName=ms.&outputTypeValue=xml&formatterValue=%23%2C%23%23%23&TextSize=0&highlight=&ArraySQL=^WS-|^Fault&'

servers.each() {host ->	jamonurls.add("http://${host}"+jamonurlsuffix)}

def fixJamonXml= {
	xml ->
	if (xml.indexOf("No data was returned") != -1) {
		return '<JAMonXML></JAMonXML>';
	}
	String content = xml.substring(xml.indexOf('<JAMonXML>'));
	rangeLabels = [ "0_10ms", "10_20ms","20_40ms","40_80ms","80_160ms","160_320ms","320_640ms","640_1280ms","1280_2560ms","2560_5120ms","5120_10240ms","10240_20480ms"];
	content = content.replaceAll( '<Label>','<Label><![CDATA[');
	content = content.replaceAll( '</Label>',']]></Label>');
	rangeLabels.each() {
		rangeLabel -> content = content.replaceAll(rangeLabel, "range_" + rangeLabel);
	}
	content = content.replaceAll(  "LessThan_0ms", "range_LessThan_0ms");
	content = content.replaceAll( "GreaterThan", "range_GreaterThan");
	return content
}

def jamonCheck= {
	url,content ->
	monitors = []
	def JAMonXML = new XmlSlurper().parseText(fixJamonXml(content))
    def parseLong =  { t ->  if (t.text().equals("")) return null; Long.valueOf(t.text().replaceAll(',', ''))}
    def parseLongString =  { t ->  if (t.equals("")) return null; Long.valueOf(t.replaceAll(',', ''))}
    def parseRange = {
		rangeText ->	// 15/10.2 (0/0/0)
		// http://docs.codehaus.org/display/GROOVY/Tutorial+5+-+Capturing+regex+groups
		rangeFormat = /(.*)\/(.*) \((.*)\/(.*)\/(.*)\)/
		matched = ( rangeText.text() =~ rangeFormat )
		if (matched.matches()) {
			return [	'label':rangeText.name() , hits : parseLongString(matched[0][1]),average:matched[0][2]]
		}
		return [	'label':rangeText.name() , hits : 0,average:0.0]
	}
	println "************************"+ url
	JAMonXML.children().each() { row ->
		monitors.add( [
			'label' : row.Label,
			'units' : row.Units,
			'hits' : parseLong(row.Hits),
			'avg'  : parseLong(row.Avg),
			'total' : parseLong(row.Total),
			'stddev' : parseLong(row.StdDev),
			'lastvalue': parseLong(row.LastValue),
			'min' : parseLong(row.Min),
			'max' : parseLong(row.Max),
			'active' : parseLong(row.Active),
			'avgActice':parseLong(row.AvgActive),
			'maxActice':parseLong(row.MaxActive),
			'firstAccess':row.FirstAccess,
			'lastAccess' : row.LastAccess,
			'ranges' : [
				'range_LessThan_0ms' :parseRange(row.range_LessThan_0ms),
				'range_0_10ms' : parseRange(row.range_0_10ms),
				'range_10_20ms' : parseRange(row.range_10_20ms) ,
				'range_20_40ms' : parseRange(row.range_20_40ms),
				'range_40_80ms':parseRange(row.range_40_80ms),
				'range_80_160ms' : parseRange(row.range_80_160ms) ,
				'range_160_320ms' : parseRange(row.range_160_320ms),
				'range_320_640ms' : parseRange(row.range_320_640ms),
				'range_640_1280ms' : parseRange(row.range_640_1280ms),
				'range_1280_2560ms' : parseRange(row.range_1280_2560ms) ,
				'range_2560_5120ms' : parseRange(row.range_2560_5120ms),
				'range_5120_10240ms' : parseRange(row.range_5120_10240ms),
				'range_10240_20480ms' : parseRange(row.range_10240_20480ms),
				'range_GreaterThan_20480ms': parseRange(row.range_GreaterThan_20480ms)]
		] )
	}
	/**
	 *  1      0      10ms
		2     10      20ms
		3     20      40ms
		4     40      80ms 
		5     80     160ms
		6    160     320ms
		7    320     640ms
		8    640    1280ms
		9   1280    2560ms
		10  2560    5120ms
		10  5120   10240ms
		12 10240   20480ms
		13 >>      20480ms
	 */
	def getPercentiles = {monitor ->
	    def ps = [0.5,0.8,0.9,0.95,0.98,0.99]
		def ranges = [];
		monitor.ranges.eachWithIndex() {it, i -> ranges.add(it.value.hits) }	
		def rangesCumulative  = [];	 
		(0..13).each() {i -> rangesCumulative.add (monitor.hits>0?ranges[i]/monitor.hits:0)}
		def percentages= (0..13).collect() {i -> rangesCumulative[1..i].sum()}
		def percentiles = ps.collect{ percentile->percentages.findIndexOf{it>=percentile}}
	   return percentiles
    }
	percentileserrors = [];
	monitors.each {
		percentiles = getPercentiles(it)
		println percentiles.join('\t') + "\t"+it.label
		if (percentiles[2]>8) {
			percentileserrors.add(it.label)
		}		
	}	

	return percentileserrors>0;
}
checkAllUrl (jamonurls,jamonCheck)

, , , ,

Leave a Comment

Through the eyes of sonar : Complexity

Posted by mestachs in java, sonar on November 26, 2012

Complexity

I’m continuing the “Through the eyes of sonar” series ( 1 & 2 ) with various code complexity issues: First a small recap of guidelines/rules and then some practical examples.

KISS Keep it simple & stupid

  1. Easy to read : Simple code doesn’t need additional documentation, or it only needs minimal documentation to be understood
  2. Easy to use : Whoever is using your code will find intuitive to use your objects.
  3. Easy to change : Simplicity means that there is no duplicated logic. A change should match a single place in your code.
  4. Doesn’t use any non-necessary third party tool or technology. Through my experience I have observed how some developers just love to use technologies, tools and frameworks just for the sake of making the project more “cool”. Every time you use them, you are adding extra complexity, and extra complexity means that is more difficult to understand, to maintain and is more prone to failures.
  5. It looks simple. If it doesn’t look simple, it is not simple! You know your code is simple if when you finish you are astonished at how simple the final solution is and you ask yourself how it was possible that it took you so long.
  6. Lean : It only does what is necessary, and nothing else. I think most seasoned developers would agree that trying to anticipate the problems your code will have to face in the future is impossible.
  7. Is straightforward. It doesn’t have unnecessary indirections; the most important operations only require a straight method call.

Some basic rules

Lets see some of the basic rules to write maintainable code :

  1. Keep methods short(one screen limit)
  2. Never ever ever reuse a variable for different purpose
  3. Use self-descriptive variable and method names
  4. Define variables as close as possible to the place of their usage
  5. No magic numbers
  6. Be friend with your language
  7. Don’t fight the convention
  8. Watch out for premature optimization
  9. Always refactor the code after you test it
  10. Don’t get sucked into over-engineering
  11. Learn new things by prototyping

All these rules are explained with concrete examples here

Cyclomatic complexity

Tools like checkstyle will check the cyclomatic complexity of methods against a specified limit.

The complexity is measured by the number of if, while, do, for, ?:, catch, switch, case statements, and operators && and || (plus one) in the body of a constructor, method, static initializer, or instance initializer. It is a measure of the minimum number of possible paths through the source and therefore the number of required tests.

Generally :

  • 1-4 is considered good,
  • 5-7 ok,
  • 8-10 consider re-factoring, and
  • 11+ re-factor now !

How we can reduce it… by implementing simple refactoring. Some of them are built-in in eclipse IDE, sample !

Simplify Boolean Return (Conditional logic can be removed)

Sometime we write unnecessary conditional logic :

public boolean occupiesPosition(int x) {
   if (x >= position && x <= position + data.length())
      return true;
   else
      return false;
}

Can be rewritten :

public boolean occupiesPosition(int x) {
  return (x >= position && x <= position + data.length())
}

Note that some developer still like to introduce a boolean temporary variable to ease the debugging or logging, may be for those developer you can disable UnnecessaryLocalBeforeReturn rule.

Simplify Conditional

Sometime our paranoia of NullPointerException leads us to write unnecessary code. In this example there’s no need to check for null before an instanceof.

public static Object[] getSelectedElements(final ISelection sel) {
   if (sel == null || !(sel instanceof IStructuredSelection))
     return null;
   IStructuredSelection strucSel = (IStructuredSelection) sel;
   ...
}

Extract boolean expression in a method

for (Iterator it = this.timeLines.values().iterator(); it.hasNext(); ) {
  final ITimeLine timeLine = (ITimeLine) it.next();
  final long start = timeLine.getStartTime().getMillis();
  final long end = timeLine.getEndTime().getMillis();
  if ( (start <= startTime && end >= startTime) ||
     (start >= startTime && end <= endTime) ||
     (start <= endTime && end >= endTime) ) {
      res.add(timeLine);
  }
}

If you put the logic in a contains method, you end up with 2 methods of lower complexity and easier to read !

for (Iterator it = this.timeLines.values().iterator(); it.hasNext(); ) {
  final ITimeLine timeLine = (ITimeLine) it.next();
  if ( contains(timeLine)) {
      res.add(timeLine);
  }
}

And with the extracted methods :

private boolean contains(ITimeLine timeLine) {
  final long start = timeLine.getStartTime().getMillis();
  final long end = timeLine.getEndTime().getMillis();
  return (start <= startTime && end >= startTime) || (start >= startTime && end <= endTime) || (start <= endTime && end >= endTime) )
}

Replace if else if else by a map or a set

For example sometimes you see a deep if else tree like this

public boolean isManual() {
      if (MANUAL_CLERCK.equals(this))
         return true;
      else if (MANUAL_MEMBER.equals(this))
         return true;
      else if (MANUAL_INSURER.equals(this))
          return true;
      else if (MANUAL_DECL.equals(this))
	     return true;
      return false;
}

Adding a Set may simplify the if-else-if cascade to a simple contains

private static final Set<IndicationOrigin> MANUAL_ORIGINS = new HashSet<IndicationOrigin>();
static {
   MANUAL_ORIGINS.add(MANUAL_CLERCK);
   MANUAL_ORIGINS.add(MANUAL_MEMBER);
   MANUAL_ORIGINS.add(MANUAL_INSURER);
   MANUAL_ORIGINS.add(MANUAL_DECL);
}
public boolean isManual() {
   return MANUAL_ORIGINS.contains(this);
}

Avoid ternary operator

the following code throws a NullPointerException… despite the usage of the ternary operator to check for null value.

DateMidnight startDate = null;
LOGGER.debug(" ==> NPE " + startDate != null ? startDate.toString(Constants.DATE_FORMAT) : "null");

In fact the priotiry in operators, will be evaluated like this (added parentheses) :

LOGGER.debug((" ==> NPE " + startDate)!= null ? startDate.toString(Constants.DATE_FORMAT) : "null");

So the boolean expression is always true (concat string) even if the startDate is null.
To fix this you either drop the ternary operator or add the necessary braces to change the evaluation priority

LOGGER.debug(" ==> correct log but full timestamp format " + startDate);
LOGGER.debug(" ==> correct log " + (startDate != null ? startDate.toString(Constants.DATE_FORMAT) : "null"));

The code maybe shorter, but you’ve won a bug, so avoid using the ternary operator.

Dodgy – Method uses the same code for two branches

This kind of constructs complexify the code :

if (userLanguage == null || userLanguage.equals("fr")) {
   formattedAddress.append(address.street.trim());
} else {
   formattedAddress.append(address.street.trim());
}

Either you can remove the if else…

formattedAddress.append(address.street.trim());

Either you have a copy/paste error

if (userLanguage == null || userLanguage.equals("fr")) {
     formattedAddress.append(address.street.trim());
 } else {
     formattedAddress.append(address.streetNl.trim());
 }

6 Comments

Through the eyes of sonar: Immutable Objects.

Posted by mestachs in java, sonar on October 29, 2012

Sonar is an open platform to manage code quality. Sonar is combining various java quality tools and collect these possible defects as violations. Let’s see how sometimes we misuse immutable objects.


Immutable Objects

I’m continuing the “Through the eyes of sonar” series with all these immutable objects you use all the times.
Immutable objects are simply objects whose state (the object’s data) cannot change after construction.

Immutable objects greatly simplify your program, since they :
– are simple to construct, test, and use
– are automatically thread-safe and have no synchronization issues
– do not need a copy constructor
– do not need an implementation of clone
– allow hashCode to use lazy initialization, and to cache its return value
– do not need to be copied defensively when used as a field
– make good Map keys and Set elements (these objects must not change state while in the collection)
– have their class invariant established once upon construction, and it never needs to be checked again
– always have “failure atomicity” (a term used by Joshua Bloch) : if an immutable object throws an exception, it’s never left in an undesirable or indeterminate state

Examples of immutable objects
– from the JDK include String, Integer, BigDecimal, Boolean, etc.
– from joda time : DateMidgnight, DateTime,…

More on this topic : ImmutableObjects

java.lang.Integer

Integer Instantiation

Avoid instantiating Integer objects. Call Integer.valueOf() instead. This allows the class to give you back an existing instance if an object was already created for this number. Note this optimization that it’s more for small values of integer and depends on the jre implementation.

 return new Integer(rowCount);

Prefer :

 return Integer.valueOf(rowCount);

java.lang.Double

Why we don’t use Doubles for Financial Calculations

    package com.bloodredsun;

    public class DoubleOperation {

        public static void main(String[] args) {

            double t1 = 10.266d;
            double t2 = 10.0d;

            //Outputs 0.266
            System.out.println(t1-t2);

            double h1 = 100.266d;
            double h2 = 100.0d;

            //Outputs 0.26600000000000534
            System.out.println(h1-h2);
        }
    }

Ouch! That is not what we want but it is the classic behavior of doubles. The inability to represent some decimals in the IEEE-754 format (as binary fractions) causes this. If we want correct precision the answer is to use BigDecimals but we have to remember to use Strings in the constructors or you end up with the same issues that you were trying to avoid.

    package com.bloodredsun;

    import java.math.BigDecimal;

    public class BigDecimalOperation {

            public static void main(String[] args) {

            BigDecimal t1 = new BigDecimal("10.266");
            BigDecimal t2 = new BigDecimal("10.0");

            //Outputs 0.266
            System.out.println(t1.subtract(t2));

            BigDecimal h1 = new BigDecimal("100.266");
            BigDecimal h2 = new BigDecimal("100.0");

            //Outputs 0.266
            System.out.println(h1.subtract(h2));
        }
    }

Notice the usage of the string constructor ! You will read more about it in the the next paragraph about java.math.BigDecimal.

java.math.BigDecimal

Avoid Decimal Literals In Big Decimal Constructor

As the BigDecimal class is immutable, all its methods modifying its value return a new BigDecimal object. The newly created object contains the value modified.

  BigDecimal dec = new BigDecimal("123.54");
  BigDecimal dec2 = new BigDecimal(123.54);
  System.out.println("dec value = " + dec);
  System.out.println("dec2 value = " + dec2);

Result displayed in the console:

 dec value = 123.54
 dec2 value = 123.5400000000000062527760746888816356658935546875

As Java doesn’t parse and represent these two numbers in the same way, the result is different.
For this reason, when you use a BigDecimal object, create it with the constructor using a String in argument.

  BigDecimal dec = new BigDecimal(123.54);
  System.out.println("dec value = " + dec);
  BigDecimal dec2 = dec.setScale(2, BigDecimal.ROUND_HALF_UP);
  System.out.println("dec2 value = " + dec2);

Result displayed in the console:

  dec value = 123.5400000000000062527760746888816356658935546875
  dec2 value = 123.54

Useless Operation On Immutable

As the setScale(newScale, roundingMode) method is used on an immutable object, it returns a new BigDecimal object with the modified value. If you don’t put the result of this method in another BigDecimal object, you will not able to work on this new value.
Remember that the example above is valid for all methods involving the modification of a BigDecimal object’s value.

  BigDecimal dec = new BigDecimal("123.54");
  dec.add(new BigDecimal("1.0"));
  dec.add(BigDecimal.ONE);
  dec.multiply(BigDecimal.TEN);
  System.out.println(dec);

This will display :

 123.54

Fixing by reassigning the dec variable :

  BigDecimal dec = new BigDecimal("123.54");
  dec = dec.add(new BigDecimal("1.0"));
  dec = dec.add(BigDecimal.ONE);
  dec = dec.multiply(BigDecimal.TEN);
  System.out.println(dec);

This will display :

 1255.40

All mathematical methods of the BigDecimal class ask you to give in argument a BigDecimal object containing the value to apply on the value of your BigDecimal object. To avoid the instantiation of a BigDecimal object just to add one or multiply your numbers by ten (as in the example), you can use the constant defined in the BigDecimal class. Use them to avoid creating multiple objects in memory.

org.joda.DateTime/DateMidnight

Use Joda Time

Don’t use the mutable java built-in classes for date/time manipulation, prefer the use the immutable and fluent api of jodatime.

Useless Operation On Immutable

DateTime is also an immutable class. It suffers of the same advantages and inconveniences of the BigDecimal class.
When you use the class, don’t forgot to provide an object to receive the result of your modification or they will be be never apply (cf. the following example) :

public void testAddMonth() throws Exception {
  DateTime time = new DateTime();
  System.out.println(time);
  time.plusMonths(1);
  System.out.println(time);
}

Result displayed in the console :

  2009-10-21T14:34:24.543+02:00
  2009-10-21T14:34:24.543+02:00

No month added… damned.

time=time.plusMonths(1);

java.lang.Boolean

Avoid Instantiation

The Boolean class is used instead of the primitive type in order to offer the possibility to have three values for a boolean (true, false and null). If you just need to use the classical true-false value, use the primitive type.

Use primitives if you can, wrappers if you must

  new Boolean("true"); // bad practice : this will create always new instances prefer Boolean.valueOf(...)

Prefer :

  Boolean.FALSE;
  Boolean.TRUE;
  Boolean.valueOf(x>10)

If it is possible, use the constant defined in the Boolean class instead of creating a new object.

  if (x > 10){
    Boolean.TRUE;
  }
  else {
    Boolean.FALSE;
  }
  Boolean.valueOf(x > 10);

The valueOf(boolean) method can receive in argument a condition which has as result true or false. As you can see in the example before, you can resolve the if-else code by using the valueOf(boolean) method.

java.lang.String

Useless Operation On Immutable

String name = "David";
name.replaceAll("D", "d");
System.out.println("BAD   "+name);
name= name.replaceAll("D", "d");
System.out.println("GOOD  "+name);

BAD David
GOOD david

Correctness – Method ignores return value

I stumble upon this code

String key = ...
key.charAt(keyPos);

Either we forgot something to the index char returned or we can simply drop the call from the codebase.

Avoid String To String

Avoid calling toString on String

    StringBuffer buffer = new StringBuffer(4);
    if (socialState != null) {
  	buffer.append(socialState.toString());
	buffer.append(" ");
     }

Simply :

    StringBuffer buffer = new StringBuffer(4);
    if (socialState != null) {
  	buffer.append(socialState);
	buffer.append(" ");
     }

Note also that in this code we should have used StringBuilder. Check this article :
Using StringBuffer instead of StringBuilder

Performance – Method invokes inefficient new String(String) constructor

Avoid creating a new string :

   protected String getCode(final Code code) {
     return new String(code.getValue() + " " + getLabel(code));
   }

Simply :

 protected String getCode(final Code code) {
     return code.getValue() + " " + getLabel(code);
   }

Correctness – “.” used for regular expression

The dot is a special character in regexp. Using String.split(regexp)… will not split

 String[] splitKeys = this.propertyDescriptor.getClassDescriptor().getName().split(".");

A simpler and safer way is to use StringUtilsor the StringTokenizer where the dot is processed as a normal character

 String[] splitKeys = StringUtils.split(this.propertyDescriptor.getClassDescriptor().getName(),".")

Inefficient String Buffering

Note that string concatenation is often badly coded… some myth tells you that StringBuffer or StringBuilder are the way to go. But one thing that most developers don’t know… is that the compiler is smarter than you ;) 

 "4564654"+sdfds+"1231321"

will be compiled to :

 new StringBuilder("4564654").append(sdfds).append("1231321").toString()

In some cases these optimizations are not enough, specially when looping on a list of records :

strBuff.append("; " + contact.targetid);
strBuff.append("; " + contact.office);
strBuff.append("; " + task.dueDate);
strBuff.append("; " + task.objectId);
strBuff.append("; " + task.statusId);

 

public static final String SEPERATOR = "; "

strBuff.append(contact.targetid);
strBuff.append(SEPERATOR);
strBuff.append(contact.office);
strBuff.append(SEPERATOR);
strBuff.append(task.dueDate);
strBuff.append(SEPERATOR);
strBuff.append(task.objectId);
strBuff.append(SEPERATOR);
strBuff.append(task.statusId);

NullSafety and trim/blank

Instead of :

  if  (anAddress.getZipCode() == null || "".equals(anAddress.getZipCode().trim()))
     return false;

Prefer the less error prone version using StringUtils :

  return StringUtils.isNotBlank(anAddress.getZipCode());

7 Comments

Through the eyes of sonar: Exception handling

Posted by mestachs in java, sonar on October 10, 2012

Sonar is an open platform to manage code quality. Sonar is combining various java quality tools and collect these possible defects as violations. Let’s see some exception handling worst practices detected by these tools.


Avoid Print Stack Trace

Instead of :

catch (Exception e) {
  e.printStackTrace();
}

This exception will often get logged in a separate file (System.err) making debugging more complex than it needs.
Use a logger call instead.


catch (Exception e) {
  logger.warn("failed to ...." ,e);
}

Note the ,e this will tell log4j/slf4j/… to log the full stacktrace. Note that you can configure sfl4j to log the root cause first

Avoid logging only exception message

catch (Exception e) {
  logger.warn("failed to ...."+e);
}

Use the “,” variant of the logging method. In case of null pointer… the log will looks like : 

 failed to ....null

This isn’t really helpful for debugging. So please use the other method :

catch (Exception e) {
  logger.warn("failed to ...." , e);
}

UseCorrectExceptionLogging and other pmd rules

Preserve Stack Trace

Intead of loosing track of the InstantiationException

 } catch (InstantiationException e) {
     throw new HibernateException("Could not instantiate resultclass: " + resultClass.getName());
 }

Preserve the stacktrace with a nested exception constructor :

 } catch (InstantiationException e) {
     throw new HibernateException("Could not instantiate resultclass: " + resultClass.getName() ,e);
 }

Avoid Catching Throwable

A catch statement should never catch throwable since it includes errors (OutOfMemoryError,…) :

 catch (Throwable t) {
  log.warn("unmonitored problem",t);
  success = false;
 }

So do this only in rare case (dictated by an api) where severe problem should catched and logged. If you are forced to do so ensure that the error is logged !

Prefer runtime exception, avoid signature that declare throws Exception

It is unclear which exceptions can be thrown from the methods. It might be difficult to document and understand the vague interfaces.

public Set getChildrenNames(Fqn arg0) throws Exception {

Use a class derived from RuntimeException. This also often force the user of your API to catch these exception and as you can saw it’s no easy to get right !
Checked exceptions I love you, but you have to go

A history of exceptions — Exceptions and how they should be handled always end in heated debates between Java developers. It isn’t surprising that Hibernate has some noteworthy history as well. Until Hibernate 3.x, all exceptions thrown by Hibernate were checked exceptions, so every Hibernate API forced the developer to catch and handle exceptions. This strategy was influenced by JDBC , which also throws only checked exceptions. However, it soon became clear that this doesn’t make sense, because all exceptions thrown by Hibernate are fatal. In many cases, the best a developer can do in this situation is to clean up, display an error message, and exit the application. Therefore, starting with Hibernate 3.x, all exceptions thrown by Hibernate are subtypes of the unchecked Runtime Exception, which is usually handled in a single location in an application. This also makes any Hibernate template or wrapper API obsolete.

Avoid Instanceof Checks In Catch Clause

Each caught exception type should be handled in its own catch clause. For example SQLException a subclass DataTruncation that contains more information and in separate catch you can log the parameter that exceeded the db length, but you will probably share a common method that will handle the rollback of the transaction.

Avoid Throwing Null Pointer Exception

NPE means programmer’s mistake and if you throw NPE you are not a nice person.
NPE may be a convention in the JDK, but perhaps it should stay there too.

if (flowName == null) {
   throw NullPointerException("FlowName cannot be null.");

Throwing NPE is really confusing, prefer IllegalArgumentException :

if (flowName == null) {
   throw IllegalArgumentException("FlowName cannot be null.");

A shorter version using spring assert :

import org.springframework.util.Assert;
Assert.notNull(flowName, "FlowName cannot be null.");

Check also commons-lang Validate or Guava Preconditions

Avoid Throwing Raw Exception Types

Rather than throw a raw RuntimeException, Throwable, Exception, or Error, use a subclassed exception or error instead.

    if (StringUtils.isEmpty(props.getString("smtp.config"))) {
      throw new RuntimeException("Bad Configuration for smtp.config");
    }

Prefer throwing specific exception like : 

    if (StringUtils.isEmpty(props.getString("smtp.config"))) {
         throw new BadConfigurationException("smtp.config");
    }

Correctness – Exception created and dropped rather than thrown

Silly mistakes. We are human after all… luckily sonar is there to spot it !

This code creates an exception (or error) object, but doesn’t do anything with it. For example, something like

    if (x < 0)
      new IllegalArgumentException("x must be nonnegative");

It was probably the intent of the programmer to throw the created exception:

    if (x < 0)
      throw new IllegalArgumentException("x must be nonnegative");

Empty Try Block

The less you have code to maintain… the less work you have. just drop this kind of code 

public void run() {
	try {
	} finally {
		try {
		} catch (Exception e) {
		}			
	}		
}

Bad practice – Method may fail to close stream on exception

Stream from java.io.File access,URLConnection,… should be closed whatever happens in a finally clause.
This will prevent also java.io.IOException: Too many open files

  File f = new File(parent.getAbsolutePath(),"TEST-"+getTestSuiteResult().getModuleName()+".xml");
    try {
	BufferedWriter bw = new BufferedWriter(new FileWriter(f));
	bw.write("<!--?xml version=\"1.0\" encoding=\"UTF-8\" ?-->" + ENDL);
	bw.write(getTestSuiteOpenTag() + ENDL);
	for (UnitTestCaseResult tc : getTestSuiteResult().getTestCaseResults()) {
 	    bw.write(getTestCaseTag(tc) + ENDL);
	}
	bw.write(getTestSuiteEndTag() + ENDL);
	bw.flush();
	bw.close();
    } catch (IOException e) {
	throw new RuntimeException(e);
    }

To fix use the org.apache.commons.io.IOUtils to finally close the writer

    File f = new File(parent.getAbsolutePath(),"TEST-"+getTestSuiteResult().getModuleName()+".xml");
    BufferedWriter bw = null;
    try {
	bw = new BufferedWriter(new FileWriter(f));
	bw.write("<!--?xml version=\"1.0\" encoding=\"UTF-8\" ?-->" + ENDL);
	bw.write(getTestSuiteOpenTag() + ENDL);
	for (UnitTestCaseResult tc : getTestSuiteResult().getTestCaseResults()) {
 	    bw.write(getTestCaseTag(tc) + ENDL);
	}
	bw.write(getTestSuiteEndTag() + ENDL);
	bw.flush();
    } catch (IOException e) {
	throw new UnitTestCaseGenerationException(e);
    } finally {
        IOUtils.closeQuietly(bw);
    }

If you are lucky to use jdk7 take a look at the The try-with-resources Statement.

5 Comments

Follow

Get every new post delivered to your Inbox.