Ayende @ Rahien

Mike Scott commented on Pitfalls

Fri, 07 Aug 2009 12:15:32 GMT

The recent fad for using the so-called "safe" type cast with the "as" keyword instead of using a regular type cast, e.g... var foo = bar as Foo; ... foo.SomeMethod(); instead of: var foo = (Foo) bar; ... foo.SomeMethod(); I'm seeing it more and more in sample code both from Microsoft and from bloggers. I think the people doing this believe that it's "safer" because "as" won't throw an exception. The problem, of course, is that it's not safe at all when used without a null check, and so it replaces a fail fast with a fail slow, where your code will blow up some time later with a useless and confusing null reference exception instead of a helpful invalid cast exception at the point of failure.

Richard Nagle commented on Pitfalls

Thu, 30 Jul 2009 14:30:32 GMT

//open the database database.Open(); //create a command var command = database.CreateCommand("GetCustomers"); //execute the command command.Execute(); agggghhhhhhhhhhhhh!

Jeremy Wiebe commented on Pitfalls

Wed, 29 Jul 2009 15:44:38 GMT

I get a little antsy when I see inheritance from List

Nick Aceves commented on Pitfalls

Wed, 29 Jul 2009 08:57:59 GMT

Methods that are more than 20 lines long. Classes more than 100 lines long. Untested code. Non-infrastructure code that reaches into an IOC container. There are legit uses, but it always makes me stop and think for several minutes about whether I'm doing something wrong. Comments that describe what the code does. It makes me worry about the readability of the code itself, because apparently whoever wrote it didn't think it was self-explanatory at the time.

Ali Hmer commented on Pitfalls

Wed, 29 Jul 2009 03:06:09 GMT

The over use of String Concatenation vs. StringBuilder.Append specially in a loop block

Cedric Vivier commented on Pitfalls

Tue, 28 Jul 2009 10:12:34 GMT

Any usage of DateTime for measuring a real-time duration, even with the modified code using DateTime.UtcNow, breakage is still possible if the clock is changed. Using Stopwatch is the way to go when measuring any duration, moreover using a monotonic clock insensitive to any clock/timezone change, it allows more concise code : private class OpenState : CircuitBreakerState { private readonly Stopwatch ttl; public OpenState(CircuitBreaker circuitBreaker) : base(circuitBreaker) { ttl = Stopwatch.StartNew(); } public override void ProtectedCodeIsAboutToBeCalled() { if(ttl.Elapsed > circuitBreaker.timeout) throw new OpenCircuitException(); circuitBreaker.MoveToHalfOpenState(); } }

Chris Smith commented on Pitfalls

Tue, 28 Jul 2009 09:00:18 GMT

@Rik Hemsley Agree entirely about tests. Unfortunately our environment is "business driven design" which basically means anything which doesn't appear on the screen isn't being productive (that includes test cases). Mitigating risk happens after the thing collapses. I shit you not. Regarding .Single() - if it wasn't for above I'd agree but it doesn't promote awareness that "that might blow up". (Yes i am losing my hair rapidly)

Reshef commented on Pitfalls

Tue, 28 Jul 2009 07:38:01 GMT

I meant suffixed...

Reshef commented on Pitfalls

Tue, 28 Jul 2009 07:23:16 GMT

One more thing - class which their names are prefixed with the word "Manager" - usually a sign for a god class.

Reshef commented on Pitfalls

Tue, 28 Jul 2009 07:19:15 GMT

1) Too many if statements that tries to handle business concerns. Usually it means that the class knows too much and tries to handle to much and violates SRP. 2) Using WCF - It is never as simple just creating a client proxy and calling it as a local object. It abstracts too many default values and ignores the fallacies of distributed computing by design and in order to handle this u have to resort to downcasting to ICommunicationObject in order to handle faulted state etc.

Jorge Alves commented on Pitfalls

Mon, 27 Jul 2009 22:52:23 GMT

Serious code style issues: * commented code * overuse of abbreviations * spelling errors (even in comments) All are signs of rushed out code, lack of attention to detail, and poor maintainabilty.

Duncan Godwin commented on Pitfalls

Mon, 27 Jul 2009 21:10:02 GMT

