bhaskar hazarika wrote:While m trying to debug i got a NPE.
Here is the piece of code:
Getting a Null value for key.. cant really make out where i have done wrong.
Sorry, but I have to say that almost every single line of this snippet has a really bad code smell to it.
Line 2: Why declare this as a local variable? It looks like a constant. Declare it as a private static String outside of the method or explore using an enum instead (perhaps DealerUserStatus.ACTIVE).
Line 3: NEVER use != with a String. Use equals() instead: if (!ACTIVE.equals(status))
Line 5: DealerUserImpl.createPrimaryKey(loginId) looks like it could be extracted to a method; a factory method: newPrimaryKey(loginId). This abstracts away the need to use DealerUserImpl to generate the key.
Line 6: The type for tableRow is DealerUserImpl. When I see "Impl" in the name, it suggests to me that it implements the interface DealerUser. If this is true, then you need to declare tableRow as a DealerUser instead.
Line 7: Why do you need to cast to DealerUserImpl? Is it because findByPrimaryKey() returns a DealerUser and not a DealerUserImpl? Then see my advice for Line 6.
Line 8: the name tableRow suggests an implementation detail: that what you're dealing with is a database table row. What exactly are you trying to set active here? A DealerUser? You are mixing intent with implementation. Not a good thing for readability and maintaining proper levels of abstraction.
Line 9: Why are you calling getDBTransaction() multiple times? Is there a possibility of getting different transactions on each call? Why not just keep a local reference to the same transaction object? IMO, a local reference would be safer since you are at least assured that the transaction that you start with is the one you commit in the end.