Steve Luke wrote:In your specific sample, you only have one piece of persistent data - the notes Map. That Map is iterated over in both the methods but doesn't appear to be modified. The problem is it is passed in from the outside, and so the Map could be modified from an external source. If that happens this class would be unsafe, and there is very little you could do to protect yourself from it.
You should make a local, read-only copy of the notes Map in the constructor. If you do that then the class should be thread safe without any further synchronization needs.
It seems odd that the Map member variable is never modified. We have an instance of an ATM, but calling withdraw() doesn't alter the state of that ATM? There's a bit of design smell there.
@OP: Maybe you have a good reason for doing that, or maybe you're just not showing us the entire class. As the code stands, Steve is correct--since you don't modify the map, as long as you make a copy at construction time, you're safe. However, if there's any way that Map can get modified then you need synchronization. And note that
while you're copying the Map for constructing an ATM, if anything else can modify that original Map, then you need synchronization.
Finally, I'll just point out that the "M" in "ATM" stands for "Machine," so your class name has some redundancy.
("Enter your PIN number into the ATM machine" --> "Enter your Personal Identification Number number into the Automatic Teller Machine machine.")