Home → Articles →

Photo credit: Nrbelex

1 | 2 | 3 | 4 | Next »

IT Management

Mentoring Software Developers

by Willie Wheeler

Wednesday, September 10, 2008

Some ideas on helping junior and midlevel software developers become more effective.

At my day job, I'm in software development management. From time to time my teams do code reviews, which I've always found to be enlightening. I see code reviews not only as a chance to help developers grow, but also as a chance for managers and senior engineers to better understand what trips developers up. It's important for technical leadership to understand that, since one of our responsibilities is to improve our teams.

I've observed that many developers struggle with the fundamentals. For various reasons, people put a lot more energy into learning new frameworks, APIs, technologies and so forth, and less into understanding core software engineering concepts and principles. People think it's more impressive to say that they know ActionScript and Hibernate than to say that they understand the conceptual (not merely textbook) difference between protected and package protected visibility in Java. No doubt that emphasis is reinforced by the behavior of technical recruiters and hiring managers.

Recently we had a production issue resulting from some buggy code. Instead of immediately telling you about the issue itself, let me show you a simplified version of the code so you can try to find the bug.

Can you find the bug?

Try to find the bug in the following code. I'm not talking about matters of style such as not using dependency injection. There's an honest-to-goodness bug in the following code, one that will lead to production issues with users calling tech support and all that.

First, here's our servlet:

Code listing: MyServlet.java

package demoapp;

import java.io.IOException;
import java.util.List;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

public class MyServlet extends HttpServlet {
    private static MyService myService = new MyService();
    
    public void doGet(HttpServletRequest req, HttpServletResponse res)
        throws IOException, ServletException {
        
        Long userId = (Long) req.getSession().getAttribute("userId");
        List userData = myService.getUserData(userId);
        req.setAttribute("userData", userData);
        req.getRequestDispatcher("/WEB-INF/jsp/list.jsp").forward(req, res);
    }
}

Here's our service object:

Code listing: MyService.java

package demoapp;

import java.util.List;

public class MyService {
    private MyDao myDao = new MyDao();
    
    public List getUserData(Long userId) {
        return myDao.readUserData(userId);
    }
}

And finally here's our simple data access object:

Code listing: MyDao.java

package demoapp;

import java.util.List;

public class MyDao {
    private List myData;
    
    public List readUserData(Long userId) {
        this.myData =
            ...do some JDBC, process the result set, get the data...
        return this.myData;
    }
}

See the bug? If not, that's OK. You'll have a second chance since I'm going to tell you about the production issue before I reveal the bug.

Here's a hint

On Friday we released the software to lots of users who hadn't been previously using it. Over the weekend users started calling tech support reporting that wrong data was showing up on their screens. The CIO himself observed the issue.

See it now?

1 | 2 | 3 | 4 | Next »

Comments (17)

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.

By Collin on Sep 10, 2008 at 11:42 AM PDT

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.

By richard zhou on Sep 10, 2008 at 1:00 PM PDT

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

By Umesh Gohil on Sep 10, 2008 at 2:56 PM PDT

@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

By Willie Wheeler on Sep 10, 2008 at 4:52 PM PDT

Code Review is not mentoring. Nice try

By bob on Sep 10, 2008 at 6:56 PM PDT

@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.

By Willie Wheeler on Sep 10, 2008 at 8:18 PM PDT

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.

By Carl on Sep 11, 2008 at 2:18 AM PDT

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).

By Willie Wheeler on Sep 11, 2008 at 8:59 AM PDT

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

By Rob Taylor on Sep 11, 2008 at 8:30 PM PDT

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)

By David Morgan on Sep 12, 2008 at 9:13 AM PDT

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.

By Willie Wheeler on Sep 14, 2008 at 8:37 PM PDT

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.

By Madan N on Sep 30, 2008 at 12:10 PM PDT

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

By Peter Szanto on Jan 25, 2009 at 3:08 PM PST

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

By Jeremy Flowers on Mar 2, 2009 at 9:58 AM PST

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.

By Jeremy Flowers on Mar 4, 2009 at 12:02 AM PST

@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.

By Willie Wheeler on Mar 26, 2009 at 12:05 AM PDT

he myth of pandora is ancient, appears in several distinct Greek versions, pandora armbandand has been interpreted in many ways. In all literary versions, Neu Eingetroffen however, Pandora Armb�nder the myth is a rosetta stone kind of theodicy, addressing the question pop information, web easy get, sports fashion, news-fashionof why there is evil in the world. In the seventh hot-winter century BC, Hesiod, both in his Theogony (briefly, without naming Pandora outright rosetta stone language, rosetta stone spanish, abercrombie and fitch, Abercrombie Fitch

By pandora schmuck on Aug 30, 2010 at 11:00 PM PDT

Mentoring Software Developers

Can you find the bug?

Here's a hint

Comments (17)

Post a comment

What's New?