Looks good to me ;)

Ouch, that's not exactly a skills showcase. I presume this wasn't somebody involved in OSS projects? :)

Apart from the invalid code (which is perhaps translated from vb.net to c#???), the candidate used hard coded values all over the place and she did not recognize there is a lot of repeated code snippets. This is also an indication of the level of the candidate.

// Ryan

PS - The method results no value so it won't compile.

LOL!

// Ryan

I think you are not allowed to use < in a case statement ... and of course missing the return value - that's why the code is not compiling!

@Ayende Maybe the candidate did receive a better offer and didn't want to hurt your feelings by refusing yours :)

It seems like if he/she would use specification pattern maybe ayende would consider hiring him/her... It is indeed interesting how much information you can get from a piece of code ....

I'm going to stick my neck out here, I know it does not compile and repeats logic etc but it is nearly there...

The big thing for me is I can understand what it should do by looking at it and if you refactored it to get rid if the repeated code you might lose some of that.

Would I employ them, nope. Would I write code like that, maybe (but it would compile and pass tests).

Perhaps someone has a nice refactored version which they can compare it to and I'll see the error of my ways.

What was his/her pet project?

Wow...and sigh, could not resist. did the person mean something like this?

https://gist.github.com/1234461

This would make a pretty good post for The Daily WTF.

@Ryan O'Neill

What I would prefer is that it should be easy to alter ranges, percentages or add new ranges without having the need to alter (some or to much) code.

// Ryan

Even if you don't consider the invalid syntax and the lack of a return value, this is a very naive approach to the problem... clearly the work of a beginner programmer.

I feel your pain!

I'm currently interviewing at work and I had a candidate copy the wrong solution from the internet and then try and explain why he choose it without event understanding what he had.

That is a prank.

It doesn't compile -- er, maybe the candidate did not had a computer !?

She knew that she not expected any return call.

Ayende, before giving her an interview, did you evaluate her passion based on open source contributions? :)

LOL.. Don't underestimate the way of expressing the solution. :P

I interviewed about 8 people for a position requiring "advanced sql" and "advanced C#" skills. All eight had 5+ years of sql/C# (if you believe their resumes). 7 out of 8: 1) Did not know what a self-join was 2) Used only webforms, but did not know how postbacks, page lifecycle, or viewstate worked 3) had "heard about" MVC 4) Did not have any personal projects in coding 5) had beginner-or-less javascript knowledge

FLOAT GetTaxs( FLOAT _salary ) { FLOAT _tax = 0.0f; //change to match country taxes if( _salary > 0.0f && _salary <= 600.0f ) _tax = 0.0f; else if( _salary > 600.0f && _salary <= 10000.0f ) _tax = 3.0f; else if( _salary > 10000.0f && _salary <= 50000.0f ) _tax = 5.0f; else _tax = 7.5f;

return ( _salary * _tax ) / 100.0f; }

can i keep the job?

Hey, Peter!

A Question from other side of the barricades of hiring (I am a candidate, Passionate .net, 5yrs).

Were you able do distinguish 8th candidate from the rest, solely by his resume? What part of the resume would help you to do that?

I had a candidate who had to do a very simple assignment in the office. The goal was to display some data from an existing database table in a pre-defined HTML/CSS template using ASP .NET Web Forms or MVC (their choice).

She chose MVC because "she loves new technology". After about 40 minutes she told me that she cannot complete the assignment because the Visual Studio on the PC was broken. What happened was she could not drag-and-drop server controls onto an MVC view.

To be fair, Ayende doesn't say how old she is or what experience she has. She could be a student who's been taught badly - you never know. Still doesn't excuse the code that doesn't compile.

Frank, Love that solution for the taxes -- about the only thing I'd want to change is to make:

var taxedSlice = salary - progressionSlices[progression] > 0 ? progressionSlices[progression] : salary;

into: var taxedSlice = Math.min(progressionSlices[progression], salary);

But I'd absolutely hire you off that -- so much better than the vast majority of the 'code' that comes through.

