This is good practice. You never know when some pesky cosmic ray might change the value of UpdateTypeCode.
Mithious
@Mithious
Best posts made by Mithious
-
RE: Representative Unit Test
Latest posts made by Mithious
-
RE: Are You Scottish? I've no idea anymore.
@blakeyrat said:
Skipping the biggest WTF... why would it matter if the employee were Scottish?
It's using it to pick which set of tax tables to use, so technically it's actually if the person is resident in Scotland rather than being Scottish. But that would have made for a less amusing title.
-
Are You Scottish? I've no idea anymore.
This code is trying to work out if the employee is Scottish.
int funcEnd = GetParameter(employeeData, "ISSCOTTISH", resultField); if (funcEnd) { strcpy_s(errorMessage, "Warning - The ISSCOTTISH Indicator Was Not Found"); ReportError(data,errorMessage, errorFile); englishScottish = 'N'; } else englishScottish = resultField[0];
if(englishScottish != 'N' && englishScottish != 'Y')
{
if (data->defUseScottishInd != '\0')
{
englishScottish = data->defUseScottishInd;
}
else
{
englishScottish = 'N';
}
}if (englishScottish == 'Y')
englishScottish = 'S';
if (englishScottish == 'N')
englishScottish = 'E';
if (englishScottish != 'S' &&
englishScottish != ' ' &&
englishScottish != '\0' &&
englishScottish != 'E')
{
strcpy_s(errorMessage,
300,
"Warning - The ISSCOTTISH Indicator Was Not Recognised");
ReportError(data,errorMessage, errorFile);
englishScottish = 'E';
}
if (englishScottish == 'S')
isScotInd = 'Y';
else
isScotInd = 'N';
Apart form the horrible formatting here are a few of the WTFs I see:
- Two separate variable which indicate whether the employee is scottish (isScotInd and scottishEnglish)
- Using the variable which is supposed to be E / S initially for Y / N, changing it, then setting the Y / N variable afterwards.
- If the ISSCOTTISH parameter is not found it will not use the default, but will if the parameter is found but doesn't have a valid value (Y or N).
- If englishScottish is ' ' or '\0' it will not be changed to 'E' (the default), luckily elsewhere the code only checks against 'S'.
-
SizeIt
Found this undocumented function in our legacy codbase:
int SizeIt(char *inputField, char *outputField, int outputFieldBufferSize, int length)
From the name you may, if lucky, correctly guess it copies input to output truncating or padding (wth spaces) to the specified length.
It also converts the string to upper case and replace all instances of ; and _ with spaces. Wasn't that obvious?
Bonus WTF: Based on some of the places it is called it looks like at some point in the distant past length was not supplied as a parameter by via a global variable.
-
RE: Outlook.com style security
I've seen this come up loads of times now, the Microsoft Passport auth system is ancient and used by a ton of legacy crap, some of which is deployed to client machines so not under their direct control for update purposes. Last time I saw a response from someone at MS they said that the Outlook website itself is fine but they are actively working to fix the issues in all these legacy system and once there are no more hangups they will remove the limitations. They do seem to be taking their sweet time over it though.
-
RE: Using while... break as an if...else replacement.
@anotherusername said:
Out of curiosity, what did LogErrorMessage do? I'm guessing it probably didn't log a useful error message...
I removed the parameters being sent to that function for anonymisation purposes, I didn't consider it particularly essential to this wtf. I would have added something like "there was an error message here" but last time I did that someone compained about the code not fitting on their (presumably) 800px wide screen.
As far as I can tell there has never been a reason for the code to loop so I'm still thinking it's a dodgy attempt at a goto. Many functions in this part of the product used to take 80+ parameters before we refactored it to use some objects, this made it very difficult to put anything into separate functions leading to some 10000+ line long functions. There's one function where the first 500 lines are purely local variable declarations.
-
Using while... break as an if...else replacement.
I just found this code.
while(1)
{
// 15 lines of codeif (count == 0)
{
LogErrorMessage();
break;
}
// 30 lines of code
break;
}Instead of something like this
// 15 lines of code
if (count == 0)
{
LogErrorMessage();
}
else
{
// 30 lines of code
}There were no other breaks or continues, and if there were lots of reasons to break it should be in a separate function anyway otherwise you're basically using a glorified goto statement.
-
RE: Representative Unit Test
This is good practice. You never know when some pesky cosmic ray might change the value of UpdateTypeCode.
-
RE: New design pattern: For loop on something you're not using that still does the work.
@AndyCanfield said:
Am I poor white trash, or do you guys on thedailyWTF all have monitors that are six feet wide and display five thousand pixels?
You should get that monitor valued, it might be worth something as an antique. That barely extended across half my screen here.
@aihtdikh said:
I trust you also changed it to do the non-j parts once even when your new list is empty and the loop doesn't run?
Correct. I know why they did it, it's because they couldn't be bothered to move the shared code off to a separate function. This is why we have some 10000 line long functions in the product :/
-
RE: Replace all? Nah, too hard; looping 10 times should be enough.
@joe.edwards said:
since then I always limit the maximum number of iterations an algorithm can take; 10 seems reasonable here.
I've done similar, in other situations I've kept a dictionary of replacements it's already encountered and thrown an exception when it finds one it's seen before.
In this case though, simply updating the substitute function to do what it should have done in the first place was the sensible course of action, the string can then contain 1000 instances of it and it'll cope fine. It's not like your example where making the substitution will potentially result in more substitutions, it simply requires one run through the string replacing each instance it finds.
-
New design pattern: For loop on something you're not using that still does the work.
Well, a new design pattern to me anyway.
for (int j = 0; j < MAXRATES; j++)
{
if (!usingRates && j > 0)
break;
// 100 lines of standard stuff that doesn't use j and fits really well into a separate function
if (usingRates)
{
// 20 lines or so which uses rates[j]
}
else
{
// 20 lines or so which doesn't use j at all.
}
}I discovered this little suprise left for me when converting the previously fixed size rates array to be a list with no artificial limit and which will now be zero length if you aren't using rates.