Thursday, June 29, 2023

Horror Code: Boolean Parsing Fest

 This article was originally posted on JRoller on September 21, 2015

A small gem I found the other day:

String value = ...
boolean booleanValue = Boolean.valueOf(value.toLowerCase()).booleanValue();

A lot of useless stuff:

  • toLowerCase is useless because the Boolean class is using equalsIgnoreCase.
  • booleanValue is unnecessary because of automatic unboxing.
  • Boolean.valueOf creates a useless temporary Boolean object.

Why does it seem so hard to parse a boolean?

boolean booleanValue = Boolean.parseBoolean(value);

And this works even if value is null.

Horror Code: FIFO Stack and LIFO Queue

This article was originally posted on JRoller on February 4, 2014 

Recently, I found this class in one of our applications:

 public class FIFOStack extends Vector {

	public synchronized Object push(Object o) {
		add(o);
		return o;
	} 

	public synchronized Object pop() {
		if (isEmpty()) {
			return null;
		} 
		return remove(0);
	} 

	public synchronized boolean empty() {
		return isEmpty();
	} 
} 

I was imagining the developer thinking: "If only I had a class like Stack, except it would work in FIFO mode". The code is really old, so there was no Queue implementation in the JDK at this time. But still, definitions of Stacks and Queues are way older than the JDK.

Out of curiosity, I looked in the code for LIFO Queues. To my surprise, I found 2, both in the JDK itself. One caught my attention, due to the comment preceding it. See for yourself, in the SynchronousQueue class:

    
    /*
     * To cope with serialization strategy in the 1.5 version of
     * SynchronousQueue, we declare some unused classes and fields
     * that exist solely to enable serializability across versions.
     * These fields are never used, so are initialized only if this
     * object is ever serialized or deserialized.
     */

      

I do not have a Java 1.5 around to check, but if I understand correctly the comment, serialization of SynchronousQueue in Java 1.5 was too tied to its implementation. So when they changed it in Java 1.6, they had to create the old LifoWaitQueue internal class just so that serialization would work with previous Java versions. Backward compatiblity can make to ugly code sometimes. The funny part is that the internal LifoWaitQueue and FifoWaitQueue classes were replaced by TransferQueue and TransferStack. Even the best of us can make naming mistakes...

Of course, the important thing to understand this post is that Stacks are supposed to be LIFO, and Queues FIFO.
I checked on my Java 17 for the SynchronousQueue class, and this comment, together with its LifoWaitQueue, are still there in the code. Time for deprecation?

Horror Code: Get Simple Class Name

 This article was posted originally on JRoller on January 29, 2014

You probably heard of one method of Class that give the simple name of the class, without all the packages. Even if you hadn't, you probably know a simple way to get the sub-String following the last dot. Well, I know someone who doesn't:


 	public static String classnameNormalize(String classname) {
		String s = new String(classname);
		int n;
		while (true) {
			n = s.indexOf(".");
			s = s.substring(n + 1, s.length());
			if (n == -1) {
				return s;
			} 
		} 
	} 
The function that gives the name of the class without its packages is Class.getSimpleName(). But if you don't know about it, and you need to get the substring after the last dot, it is better to use the String.lastIndexOf() method instead of this whole loop.


Double Timeout

 This article was posted on JRoller on October 9, 2013

The other day, I needed to check if a configuration parameter giving a timeout in number of seconds was really used by our communication layer. I came to the point in the code where we are sending a request and giving the timeout parameter to the lower layer method (a standard market product). The thing that caught my attention was that the method timeout parameter was of type double.

So I decided to decompile the code just to check why on earth they decided upon a double. This led me to the following conversion method (this is decompiled code, so the constants are already inlined):

static final long toMillis(double d)
{
    if(d == 0.0D)
        return 0L;
        
    double d1 = (d + 0.00050000000000000001D) * 1000D;

    if(d1 >= 9.2233720368547758E+018D)
        return 9223372036854775807L;

    return (long)d1;
}

So yes, it is converted to a long in some strange way. Maybe they want to cope with milliseconds or even nanoseconds in a future version?

Horror Code: You Said Naming Conventions?

 This article was originally posted on September 19, 2013

I remained speechless...

public final class AbstractButtonList

The class name says abstract, but the modifiers say final. Quite misleading...

Horror Code: How to Align two Labels

 This article was originally posted on JRoller on September 13, 2013

