Archive | January 2017

Dev Team Interactions: Conducting Good Code Reviews

In part 2 of my series on dev team interactions, I’d like to talk about conducting good code reviews. Most dev teams will find themselves in a situation where code reviews are necessary, and in my experience many do them very poorly. I’ve even worked in companies that had such a negative code review culture that people left the review sessions upset, even considering quitting. With a few easy adjustments, you can quickly learn to conduct excellent and positive code reviews with your team.

The Ground Rules

A code review is a process. Like any good process, clear rules need to be established and followed to ensure a consistent experience. Here are mine:

  • Attack the code, never the person. Criticizing code is OK, but people are not code. It’s never OK to criticize the person and make them feel bad. Focus strictly on the code output and never make it personal.
  • Don’t laugh or make negative jokes. A person who has their work on display – often on a projector in front of others – is feeling self-conscious as it is. Don’t snicker at their work. Avoid joking about their decisions. I assure you they are trying to do the right thing.
  • Set a strict time limit as a means of focus. Make the code review 15 minutes, 30 minutes, or even 1 hour. Stick to this schedule. This forces you to prioritize the important stuff and ensures (intentionally) that you can’t review absolutely everything the person has done. Don’t take unlimited time to scrutinize every single line of code written. Ever had someone comb through your code line by line, making commentary as they go? “And how did that make you feel?”
  • Thank the person for sharing their code with you. A code review is an intimidating, scary thing – especially for developers on the junior side of the spectrum. Set the tone correctly by being appreciative of their time and sharing. Make it known that they are valued and you appreciate their work up front. This will help them feel relaxed and learn to enjoy code reviews, which in turn will cause them to want to share more of their code willingly in the future as well.

The above rules serve only as an example from my personal experience. You should create rules that work for your company and work culture. Use real and practical standards that matter to your team, not just theoretical ideologies that someone on your team read in a book. Always remember: a process is only as good as the people that follow it, so try to be consistent with whatever rules you decide on. And if they aren’t working well in practice, change them up!

Seek Understanding, Not Power

The general goal of your code review attendees should be to seek understanding, not explanations. Avoid an us-them conflict or standoff. This can be easily accomplished with a subtle shift in communication approach. Rather then asking aggressive questions that demand the reviewee explain the code to your team such as “why would you do it that way?” or “how could that possibly work?” you need to ask questions to promote understanding the code instead. Once your team and the reviewee correctly understand the code being reviewed, you can proceed to discuss a different approach without conflict or hurt feelings.

When you see some code that makes you think “what the hell were you smoking?” (explain) you should instead ask “Can you tell me why you wrote the method this way?” (understand). These statements are similar but the former gives the power to you while the latter empowers the code reviewee to share their knowledge and thought process in a non-defensive way. This approach to questioning takes a bit of practice, but is very powerful. Some other examples of how you’d change common code review aggressive thoughts and statements from explaining to understanding:

  • “This class name is wrong” becomes “This class name doesn’t conform to our standards, was that intentional?” You’ll probably find that they say it wasn’t, and agree to fix it on their own volition.
  • “I see a bug in your code!” becomes “I think there’s a null reference exception on line 28 for variable X, do you see how it happens?”
  • “This is terrible” becomes nothing – keep your mouth shut. There’s no value in such a statement other than to make the person feel bad. If the code truly is terrible, express these concerns to the person (and maybe their manager) privately for further investigation and resolution.

People are not robots, and will never conform perfectly to your shop’s coding standards. That’s OK. Pick your battles, and call out only the major violations. Let the little things (like naming and spacing) slide. If the dev is hitting 90% of the standards, the rest of the team can pick up on the 10% deviation without much worry or effort. That is to say, the code won’t be that different than what they expect to see.

If you seek understanding consistently, you’ll find that the person drops their defenses and ego, and instead feels encouraged by your positive approach and attitude. They’ll even start pointing out and suggesting fixes to their code themselves – right on the spot – which is the golden sign of trust and confidence. The best kind of code reviews are the ones that a person points out issues themselves to fix, rather than the team having to do it for them.

