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; } -
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
-
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; }This code reads like someone who codes placeholders and doesn't add comments about why he put in a placeholder. It also reads like code written by certain co-workers who can't code, comes running to you for help, mashes your suggestions into unrecognizable pulp and then presents their "work". Leaving you wondering what they were thinking. I'll leave placeholders if I get requirements that are a confusing mishmash of illogical thinking. But I'll add comments about why my code is illogical.
-
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; }