Ayende, What's your pre-intervew process look like? I'll be honest that I'm a bit surprised that someone who writes code like this got to an in-person interview. When doing hiring for my dept, I put a lot of effort into weeding out candidates before they make it to the office. An in-person interview is expensive (4-6 hrs of me and my team's time). The half-hour phone screen or having them submit code or complete a written test beforehand saves us a lot of time and lets us focus on the worthwhile candidates.

@Valera

My comment was based on the interviews. I only mentioned their resumes since they all had held coding jobs for 5+ years. We do parse resumes also, mainly to see if they jump around too much from job to job. We also did a phone interview first before calling them in. The person we ended up hiring made a very nice derived table solution to my little simple sql problem, and had very definite ideas about the proper way to code. I especially liked him since he agreed to disagree with me about certain coding conventions. He was actually older than the others (around 60 I would estimate, but I didn't ask).

Another minor (in the scheme of things) nitpick:

case < 5070: /* ... / case < 8660: case > 5071: / ... */

So...What happens if salary == 5070 or salary == 5071

Dont tell me if it wouldn' be nice have that sintax on C# XD

@Andy -- wouldn't that be caught by "case < 8660" though?

Here's a functional stylee version! https://gist.github.com/1235276

Grrr, again. Posting comments really needs a preview. (feel free to nuke my previous attempts).

let taxOf = function
	| x when x < 5070m -> 0.1m * x
	| x when x < 8660m -> 0.14m * x
	| x when x < 14070m -> 0.23m * x
	| x when x < 21240m -> 0.3m * x
	| x when x < 40230m -> 0.33m * x
	| x -> 0.45m * x
	
taxOf 10000m

Am I the only one here who feels bad for her? She probably reads this blog, and her code is being mocked.

I generally respect Ayende, and even enjoy his curmudgeonly informative attitude, but does this post really serve any purpose outside of venting some frustration?

There's not a doubt in my mind that she knew the code was poor, but for some reason or other felt pressured to submit it.

I personally would consider it a breach of trust if someone I was interviewing for posted my code as an example of what not to do.

at least she have a pet project. otherwise you was not call her for interview...

Hmm... From a readability standpoint, am I the only one who doesn't like Frank's solution at all?

If I was a developer who nothing about progressive taxing and had to come along and make a change to this code, I think that this implementation would confuse me even more.

The most glaring problem to me was that the solution had 2 independent arrays with the values declared. This would immediately lead me down a path of thinking that those arrays could change independently. Not until I went through multiple iterations of the examples in my head did it click that the code assumed you knew how the feature worked and that there is a need to keep the 2 arrays "in sync" by index, otherwise none of the logic works.

To me, those are some of the most frustrating kinds of issues to deal with, where you have to spend time realizing a developer assumption/mistake before you can see what the code is trying to accomplish.

Here's an implementation using the LINQ extension methods. It's a bit hard to read if you ask me.

    public Decimal CalculateTax(Int32 salary)
    {
        var taxData = new[]
        {
            new { Rate = 0m,   Cap = 0 },
            new { Rate = 0.1m, Cap = 5070 },
            new { Rate = 0.14m, Cap = 8660 },
            new { Rate = 0.23m, Cap = 14070 },
            new { Rate = 0.3m, Cap = 21240 },
            new { Rate = 0.33m, Cap = 40230 },
            new { Rate = 0.45m, Cap = Int32.MaxValue },
        };

        return taxData
            .Zip(taxData.Skip(1), (prev, current) => new { Rate = current.Rate, PreviousCap = prev.Cap, BracketSize = current.Cap - prev.Cap })
            .Select(x => Math.Min(Math.Max(salary - x.PreviousCap, 0m), x.BracketSize) * x.Rate)
            .Sum();

    }

Like a simpleton my previous example only calculated basic tax rate. F# is still a good choice for this, here is an example that calculates cumulative tax rate. It uses currying to compose higher order functions with different tax rates:

Gist here: https://gist.github.com/1236106

//generic routine to calculate tax
let taxOf salary taxRates = 
	taxRates 
		|> Seq.mapi(fun i (rate, band) -> 
			let prevBand = (if i > 0 then taxRates |> Seq.nth (i-1) |> snd else 0m)
			match salary with 
				| x when x > band -> (band - prevBand) * rate
				| x when x < prevBand -> 0m
				| x -> (x - prevBand) * rate
			)
		|> Seq.sum

//define custom tax bands and rates
let usTaxRates = [
	( 0.1m, 5070m); 
	(0.14m, 8660m); 
	(0.23m, 14070m); 
	( 0.3m, 21240m); 
	(0.33m, 40230m); 
	(0.45m, System.Decimal.MaxValue)]


//use currying to build a higher order function to calculate US Tax Rates
let taxOfUS salary = usTaxRates |> taxOf salary
				
taxOfUS 10000m  //= 1317.80

PhilB - Nice solution. I'd never heard of .Zip on IEnumerable before, opened my eyes.

Here's my earlier effort using a fold (it's called aggregate in LINQ for some reason)