Avoid Opinion Wars

When reviewing code, there will be two general categories of issue:

  1. Objective, fact-based issues
  2. Subjective, opinion-based issues

Focus explicitly on objective issues, and disregard all subjective issues. An objective issue is an exception or oversight in conforming to well-defined coding standards. A subjective issue is you not liking the way the person solved the problem, or feeling that what they did is not what you would have done. You’re right because you’re different people. Striving to make others conform to your thought process as the one true standard is egotistical, destructive, and stressful. In the long run you will be unhappy because there will always be a gap between others and yourself. This is natural so go with the flow. Allow members of your team to be individuals and write code with their own flair and flavor. It’s OK, it will be OK, and you will be OK. I promise.

Be Humane

Code is for computers, but programmers are humans. Be kind to each other and always remember that the thing on the other end of the code review is a real live person with feelings and emotions. Treat them with respect in what you say and do. Recognize that they are valuable and thank them for their hard work. Together you will create a great team that others will love being a part of!

Dev Team Interactions: Accountability & Blame

As a developer working for a company, you probably work on a team. The interactions on these teams are sometimes pleasant, and other times hostile. What’s interesting to me is that a lot of the time, a hostile interaction could have been a pleasant one if only approached differently. Hostile teams are created by the actions of the people on them, not by the situations they encounter. One such hostile action is blame.

Blame

Blame is assigned externally – it comes from other people. You cannot control blame when it is directed at you. Blame tends to surface under tense circumstances, such as when the build has broken or a project has failed. It often rears its ugly head in the form of pithy and simplistic statements. “Jane broke the build again!” or “this is Mohinder’s fault!”

Blame is often facilitated – if not encouraged – by a poor workplace culture. When the patterns and practices of the management team demand that heads roll when mistakes are made, blame is the obvious solution. It allows you shift the burden of responsibility to someone else, forcing them to defend themselves from an attack while you retreat to safety.

Blame Never Helps Anything

Blame may solve the immediate problem of your head being on the proverbial chopping block, but it never solves the actual problem at hand. If anything it tends to distract from the issue as the discussion changes from “what went wrong” to “who caused this?” A challenging situation becomes even more complicated as strong negative feelings start to emerge.

Another effect of blame is that people feel alienated and attacked. Team members disengage, afraid to speak up or make changes because they may be attacked for doing so. In this way it damages the innovation and progression of your people, product, and entire company. Blame holds your entire organization hostage.

It’s not hard to see that a workplace which embodies blame culture is really the root problem in-and-of itself.

Accountability

Unlike blame, accountability is assigned internally – it comes from within. It is a positive force of empowerment that requires courage and compassion. Accountability allows people to take control over themselves and their own actions. It destroys “fault” culture while building trust and communication channels with other team members. Statements like “that was my fault team, sorry about that” are welcomed and encouraged, but not necessary.

Accountability can only thrive if the workplace culture is tolerant and accepting. In a blame culture, accountability cannot exist. The mere act of owning up to a mistake assigns the blame to you, and scrutiny and / or punishment is sure to follow. A good work culture is intolerant of blame, instead focusing on people learning from their mistakes and growing as professionals.

A Blameless Workplace

My personal take on accountability as a manager is that “everybody makes mistakes, and as long as you make new and interesting mistakes – rather than repeating them – you have nothing to worry about.”

Mistakes are normal; we all make them regularly. A workplace culture that understands this fact and incorporates it into professional and personal development is one that succeeds with vibrant and engaged teams. Allow people to make mistakes, and follow-up only if they do not learn and grow from those mistakes. Shut down any and all blame conversations immediately. Communicate the difference between assigning blame to someone for their mistakes, and taking ownership of those mistakes themselves. Foster a workplace culture that is blameless.

At Stack Overflow, we do this via a tool that we call the Wheel of Blame. When something goes wrong, anyone in a company chat room can say “not my fault” to spin the wheel. The wheel arbitrarily picks a person from those present in the room and blames them. With blame quickly and definitively assigned, we then move on to fixing the problem.

