Another great article Willie! Situations where authenticated users see other users data is a dangerous thing to happen. Upon seeing this behavior some developers may tend to believe that somehow a users session was getting crossed with someone elses, but as you pointed out in your article, this behavior can be resolved by proper usage of access modifiers of variables within classes and knowing which classes should be made static. Mentoring developers on these fundamental programming practices is vital to any organization.

Great article. That is why doing code review is really important. It is a great opportunity to share best coding and design practice and also help junior developers to beef up the fundamentals. It is an effective way to avoid such costly bugs introduced into production.

I too agree with comments, Class level variable are never tread safe.

@Umesh: Well, I wouldn't go *that* far. If all of your access to the static variable is either read-only or else properly synchronized then there's no threadsafety problem. But in practice what people do is share data (whether static or instance--the problem is the sharing, not the static scope) across threads either without realizing they're doing so, without properly controlling it, or honestly without even trying to propertly control it because they think it's too hard to understand all that "thread stuff." It is a little hard to learn--no doubt--but it's definitely part of the job description for anybody who builds web apps. :-D

Code Review is not mentoring. Nice try

@bob: Yep, I agree. Mentoring is more an ongoing 1-on-1 relationship that's as much personal in nature as it is professional. What I mean is that the mentor makes a personal investment in the mentee. That's decidedly different than what you see in a code review.

The article isn't really intended to be about code reviews per se. Rather I'm trying to help senior software developers overcome their "expert blindspot" (to use a phrase one of my university professors used to use) so they can more effectively grow the technical capability of their teams. The expert blindspot idea is that once you gain expertise in a certain area you develop a blindness to how non-experts conceptualize it. I do think that code reviews are a nice way to limit that blindness though that's really more a tangential point.

Having said that, in retrospect it probably would have made sense to call the article "Growing Software Developers" instead of "Mentoring Software Developers" since there isn't really anything here specific to mentorship. Fair 'nuff.

I saw the problem but maybe I'm missing something - I'm not sure you present the solution too clearly - presumably you've dispensed with the service in the servlet and you are creating a new MyDAO for each request.

Hi Carl. No, the idea was that the servlet maintains a single instance-scoped reference to the service, and the service in turn maintains a single instance-scoped reference to the DAO. There's no reason to keep recreating the DAO because it's stateless (at least once you apply the fix).

well written! i'm going to direct a couple of engineers to this article :)

I would add to the Concrete Suggestions:

The best way to expose the value of an instance field is to encapsulate it in a public property instead of returning it from a public method. In the MyDao class, a public property encapsulating the private myData field would have made it obvious to a consumer of the object that the object was stateful. And it probably would have forced the developer of the MyDao class to reconsider either the readUserData method or the design of the class itself. (This is assuming that perhaps there was a reason for the myData property being defined at the class level in the first place). The exposed interface of the MyDao class was not well defined. There was disconnect between its internals and externals. I mean this as a corollary to the variable scoping suggestion. You should take care that your API implies its proper use and internal state.

I would probably counter the second concrete suggestion, however:

I would recommend starting out in an application assuming that any instance creatable object (in .Net a class can be marked as static) is stateful and shy away from putting the object in a static variable. Threading problems are difficult and are not something you are likely to catch on your developer machine during development when it's just you starting and stopping the application. It is also something that probably won't be seen in QA either with one tester on the app. And it is probably not something you would catch with load testing since the screens themselves are not viewed by anyone. However, performance degradation due to repeated expensive object creations is something you can likely catch in load testing and even sometimes during development with profilers, etc. So I am usually suspicious, myself, when I see objects kept in static variables since there will always be thread safety issues. If you were going to error in one direction I would error in viewing all instances of objects as stateful and create and release them per call. Then go back and deal with performance issues if any (and code maintenance!). In .Net, db connection pooling is handled by the framework itself (but I have even seen weird issues with connection pooling). So in some cases optimizations have been made at a lower level making them unnecessary at a much higher level such as a web page. But that probably depends on your platform. Performance problems are easier to catch than threading problems. And they're easier to solve since all you have to do is buy more hardware :)

I would further suggest that expensive calls should be optimized in base classes or libraries. So that by the time a junior developers comes along and uses them they are less likely to make bad mistakes. One way that senior developers can help junior developers is by writing base classes and libraries that can be used directly and serve as examples.

Perhaps the designer of the MyService class could have made it easier on the servlet developer if the methods had been made as static methods. That way the statelessness intent of the class could have been more apparent and the servlet developer would have known *exactly* how to use it.


Can a Java developer explain what this line is doing?

req.getRequestDispatcher("/WEB-INF/jsp/list.jsp").forward(req, res)

Hi David. Regarding the getRequestDispatcher() line, that's just how to forward a request to a specific page for viewing. The servlet processes the request and at the end forwards it off to a view. Most Java web frameworks simplify that piece.

Your first suggestion regarding public fields is pretty interesting. I've seen (and indeed follow) the same suggestion regarding something called "transfer objects" in J2EE. The idea in the TO case is to expose the field as part of the API to make it perfectly clear that setting a value on the TO won't affect the persistent instance:

http://java.sun.com/blueprints/corej2eepatterns/Patterns/TransferObject.html

Great additional ideas in your post.

This is really helpful for mid-level and junior-level developers to know the importance of Code Review.

We tend to curse the managers who wants to do a code review on the stuff we do... but rather we must realize that these were done for the beneficiary of the developers and the application.

Please provide any further gotchas you have come across your work experience.

Hi

I am trying to introduce code review in my team ,but havent found the right tool/method. It would be interesting to know what how and when others do it.

Regards

Peter

I'm aware of Checkstyle and PMD. I think that there is one called Jalopy too. Haven't used that.

Reading this article yesterday prompted me to get Java Concurrency in Practice off my bookshelf and start reading it. In it they also talk about FindBugs too. Haven't tried this yet. But thought you might find this useful too Peter.

@Peter: This response is late :-) but several years ago my team played around with an Eclipse plug-in called Jupiter. It's specifically designed to support code reviews. I don't remember the details of how it worked but I remember liking it quite a bit.

The various tools Jeremy mentions are all useful for code reviews as well. While they're not code review tools per se ( Checkstyle and Jalopy allow you to enforce code standards, while PMD and Findbugs are static analysis tools for uncovering bugs), the output of those tools?especially the static analysis tools?is very much the sort of thing that makes sense to discuss in code reviews.