- Overuse of exceptions - Lots of duplicate cut&paste style code with minor differences - Assigning a constructor argument to an static variable - Having a single static database connection in ASP.NET - Roll your own locks, such as using ASP.NET's cache to object to indicate you are doing something, e.g. Cache["rebalancingTables"]. - lock (new object())

Andrew commented on Pitfalls

Mon, 27 Jul 2009 20:37:35 GMT

- When anyone I work with explains a solution to a problem and it takes more than a few sentences. If that happens, I'll then go look at the code expecting the worst. - Anytime I see a #region within a method call, that's always scary because that means the method is probably huge and over-engineered.

Stephen commented on Pitfalls

Mon, 27 Jul 2009 19:04:17 GMT

I don't think dumb objects are pitfalls, if anything they are just overly safe, they don't try and do anything knowing that any functionality they give would be contextual.. and saying static classes work to assist them is just trying to make them sound terrible.. dumb data objects certainly never imply static helpers. I think one of the biggest challenges for a developer is picking when to embed functionality vs having stood off functionality..

janwillemb commented on Pitfalls

Mon, 27 Jul 2009 18:45:42 GMT

The use of an object that implements IDisposable, but not properly calling Dispose or make use of "using". It is (or should be) an obvious bug, but happens a lot for example with ResponseStreams and RequestStreams of WebRequests, because you often can come away without Dispose()-ing. And, according to Eccl 8:11, "When the sentence for a crime is not quickly carried out, the hearts of the people are filled with schemes to do wrong."

mattmc3 commented on Pitfalls

Mon, 27 Jul 2009 17:01:58 GMT

@Dmitry - I'm glad you mentioned checking the length of a string before calling Substring(). Run time errors due to poor substring calls were such a common occurrence in a project I was working on that I finally broke down and created an extension method on the String class that will always succeed and doesn't care about starting offset or length. Now, whenever I see Substring() used at all in this codebase without a comment about why an exception due to offset/length is legit, it's a code smell.

Diogo commented on Pitfalls

Mon, 27 Jul 2009 16:26:12 GMT

- Parsing DateTime/decimal withou using CultureInfo - Checking for empty strings using == "" instead of String.IsNullOrEmpty

Jason Y commented on Pitfalls

Mon, 27 Jul 2009 15:32:08 GMT

-doing math with dates -representing too much data with strings (e.g. comma-separated lists of things, including dates or numbers) -any interaction with legacy databases

Dmitry commented on Pitfalls

Mon, 27 Jul 2009 15:22:20 GMT

@NotMyself, I think that's the definition of spaghetti code which is certainly a huge pitfall.

NotMyself commented on Pitfalls

Mon, 27 Jul 2009 14:58:05 GMT

I don't know if this is a pitfall, but it always makes me pause and pay closer attention to what I am looking at. if(Call.ToSomethingByName("Name") == "foo") { if(Call.ToSomethingByName("Name") !="bar") var something = Call.ToSomethingByName("Name"); else var someotherthing = Call.ToSomethingByName("Name"); } Just seeing this kind of construct tells me the person who wrote it was not thinking about anything other than "make it work". My current codebase is littered with gems like this. I try to refactor them to a single call to the by name method when ever I find them.

Kyle Szklenski commented on Pitfalls

Mon, 27 Jul 2009 14:14:54 GMT

I will be writing a series of blog posts about this soon. I'm moving my blog, so I'll come back and post when I do. Here's a hint of things to come: I second Neil's downcasting comment. Here's what I'll add: 1) Enums - if I see enums, I immediately start looking for the switch statement and/or if-elseif-else statements. 2) Switches/if-elses based on types - self-evident 3) Inheritance structures longer than, say, three deep 4) Extremely dumb objects, e.g. data-holders - DTOs are fine, but when you have "business objects" that have no functions because anything that is done to them is stored in some static class, you're going to have trouble with that. This points straight to anemia of the domain model. 5) Close to Jeff Brown's number 3, when a separate class has to validate that a certain class is currently in a valid state - For example, I recently saw code where a person had a class that had a generic List in it. This class was like number 4 above, and as such, when another class had to use it, the other class had to first check to see if that list was null, and if it was, instantiate it! That just about made me vomit on my keyboard. 6) Parameter lists longer than two - 99% of the functions you write probably don't need more than two parameters passed in. If more than that are necessary, I think that points to a lack of OO thinking in the design.

Dmitry commented on Pitfalls