The Wheel of Blame in action. I'm bad at making gifs so it's not quite right but whatever, I tried.

The Wheel of Blame in action. I’m bad at making gifs so it’s not quite right but whatever, I tried.

Creating Accountability

Despite the Wheel assigning blame – which clearly goes against the blameless culture which I described earlier – this tool is very powerful. It’s a clear and obvious signal to seasoned and new employees alike that we don’t care about blame. When someone is randomly assigned, it’s almost always the case that the problem wasn’t actually their fault. More often than not they had nothing at all to do with the current situation. An ironic and hilarious side effect of the Wheel is that when it does randomly assign the blame to the person who is accountable to the issue, it results in uncontrollable laughter and joking. This alone makes the Wheel worth using: it lightens the mood and communicates our rules around blame in a tribal way (there is no policy written anywhere).

To shift a culture from blame to accountability, begin by shutting down blame discussions. Explain that blame is not helpful, and that the team needs to immediately focus on resolving the issue at hand. Once the problem is solved, consider doing a retrospective which explains what went wrong and who was involved. Circulate this retrospective to any and all interested parties with the clear intention of spreading information, not assigning blame. This allows people to learn and grow from what happened. Continuing this pattern will slowly shift the team away from blame. The trick here is that you don’t have to work on accountability: it will naturally follow when people stop fearing blame.

I encourage you to work towards a culture of accountability on your development team, starting today. Sometimes this transformation can be very difficult to accomplish; particularly when the blame culture starts at the executive level. Nonetheless, do what you can to change the behavior in the part of the company that you influence. Your peers and reports will thank you for it.

Custom ASP.NET MVC Action Result Cache Attribute

If you’re working on an application built using ASP.NET MVC, you’re hopefully aware of the OutputCacheAttribute attribute which can be used to statically cache your dynamic web pages. By adding this attribute to a controller or action method, the output of the method(s) will be stored in memory. For example, if your action method renders a view, then the view page will be cached in memory. This cached view page is then available to the application for all subsequent requests (or until the item expires out of the cache), which can retrieve it from the memory rather than redoing the work to re-create the result again. This is the essence of caching: trading memory for performance.

The OutputCacheAttribute is a really powerful way to improve performance in your MVC application, but isn’t always the most practical. Because it caches the entire page as raw HTML, it circumvents a large part of the MVC pipeline and thus also skips the code that runs to generate the page. This means that if your view has dynamic content that comes from session or ViewData, such as displaying the currently logged in user’s name in the top bar, or the current time of day, or the resulting view of an invalid form post which tells your user to correct their input errors, you’ll quickly discover the error of your ways when you try to cache that page. When David accesses the logged in page for the first time and caches it, everybody else who logs in will be called David on the page. And if David fills out your empty form and presses submit, only to cache the resulting input validation error page, then everybody will see David’s completed form when they have errors too – maybe even including sensitive data like his username, password, or even his credit card information. I think that most of us have seen this kind of (often humorous) caching error before. It’s scary stuff, nonetheless.

A great way to balance the benefits of output caching with the dynamic content and features that the modern ASP.NET MVC web application offers is to create a custom caching attribute. This attribute can cache the ActionResult instead of the raw HTML of the page, and in doing so will allow you to cache all of the work that is done to generate the ActionResult (be it ViewResult or otherwise). By executing within the MVC pipeline, this custom caching attribute will not interrupt or short-circuit the MVC pipeline. This allows for things like SessionState or ViewData to vary per cached request! It’s not quite as efficient as the true OutputCacheAttribute, but my custom ActionResultCacheAttribute is an excellent tradeoff between performance and dynamic data:

/// <summary>
/// Caches the result of an action method.
/// NOTE: you'll need refs to System.Web.Mvc and System.Runtime.Caching
/// </summary>
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Class, AllowMultiple = false, Inherited = false)]
public class ActionResultCacheAttribute : ActionFilterAttribute
{
    private static readonly Dictionary<string, string[]> _varyByParamsSplitCache = new Dictionary<string, string[]>();
    private static readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();
    private static readonly MemoryCache _cache = new MemoryCache("ActionResultCacheAttribute");

