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!

«‹›1503 »

New Yorp New Yorp: Jul 18, 2003; Only in Kenya.; Pillbug

This one made me weep today:

code:

public bool HasErrors() 
{
     return errorList.GetEnumerator().MoveNext();
}

I replaced it with

code:

public bool HasErrors() 
{
     return errorList.Count > 0;
}

Of course, I had to explain to someone why doing it the original way was staggeringly inefficient, but it was worth it.

# ? Oct 8, 2011 02:34

Adbot: ADBOT LOVES YOU

# ? May 25, 2024 14:58

Sedro: Dec 31, 2008

The horror is not disposing that enumerator.

# ? Oct 8, 2011 03:41

Cocoa Crispies: Jul 20, 2001; Vehicular Manslaughter!; Pillbug

Aleksei Vasiliev posted:

make a pull request that just requests the file be deleted

Those are the best: "+ 426 additions, - 24,623 deletions" https://github.com/tomdale/everyjs.com/pull/26

# ? Oct 8, 2011 04:20

New Yorp New Yorp: Jul 18, 2003; Only in Kenya.; Pillbug

Sedro posted:

The horror is not disposing that enumerator.

Trust me, all of my disposables are in using blocks.

# ? Oct 8, 2011 05:04

Pimblor: Sep 13, 2003; bob; Grimey Drawer

I ran across this today.

code:

                    xmlNodeNav = XmlConfigDoc.CreateNavigator().SelectSingleNode("SOMECOMPANYLOLIX.Common/TokenStandardExpiryInMins")
                    If Not xmlNodeNav Is Nothing AndAlso xmlNodeNav.Value.Trim() <> String.Empty Then
                        TokenStandardExpiryInMins = CType(xmlNodeNav.Value.Trim(), Integer)
                    End If

                    xmlNodeNav = XmlConfigDoc.CreateNavigator().SelectSingleNode("SOMECOMPANYLOLIX.Common/TokenChangePasswordExpiryInMins")
                    If Not xmlNodeNav Is Nothing AndAlso xmlNodeNav.Value.Trim() <> String.Empty Then
                        TokenChangePasswordExpiryInMins = CType(xmlNodeNav.Value.Trim(), Integer)
                    End If

VB is certainly an abomination of course, but this class is exactly the same for 3,214 lines. Yes, that's right. 3,214 lines manual of XML de-serialization.

That is some quality poo poo right there. :smug:

# ? Oct 8, 2011 05:22

ninjeff: Jan 19, 2004

Ithaqua posted:

This one made me weep today:
code:
public bool HasErrors() 
{
     return errorList.GetEnumerator().MoveNext();
}
I replaced it with
code:
public bool HasErrors() 
{
     return errorList.Count > 0;
}
Of course, I had to explain to someone why doing it the original way was staggeringly inefficient, but it was worth it.

That's not a real horror IMO; just misguided. It's a good technique to use where errorList isn't backed by an array, and where you actually dispose the enumerator (as already mentioned).

Of course, Enumerable.Any is way better than both of these implementations. :smuglinq:

# ? Oct 8, 2011 08:23

Jabor: Jul 16, 2010; #1 Loser at SpaceChem

ninjeff posted:

That's not a real horror IMO; just misguided. It's a good technique to use where errorList isn't backed by an array, and where you actually dispose the enumerator (as already mentioned).

It's certainly better than foo.Count(), but the general contract of a property is that it functions like a field with a bit of extra functionality hooked up to it.

If something provides a Count property of its own accord, you can take it as given that that is efficient to access. If it implements an interface that requires it to implement that property, then you can generally assume that it's doing the necessary caching/maintaining that value behind the scenes.

If something is expensive to determine and the object doesn't cache that value, then it should be a retrieval method, not a property.

# ? Oct 8, 2011 08:36

ninjeff: Jan 19, 2004

Jabor posted:

It's certainly better than foo.Count(), but the general contract of a property is that it functions like a field with a bit of extra functionality hooked up to it.

If something provides a Count property of its own accord, you can take it as given that that is efficient to access. If it implements an interface that requires it to implement that property, then you can generally assume that it's doing the necessary caching/maintaining that value behind the scenes.