public static decimal GetTaxes(decimal salary) { var taxBands = new[] { new Tuple<Decimal, Decimal, Decimal>(0, 5070, 0.1m), new Tuple<Decimal, Decimal, Decimal>(5070, 8660, 0.14m), new Tuple<Decimal, Decimal, Decimal>(8660, 14070, 0.23m), new Tuple<Decimal, Decimal, Decimal>(14070, 21240, 0.3m), new Tuple<Decimal, Decimal, Decimal>(21240, 40230, 0.33m), new Tuple<Decimal, Decimal, Decimal>(40230, Decimal.MaxValue, 0.45m) };

return taxBands.Aggregate(0.0m, (taxOwed, taxband) =>
    {
        if (salary < taxband.Item1)
            return taxOwed;

        if (salary < taxband.Item2)
            return taxOwed + ((salary - taxband.Item1) * taxband.Item3);

        return taxOwed + ((taxband.Item2 - taxband.Item1) * taxband.Item3);
    });

}

Wouldn't it be better to just have the tax rates in a table somewhere?

Publicly shaming someone like this is a bit of a prick move, sure the candidate might not be ready for the position and could have a long way to go, but if I were this candidate I would rather have someone tell me I might be doing the wrong thing and not in a public forum be made out to be an idiot.

The problem here is her version is quite readable apart from being non-working, and other versions show that problem is not so simple. And common it's not public shaming, because none of us will ever know who's code was it. If she reads this she will know what was wrong with her code and how to fix it. It won't make her better coder instantly but she will have far greater chances when applying for a job like this next time.

What Bob said. It's not even close to the worst code I've ever seen. At least 'she' knows how to fall through on case statements.

Besides who wants to write .NET code anyways? :P

Hi Ayende,

I personally think that anyone who posts blog entries like this should at least get their spelling and grammar correct before trashing the work of others.

Cheers, Steve

For all those who hard coded the tax table inside the method, remember, this is Ayende! Sure it compiles, but do you think he'll hire you for doing that? Either an instance field/property or better yet something that returns the tax rate based on variable parameters, such as year, state/province, country.

Your demonstration of professional developer behaviour through a public review of a person rather than a constructive critique of the code has been most... enlightening... and I wish you well on your continuing journey down the road of dealing with software and humans.

I habe to agree with Bob, Luke and the others.

This post is quite harsh and in no way constructive.

The post may or may not be harsh, but I disagree that it is not constructive: it produced some nice functional tax algorithms.

I feel extremely sorry for this person if she's read this post. You may have destroyed her confidence and affected her future in this industry.

As an HR Manager of a Software Development company who also uses code tests as part of our recruitment process I was interested to know if you discussed with the candidate why they failed or asked for their permission to post the submission to your blog.

Ayende,

Did you contact the person who submitted this and let them know what the problem was before posting it here? I would hate for them to see it on your blog without even hearing back from you.

Caleb

Dammit, the Dark Side of the Force got hold of Yedi Ayende again.

I hate these posts. Grow up.