Or rather how not to. This is a code a colleague found on our GUI. The developper probably did not know how to align two components, so he decided to do it his own way. There is no easy way to set the size of a JLabel, but there is a constructor for JTextfield on which you can set a size in number of characters. So he took a JTextfield, removed the border, set it as non editable, and it becomes magically a JLabel to the outside world.

        final JPanel lbgpanel = new JPanel();
        lbgpanel.setLayout(new FlowLayout(FlowLayout.LEFT));
        final JTextField lbgLabel = new JTextField("Background :", 7);
        lbgLabel.setBorder(null);
        lbgLabel.setEditable(false);
        lbgpanel.add(lbgLabel, BorderLayout.EAST);
        lbgpanel.add(_colorBackgroundBt, BorderLayout.EAST);
        lbgpanel.add(_resetBg, BorderLayout.EAST);

        final JPanel lfgpanel = new JPanel();
        lfgpanel.setLayout(new FlowLayout(FlowLayout.LEFT));
        final JTextField lfgLabel = new JTextField("Foreground :", 7);
        lfgLabel.setBorder(null);
        lfgLabel.setEditable(false);
        lfgpanel.add(lfgLabel, BorderLayout.EAST);
        lfgpanel.add(_colorForeGroundBt, BorderLayout.EAST);
        lfgpanel.add(_resetFg, BorderLayout.EAST);

As a side note, I never understood the use of the BoderLayout.EAST constant in a FlowLayout.

Reading these old articles, I can see now that I was never really explicit in my explanations... Here, the real WTF is not that he used a JTextfield instead of a JLabel, although I'm sure the JLabel would have been a better fit. It is rather the use of a BorderLayout constant with a FlowLayout, that luckily would just ignore it. It might be because of a previous try with a BorderLayout, but then, setting all components on the eastern border will not yield the expected behavior...

Horror Code: ToString Overflow

 This article was originally posted on JRoller on June 12, 2013

