Friday, April 25, 2014

Copying In Object-Oriented Languages: Be Careful What You Ask For

My boss brought to my attention that one part of the application showed 20 movement records for a particular offender -- but when we brought up the list, it showed 20 copies of the same movement record.  The Informix SPL being called returned 20 rows, all different.

On investigation, it turned out that the code was doing something like:

CNoteVO cnoteVO = some trash that filled it in
ArrayList resultList = new ArrayList();
and then for every row of data returned by the SPL:

 loadData(cnoteVO);
  resultList.add(cnoteVO);

Of course, because cnoteVO was the same object each time, the resultList.add call was just storing a pointer to the same cnoteVO object each time, and at the end of processing those 20 rows, we now had twenty pointers to the same cnoteVO -- which meant that whatever was stored in cnoteVO most recently was duplicated 20 times.

The solution was to write a clone() method -- but of course, nothing is ever that simple, because CNoteVO is a superclass, and you can't really easily write a clone() method that will handle all the subclasses.  (There is a way, with reflection, but I wanted to make sure that I understand this perfectly.)  Instead, I defined clone() in CNoteVO as an abstract class, and defined a concrete clone() method in each of the subclasses.  Now:

   cnoteVO = loadData();
   CNoteVO curRow = cnoteVO.clone();
   resultList.add(curRow);

Now every row is a distinct and different object, and I get 20 rows that are all different.

It has been many years since I ran into an issue like this; it is the only thing that helps me keep my sanity at work.

UPDATE: Of course, this isn't specific to OOPs.  I have seen this mistake made (and probably made it myself) in C, where the temptation is strong to reuse a malloced block, and then store the pointer to that block instead of doing another malloc.

6 comments:

  1. Sounds like somebody skipped some unit testing in the past...

    ReplyDelete
  2. I must be missing something...

    cnoteVO = loadData();

    resultList.add(cnoteVO);

    Unless loadData is returning the same object reference on each call, cnoteVO should have a different reference, to a different object, each time you do the two statements above.

    Could it be that cnoteVO = loadData() was actually cnoteVO.loadData() (dot instead of equals, in case the comment system messes it up)?

    ReplyDelete
  3. StormChaser: you are right. I was working from memory. loadData was passed the cnoteVO.

    Unknown: Yes, there was no operational unit testing when I arrived on this project. And what little there had been was done completely wrong.

    ReplyDelete
  4. But I thought these garbage-collected languages were supposed to solve all of our problems with memory management!

    If your language treats user defined types differently than fundamental types (e.g. allows them to only live on the heap, and not the stack), it's not truly object-oriented. It's only object-oriented-esque.

    I hate garbage-collected languages. The make proper use of RAII impossible, and they are predicated on the idea that you can't possibly manage the lifetime of resources, so just drop them on the floor and don't worry about cleaning them up. Like that ever works.

    ReplyDelete
  5. Garbage collection never solves the problem of reusing a block and storing a pointer to it.

    ReplyDelete
  6. "If your language treats user defined types differently than fundamental types (e.g. allows them to only live on the heap, and not the stack), it's not truly object-oriented. It's only object-oriented-esque. "

    Nonsense. Where the object resides is an implementation artifact. Whether the language passes a reference (pointer) or the actual object on the stack is irrelevant.

    ReplyDelete