2
Aug
2009
This is Real
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.
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.
A rare instance where saying something loud enough really does make it true.
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
Sorry Steve but I can´t believe that a good programmer would do that even in a hurry… 🙁
@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.
Should I take it you guys would like to see more of these pearls?
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!
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 🙂
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?
Should that crash if Roles.GetAllRoles() actually returns something? When you call ToUpper() on the null role string, it would get a NullReferenceException.
@Andrew Hall
Haha… Just noticed someone else caught that.
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.