This code may not quite do what it says it does.
-
In preparation for some redevelopment work, I was having a look through some legacy code provided by a contractor. I have a feeling that it does not do exactly what the comments suggest it does.
/// <summary> /// Determines whether this person already has an application of that type /// </summary> /// <param name="appTypeID">The app type ID.</param> /// <param name="registrationNumber">The registration number.</param> /// <param name="app">The Application</param> /// <returns> /// true if they have an unfinished application of the given type /// </returns> private bool HasApplicationAlready(int appTypeID, int registrationNumber, ref Application app) { bool foundOne = false; List<Application> matches = new Application().GetApplicationListForContact(DataLocation.OnlineDataBase, Int32.Parse(Profile.RegistrationNumber)); // We now have all their applications // Find their latest application of the correct AppType Application bestMatch = null; foreach (Application singleApp in matches) { // See if it's the correct type if (singleApp.Application\_type\_id.GetValueOrDefault(0) == appTypeID) { // See if it's the most recent application of that type if (bestMatch == null) { bestMatch = singleApp; } else { // There's already a match, but see if this one is more recent if (singleApp.ApplicationId > bestMatch.ApplicationId) { bestMatch = singleApp; } else { bestMatch = singleApp; } } } } if (bestMatch != null) { app = bestMatch; foundOne = true; } else { foundOne = false; } return foundOne; }Another gem I found was:
/// <summary> /// Does the Final validation. /// </summary> /// <returns></returns> protected bool FinalValidation() { return true; } -
It's funny because I know what he was thinking, I still make similar mistakes from time to time. But when I find myself typing something like this:
if (singleApp.ApplicationId > bestMatch.ApplicationId)
{
bestMatch = singleApp;
}
else
{
bestMatch = singleApp;
}It's usually a sign that it's time to slap yourself and get go outside for five minutes. :-D
Giraffes are not real.
Yeah I think we've all written code like that on a bad day. But most of us probably notice immediately and take a little break. The worst thing about the original example is the way it returns a value and assigns to instance variables, I think (as well as the 'best match' logic being broken, but that's just a mistake, it's not really insidious). It could just return bestMatch and if it's null then there wasn't one, and then it wouldn't have side effects.
-
The second gem was really funny. :laugh:
Good judgment comes from experience, and experience comes from bad judgment. Barry LePatner
...it's our division that makes us sane(r), and their unity that makes them crazy. Ian Shlasko
Yes, although if it were virtual, it could make sense. Even as it stands if it's a placeholder for something to be added later, it makes sense. (If public, I'd say it was probably an interface implementation; I often have trivial methods or property getters for that purpose.)
-
Yes, although if it were virtual, it could make sense. Even as it stands if it's a placeholder for something to be added later, it makes sense. (If public, I'd say it was probably an interface implementation; I often have trivial methods or property getters for that purpose.)
BobJanova wrote:
Even as it stands if it's a placeholder for something to be added later, it makes sense.
I agree. But I find it funny to have a method called FinalValidation and does no validation at all. Maybe I just have a weird sense of humor.
Good judgment comes from experience, and experience comes from bad judgment. Barry LePatner
...it's our division that makes us sane(r), and their unity that makes them crazy. Ian Shlasko
-
BobJanova wrote:
Even as it stands if it's a placeholder for something to be added later, it makes sense.
I agree. But I find it funny to have a method called FinalValidation and does no validation at all. Maybe I just have a weird sense of humor.
Good judgment comes from experience, and experience comes from bad judgment. Barry LePatner
...it's our division that makes us sane(r), and their unity that makes them crazy. Ian Shlasko
-
So
Finalis obviously valid! ;PSoftware rusts. Simon Stephenson, ca 1994.
-
In preparation for some redevelopment work, I was having a look through some legacy code provided by a contractor. I have a feeling that it does not do exactly what the comments suggest it does.
/// <summary> /// Determines whether this person already has an application of that type /// </summary> /// <param name="appTypeID">The app type ID.</param> /// <param name="registrationNumber">The registration number.</param> /// <param name="app">The Application</param> /// <returns> /// true if they have an unfinished application of the given type /// </returns> private bool HasApplicationAlready(int appTypeID, int registrationNumber, ref Application app) { bool foundOne = false; List<Application> matches = new Application().GetApplicationListForContact(DataLocation.OnlineDataBase, Int32.Parse(Profile.RegistrationNumber)); // We now have all their applications // Find their latest application of the correct AppType Application bestMatch = null; foreach (Application singleApp in matches) { // See if it's the correct type if (singleApp.Application\_type\_id.GetValueOrDefault(0) == appTypeID) { // See if it's the most recent application of that type if (bestMatch == null) { bestMatch = singleApp; } else { // There's already a match, but see if this one is more recent if (singleApp.ApplicationId > bestMatch.ApplicationId) { bestMatch = singleApp; } else { bestMatch = singleApp; } } } } if (bestMatch != null) { app = bestMatch; foundOne = true; } else { foundOne = false; } return foundOne; }Another gem I found was:
/// <summary> /// Does the Final validation. /// </summary> /// <returns></returns> protected bool FinalValidation() { return true; }Why don't you just do...
private bool HasApplicationAlready(int appTypeID, int registrationNumber, ref Application app)
{
app = Application().GetApplicationListForContact(DataLocation.OnlineDataBase, Int32.Parse(Profile.RegistrationNumber)).OrderByDescending(s=>s.ApplicationId).FirstOrDefault(s=>s.Application_type_id==appTypeID);
return app==null?false:true;
}... or something similar?
-
Why don't you just do...
private bool HasApplicationAlready(int appTypeID, int registrationNumber, ref Application app)
{
app = Application().GetApplicationListForContact(DataLocation.OnlineDataBase, Int32.Parse(Profile.RegistrationNumber)).OrderByDescending(s=>s.ApplicationId).FirstOrDefault(s=>s.Application_type_id==appTypeID);
return app==null?false:true;
}... or something similar?
-
But if it is null rather than the result you expected and you want to dig in a bit to find where it is going wrong it is a nightmare so you end up expanding it out a bit to trace then perhaps collapsing again once it is working (or more likely leaving well alone). While I like the concept of the single line solution until you can step through the parts I can see why you would validly avoid it.
-
In preparation for some redevelopment work, I was having a look through some legacy code provided by a contractor. I have a feeling that it does not do exactly what the comments suggest it does.
/// <summary> /// Determines whether this person already has an application of that type /// </summary> /// <param name="appTypeID">The app type ID.</param> /// <param name="registrationNumber">The registration number.</param> /// <param name="app">The Application</param> /// <returns> /// true if they have an unfinished application of the given type /// </returns> private bool HasApplicationAlready(int appTypeID, int registrationNumber, ref Application app) { bool foundOne = false; List<Application> matches = new Application().GetApplicationListForContact(DataLocation.OnlineDataBase, Int32.Parse(Profile.RegistrationNumber)); // We now have all their applications // Find their latest application of the correct AppType Application bestMatch = null; foreach (Application singleApp in matches) { // See if it's the correct type if (singleApp.Application\_type\_id.GetValueOrDefault(0) == appTypeID) { // See if it's the most recent application of that type if (bestMatch == null) { bestMatch = singleApp; } else { // There's already a match, but see if this one is more recent if (singleApp.ApplicationId > bestMatch.ApplicationId) { bestMatch = singleApp; } else { bestMatch = singleApp; } } } } if (bestMatch != null) { app = bestMatch; foundOne = true; } else { foundOne = false; } return foundOne; }Another gem I found was:
/// <summary> /// Does the Final validation. /// </summary> /// <returns></returns> protected bool FinalValidation() { return true; } -
But if it is null rather than the result you expected and you want to dig in a bit to find where it is going wrong it is a nightmare so you end up expanding it out a bit to trace then perhaps collapsing again once it is working (or more likely leaving well alone). While I like the concept of the single line solution until you can step through the parts I can see why you would validly avoid it.
Well, when I write code, I write it so it doesn't have to be debugged(almost:). Only thing that can go wrong here, would be that the database connection is down, or some sore of foreign key exception. It would result in an exception(with a nice description), but that's a whole different story. My point is, that I would always rather write(and debug) 2 lines of code, than 22, no mather how complicated they may be...but that's just me...
-
Well, when I write code, I write it so it doesn't have to be debugged(almost:). Only thing that can go wrong here, would be that the database connection is down, or some sore of foreign key exception. It would result in an exception(with a nice description), but that's a whole different story. My point is, that I would always rather write(and debug) 2 lines of code, than 22, no mather how complicated they may be...but that's just me...
also im not sure the lambda notation existed(in c#) when this code was made. i may be wrong.
-
also im not sure the lambda notation existed(in c#) when this code was made. i may be wrong.
-
Yes, although if it were virtual, it could make sense. Even as it stands if it's a placeholder for something to be added later, it makes sense. (If public, I'd say it was probably an interface implementation; I often have trivial methods or property getters for that purpose.)
-
Unless it was filling in an interface (and you have no control over the interface definition; third party, etc.), it's just code bloat. YAGNI: you ain't gonna need it. The best designs have the minimum of moving parts.
-
BobJanova wrote:
Even as it stands if it's a placeholder for something to be added later, it makes sense.
I agree. But I find it funny to have a method called FinalValidation and does no validation at all. Maybe I just have a weird sense of humor.
Good judgment comes from experience, and experience comes from bad judgment. Barry LePatner
...it's our division that makes us sane(r), and their unity that makes them crazy. Ian Shlasko
At a former place of employment we were told "Names mean nothing." If the function name is "Print" it may not get around to doing any. And for this crapware it was true. Typically functions were not single purpose. They might do 10 different things. These functions would be called by other functions only interested in the output of two of the operations and with the hope that the other eight did not cause any problems. A colleague and I traced a subroutine down 25 levels making calls to these routines, we totally lost what the intended result was supposed to be and never reached bottom.
Psychosis at 10 Film at 11
-
In preparation for some redevelopment work, I was having a look through some legacy code provided by a contractor. I have a feeling that it does not do exactly what the comments suggest it does.
/// <summary> /// Determines whether this person already has an application of that type /// </summary> /// <param name="appTypeID">The app type ID.</param> /// <param name="registrationNumber">The registration number.</param> /// <param name="app">The Application</param> /// <returns> /// true if they have an unfinished application of the given type /// </returns> private bool HasApplicationAlready(int appTypeID, int registrationNumber, ref Application app) { bool foundOne = false; List<Application> matches = new Application().GetApplicationListForContact(DataLocation.OnlineDataBase, Int32.Parse(Profile.RegistrationNumber)); // We now have all their applications // Find their latest application of the correct AppType Application bestMatch = null; foreach (Application singleApp in matches) { // See if it's the correct type if (singleApp.Application\_type\_id.GetValueOrDefault(0) == appTypeID) { // See if it's the most recent application of that type if (bestMatch == null) { bestMatch = singleApp; } else { // There's already a match, but see if this one is more recent if (singleApp.ApplicationId > bestMatch.ApplicationId) { bestMatch = singleApp; } else { bestMatch = singleApp; } } } } if (bestMatch != null) { app = bestMatch; foundOne = true; } else { foundOne = false; } return foundOne; }Another gem I found was:
/// <summary> /// Does the Final validation. /// </summary> /// <returns></returns> protected bool FinalValidation() { return true; } -
In preparation for some redevelopment work, I was having a look through some legacy code provided by a contractor. I have a feeling that it does not do exactly what the comments suggest it does.
/// <summary> /// Determines whether this person already has an application of that type /// </summary> /// <param name="appTypeID">The app type ID.</param> /// <param name="registrationNumber">The registration number.</param> /// <param name="app">The Application</param> /// <returns> /// true if they have an unfinished application of the given type /// </returns> private bool HasApplicationAlready(int appTypeID, int registrationNumber, ref Application app) { bool foundOne = false; List<Application> matches = new Application().GetApplicationListForContact(DataLocation.OnlineDataBase, Int32.Parse(Profile.RegistrationNumber)); // We now have all their applications // Find their latest application of the correct AppType Application bestMatch = null; foreach (Application singleApp in matches) { // See if it's the correct type if (singleApp.Application\_type\_id.GetValueOrDefault(0) == appTypeID) { // See if it's the most recent application of that type if (bestMatch == null) { bestMatch = singleApp; } else { // There's already a match, but see if this one is more recent if (singleApp.ApplicationId > bestMatch.ApplicationId) { bestMatch = singleApp; } else { bestMatch = singleApp; } } } } if (bestMatch != null) { app = bestMatch; foundOne = true; } else { foundOne = false; } return foundOne; }Another gem I found was:
/// <summary> /// Does the Final validation. /// </summary> /// <returns></returns> protected bool FinalValidation() { return true; }So that second gem is not totally useless if it's trying to fill out an Interface of some type. In PHP I've done crazy stuff like this b/c PHP only supports functions in Interfaces so you end with "constants" as "functions". As to your first case, this has a full boat of failures. That whole block of if... elseif... else -> do the same thing is actually pretty comical. The fact that it's in a for loop and "finds something" but doesn't break is really just odd. But the kicker to me is that he has sets the
appvariable and then has a return value of true. Which basically says "hey check the app variable you passed me, I hammered it and returned the app I think I found" Thank goodness it's private and hopefully caused minimal destruction :) -
The second gem was really funny. :laugh:
Good judgment comes from experience, and experience comes from bad judgment. Barry LePatner
...it's our division that makes us sane(r), and their unity that makes them crazy. Ian Shlasko
XKCD Deja-Vu Image tags wouldn't work