    /// <summary>
    /// The comma separated parameters to vary the caching by.
    /// </summary>
    public string VaryByParam { get; set; }

    /// <summary>
    /// The sliding expiration, in seconds.
    /// </summary>
    public int SlidingExpiration { get; set; }

    /// <summary>
    /// The duration to cache before expiration, in seconds.
    /// </summary>
    public int Duration { get; set; }

    /// <summary>
    /// Occurs when an action is executing.
    /// </summary>
    /// <param name="filterContext">The filter context.</param>
    public override void OnActionExecuting(ActionExecutingContext filterContext)
    {
        // Create the cache key
        var cacheKey = CreateCacheKey(filterContext.RouteData.Values, filterContext.ActionParameters);

        // Try and get the action method result from cache
        var result = _cache.Get(cacheKey) as ActionResult;
        if (result != null)
        {
            // Set the result
            filterContext.Result = result;
            return;
        }

        // Store to HttpContext Items
        filterContext.HttpContext.Items["__actionresultcacheattribute_cachekey"] = cacheKey;
    }

    /// <summary>
    /// Occurs when an action has executed.
    /// </summary>
    /// <param name="filterContext">The filter context.</param>
    public override void OnActionExecuted(ActionExecutedContext filterContext)
    {
        // Don't cache errors
        if (filterContext.Exception != null)
        {
            return;
        }

        // Get the cache key from HttpContext Items
        var cacheKey = filterContext.HttpContext.Items["__actionresultcacheattribute_cachekey"] as string;
        if (string.IsNullOrWhiteSpace(cacheKey))
        {
            return;
        }

        // Cache the result of the action method
        if (SlidingExpiration != 0)
        {
            _cache.Add(cacheKey, filterContext.Result, TimeSpan.FromSeconds(SlidingExpiration));
            return;
        }

        if (Duration != 0)
        {
            _cache.Add(cacheKey, filterContext.Result, DateTime.UtcNow.AddSeconds(Duration));
            return;
        }

        // Default to 1 hour
        _cache.Add(cacheKey, filterContext.Result, DateTime.UtcNow.AddSeconds(60 * 60));
    }

    /// <summary>
    /// Creates the cache key.
    /// </summary>
    /// <param name="routeValues">The route values.</param>
    /// <returns>The cache key.</returns>
    private string CreateCacheKey(RouteValueDictionary routeValues, IDictionary<string, object> actionParameters)
    {
        // Create the cache key prefix as the controller and action method
        var sb = new StringBuilder(routeValues["controller"].ToString());
        sb.Append("_").Append(routeValues["action"].ToString());

        if (string.IsNullOrWhiteSpace(VaryByParam))
        {
            return sb.ToString();
        }

        // Append the cache key from the vary by parameters
        object varyByParamObject = null;
        string[] varyByParamsSplit = null;
        bool gotValue = false;

        _lock.EnterReadLock();
        try
        {
            gotValue = _varyByParamsSplitCache.TryGetValue(VaryByParam, out varyByParamsSplit);
        }
        finally
        {
            _lock.ExitReadLock();
        }

        if (!gotValue)
        {
            _lock.EnterWriteLock();
            try
            {
                varyByParamsSplit = VaryByParam.Split(new[] { ',', ' ' }, StringSplitOptions.RemoveEmptyEntries);
                _varyByParamsSplitCache[VaryByParam] = varyByParamsSplit;
            }
            finally
            {
                _lock.ExitWriteLock();
            }
        }

        foreach (var varyByParam in varyByParamsSplit)
        {
            // Skip invalid parameters
            if (!actionParameters.TryGetValue(varyByParam, out varyByParamObject))
            {
                continue;
            }

            // Sometimes a parameter will be null
            if (varyByParamObject == null)
            {
                continue;
            }

            sb.Append("_").Append(varyByParamObject.ToString());
        }

        return sb.ToString();
    }
}

You can use this method on a controller to affect all action methods:

