This week's book giveaway is in the OO, Patterns, UML and Refactoring forum. We're giving away four copies of Refactoring for Software Design Smells: Managing Technical Debt and have Girish Suryanarayana, Ganesh Samarthyam & Tushar Sharma on-line! See this thread for details.
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 ]
"Eagles may soar but weasels don't get sucked into jet engines" SCJP 1.6, SCWCD 1.4, SCJD 1.5,SCBCD 5
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 ]
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.