A colleague of mine showed me this code from one of the libraries we are using:

    @Override
    public String toString()
    {
        final boolean reuse = false;
        try
        {
            this.ssc.socket().getReuseAddress();
        }
        catch (SocketException exception)
        {
            LOGGER.error(this+ \" SocketException\", exception);
        }

        return String.format(\"AccId[%1$10s] Port[%2$s] reuse=%3$s backlog=%4$s\", hashCode(), this.listenPort, reuse, BACKLOG); 
    } 

The strange part is the code trying to perform some socket operation during a toString. My guess is that they wanted to return something into that poor boolean that just seem stick to a false value. But what does really happen if you get that dreaded SocketException? Do you see it? Yes, that’s a Stack Overflow!

For those who did not see it at the time, here is the reason 10 years later: the logging of (this + something) will cause a call to the same toString method.

Horror Code: Empty Integer

 This article was originally published on JRoller on July 20, 2012

I just found this interesting piece of code:

  Integer oldGroupId = ...
  Integer groupId = (oldGroupId.toString().trim().equals(GUIConstants.EMPTY_STRING)) ? NumberConstants.ZERO_INTEGER : oldGroupId;

At first, I was not quite sure why he used the Integer class. Either he would expect the value to be possibly null, and a nice NPE would result of the following code. Or he wanted to use the toString method, ignoring that a static version exists in the Integer class.

And then, he trims the String, maybe not trusting that those guys developing Java would return a clean String. Better be on the safe side.

But the nicest touch of all, is that all of this is for testing if the trimmed String is empty. One must now take some minutes to consider what possible value of an integer can give back an empty String. When one finished pondering, one will just remove all this jibber-jabber and replace it with a simpler int assignment.

Horror Code: Interning String Literals

 This article was originally posted on JRoller on June 29, 2012

I found this kind of code in our application:

  public static final String CONSTANT_STRING = "Some value".intern();

Interning a String means storing it into a common pool of Strings managed by the JVM, so that if I already used this String somewhere, I will only get back its reference. It may be a good idea sometimes, but you have to know that all String literals are already interned by the JVM. As useless as calling toString() on a String.

Horror code: Double salto

 This article was posted originally on JRoller on February 24, 2012

A colleague noticed this code yesterday, which is the Java equivalent of jumping a double salto, then falling back at the same place from where you started:

  private List allHosts;

  public void myMethod(Host theHost) {
    Host myHost = allHosts.get(allHosts.indexOf(theHost));
    ...
  }

I did not put much context in my original post. The Host object was not comparable, so the indexOf operation will look for the object by reference. It would then retrieve the same Host object passed as a parameter. Since Host objects were always to be found in the list, and Exceptions were not even handled, we removed the line, and the program just worked as before.

Horror code: Eight-State Boolean

This article was originally posted on JRoller on May 27, 2011

This is probably the worst code I ever seen.

Imagine an application with a table, where the user can filter lines according to some criterium. Someday, someone asked to be able to combine criteriums with "OR" or "AND" operands. The developer at the time thought that since the operand can only have two values, it is logical to store it as a boolean.

Some later day, the user probably complained that he can not force priority of the operands, say with parenthesis. Or maybe it was the developer's idea. So he added the possibility in the user interface to choose between eight types of operands, in a combo box: OR, AND, OR(, AND(, )OR, )AND, )OR(, )AND(. And here are the constants declaration:

    public final static Boolean  OR  = Boolean.TRUE;
    public final static Boolean  AND  = Boolean.FALSE;
    public final static Boolean  _OR  = Boolean.FALSE;
    public final static Boolean  _AND  = Boolean.FALSE;
    public final static Boolean  OR_  = Boolean.TRUE;
    public final static Boolean  AND_  = Boolean.FALSE;
    public final static Boolean  _OR_  = Boolean.TRUE;
    public final static Boolean  _AND_  = Boolean.FALSE;

Notice how all those values still fit nicely into a Boolean. Now we need the code that parses the operand String from the combo:

    static public Boolean getOperande(String aOp)
    {
        if (aOp == null)
            return OR;
        if (aOp.equals("OR"))
            return OR;
        else
            if (aOp.equals("AND"))
                return AND;
            else
                if (aOp.equals("AND("))
                    return AND_;
                else
                    if (aOp.equals(")AND"))
                        return _AND;
                    else
                        if (aOp.equals("OR("))
                            return OR_;
                        else
                            if (aOp.equals(")OR"))
                                return _OR;
                            else
                                if (aOp.equals(")AND("))
                                    return _AND_;
                                else
                                    if (aOp.equals(")OR("))
                                        return _OR_;
                                    else
                                        return _OR;
    }

And the opposite operation for displaying nicely the filter:

    static public String getOperande(Boolean aOp)
    {
        if (aOp == OR)
            return "OR";
        else
            if (aOp == AND)
                return "AND";
            else
                if (aOp == AND_)
                    return "AND(";
                else
                    if (aOp == _AND)
                        return ")AND";
                    else
                        if (aOp == OR_)
                            return "OR(";
                        else
                            if (aOp == _OR)
                                return ")OR";
                            else
                                if (aOp == _AND_)
                                    return ")AND(";
                                else
                                    if (aOp == _OR_)
                                        return ")OR(";
                                    else
                                        return "OR";
    }

Both methods apply the same principle for using at best the horizontal size of your screen. I wonder if the JVM will actually optimize out the useless if clauses...

For completeness, here is part of the code that will apply the operands:

    protected boolean newApplyFilter(int i, int aRow)
    {
        boolean previousRes = true;
        boolean res = true;
        Boolean op = OR_;
        for (int j = i ; j < critereList.size() ; j++)
        {
          Critere c = (Critere)critereList.get(j);

          res = ...;

            if (j != i)
          {
              if (op == AND)
                  res = previousRes && res;
              else
                    if (op == OR)
                      res = previousRes || res;
                  else
                        if (op == AND_)
                          return (previousRes && newApplyFilter(j, aRow));
                      else
                            if (op == OR_)
                              return (previousRes || newApplyFilter(j, aRow));
                          else
                                if (op == _AND)
                                  return (previousRes && newApplyFilter(j, aRow));
                                else
                                    if (op == _OR)
                                      return (previousRes || newApplyFilter(j, aRow));
                                  else
                                        if (op == _AND_)
                                          return (previousRes && newApplyFilter(j, aRow));
                                      else
                                            if (op == OR_)
                                              return (previousRes || newApplyFilter(j, aRow));
          }
          previousRes = res;
          op = c.operande;
        }
        return res;
    }

That's the point where my brain just signed off, and I asked the poor guy who inherited the code to "remove the whole sh***".

I didn't know that the guy just discovered the power of regular expressions and was ready to unleash it, even for this rather simple task. Here is the parsing code he commited:

private static  final Pattern  patternOperator = Pattern.compile("OR|AND");
static public Boolean getOperande(String aOp)
{
        Matcher matcherFilter  = patternOperator.matcher(aOp);
        if (matcherFilter != null && matcherFilter.matches())
        {
          String strOperator = matcherFilter.group(0);
               return ((strOperator != null) && (strOperator.equals("AND"))) ? AND
                                  : OR;
        }
        return OR;
}

Yep, the eight state boolean is still the worse piece of code I ever met. Replacement code is just a minor WTF in comparison.

Horror Code: For Each Remove

 This article was posted originally on JRoller on September 13, 2011.

I found this new interesting pattern to loop over all elements of a list in our production code:

    try {
        for (Object element = myList.remove(0); ; element = myList.remove(0)) {
            ...
        }
    } catch (ArrayIndexOutOfBoundsException e) {
        // It's ok
    }

For those who are thinking right now about using it: PLEASE DON'T!

At the time I wrote this article, I had enough faith to believe that people who will read it will understand how much of a bad code it is. In case my faith was wrongly placed, here are two important rules:

1. Do not modify your list while looping. Here, the side effect is that you will end up with an empty list.

2. Do not use an exception for normal code branching. In spite of the comment, it is not ok.

Monday, June 5, 2023

Horror Code: The Uptime Comparator of Death

This article was originally posted on JRoller on December 23, 2011.

We have an application that monitors other applications, something like the Windows Task Manager. The developers in charge of this application were given the task to add an uptime column to the table. We advised them to use a tool from the Linux machine on which the application was running which gives back the uptime in seconds. All they had to do was format this information nicely and print it in the cell.

Not agreeing, they felt it would be easier to use another small program existing on the Linux machine, that was already presenting the information nicely. The column looked actually OK, except when someone wanted to sort the table according to the uptime... The String comparison did not yield the expected results. But our developers, instead of backing up to the first proposed solution, felt that they could simply work around the problem with the use of a regular expression. Maybe, but there are regulars expressions and "wait, let me find out how this thingy works" regulars expressions. Here is their uptime comparator:
/**
*
* UptimeComparator : A comparator to compare strings containing
* numbers in the natural order instead of the  ASCII order.
*/
class UptimeComparator implements Comparator {    
  private static final String NUMBER_PATTERN = "(\\d+)";    
 
  @Override    
  /**    
   * Expecting the uptime string  in the below formats only.    
   * Otherwise this method may not work according to expectation !!!    
   * 1) 3 min    
   * 2) 4:48 hour    
   * 3) 10 days 4:48 hours    
   * 4) 10 days 3 min    
   */    
  public int compare(String arg0, String arg1) {        
    // To avoid comparison issues, due to white spaces, remove excess spaces        
    arg0 = arg0.replaceAll("[\\s]+", " ");        
    arg1 = arg1.replaceAll("[\\s]+", " ");        
    /**        
     * Time mentioned in minute is always smaller than        
     * that mentioned in days or hours        
     */        
    if ((arg1.matches("[\\d]+ min")
        && arg0.matches("[\\d]+ days [\\d]+:[\\d]+ hour"))                
      || (arg1.matches("[\\d]+ min")                    
        && arg0.matches("[\\d]+ days [\\d]+ min"))                
      || (arg1.matches("[\\d]+ min")                    
        && arg0.matches("[\\d]+:[\\d]+ hour"))) {            
          return 1;        
    }        
    if ((arg0.matches("[\\d]+ min")                    
        && arg1.matches("[\\d]+ days [\\d]+:[\\d]+ hour"))                
      || (arg0.matches("[\\d]+ min")                    
        && arg1.matches("[\\d]+ days [\\d]+ min"))                
      || (arg0.matches("[\\d]+ min")                    
        && arg1.matches("[\\d]+:[\\d]+ hour"))) {            
          return -1;        
    }        
    /**        
     * Time mentioned in hours is always smaller than        
     * that mentioned in days.        
     */        
    if ((arg1.matches("[\\d]+:[\\d]+ hour")                    
        && arg0.matches("[\\d]+ days [\\d]+:[\\d]+ hour"))                
      || (arg1.matches("[\\d]+:[\\d]+ hour")                    
        && arg0.matches("[\\d]+ days [\\d]+ min"))) {            
          return 1;        
    }        
    if ((arg0.matches("[\\d]+:[\\d]+ hour")                    
        && arg1.matches("[\\d]+ days [\\d]+:[\\d]+ hour"))                
      || (arg0.matches("[\\d]+:[\\d]+ hour")                    
        && arg1.matches("[\\d]+ days [\\d]+ min"))) {            
          return -1;        
    }        
    /**        
     * If both days are not equal, compare only the days.        
     */        
    if((arg0.matches("[\\d]+ days ([\\w]|[\\W])*"))                
        && (arg1.matches("[\\d]+ days ([\\w]|[\\W])*"))){            
          String day1 = arg0.split("[\\s]+")[0];            
          String day2 = arg1.split("[\\s]+")[0];            
          if (!day1.equals(day2) && day1.matches(NUMBER_PATTERN)                    
            && day2.matches(NUMBER_PATTERN)) {                
              return (int)(Double.parseDouble(day1) - Double.parseDouble(day2));            
          }        
    }        
    /**        
     * If both days are equal....        
     * Eg.  "X days Y min" is always less than "X days P:Q hour"        
     */        
    if ((arg1.matches("[\\d]+ days [\\d]+ min")                
      && arg0.matches("[\\d]+ days [\\d]+:[\\d]+ hour"))) {            
        return 1;        
    }        
    if ((arg0.matches("[\\d]+ days [\\d]+ min")                
      && arg1.matches("[\\d]+ days [\\d]+:[\\d]+ hour"))) {            
        return -1;        
    }        
    /**        
     * If both string are identical, compare based on the integer in them.        
     */        
    return compareStringWithNumber(arg0, arg1);    
  }    
 
  /**    
   * Compare two strings with numbers in the natural order.    
   * compareStringWithNumber.    
   *    
   * @param str1    
   * @param str2    
   * @return    
   */    
  public int compareStringWithNumber(String str1, String str2) {        
    if (str1 == null || str2 == null) {            
      return 0;        
    }        
    List split1 = split(str1);        
    List split2 = split(str2);        
    int diff = 0;        
    for (int i = 0; diff == 0 && i < split1.size() && i < split2.size(); i++) {
      String token1 = split1.get(i);
      String token2 = split2.get(i);
      if (token1.matches(NUMBER_PATTERN)                    
        && token2.matches(NUMBER_PATTERN)) {                
          diff = (int) Math.signum(Double.parseDouble(token1)                          
            - Double.parseDouble(token2));            
      } else {                
        diff = token1.compareToIgnoreCase(token2);
      }        
    }        
    if (diff != 0) {            
      return diff;        
    }        
    return split1.size() - split2.size();    
  }    
 
  /**    
   * Splits a string into strings and number tokens.    
   */    
  private List split(String s) {        
    List list = new ArrayList();        
    Scanner scanner = new Scanner(s);        
    int index = 0;        
    String num = null;        
    while ((num = scanner.findInLine(NUMBER_PATTERN)) != null) {            
      int indexOfNumber = s.indexOf(num, index);            
      if (indexOfNumber > index) {                
        list.add(s.substring(index, indexOfNumber).trim());            
      }            
      list.add(num);            
      index = indexOfNumber + num.length();        
    }        
    if (index < s.length()) {
        list.add(s.substring(index));
    }
    return list;
}

Before you utter the WTF words, let me explain how this works. In fact it is pretty simple.

First, we notice that there are altogether four possibilities, as you can see in the comments. So we make a nice hierarchy of the possibilities: if one uptime has only minutes while the other has not, then we know that the one that has only minutes is the smallest. Clever!

Repeat this thinking process with hours. Great!

Now let's take care of another case: the two uptime Strings contain days, and we are able to quickly compare the number of days. Youpee!

Same number of days? We can check that if we have one uptime containing only minutes and the other one only hours, then we have a winner. Woohoo!

Still no decisions? It is time to use the hammer, the "compareStringWithNumber" method.

This method uses another helper method called "split". What it does is that it creates a list of all the numbers and other characters in the uptime String. So we use a regular expression to find all the numbers, and try to look for it again in the String (because those damn Scanners can not give us this information). This whole method is of course buggy, I let you find the cases when it does not work (hint: 11:11 hour will not for instance).

Now let's go back to the hammer. We have our two lists of numbers and things, with probably the same numbers of element (or not?). We check again that we are really dealing with numbers, because we should have the same format, but we are not sure. So just in case, we insert a String comparison. We might have days before minutes, but we should not. Or should we? No, I'm pretty confident all this works.

I told you, all very simple indeed.

Unfortunately, because of the way the article was stored (HTML does not like comparison characters), part of the code was lost. But I find that even like this, you can still enjoy its cleverness.

UPDATE: I found the complete version of this article on the Wayback Machine! So now you get the code in its complete splendor.