[ActionResultCache(Duration = 60 * 60 * 24)]
public class HomeController : Controller
{
    public async Task<ActionResult> TermsOfService()
    {
        return View();
    }
}

Or just apply it to individual action methods:

[ActionResultCache(Duration = 60 * 60 * 24)]
public async Task<ActionResult> TermsOfService()
{
    return View();
}

You can also use it with the VaryByParam property to vary the cached result by the parameter(s) of the action method:

[ActionResultCache(Duration = 60 * 60 * 24, VaryByParam = "username")]
public async Task<ActionResult> ViewUser(string username)
{
    var model = new UserModel
    {
        Username = username,
        ...
    };

    return View(model);
}

The main benefit of this custom caching attribute is that your session state and all global action filter attributes, etc. still run in the MVC pipeline as they would normally. The only code cached and skipped over is the method body of the action method.

Please use and enjoy! Feedback welcomed in the comments.

Our Industry Needs Compassion

Well, I’ve utterly failed to blog at regular intervals, writing only three posts in 2016. Ouch. To be fair, one of those posts is insanely famous (the one about NPM and left-pad.js), but still, I’ve really let my readers – and myself – down.

So, I resolve to write a blog post every single week of 2017, starting today. This will probably mean that I write slightly shorter posts, and maybe even multi-part series posts. My traditional style has been “come upon something that is really bothering me or is really tricky, and proceed to blog about it in great detail writing thousands of words for all to benefit from” which doesn’t really scale well. Instead I plan to take the approach of “write about a new or interesting topic each week, and see what people like and what they don’t like” which will hopefully be better.


This week’s post is about compassion, especially in the field of programming. Let’s use an example that is both recent and practical, if a tad emotional.

3 days ago our dog Ruby died. She was 16 and lived a good, long life. Having said that, her death was unexpected and rather painful to go through. My wife and I were up with Ruby all night comforting her as she slowly passed away from sudden heart failure. The last breath she exhaled will forever be seared into my brain. It feels like it’s all I can think about right now.

My wife and I have been grieving for the past 72 hours in our own ways. This has surfaced as mostly a mix of depression, tears, quick but sad chats about the things we miss about Ruby, and sorrow every time we look somewhere in the house for her only to find that she is no longer there. It sucks.

What’s important to realize is that I am not unique or special in this circumstance. Animals die all of the time, and you don’t feel sad about them because you never met them. They are out of sight, out of mind. Yet they are out there, and the passing of good dogs and cats – as well as other households pets – is happening every single day. Even as you read this.

These events occur in the scope of individual’s lives. There is no national news story about the passing of Ruby to make us all aware of it and help us all feel the effects of it. There never will be. This event is confined to the lives of me, my wife, and our extended family. We suffer through it silently (other than my writing about it and a few social media posts).

This silent suffering is the point that I want to drive home. Others out there are silently suffering too. It might be the death of a family pet, or it might be something else entirely. It’s not a contest of who has it the worst. I want you to simply realize that crappy things are happening to people, even if they are not in your domain and thus you are not aware.

Odds are that someone in your life or workplace is suffering with something that troubles them. Right now for me it’s the passing of my dog. For a close friend of mine it’s stress about work and finding the next permanent job. For another person I know it’s about being a brown man in America and how scared they feel about the future. We all battle demons every single day.

With this in mind, I ask you to be compassionate everywhere, but especially at work. You work with people, not cogs of some machine (even if management may think of them in that way). These people have real feelings and some of them are surely fighting battles you’ve never even fathomed.

Your responsibility as a good member of the IT field – and as a good citizen of this planet – is to demonstrate compassion. Be nice to everyone you work with until you are given a reason not to be. Consider that they might be having a bad day if you feel they’re lashing out or being an asshole, and forgive them. Try to sympathize with what frustrates them. Work to understand them beyond their role at the company and their job output.

We are all in this together. Now more than every we need to be compassionate, respectful, kind, and considerate to others. It is my opinion that our industry has a long history of being hostile to certain people and demographics – such as women – and we must each work individually to change that.

My last request to you: please think hard about what you’ve read here as you begin your first week of work in 2017. I know I will.