Some anti-patterns I am experiencing first hand

I am working on a rather interesting WPF project, which “implements” so many anti-patterns I had to blog about it. The most interesting thing about this project is that it was written by some of the best and smartest programmers I know, so the anti-patterns stem not so much from incompetence, but from general conditions these programmers were forced to face and maybe general lazyness and go-with-the-flow.

1. Prototype-turned-project. The code is simply not production quality. Logging is non-existent, errors are swallowed (as in “try {...} catch {}“), user facing messages have form of exception traces without headers and icons, database tables don’t have indexes, and so on, and so forth.

   I was told that initially the customer gave the programmers a long list of features and a very short deadline (“demand the impossible if you want to get the maximum”). The results were good functionally, but code quality suffered. Of course, this is only one side of the story.

2. Cut-and-paste code. Well, you know what I am talking about. When you have SourceFilterButton and SourceFilterButtonA, and everything in code has a regular version and an “A” version that differs ever so slightly, you have a case of cut-and-paste programming.
3. Using generic DataSets all over the place. This definitely takes the Object-Relational Mapping issues out of the way, because there are no objects. But then everything is just a glorified array, and the dependencies on data are implicit. E.g. by looking at...
   C#

   void ProcessScenario( DataRow dtScenario );

   ...it is impossible to tell what columns this method expects to see in dtScenario and what types they should be. This dependency is implicit, and implicit dependencies are evil, because they are broken easily. This code also assumes that all information about a scenario can be coded in a single data row. If the scenario becomes something more involved, e.g., it now includes an array of sub-scenarios with their own settings, this code will have to be heavily modified.

   Also, as DataTable and DataRow become prevalent parameter types, they start to be used as a means of communication between different parts of the code, without ever going to the database:

   C#

   void ProcessScenario( DataRow row )
   {
       int id = (int)row["ID"];
       double amount = (double)row["AMOUNT"];
       ...
   }
   
   void CallProcessScenario( double amount )
   {
       DataRow row = scenarioTable.NewRow();
       row["ID"] = myId;
       row["AMOUNT"] = amount;
       ProcessScenario(row);
   }

   C#

   void ProcessScenario( DataRow scenario );

4. Mixed business and presentation logic. It is mauvais ton to pass data between two business logic units through UI. This makes writing unit tests impossible:
   C#

   moviesGrid.DataContext = SomeTable;
   ...
   void LookupMovieById(string id)
   {
       DataTable movies = (DataTable)moviesGrid.DataContext;
       ...
   }

   The movies table should be stored in a model object and UI should not be involved:

   C#

   class Model
   {
       public DataTable Movies { get; private set; }
   
       public void LookupMovieById( string id )
       {
            .... Movies ...
       }
   }
   
   class MyView
   {
       Model _model;
       ...
       moviesGrid.DataContext = _model.Movies;
       ...
       _model.LookupMovieById(...);
   }

5. Pseudo Hungarian notation and abbreviations in variable names. Names like dtCurrVect are evil. currentVector is better, currentPriceHistory is better still.
6. Big classes using #regions Whenever you feel like you need to add a “#region” to your class, it probably has become too big. It should be split into smaller classes. I've seen a class responsible for three totally different (albeit) related screens, handling both business and presentation logic for all. Figuring out what depends on what was a nightmare.
7. Mixing data and UI concerns is evil and makes business logic impossible to unit test. The data must be calculated in a manner totally segregated from UI, and then, if necessary, assigned as a data source to the UI object. WPF is property based, so exposing data sources as properties allows to use bindings in XAML and generally move closer to nirvana. E.g.
   C#

   void Bind( DataGrid grid, string filter )
   {
      BindImpl(grid, fitler, field);
   }
   
   void BindImpl( DataGrid grid, string filter, string field )
   {
      grid.ItemsSource = new DataView(... filter, ... field);
   }
   
   Bind(gird, "foo=3");

   is refactored to:

   C#

   DataView GetData(string filter )
   {
      return GetDataImpl(filter, field);
   }
   
   DataView GetDataImpl( string filter, string field )
   {
      return new DataView(...);
   }
   
   grid.ItemsSource = GetData("foo=3");

   This way, obtaining the data (business logic) is clearly separated from assigning this data to the grid (presentation).

8. Keeping logic in stored procedures. Stored procedures are written in another language, they are difficult to debug, difficult to put in source control, difficult to refactor, and generally they belong to another world several light years away. If you need server-side processing, write a layer on top of your database (a web service perhaps?) and do it there. In my book stored procedures are warranted only in case of extreme performance problems, when they save tons of data going back and forth between the database and the web server.

   In the course of this project, a significant piece of logic was moved from C# to a stored procedure (because it seemed more efficient that way), and then back, when we needed to add something a database cannot handle, like solving an equation using binary search. Each movement between C# and stored procedure required a lot of work and created a number of bugs.

Conclusion

Never, ever let your prototypes to become product. If there is a remote chance of this happening, treat your prototype as the real thing, or you will be sorry later. Whenever a sales person approaches you asking to write "a little throw-away demo" know that they are most likely lying. They will never tell the customer this is a throwaway, and if they succeed in selling that demo, the customer will assume it's already a product, and will demand new features, that you will be pressed to deliver ASAP. So, you have a good chance to get stuck with that "throw-away demo" for a long time.

CodeProject