If something is expensive to determine and the object doesn't cache that value, then it should be a retrieval method, not a property.

I agree, but the code posted was a method.

edit: maybe that's the real horror

ninjeff fucked around with this message at 08:53 on Oct 8, 2011

# ? Oct 8, 2011 08:50

New Yorp New Yorp: Jul 18, 2003; Only in Kenya.; Pillbug

Jabor posted:

It's certainly better than foo.Count(), but the general contract of a property is that it functions like a field with a bit of extra functionality hooked up to it.

If something provides a Count property of its own accord, you can take it as given that that is efficient to access. If it implements an interface that requires it to implement that property, then you can generally assume that it's doing the necessary caching/maintaining that value behind the scenes.

If something is expensive to determine and the object doesn't cache that value, then it should be a retrieval method, not a property.

In that particular case, errorList had a Count property. There was another instance that did the same thing but was IEnumerable, which I replaced with .Any().

ninjeff posted:

I agree, but the code posted was a method.

edit: maybe that's the real horror

Most of this software is a horror. :cry:

I'm improving what I can, when I can, without freaking the Crusty Old Dudes out. Sadly, during the interview process, I took the presence of Crusty Old Dudes to be a good sign.

I hope that when I'm 60, some guy half my age isn't posting on SomethingTwitterBlogIpad.com making fun of me.

New Yorp New Yorp fucked around with this message at 09:17 on Oct 8, 2011

# ? Oct 8, 2011 09:10

Zombywuf: Mar 29, 2008

The real horror is that ICollection doesn't provide a way to tell if it's empty.

# ? Oct 8, 2011 10:37

plushpuffin: Jan 10, 2003; Fratercula arctica; Nap Ghost

Jabor posted:

It's certainly better than foo.Count(), but the general contract of a property is that it functions like a field with a bit of extra functionality hooked up to it.

If something provides a Count property of its own accord, you can take it as given that that is efficient to access. If it implements an interface that requires it to implement that property, then you can generally assume that it's doing the necessary caching/maintaining that value behind the scenes.

If something is expensive to determine and the object doesn't cache that value, then it should be a retrieval method, not a property.

I have to work on an application for which this isn't true, and it's a nightmare. Calling A.B.C.D.E.F is pretty commonplace, and each call always involves a SQL query and a home-made cache implementation (which never, ever removes anything from its cache).

Here's what happens when you call a property on an object to get a related object:

A.B calls ObjectCache.GetB(Guid of B).
GetB() runs a SQL query generated by the Visual Studio Designer (this is important later).
GetB() passes the DataTable result to RetrieveObject<T>(DataTable).
RetrieveObject<T>() calls RetrieveObjects<T>(DataTable) and returns the first element in the returned array.
RetrieveObjects<T>() merges the DataTable passed in with the ObjectCache's DataSet (updating any rows that have changed in the database).
RetrieveObjects<T>() looks for the Guids of B[] in a Dictionary<Guid, CacheObject>, and returns an array B[] if found (A, B, C, etc, are all derived types of type CacheObject, and each CacheObject is just a wrapper around a DataRow).
If not found, RetrieveObjects<T> must now find the DataRows in the DataSet and construct wrapper objects around them. It does this by trying to parse the where clause from step #2 and passing it to DataTable.Select("X and Y and Z etc") of the DataTable associated with type B in the DataSet.
If the query was too "complicated" to be used in this fashion (if it had a "JOIN" in it) then it performs multiple primary key lookups on the DataTable using DataTable.Find(object) and constructs an array B[] with the references to these cache objects and returns them. Note that this definition of "complicated" is laughable because it doesn't check for anything (eg: UNION, IN, implicit join) other than "JOIN" (and the search for "JOIN" is case-sensitive, because all of the original SQL queries in the XSD file were created using a visual designer that capitalized all of these keywords).

Performing any operation on more than one base object at once takes minutes or hours thanks to this object model, and since the cache never throws anything out, ever, memory usage just keeps growing indefinitely until the application is closed.

