This is Real

Category:UncategorizedTag: :

Real code in a production system which will remain unnamed, as will the source.

Drink it in.

class Customer
{
    public string Role;
    public void Save()
    {
        // lots of code here        
        string[] roles = Roles.GetAllRoles();
        foreach (string role in roles)
        {
            if (Role.ToUpper() == role.ToUpper())
            {
                Role = role;
                break;
            }
        }
        // lots more code    
    }
}

It actually took me a moment to see the real beauty in this.

12 thoughts on “This is Real

  1. rofl! Man, that smells so much like a “Damn, it’s 18:00 and this has to demo tomorrow morning.” Don’t think, just do.

  2. I’m not saying it’s pretty code, but it does do something. It’ll correct the capitalization of the Role variable on the customer object.

    Obviously that shouldn’t be the Customer objects responsibility though

  3. Sorry Steve but I can´t believe that a good programmer would do that even in a hurry… 🙁

  4. @Eric : It’s not about whose responsibility! This ain’t ayende’s post :).

    If you see closely, “Role” has never been assigned, so it will have a Null value.

  5. Who said anything about it being a “good” programmer? Well, at least a good, “sober” programmer. 😀

    Here’s a doosy I had to sift out of a production system recently. Ironically, AFAIK you cannot do this in C#…

    Public Interface IRiskBusinessLayer

    Function LoadPreviousVersionsOfRiskReviews(ByVal riskID As Integer) As dstResolvedRisk.ReviewDataTable
    Function LoadFinalisedRiskReviews(ByVal riskID As Integer) As dstResolvedRisk.ReviewDataTable

    End Interface

    Public Class RiskBusinessLayer Implements Risk.IRiskBusinessLayer

    Public Function LoadPreviousVersionsOfRiskReviews(ByVal riskID As Integer) As dstResolvedRisk.ReviewDataTable Implements IRiskBusinessLayer.LoadFinalisedRiskReviews

    End Function

    Public Function LoadFinalisedRiskReviews(ByVal riskID As Integer) As dstResolvedRisk.ReviewDataTable Implements IRiskBusinessLayer.LoadPreviousVersionsOfRiskReviews

    End Function
    End Class

    Let s/he who has not N 1’d cast the first stone!

  6. 1) Considering the fact that we compare Role and role for equality and if they are (uppercase) equal we set them (again) to be equal.
    The only (possible) sense in this would be if we would have two different roles with different casing (which I doubt is the case)

    2) Considering the fact that Role is null at the beginning of the loop, this if statement would never evaluate true and Role would always stayed empty.

    3) The only way how this could work is that someone do

    var customer = new Customer();
    customer.Role=”Admin”;
    customer.Save();

    in which case the complete looping is useless (unless #2)

    Yeap, it is a beauty 🙂

  7. My first thought was that Roles.GetAllRoles() could somehow be accessing the entire array/file/database of customers, which means there’s a chance that the first value of role to come up would be the one already assigned to the Customer. So, for example, you may have customers with roles “BOSS”, “CEO”, “peon”, “BOSS” and “Peon”. Suppose we’re looking at the customer with role “Peon”; iterating through the list in the order given would change that to “peon”, which may or may not be a bad thing depending on how the boss, Boss and BOSS happen to feel about their staff… But of course that would be silly… right?

  8. Should that crash if Roles.GetAllRoles() actually returns something? When you call ToUpper() on the null role string, it would get a NullReferenceException.

  9. Yes, it is correcting the casing of the Customer’s Role string. I don’t see anything wrong with the concept, if that’s the requirement. Needs some refactoring–its own function at least–but it’s not stupid in itself.

Comments are closed.