Win a copy of Functional Reactive Programming this week in the Other Languages forum!
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

good or bad

 
Chris Hurst
Ranch Hand
Posts: 443
3
C++ Eclipse IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Genius or Madness ... bug or feature

I've inherited some code, no comments, no idea of programmer ability can't talk to either but two seperate programmers
appear to be following 'an interesting' pattern.


They're basically locking a member variable then changing the reference used to locate the locked object within the synchronized block,
i.e. I exepcetd them to lock on the same object but they don't necessarily.


Object m_state = state1; // where state1, state2 etc are static final objects created elsewhere


in one public member code of this form ...

and in another public member code of this form ...

My problem was that without comments a quick glance the intention of the code wasn't obvious (I suspect its not what the author intended at all)
and led to a whole list of further evils, too many to list and what they actually intended I suspect could probably be achieved in one of many far more simpler options.


Now I'm not sure if the author intended what they have written but would it be reasonable to say this in a code review ...


1) If your not an expert in java threading never change the reference to the variable you sync on i.e. introduce a new object to lock on without making it very obvious.
2) If you are an expert comment your code to warn others and explain what the intention of the synchronization is i.e. what it does and what it
doesn't.



i.e. my feeling is such code is bordering on evil even if intentional without comments , or are you all doing it ;-).
,
[ October 11, 2006: Message edited by: Jim Yingst ]
 
Ernest Friedman-Hill
author and iconoclast
Marshal
Pie
Posts: 24212
35
Chrome Eclipse IDE Mac OS X
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I agree that it's almost without a doubt a mistake.
 
Jim Yingst
Wanderer
Sheriff
Posts: 18671
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Yep. Bad craziness. Can't think of any good "expert" reason to do anything like this. Well, maybe if only one thread ever changes the reference, it might work, but... ewww. Much, much more likely to be a mistake on the part of the original coder(s).

Chris, I addded code tags to your message above. I recommend using them in the future for readability.
[ October 11, 2006: Message edited by: Jim Yingst ]
 
Edward Harned
Ranch Hand
Posts: 291
Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Not every programmer is sane. That's what makes life interesting.

m_state is not an object but a reference to an object. So if m_state == state1, a thread will sync on the state1 object. If that thread changes the m_state reference to state2, another thread will sync on the state2 object. If these threads run concurrently, they may step on each other since they hold different monitors.

Either the author was nuts or he intentionally introduced a nasty code bomb for someone else to experience.

If the latter, he should be taken out at dawn and shot.
 
Dan Bizman
Ranch Hand
Posts: 387
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Originally posted by Chris Hurst:
Genius or Madness ... bug or feature



Yuck.

I could be wrong, but I think (judging by the name "state") they meant to do something more like:



but they probably thought they were being "clean" and "efficient" by using one variable/object to do two things.
[ October 12, 2006: Message edited by: Dan Bizman ]
 
  • Post Reply
  • Bookmark Topic Watch Topic
  • New Topic