plushpuffin fucked around with this message at 19:22 on Oct 8, 2011

# ? Oct 8, 2011 18:15

Cocoa Crispies: Jul 20, 2001; Vehicular Manslaughter!; Pillbug

plushpuffin posted:

I have to work on an application for which this isn't true, and it's a nightmare. Calling A.B.C.D.E.F is pretty commonplace, and each call always involves a SQL query and a home-made cache implementation (which never, ever removes anything from its cache).

[�]

Performing any operation on more than one base object at once takes minutes or hours thanks to this object model, and since the cache never throws anything out, ever, memory usage just keeps growing indefinitely until the application is closed.

Whoa you mean a code horror is in fact a code horror, you don't say?

(ps ActiveRecord supremacy:)

code:

Client.includes("orders").where(:first_name => 'Ryan', :orders => {:status => 'received'}).count

# => SELECT count(DISTINCT clients.id) AS count_all FROM clients
#  LEFT OUTER JOIN orders ON orders.client_id = client.id WHERE
#  (clients.first_name = 'Ryan' AND orders.status = 'received')

And if you've already materialized the query into an Array before you call count, it just uses Array.count.

# ? Oct 8, 2011 19:49

Dessert Rose: May 17, 2004; awoken in control of a lucid deep dream...

Zombywuf posted:

The real horror is that ICollection doesn't provide a way to tell if it's empty.

!IEnumerable.Any() seems to work well enough?

# ? Oct 8, 2011 21:40

Sedro: Dec 31, 2008

Implementing ICollection implies that Count and Any() AKA GetEnumerator().HasNext() are both trivial operations so it doesn't really matter what you do. IEnumerable doesn't guarantee that the first item has been fetched so Any() might be nontrivial but you have no other choice. You could ensure that Count is called even on an upcasted ICollection like Microsoft does with Count()...

code:

public static bool IsEmpty<T>(this IEnumerable<T> source)
{
    if (source == null)
        throw new ArgumentNullException("source");
    if (source is ICollection<T>)
        return ((ICollection<T>)source).Count == 0;
    if (source is ICollection)
        return ((ICollection)source).Count == 0;
    return !source.Any();
}

But the checks are pointless, hence why MS didn't implement Any() this way (last I checked).

# ? Oct 8, 2011 22:49

Cocoa Crispies: Jul 20, 2001; Vehicular Manslaughter!; Pillbug

Sedro posted:

code:

public static bool IsEmpty<T>(this IEnumerable<T> source)
{
    if (source == null)
        throw new ArgumentNullException("source");
    if (source is ICollection<T>)
        return ((ICollection<T>)source).Count == 0;
    if (source is ICollection)
        return ((ICollection)source).Count == 0;
    return !source.Any();
}

Why is there a check for if the source is ICollection<T> when you just try treating it as an ICollection next?

# ? Oct 8, 2011 22:53

Sedro: Dec 31, 2008

Because ICollection<T> doesn't implement ICollection :doh:

# ? Oct 8, 2011 23:35

tef: May 30, 2004; -> some l-system crap ->

i guess you're saying you want type erasure? :v:

# ? Oct 9, 2011 01:10

Cocoa Crispies: Jul 20, 2001; Vehicular Manslaughter!; Pillbug

Sedro posted:

Because ICollection<T> doesn't implement ICollection

ICollection<T> can't even be cast to a ICollection?

# ? Oct 9, 2011 01:16

Cocoa Crispies: Jul 20, 2001; Vehicular Manslaughter!; Pillbug

tef posted:

i guess you're saying you want type erasure?

I want duck typing :shobon:

# ? Oct 9, 2011 01:19

Zhentar: Sep 28, 2003; Brilliant Master Genius

BonzoESC posted:

ICollection<T> can't even be cast to a ICollection?

MSDN posted:

public interface ICollection<T> : IEnumerable<T>, IEnumerable

So no.

Sedro posted:

code:

