TechFindings

For someone like me working in a (very) small development team SonarQube is good. It reviews my code and it teaches me about both old and new things that I did not know about.

There are some things I don’t agree about though (all JavaScript below).

“switch” statements should have at least 3 “case” clauses

I understand the point. But I have my reasons. Let us say I have orders with different statuses (DRAFT,OPEN,CLOSED). Then often I have switch statements like:

switch (order.status) {
case 'DRAFT':
  ...
  break;
case 'OPEN':
  ...
  break;
case 'CLOSED':
  ...
  break;
default:
  throw new Error(...);
}

This switch over order.status happens in many places. Sometimes it is just:

switch (order.status) {
case 'CLOSED':
  ...
  break;
}

I don’t want to refactor the (rewrite if with switch) code if/when I need to do something for other states. And I like a consistent form.

Another case can be that I allow the user to upload files in different formats, but for now I just allow PDF.

switch ( upload.format ) {
case 'pdf':
  process_pdf();
  break;
default:
  throw new Error('Unsupported upload.format: ' + upload.format);
}

A switch is much more clear than an if-statement. However, if the above code was in a function called uploadPdf() then the code would have been:

if ( 'pdf' !== upload.format ) throw new Error(...);
...
...

But in conclusion, I think switch statements with few (even 1) case clause are often fine.

This branch’s code block is the same as the block for the branch on line …

This happens to me often. A good rule is to never trust user input. So I may have code that looks like this (simplified):

if ( !(user = validateToken(request.params.token)) ) {
  error = 401;
} else if ( !authorizeAction(user,'upload') ) {
  error = 401;
} else if ( !reqest.params.orderid ) {
  error = 400;
  errormsg = 'orderid required';
} else if ( user !== (order = getOrder(request.params.orderid)).owner ) {
  error = 401;
} else if ( !validateOrder(order) ) {
  error = 400;
  errormsg = 'order failed to validate';
} else ...

The point is that important part of the code is not the code blocks, but rather the checks. The content of the code blocks may, or may not, be identical. That hardly counts as duplication of code. The important thing is that the conditions are executed one-by-one, in order and that they all pass.

I could obviously write the first two ones as the same (with a more complex condition), since both gives 401. But code is debugged, extended and refactored and for those purposes my code is much better than:

if ( !(user = validateToken(request.params.token) ) ||
     !authorizeAction(user.upload) ) {
  error = 401;
} else ...

My code is easier to read, reorganize, debug (add temporary breakpoints or printouts) or improve (perhaps all 401 are not equal one day).

Remove this …… label

The reason I get this warning is mostly that I use what i learnt in my own post Faking a good goto in JavaScript.

SonarCube also complained when I wanted to break out of a nested loop this way:

break_block {
  for ( i=0 ; i<10 ; i++ ) {
    for ( j=0 ; j<10 ; j++ ) {
      if ( (obj=lookForObjInMatrix(matrix,i,j) ) ) break break_block;
    }
  }
  // obj not found, not set
}

It is a stupid simplified example, but it happens.