Coding Horrors: You can gather all your technical debt into one easy framework!

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›3 »

dwazegek: Feb 11, 2005; WE CAN USE THIS

Ranma4703 posted:

code:

private boolean compareInts(int one, int two)
{
    boolean areEqual = false;
    if(one == two)
    {
        areEqual = true;
    }
    return areEqual;
}

I have a colleague who also does stuff like this. For whatever reason, he refuses to use return anywhere, except the last line of a method. This frequently results in methods with absurd amounts of indentation, or stuff like this:

code:

public bool HasValidItems(SomeClass[] items)
{
  bool retVal = false;
  foreach(SomeClass item in items)
  {
    retVal = retVal || item.IsValid;
  }
  return retVal;
}

public SomeClass FindSomeItem(SomeClass[] items)
{
  SomeClass retVal = null;
  foreach(SomeClass item in items)
  {
     if(item.MatchesWhateverWeAreLookingFor)
     {
       retVal = item;
     }
  }
  return retVal 
}

Who cares if we've already found what we're looking for, let's keep on looking :cool:

# ¿ Mar 21, 2008 18:26

Adbot: ADBOT LOVES YOU

# ¿ Apr 28, 2024 23:15

dwazegek: Feb 11, 2005; WE CAN USE THIS

Bonus posted:

Also what the hell is up with setting a variable to false and then doing var = var || x

So that if you find one true item, the true value is retained throughout the rest of the loop, and because retVal is true, item.IsValid is no longer evaluated.

Anyway, those two examples are pretty old, probably from when he first started working here, and seeing as I cringe at my old code as well, I can't really blame him. I found this abortion in a project I wrote about a year ago (and even then, I should have known better):

code:

public static void main(string[] args)
{
  //first argument passed is a Url, if a second and third argument are passed,
  //then they're considered to be the user name and password to use for authentication
  String url = args[0];
  String userName = null;
  String password = null;
  try
  {
    userName = args[1];
    password = args[2];
  }
  catch(ArrayIndexOutOfBoundsException ex) { }
  
  Open(url, userName, password); 
}

# ¿ Mar 21, 2008 20:24

dwazegek: Feb 11, 2005; WE CAN USE THIS

wwb posted:

True that. Why all the hate for "single point of return"? I don't follow it 100%, but it can definitely make sense in some scenarios.

It really depends on the circumstances, but I've seen situations where it just looks like he's jumping through hoops in order to maintain that pattern, instead of writing code readable code.

One of my gripes with it is the "retVal" variable (it always seems to be named that way), in the vast majority of the cases, the first "real" value that is assigned to it, is the value that it's going to have at the end of the method. So why not return the value immediately?

Personally, I prefer returning the value immediately, because it makes reading the code easier (or at least, easier for me); if I see a branching statement that immediately returns a value, then I know that the method is done if that branch is hit. If you go with the "single point of return" method, then this is less clear.

# ¿ Mar 22, 2008 01:44

dwazegek: Feb 11, 2005; WE CAN USE THIS

Incoherence posted:

But at the same time, not using it ever is an overreaction. If it makes sense, use it. If it doesn't, don't use it.

I don't think anyone will argue with that, my annoyance with it is that my colleague always uses it, sometimes to the point of absurdity.

Imagine methods with 10+ levels of indentation and stuff like this appearing regularly:

code:

Incoherence posted:

Personally when I write a IsFooCondition() that calls for such a variable, I feel stupid calling the variable some variant of isFooCondition.

I'm partial to the almost always applicable "result" :downs:

dwazegek fucked around with this message at 01:56 on Mar 22, 2008

# ¿ Mar 22, 2008 01:54

dwazegek: Feb 11, 2005; WE CAN USE THIS

such a nice boy posted:

code:

String numPorts;

String result = null;
try {
   doSomething(Integer.toInt(Integer.toString(getIntValue(numPorts))), result);
} catch (Exception e) {
}

if (result != "worked") {
   ...error handling code...
}

Does that even work? My java's a little rusty, but in C# you'd have to pass result with either the ref or out keyword to change it.