public static bool IsEmpty<T>(this IEnumerable<T> source)
{
    if (source == null)
        throw new ArgumentNullException("source");

My single favorite thing about programming in Objective-C is that things like [myArray count] will return 0 whether myArray is null or empty. 99% of the time I want to know if the list contains something, and whether it exists and is empty or does not exist at all is irrelevant, but pretty much every other C-style language makes you spell it out every time.

# ? Oct 9, 2011 03:11

Sedro: Dec 31, 2008

You don't have to throw in an extension method but it can be a little awkward because people expect an instance method (or what appears to be an instance method) to throw when called on a null value.

code:

[NotNull]
public static IEnumerable<T> OrEmpty<T>([CanBeNull] this IEnumerable<T> source)
{
    return source ?? Enumerable.Empty<T>();
}

code:

int count;
if (mList != null)
    count = myList.Count();
else
    count = 0;

// becomes...
int count = myList.OrEmpty().Count();

Null is the real horror.

# ? Oct 9, 2011 03:57

ninjeff: Jan 19, 2004

Sedro posted:

Null is the real horror.

please please please non-nullable reference types in the next windows platform M$

# ? Oct 9, 2011 09:28

Zhentar: Sep 28, 2003; Brilliant Master Genius

Sedro posted:

You don't have to throw in an extension method but it can be a little awkward because people expect an instance method (or what appears to be an instance method) to throw when called on a null value.
code:
[NotNull]
public static IEnumerable<T> OrEmpty<T>([CanBeNull] this IEnumerable<T> source)
{
    return source ?? Enumerable.Empty<T>();
}

Oh hey! I knew that throwing exceptions on null this in extensions was optional, and I'd started considering taking advantage of that, but it hadn't yet occurred to me to allow chaining by returning a default value. Looks like I'm gonna be refactoring some code this weekend!

# ? Oct 9, 2011 21:25

Xenogenesis: Nov 8, 2005

https://code.google.com/p/phpreboot/

Surely this is some brilliant troll?

# ? Oct 9, 2011 21:42

nielsm: Jun 1, 2009

Xenogenesis posted:

https://code.google.com/p/phpreboot/

Surely this is some brilliant troll?

"It has keywords such as if and elseif just like PHP, so therefore it is pretty much the same as PHP."

It's written in Java.

Sure, it may be interesting in its own right, it doesn't look like a terrible idea, but it has those two major faults.

E: Hey you know, they should call it PHPScript. It's related to PHP the same way JavaScript is related to Java.

# ? Oct 9, 2011 21:55

Jabor: Jul 16, 2010; #1 Loser at SpaceChem

It's like they deliberately took the worst parts of Javascript as well - automatic semicolon insertion? Seriously?

# ? Oct 9, 2011 22:13

Vanadium: Jan 8, 2005

They should just take the "don't need semicolons" thing from python instead of javascript.

# ? Oct 9, 2011 22:38

baquerd: Jul 2, 2007; by FactsAreUseless

Vanadium posted:

They should just take the "don't need semicolons" thing from python instead of javascript.

Forced fixed indentation mixed with php would be so much fun since you can interleave html with php. Maybe mix some tapestry in there for good luck.

code:

<html>
  <body>
    <table>
      <tr>
        <td>Header 1</td>
        { 
          headersCondensed = 
        } 
          <span jwcid="@Insert" value="ognl:areHeadersCondensed" raw="true">
        {
          if (headersCondensed ) {
        }  
            <td>
              <span jwcid="@Insert" value="ognl:condensedHeaderField" raw="true">
            </td>
        {
          } else { 
        }
            <td jwcid="@Foreach" source="ognl:headerList" value="ognl:header" element="td" />
        {
          } //end if.. no confusion with the php opening tags due to indentation
        }
      </tr>
      <!-- table data -->
    </table>
  </body>
</html>

I get a hardon just thinking about debugging that on a per hour basis.

# ? Oct 9, 2011 23:17

tef: May 30, 2004; -> some l-system crap ->

Xenogenesis posted:

https://code.google.com/p/phpreboot/

Surely this is some brilliant troll?

I think this is cool :3:

# ? Oct 10, 2011 00:26

Plorkyeran: Mar 22, 2007; To Escape The Shackles Of The Old Forums, We Must Reject The Tribal Negativity He Endorsed

baquerd posted:

Forced fixed indentation mixed with php would be so much fun since you can interleave html with php.

no semicolons and semantically significant indentation have nothing to do with each other

# ? Oct 10, 2011 01:01

NotShadowStar: Sep 20, 2000

While a good effort it will never, ever catch on. A long time PHP programmer's response is still 'gently caress it, just echo html back out to the page" even in frameworks that do everything they can to discourage that poo poo. I still see that poo poo suggested when working with say, drupal, where there's a clearly defined rendering path at the end of the processing chain, people seriously suggest just vomiting html straight to the page in the middle of processing functions. Ergh.

# ? Oct 10, 2011 01:12

Opinion Haver: Apr 9, 2007

I think my favorite part is how curly braces are used both for control flow and for delimiting code from HTML. Also raw XPath and URIs, both of which contain the end-of-line comment token :allears:

# ? Oct 10, 2011 01:16

baquerd: Jul 2, 2007; by FactsAreUseless

Plorkyeran posted:

no semicolons and semantically significant indentation have nothing to do with each other

Right, my example was just running with the python mention.

# ? Oct 10, 2011 01:17

nielsm: Jun 1, 2009

yaoi prophet posted:

I think my favorite part is how curly braces are used both for control flow and for delimiting code from HTML. Also raw XPath and URIs, both of which contain the end-of-line comment token

Well the first thing is no better in JSP, they also have XML-ish tags for serverside processing that seem to be able to live just fine inside non-XML, as far as I know. They also have some curly brace function/data insertion syntax thing. (And then there's the three different processing instruction tags.) In summary, JSP has three kinds of special tags, one of which has yet three sub-categories.

I really don't see how an URI should be a syntax element of its own, and not just use string literals. It can't be a good idea to put them directly in the code anyway, that kind of stuff IMO falls under configuration.

# ? Oct 10, 2011 01:33

ToxicFrog: Apr 26, 2008

Jabor posted:

It's like they deliberately took the worst parts of Javascript as well - automatic semicolon insertion? Seriously?

Is there a significant difference between "automatic semicolon insertion" and "optional semicolons"? I can't see one, but the former seems to be reviled whereas most people seem to either not care about the latter or consider it a good thing.

# ? Oct 10, 2011 04:11

Opinion Haver: Apr 9, 2007

In Javascript

code:

i
++

gets parsed as

code:

i;
++

which is obviously an error.

# ? Oct 10, 2011 04:16

Jabor: Jul 16, 2010; #1 Loser at SpaceChem

It gets even better!

code:

var x = foo
  (doStuff())

Are you assigning a function to a variable, and then calling a different function? Or are you assigning the result of a function call to a variable? :iiam:

Or the one that always gets people:

code:

return
  doFirst() +
  doSecond()

# ? Oct 10, 2011 04:44

ToxicFrog: Apr 26, 2008

Oh. So "automatic semicolon insertion" is "a semicolon is inserted at each newline if one is not already present".

:gonk:

# ? Oct 10, 2011 04:46

Pimblor: Sep 13, 2003; bob; Grimey Drawer

yaoi prophet posted:

In Javascript
code:
i
++
gets parsed as
code:
i;
++
which is obviously an error.

gently caress everything about that. That and the weird requirement to use embracing style of braces, even though it's technically legal syntax to use the Allman style. I can deal with non Algol like languages like Python or god forbid VB, but gently caress JavaScript in its rear end for that one.

# ? Oct 10, 2011 04:47

Adbot: ADBOT LOVES YOU

# ? May 25, 2024 14:58

Plorkyeran: Mar 22, 2007; To Escape The Shackles Of The Old Forums, We Must Reject The Tribal Negativity He Endorsed

ToxicFrog posted:

Oh. So "automatic semicolon insertion" is "a semicolon is inserted at each newline if one is not already present".

The rules are actually significantly more complicated than that, but it pretty much comes down to that if a line doesn't end in a semicolon and it would syntactically valid to have a semicolon at end of the line, the interpreter tries its hardest to do the opposite of what you wanted it to.

# ? Oct 10, 2011 04:59

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!

«‹›1503 »