I recently stumbled across a "lessons I've learned" style post. One of the lessons the author shares is to think very carefully about refactoring code, and I agree.

Put yourself into the shoes of the business or product owner and try to justify this rewrite. If this code is already in production, it’s already making someone money. Best case scenario, your newly rewritten code would do exactly what it already does from a feature perspective; worst case, and very commonly, the code is rewritten and features that customers use or that make money are were accidentally left out along the way. And hopefully you don’t introduce bugs.

I've fallen into that trap before. There's something alluring about consuming a piece of code and, confident I fully understand its purpose and shortcomings, rewriting it to be "better". The problem is, while "better" might be faster or more efficient, it's just as likely to just mean shorter or prettier. In other words, unnecessary. And sometimes, we find out later that we didn't understand its purpose as well as we thought. This is where writing tests can help immensely (hopefully by the original developer who understood the edge cases!), but even that won't catch everything.

So, I thought a little exercise might be fun for anyone who likes a coding challenge.

Here are 5 blocks of code, each in a different language. In each case, assume the first block of code (some_function_1) is the "correct" one, and that the second block of code (some_function_2) was refactored - for brevity or to use newer language constructs or just an attempt to make things clearer. (This doesn't mean that what the first block of code is trying to accomplish is the best way to do it... or even necessarily a good way to do it - some of them use side-effects).

Are they identical in each case? Can you find the bug that was unintentionally introduced? Hopefully you have fun with these... I had fun making them. And don't be afraid of refactoring, just be careful - it's necessary for a healthy codebase! :)

(A suggestion... try figuring each one out by reading first, especially if it's a language you're familiar with. Then check out the links to various online IDEs where the code is available to run.)

1 - C#

using System;
using System.Collections.Generic;
using System.Linq;
					
public class Program
{
	public static void Main()
	{
		var deductibles = new List<decimal> {2.47m, 4.13m, 1.33m};
		
		var t1 = TotalDeductibleDollarAmount1(deductibles);
		var t2 = TotalDeductibleDollarAmount2(deductibles);
		
    	Console.WriteLine(t1);
    	Console.WriteLine(t2);
	}
	
	public static int TotalDeductibleDollarAmount1(List<decimal> deductibles)
	{
		decimal total = 0;
		
		foreach (var deductible in deductibles)
		{
			total += deductible;
		}
		
		return Convert.ToInt32(total);
	}
	
	public static int TotalDeductibleDollarAmount2(List<decimal> deductibles)
	{
		return (int)deductibles.Sum();
	}
}

Try it out: .NET Fiddle

2 - Ruby

def is_user_non_admin_it_1(level, department) 
  not is_user_admin(level) and is_user_infotech(department)
end

def is_user_admin(level)
  level == :admin or
  level == :exec or
  level == :sysadmin
end

def is_user_infotech(department)
  department == :it or
  department == :is or
  department == :devops
end

def is_user_non_admin_it_2(level, department) 
  not [:admin, :exec, :sysadmin].include?(level) && [:it, :is, :devops].include?(department)
end

puts "TEST 1:\r\n-------"
puts is_user_non_admin_it_1(:admin, :it)
puts is_user_non_admin_it_1(:normal, :it)
puts is_user_non_admin_it_1(:admin, :finance)
puts is_user_non_admin_it_1(:normal, :finance)

puts "\r\nTEST 2:\r\n-------"
puts is_user_non_admin_it_2(:admin, :it)
puts is_user_non_admin_it_2(:normal, :it)
puts is_user_non_admin_it_2(:admin, :finance)
puts is_user_non_admin_it_2(:normal, :finance)

Try it out: RubyFiddle

(This StackOverflow thread may help too.)

3 - Python

Grades = {1:'F', 2:'D', 3:'C', 4:'B', 5:'A'}
Pass = {True: 'passed', False: 'failed'}

def print_grade_message1(grade):
    print("Your \"" + Grades[grade] + "\" means you " + Pass[grade != 1] + "!")

print_grade_message1(5)
print_grade_message1(3)
print_grade_message1(1)

print ""


GradingSystem = {1:'F', 2:'D', 3:'C', 4:'B', 5:'A', True: 'passed', False: 'failed'}

def print_grade_message2(grade):
    print("Your \"" + GradingSystem[grade] + "\" means you " + GradingSystem[grade != 1] + "!")

print_grade_message2(5)
print_grade_message2(3)
print_grade_message2(1)

Try it out: Python Fiddle

4 - Java

class JavaSample
{
    public static void main (String[] args)
    {
        System.out.println("1 - " + is_same_basic_ascii_character_1('D','D'));
        System.out.println("1 - " + is_same_basic_ascii_character_1('®','®'));
        
        System.out.println("2 - " + is_same_basic_ascii_character_2('D','D'));
        System.out.println("2 - " + is_same_basic_ascii_character_2('®','®'));
    }
	
    public static boolean is_same_basic_ascii_character_1(int first, int second)
    {
        Integer firstCode = first;
        Integer secondCode = second;
        
        return firstCode == secondCode;
    }
    
    public static boolean is_same_basic_ascii_character_2(int first, int second)
    {
        return first == second;
    }
}

Try it out: ideone

5 - Erlang

-module(listcomp).

-export([get_commissions_1/0, get_commissions_2/0]).

%%% External Functions

get_commissions_1() ->
    Names = load_names(),
    Commissions = load_commissions(),
    Report = build_report(Names, Commissions, []),
    {success, lists:reverse(Report)}.

get_commissions_2() ->
    {success, [Name ++ " has earned $" ++ proplists:get_value(Name, load_commissions(), "0") ++ "." || Name <- load_names()]}.

%%% Internal Functions

build_report([], _Commissions, Report) ->
    Report;
build_report([Name|Names], Commissions, Report) ->
    Commission = proplists:get_value(Name, Commissions, "0"),
    build_report(Names, Commissions, [Name ++ " has earned $" ++ Commission ++ "." | Report]).

%%% Database Accessors ... use your imagination

load_names() ->
    % load names from database.. takes half a second
    timer:sleep(500),
    ["Bob", "Charlie", "Kelly", "Mary", "Sue", "Tom", "Zeke"].

load_commissions() ->
    % load commissions from database.. takes a second
    timer:sleep(1000),
    [{"Bob","3000"}, {"Kelly","11000"}, {"Sue","8000"}, {"Tom","15000"}].

%%% Unit Tests

-include_lib("eunit/include/eunit.hrl").

get_commissions_test_() ->
    {setup,
     fun() ->
         ["Bob has earned $3000.", "Charlie has earned $0.", "Kelly has earned $11000.",
          "Mary has earned $0.", "Sue has earned $8000.", "Tom has earned $15000.",
          "Zeke has earned $0."]
     end,
     fun(ExpectedValue) ->
         [
             {timeout, 10, ?_assertEqual({success, ExpectedValue}, get_commissions_1())},
             {timeout, 10, ?_assertEqual({success, ExpectedValue}, get_commissions_2())}
         ]
     end}.

Try it out: codingground

The above code (and the compiled beam file) are on the left. The bottom half of the screen is the shell. Start up an erlang shell with erl, then try out the functions with listcomp:get_commissions_1(). and listcomp:get_commissions_2(). Run the tests with listcomp:test().