We used to have a control that could display multiple other controls in a grid, which we used to display multiple video streams simultaneously. For whatever reason, if you changed the number of cells in the grid from (e.g.) 4 to 6, the control would first switch to 5 cells, and then to 6. Also, if you wanted less cells than you had, it word reset itself to 1 cell, and then count up until it reached the desired amount.

Turns out the resizing code looked something like this:

code:

public void SetCellCount(int cellCount)
{
  if(cellCount == currentCellCount)
  {
    return;
  }
  if(cellCount < currentCellCount)
  {
    Reset(); //among other things, currentCellCount gets reset to 1 here.
  }
  while(currentCellCount < cellCount)
  {
    currentCellCount++;
    SetCellCountInternal(currentCellCount);
  }
}

Okay, so that explains the messed up behaviour of the control, but why not just set the cell count to what you want to have immediately? Well, this is what SetCellCountInternal looked like:

code:

private void SetCellCountInternal(int cellCount)
{
  switch(cellCount)
  {
    case 1:
      //create one cell and set it's location
      break;
    case 2:
      //remove first control from grid
      //set first control to a different size and location
      //create second control and set it's location
      break;
    case 3:
      //remove first control
      //remove second control
      //set first control to a different size and location
      //set second control to a different size and location
      //create third control and set it's location
      break;
    //etc, 12 cases in all
}

The method was over 200 lines long, the last case alone was 24 lines of code.
Seems that the reason why it always counted up when adding controls, was that for case N to work, case N - 1 had to have run.

# ¿ Mar 22, 2008 23:20

dwazegek: Feb 11, 2005; WE CAN USE THIS

A piece of code that reads items from a buffer, skips a user-definable amount of items, and processes the rest:

code:

public int ItemsToSkip { get; set; }

public void ProcessNextItem()
{
  int skipAmount = ItemsToSkip;
  while(ItemsToSkip > 0)
  {
    GetItem();
    ItemsToSkip--;
  }
  SomeClass item = GetItem();
  Process(item);
  ItemsToSkip = skipAmount;
}

To make things worse, there were two places in the UI where ItemsToSkip could be altered, one was a slider, the other was a pair of buttons that called ItemsToSkip++ or --, and thus behaved pretty much randomly.

The same guy who wrote this also wrote a UserControl that disposed and recreated all its child controls in the OnPaint :downs:

# ¿ Mar 31, 2008 19:40

dwazegek: Feb 11, 2005; WE CAN USE THIS

code:

private bool AssertPath(string p)
{
    if (Directory.Exists(p))
        return true;
    try
    {
        Directory.CreateDirectory(p);
        return true;
    }
    finally
    {
        Logging.TraceError(string.Format("Path {0} not available and unable to create.", p));
    }
}

Yeah, I don't know either :sigh:

# ¿ Apr 2, 2008 11:15

dwazegek: Feb 11, 2005; WE CAN USE THIS

tef posted:

You could probably have a large lookup table for each one per project, and once you start using these 64-bit numbers it will come naturally.

How about an IDE plugin that automatically translates them into something that's appealing to the reader? That way everyone can have their own crazy naming scheme without conflicts!

# ¿ Apr 2, 2008 11:17

dwazegek: Feb 11, 2005; WE CAN USE THIS

(C#) I'd just like to know, am I an idiot for considering adding extension methods for stuff like string.IsNullOrEmpty() and Object.ReferenceEquals because I prefer this syntax:

code:

string s = ...
if(s.IsNullOrEmpty())
{
   //
}

over this syntax:

code:

string s = ...
if(string.IsNullOrEmpty(s))
{
   //
}

# ¿ Apr 7, 2008 18:41

dwazegek: Feb 11, 2005; WE CAN USE THIS

_aaron posted:

Yeah, you're probably an idiot, because won't s.IsNullOrEmpty() throw an exception is s is null?

No, extension methods are pretty much just syntactic sugar for a call to a method in a referenced static class.

code:

public static class SomeClass
{
  public static bool IsNullOrEmpty(this string s)
  {
    return string.IsNullOrEmpty(s);
  }
}

//these two calls are the same:
s.IsStringNullOrEmpty();
SomeClass.IsNullOrEmpty(s);

Since you're not actually calling an instance method of "s", but actually passing it as the first parameter to an extension method, it doesn't matter if it's null or not.

# ¿ Apr 7, 2008 18:57

dwazegek: Feb 11, 2005; WE CAN USE THIS

I just came upon this. There are about 20 different variations for different properties, all almost identical, only the property names and default values differ.

code:

if (config["someProperty"] != null && config["someProperty"].Trim() != "")
{
    someProperty = config["someProperty"];
}
else
{
    someProperty = "defaultValue";
}

# ¿ Jun 24, 2008 10:42

dwazegek: Feb 11, 2005; WE CAN USE THIS

Ryouga Inverse posted:

I actually wrote a String C# extension method to do something like this. But I think it was String.NullOrEmpty() or something.

That's not that bad, it's actually pretty understandable.

The horror from that snippet is that he's copy/pasted the same 8 identical lines of code 20 times instead of just sticking it in a method and calling that method multiple times (which would drop the length of the method from ~170 lines to ~30). He's also doing the same look up every time he needs the value, instead of just doing it once and reusing that result.

Another horror that can't be seen from that snippet is that the entire operation is completely wrong. If the key doesn't exist the dictionary will throw an exception, it won't return null. And if it does return null or an empty string, then it's because that's the value that has been entered, probably for a reason.

The correct method would be something like:

code:

private static string GetProperty(IDictionary<string, string> dict, string propertyName, string defaultValue)
{
   string value;
   return dict.TryGetValue(propertyName, out value) ? value : defaultValue;
}

# ¿ Jun 25, 2008 08:01

dwazegek: Feb 11, 2005; WE CAN USE THIS

Haven't posted in this thread in a while, mostly because the WTF-inducing guys have all left the company, and the entire codebase has cleaned up quite nicely.

However today I had to go through some old (unused) code and found this beauty:

code:

/// <summary>
/// Creates a unique code.
/// </summary>
/// <returns>A unique code</returns>
private string CreateActivationCode()
{
    double seed = 0.16;
    int year = DateTime.Now.Year;
    int month = DateTime.Now.Month * 12;
    int day = DateTime.Now.DayOfYear;
    int hour = DateTime.Now.Hour * 24 * 60 * 60;
    int min = DateTime.Now.Minute * 24 * 60 * 60;
    int sec = DateTime.Now.Second * 24 * 60 * 60;
    int milsec = DateTime.Now.Millisecond * 24 * 60 * 60 * 1000;

    double basevalue = Convert.ToInt32(((double)(((year * month * day) + (hour + min + sec + milsec))) * seed));
    int len = basevalue.ToString().Length;
    if (len < 10)
    {
        string bvstring = (basevalue.ToString() + "0000000000");
        basevalue = double.Parse(bvstring.Substring(0, 9));
    }
    if (len >= 10)
    {
        len++;
    }
    if (len < 10)
    {
        len = 10;
    }
    if (len > 16)
    {
        len = 10;
    }

    return string.Format("{0}-{1}", len.ToString("X"), Math.Abs(Math.Round(basevalue, 0)));
}

# ¿ Nov 20, 2008 13:39

dwazegek: Feb 11, 2005; WE CAN USE THIS

ehnus posted:

Relying on the garbage collector to free resources other than memory (sockets, database connections, etc.) is pretty dumb...

The simple solution is to manually force garbage collection every time you're done with those resources. :downs:

An ex-colleague of mine used to actually do this every time he used FileStreams, he never closed them, just let them drop out of scope, or nulled all references. His code was always peppered with GC.Collect.

# ¿ Jan 17, 2009 23:08

dwazegek: Feb 11, 2005; WE CAN USE THIS

mr_jim posted:

Christ, that's terrible even for single-point-of-return advocates. Usually they'll stick with creating extraneous variables for return values. Abusing a loop statement to avoid using a return statement is just awful.

Even if you subscribe to single point of return, that function doesn't make sense, a regular if-block would do.

# ¿ Jul 23, 2009 19:43

dwazegek: Feb 11, 2005; WE CAN USE THIS

Zombywuf posted:

You score 1 point. The other flaws are in the 2 functions posted. There are several flaws.

The maxLength < 0 check is stupid, just throw an argument exception. Or at least check for <= 0.

Calling SubString even if no truncation is needed.

# ¿ Aug 17, 2009 17:28

dwazegek: Feb 11, 2005; WE CAN USE THIS

jarito posted:

Right, but that's not what we are complaining about. This is:
code:
void someFunc(var a, var b, var c) 
{
    var d = a.Object.DoSomething();
    d.execute();
}
Using var where the typing information is unneeded is fine, but it gives idiots a crutch to turn C# into a weakly typed language, which it is not.

You can't use var in parameters in C#...

You could use dynamic in C# 4.0, but dynamic and var are two different concepts.

edit:
And var in C# has absolutely nothing to do with weak/strong typing.

# ¿ Jan 27, 2010 15:46

dwazegek: Feb 11, 2005; WE CAN USE THIS

Milotic posted:

The problem I have with var is that it binds to the strictest return type of a method/constructor, which tends to discourage polymorphism and hence hinder refactoring.

~~I can somewhat understand your point about it discouraging polymorphism (although I don't agree with it), but how does it hinder refactoring?~~

edit:
In what way does it do either of those two things?

dwazegek fucked around with this message at 22:55 on Jan 27, 2010

# ¿ Jan 27, 2010 22:52

dwazegek: Feb 11, 2005; WE CAN USE THIS

PhonyMcRingRing posted:

-Replaces " with "

This made me smile :v:

# ¿ Apr 21, 2010 21:09

dwazegek: Feb 11, 2005; WE CAN USE THIS

PT6A posted:

2) A parsing method, to retrieve the hostname from an HTTP header, which returns 'void'. Since we are explicitly not allowed to change the provided interface, I've made this method set a member variable which is then read by the method that calls it, which is a coding horror in and of itself. Of course, it could just return String, in which case everything would be simple.

I have to deal with a 3rd party API which does something similar. There's a function to get certain information from devices, which is void, so how do you actually get the info? Why it's simple:

1. You give the instance that represents the device a GetInfoCallback.
2. You call RequestInfo, which returns immediately.
3. The callback is called on a separate thread.
4. In the callback you call GetInfo, which gives you the last requested info.

If you have two simultaneous info requests, you'll get both results, but there's no way to tell which is which.

Oh, and if an error occurs, then the callback is never called, but you're also never notified that something went wrong.

# ¿ Sep 28, 2010 08:31

dwazegek: Feb 11, 2005; WE CAN USE THIS

code:

public void DoSomething(int parameter1, SomeObject parameter2 = null)
{
  if(parameter2 == null)
    throw new ArgumentNullException("parameter2");

  ...
}

# ¿ Oct 7, 2010 08:26

dwazegek: Feb 11, 2005; WE CAN USE THIS

Ugg boots posted:

Honestly, I'd rather have it the Java way instead of making things internal in C# just so that I can access a nested class's private field for special construction without exposing it to the entire namespace.

Just make the class private, if you're exposing an inner class, chances are that it shouldn't be an inner class in the first place.

# ¿ Oct 20, 2010 19:02

dwazegek: Feb 11, 2005; WE CAN USE THIS

Constructor arguments are for scrubs:

code:

public static string Argument1Init { get; set;}
public static int Argument2Init { get; set;}

private string _argument1;
private int _argument2;

public SomeClass()
{
  _argument1 = Argument1Init;
  _argument2 = Argument2Init;
}

(names are changed)

# ¿ Jan 28, 2011 12:16

dwazegek: Feb 11, 2005; WE CAN USE THIS

Aleksei Vasiliev posted:

Its main class has more than 300 public class/instance variables. Their order in the code is important; if they're sorted alphabetically (Reflector does this) the game will fail due to hitting nulls/going out of bounds of arrays.

How the gently caress do you even accomplish this?

# ¿ May 26, 2011 09:35

dwazegek: Feb 11, 2005; WE CAN USE THIS

Geekner posted:

I could only imagine some variables refer to previous variables, and rely on the specific initialization order.

int ONE = 1;
int TWO = ONE+ONE;
ect.

Wouldn't compile though, you can't reference the current instance during initialization.

Although you can do it with static members. I thought he was referring specifically to instance members, but I must've misread.

# ¿ May 26, 2011 11:41

dwazegek: Feb 11, 2005; WE CAN USE THIS

Hughlander posted:

Wouldn't the reflector just name those in alphabetical order since that's the way they're initialized? IE: One becomes int aInt, TWO becomes int bInt or however reflector names it's variables?

No, the names of class members, even private ones, are preserved in the MSIL, so reflector can just show them as they're named.

I guess it should also be possible to see in which order they're initialized, and order them based on that, but I guess reflector never implemented that feature.

# ¿ May 26, 2011 16:04

dwazegek: Feb 11, 2005; WE CAN USE THIS

Ryouga Inverse posted:

I'm pretty sure that auto-initializers are just syntactic sugar that the compiler writes into the constructor anyway, so Reflector would just see the constructor code, not the fact that they're auto-initializers.

It's not entirely syntactic sugar. Initializers of derived types run in the reverse order that the constructors run in, so there is a difference between initializing a member in the declaration or initializing it in the constructor. Of course, if you actually write code that depends on this behavior, it's most likely also a horror.

Say you have a class C that derives from B, that derives from A. If you instantiate C, then the initializers run in the order C, B, A, followed by the constructors in the order A, B, C.

You're right that this is achieved by sticking all the code in the constructor though, just not in a way that reflector can translate back to C#. Class C's constructor would look like

1. C's initializers
2. Call B's Constructor
3. C's constructor code

# ¿ May 26, 2011 19:47

dwazegek: Feb 11, 2005; WE CAN USE THIS

PhonyMcRingRing posted:

Reflection's not as terribly slow if you cache what it was you were looking for. Still not a good idea for anything requiring low latency/overhead.

If performance is a problem, you can use the System.Linq.Expressions stuff to generate a bunch delegates that do the real work. The initial cost could be pretty high, but even that can be mitigated by compiling it to an assembly and saving that to disk.

If you're on an older version of .NET that doesn't have Expressions, you could use System.Reflection.Emit to do the same thing, but that's a quite a bit more work.

# ¿ May 27, 2011 08:40

dwazegek: Feb 11, 2005; WE CAN USE THIS

Broken Knees Club posted:

Also, this is what String.Empty looks like on the inside: public static readonly string Empty = "";

Static readonly?

Which is weird because you can't change non-static readonly members through reflection, only static ones.

# ¿ May 28, 2011 12:29

dwazegek: Feb 11, 2005; WE CAN USE THIS

shrughes posted:

It works correctly? What's the problem? You mean if it equals "0", right?

ob1.id = 0
ob2.id = "5"

sortById(ob1, ob2) gives -1
sortById(ob2, ob1) gives -1

So ob1 is both lesser AND greater than ob2?

Also:

ob3.id = 0

sortById(ob1, ob3) gives -1
sortById(ob3, ob1) gives -1

Shouldn't they be equal?

# ¿ Jun 23, 2011 12:56

dwazegek: Feb 11, 2005; WE CAN USE THIS

When I started working here, they had every solution in its own VSS repository (or whatever VSS calls them), so if solution 1 used projects A, B and C and solution 2 had used projects A, B and D, then A and B would exist in 2 different VSS repositories. I can't even begin to number the amount of ways that could go horribly wrong, and I have no idea how they actually used it.

# ¿ Sep 1, 2011 09:00

dwazegek: Feb 11, 2005; WE CAN USE THIS

code:

semaphore = new System.Threading.Semaphore(0, 32000);

# ¿ Nov 14, 2011 11:30

dwazegek: Feb 11, 2005; WE CAN USE THIS

hieronymus posted:

It's mostly a "what the gently caress would you be doing with 32k threads" issue. A semaphore limits the number of threads that can access a resource or resource pool concurrently. 32k is an enormous number of threads - on my machine, explorer.exe is using 32 threads (you can check in task manager what apps are using how many threads.)

In this case, they're using it as a sort of queuing mechanism. Thread A runs a loop where it waits on the Semaphore and then does dome other work, Thread B then uses Release to control how often Thread A loops.

Sinestro posted:

I bet most of it would be of a similar... quality/ level.

From what I saw, the rest isn't that bad, there's a couple of unused members, and the occasional resource that isn't Disposed properly (but at least the GC takes care of those :v:

). Still WTFs, just not particularly interesting ones.

And after rereading it, that Semaphore is mostly just an ugly hack to fix a really unlikely (but possible) problem. They should've just fixed the overall design so that the problem can't occur, but, for the most part, that fix does solve it as well. Of course, a bit of documentation on what the gently caress they were hoping to achieve would've been useful.

# ¿ Nov 14, 2011 13:18

dwazegek: Feb 11, 2005; WE CAN USE THIS

Zombywuf posted:

Assuming the semaphore has an efficient implementation this sounds like a perfectly good use of a semaphore. Although the hardcoded max suggests it's a bit more horrific than this description gives credit for.

Yeah, the use of the semaphore is fine. My knee-jerk reaction was "32000 threads :wtf:

", but I spoke too soon and that's really not the case here.

32000 is just a bullshit number that was deemed "high enough" to work around an existing problem in their design. Instead of just fixing the problem in the first place.

# ¿ Nov 14, 2011 14:55

dwazegek: Feb 11, 2005; WE CAN USE THIS

Jabor posted:

I honestly do not get this.

I mean, how do you check in code that doesn't work? I don't mean buggy code, I mean "your code does not even compile" broken. Do these people ... just do a commit without even trying to run their code first or something?

Something like that.

A colleague of mine always works on about 5 projects at a time. He'll start on one project, work on that for a while until he gets side-tracked into another project and then just leave the first project half finished for days/weeks/months.

This means that he'll leave files checked out for months at a time. So, when he finally does check it in, there's a good chance that the files have already been altered by someone else. Auto-merging the changes works surprisingly often, but sometimes it results in code that doesn't compile. Since his code compiled before, and the merge tool didn't throw up any warnings, there's no need to check the code again, right? :suicide:

# ¿ Nov 22, 2011 09:38

dwazegek: Feb 11, 2005; WE CAN USE THIS

Zhentar posted:

The T_#_SECONDS appear stupid, but they only make up 101 lines out of 1208, and are presumably only there for symmetry with non-seconds definitions. T_7_DAYS on the other hand, is obviously more understandable than 25200, less error prone than 60*60*24*7, and faster than doing the math at runtime with a library, as justified in the README.

It is perhaps premature optimization (or maybe it was well justified from profiling!), but it's definitely not ' what were they thinking?!'.

Fair enough for seconds, minutes, hours, days and even weeks. But months and years?

# ¿ Feb 2, 2012 09:32

dwazegek: Feb 11, 2005; WE CAN USE THIS

pokeyman posted:

I ended up getting it working, found some random site with a text file that described the Windows rand() implementation and dicked around with dealing patterns until it clicked.

I did the same thing :v:

But I only managed to find a VB6 implementation of rand(), and since VB6 uses different rules for integer overflows, they guy that had written it had added a whole bunch of code to adjust for that. So I had to rewrite it again to remove the adjustments.

# ¿ Apr 11, 2012 09:35

dwazegek: Feb 11, 2005; WE CAN USE THIS

Ender.uNF posted:

I am seriously considering never opening another PDF file.

How safe is it to use an alternate PDF reader? I've been using SumatraPDF with the browser plugin disabled, but all this poo poo has me in :tinfoil:

-mode

# ¿ May 7, 2012 15:11

dwazegek: Feb 11, 2005; WE CAN USE THIS

Zombywuf posted:

Doesn't log you on with an incorrect password once out of every 256 times.

Am I understanding this correctly? If you just make a couple of attempts with whatever password, you'll get in?

gently caress.

# ¿ Jun 12, 2012 09:07

Adbot: ADBOT LOVES YOU

# ¿ Apr 28, 2024 23:15

dwazegek: Feb 11, 2005; WE CAN USE THIS

SavageMessiah posted:

There's more though! I have to limit lines to 80 char width. And despite the single point of return thing, I CAN USE EXCEPTIONS. I can only type "return" once, but "throw" many times!

Whenever you'd want to do a return just stick your return value in an exception and throw it. Then wrap the entire method in a try/catch block, extract the return value and return it. Single point of exit. :smug:

# ¿ Jun 20, 2012 13:48

The Something Awful Forums > Discussion > Serious Hardware/Software Crap > The Cavern of COBOL > Coding Horrors: You can gather all your technical debt into one easy framework!

«‹›3 »