Here is what would sell very easily to a business analyst at an American bank :

    private decimal calcTax(decimal salary)
    {
        decimal tax = 0;
        int salaryInt = Convert.ToInt32(salary);

         // Tax each dollar one at a time in each tax bracket.
        for (int i = 1; i <= salaryInt; i++)
        {
            if (i > 40230)
                tax += 0.45m;
            else if (i > 21240)
                tax += 0.33m;
            else if (i > 14070)
                tax += 0.3m;
            else if (i > 8660)
                tax += 0.23m;
            else if (i > 5070)
                tax += 0.14m;
            else
                tax += 0.10m;
        }

        return tax;
    }

It taxes each dollar one at a time. I did a performance test using 80,000 dollars, and Visual Studio 2010 cannot detect a difference between this and the "Slices" version even down to the Tick count of the CPU.

I guess we can call this one "The Buffet Function". It taxes 800,000 bucks at $352,568.10.

I Believe it should work and calculate the tax, i know code is ugly, but u cant write better ode u r bored and checking the random blogs, was searching for signalR Example and got hooked to this one, strange but true.

static void Main(string[] args) { float[,] slabs = new float[5, 2] { { 5070, 0.10f }, { 3590, 0.14f }, { 5410, 0.23f }, { 7170, 0.30f }, { 18990, 0.33f } };

        float income = 50000f;
        float totalTax = 0;
        for (int i = 0; i < slabs.GetLength(0); i++)
        {
            float currValue = slabs[i, 0];
            float currTaxSlab = slabs[i, 1];

            if (income >= currValue && (i != (slabs.Length - 1)))
            {
                income = income - currValue;
                totalTax += currValue * currTaxSlab;

            }
            else if (income < currValue && (i != (slabs.Length - 1)))
            {
                totalTax += income * currTaxSlab;
                income = 0;
            }

        }

        if (income > 0)
        {
            totalTax += income * 0.45f;
        }
        Console.Write("Total Tax: {0}", totalTax.ToString());
        Console.ReadKey();
    }

sorry for my bad English... this code is full of syntax errors...:D incorrect use of switch-case syntax , declare method by int return type and then return a double value,net and tax is not Initialized, ...

GetTax method can simply wrote:

public decimal GetTax(decimal sal) { var taxFactor = new[] { new { fac = 0.45m, Lim = 40230m }, new { fac = 0.33m, Lim = 21240m }, new { fac = 0.3m, Lim = 14070m }, new { fac = 0.23m, Lim = 8680m }, new { fac = 0.14m, Lim = 5070m }, new { fac = 0.1m, Lim = 0m }

                        };

        decimal tax = 0;
        foreach (var item in taxFactor)
        {
            if (sal > item.Lim)
            {
                tax += (sal - item.Lim) * item.fac;
                sal = item.Lim;
            }
        }

        return tax;
    }

even the method name has a spelling mistake.

@Toni-anne Collins Permission to post the code? Good question, maybe Ayende should require candidates to sign some kind of agreement saying that the code submitted may or may not be published, that will definitely weed out candidates, but there might not be any candidates then? :)

Anyway, no personally identifiable information, no defamation, no harm, if she does read the blog, there have been plenty of good solutions to learn from. As for, '...destroyed her confidence and affected her future in this industry.', a bit dramatic, we're not in high school.

@ayende How do you feel about googling the problem and copy/pasting the solution. I mean if you give a candidate a standard problem to solve on an interview, like this tax thingy or a basic algorithm problem described from Knuuth's book are you really just testing his coding skills or are you also testing his skills of finding information on the web and optionaly modifying it.

Or to put it in other words. Would you hire a programmer that would finish the test in 5 minutes and say "Here is the solution. The task that you gave me was a standard xx problem, so I googled the code and copy/paste it".?

Ask yourself this, is your blog post going to encourage people (even those that have the skills) to want to work for/with you? Your right, she doesn't have the developer skills to get the job, but then again, she and others probably wouldn't want the job now after considering your social skills.

For those criticizing Ayende for posting this get real. If he was hard on the candidate he is doing him/her a favor by letting them know that they are either in the wrong field or need to rededicate a little bit to their craft and do better. Either way it would be no favor to the candidate to not tell them the truth. Political correctness and dancing around the issue will not be good for anyone involved and doesn't advance the field of software development.