Mon, 27 Jul 2009 12:20:46 GMT

1. "str == String.Empty" instead of "String.IsNullOrEmpty(str)" 2. Calling "str.Substring()" method without checking the string length. 3. Implementing Dispose() method without the Dispose(bool) virtual method in non-sealed classes. 4. Creating finalizers in classes without unmanaged code/expensive resources. 5. Using "lock(typeof(MyClass))" instead of having a separate syncRoot object can cause deadlocks. 6. Returning IEnumerable

Neil Mosafi commented on Pitfalls

Mon, 27 Jul 2009 11:56:02 GMT

Downcasting

Demis Bellot commented on Pitfalls

Mon, 27 Jul 2009 11:50:26 GMT

Well DateTime.Now has its own problems like you can't rely on it for measuring micro-benchmarks as its inaccurate and you need to use Stopwatch() instead. Other than that I think the biggest problems actually have to do with how most architecture is centred around proposed Microsoft technology which overtime has proved to be inferior time after time. e.g. DataSets/DataTables (as opposed to ORM) - Filling generic datasets/datatables and have them binded directly on the view. ASP.NET webforms (as opposed to MVC) - Encourages building coupled (spaghetti-style) / stateful / non-testable websites. - SOAP 1.x / WS-* (as opposed to REST + POX / JSON) - Is pretty much an unnecessary technology that just adds development and runtime performance overhead and complexity. - Operation / ServiceContracts - ServiceContracts encourages the use of chatty RPC-style service calls instead of SOA document-centric-style ones. This is a problem because whenever you cross a process-boundary you want to reduce the chatter as much as possible (Note: My opensource web-services stack effectively encourages building SOA-style services: [http://code.google.com/p/servicestack](http://code.google.com/p/servicestack)). - No Distributed Hash Table (starting now with Velocity) - Basically SQL Server is positioned as the hammer to handle all your persistence means, which effectively limits your performance and scalability unless you've implemented your own sharding solution from the start. - Strong emphasis on web services and poor MQ technology (and no pub/sub). - Web services are not always the best technology to communicate between services, they have no-reliability, high coupling between endpoints, poor support for long running transactions etc. Certain tasks like offline and processor intensive tasks, extensible and low coupling between services, long running tasks, high payloads (well not MSMQ anyways) etc, are just better handled with a MQ technology and MSMQ is very lacking in this area. Unfortunately the ease of use and productivity of Microsoft products can lead projects down the wrong architecture path for their application if they're solely relying on Microsoft technology (and documentation) for their applications architecture.

James commented on Pitfalls

Mon, 27 Jul 2009 11:23:20 GMT

Not using frameworks like xVal and duplicating validation logic in both the model and the views.

Jeff Brown commented on Pitfalls

Mon, 27 Jul 2009 11:13:10 GMT

Lots of stuff... 1. Any code that creates a thread, starts a timer, uses a lock or accesses shared state. 2. Classes that implement IDisposable or store disposable resources in fields. 3. Classes that can exist in a partially initialized state (eg. have a separate Initialize method). 4. Finalizers. 5. Recursive deletion of directories. (Did the code handle symlinks? Were the paths correctly constructed?) 6. Retry loops and compensatory actions. 7. Anything "clever."

Rik Hemsley commented on Pitfalls

Mon, 27 Jul 2009 11:11:44 GMT

First and FirstOrDefault have different intentions from Single and SingleOrDefault.

Rik Hemsley commented on Pitfalls

Mon, 27 Jul 2009 11:10:45 GMT

I don't ensure code is bug free by simply reading it. Code without tests is code which sets off alarm bells.

Demis Bellot commented on Pitfalls

Mon, 27 Jul 2009 11:07:08 GMT

@Chris If you don't want it to fail you probably want to be using .First() or .FirstOrDefault()

Boris Modylevsky commented on Pitfalls

Mon, 27 Jul 2009 10:55:38 GMT

It depends on project, but there are general things that "causes a flashing neon", like: 1. String comparison using == operator when case is irrelevant, instead of String.Compare(str1, str2, true) 2. Complex dynamic SQL statements for searching "Google-like" results, like "WHERE col1 LIKE '%x%' OR col2 LIKE '%y%' " etc 3. Incorrect usage of data structures, for example List<> when O(1) access is required etc