CodeSOD: Abuse of Properties

Every .NET programmer is familiar with the concept of properties. They're a nice language feature, allowing the programmer to inject little bits of logic into the process of retrieving or setting the value of a field. A getter can, for example, lazily initialize a field when it's first used, and a setter can validate the value before it's set. Even a simple property with no logic beyond providing access to a backing field can be made useful by appearing in an interface or being overridden by a derived class.

An important aspect of properties is that to the outside user, they appear almost no different from a regular field. As such, they're supposed to behave like regular fields. Any visible side effects beyond what's necessary are heavily frowned upon, and the developer who uses the property can reasonably expect it to provide transparent access to data from his point of view.

KallDrexx's coworker, however, was forced to deal with code made by people who didn't seem to understand the "transparent" part- or, in fact, any part"

public IRevObjApi SelectedRevObj{ get { if (IdList.Count == 0) return null; if (IdList.SelectedIndex == -1) IdList.SelectedIndex = 0; IdList.SDescrColumnName = "PIN"; IdList.DescrColumnName = "AIN"; return (IRevObjApi)IdList.List[IdList.SelectedIndex]; }}

This monstrosity lurked in one of the fundamental classes of a web framework his company was using. Along with the reasonable task of retrieving the selected value from the list, it silently changes the list's describing column to what it thinks is right- except in this case, there is no such column, and the application falls flat on its face.

Digging further uncovered even more WTFs...

public int SelectedRevObjID{ get { if (IdList.Count == 0) return -1; if (IdList.SelectedId == -1) IdList.SelectedIndex = 0; //Check to see if there is really anything in the list //if not, go to search //if (IdList.SelectedId == -1) //tfs 40899 commented automatic Search // Navigate("CVGSCriteria"); IdList.SDescrColumnName = "PIN"; IdList.DescrColumnName = "AIN"; return IdList.SelectedId; } set { //First check to be sure id != 0 if (value != 0) { //Need to build the IDList var list = new ArrayList(); IRecordsApi rapi = ApiFactory.CreateRecordsApi(View.CurrentDataAccessContext); IRevObjApi roApi = rapi.GetRevObjAllById(Convert.ToInt32(value), View.SecurityToken); list.Add(roApi); IdList.List = list; IdList.IdColumnName = "Id"; IdList.SelectedIndex = 0; IdList.SDescrColumnName = "PIN"; IdList.DescrColumnName = "AIN"; } }}

This property is supposed to retrieve the ID of the currently selected object. The getter is every bit as bad as the previous one. Luckily, someone came to their senses and commented out the part that redirects the user to a search page out of the blue if no object was selected.

The setter manages to be even worse. It does nothing when passed a zero, without informing the user of their mistake- then it gets the object from an API using the provided ID (which is helpfully converted from an integer to an integer), erases the current list, and creates a new one with the newly obtained object as its sole item. At the end, as if to spite the user even more, it resets the column names again.

The coworker bravely continued digging into other parts of this code- only to be confronted by what, if the codebase were a video game, would be the final boss:

/// /// Gets the datasource for the IdList grid/// private DataTable IDListSectionDataSource{ get { IList idList = Controller.IdList.List; if ( idList == null ) return null; string sDescrColumnName = Controller.IdList.SDescrColumnName; string descrColumnName = Controller.IdList.DescrColumnName; string idColumnName = Controller.IdList.IdColumnName; int idColumn = Controller.IdList.IdColumn; int currentId = -1; string descr = ""; string sDescr = ""; object objId; PropertyInfo property; MethodInfo method; if ( ( idList is LiteCollection ) || ( idList is DataView ) ) { // Check if the embedded table can be used directly DataTable embeddedTable; if ( idList is LiteCollection ) embeddedTable = ( (LiteCollection) idList ).DataView.Table; else embeddedTable = ( (DataView) idList ).Table; if ( embeddedTable.Columns[ idColumnName ] != null ) return embeddedTable; else return null; } // Define the bind table; DataTable bindTable = new DataTable(); bindTable.Columns.Add( idColumnName ); if ( sDescrColumnName != null ) bindTable.Columns.Add( sDescrColumnName ); // Otherwise we have to loop through all entries int count = idList.Count; for ( int i = 0 ; i < count ; i++ ) { DataRow newRow = bindTable.NewRow(); descr = ""; sDescr = ""; object item = idList[ i ]; if ( item == null ) continue; // Check if this object has a public property for Id property = item.GetType().GetProperty( idColumnName ); if ( property == null ) continue; method = property.GetGetMethod(); if ( method == null ) continue; objId = method.Invoke( item, new object[ 0 ] ); if ( objId == null ) continue; try { switch ( objId.GetType().Name.ToLower() ) { case "int32": currentId = (int) objId; break; case "decimal": currentId = decimal.ToInt32( (decimal) objId ); break; } } catch { continue; } newRow[ idColumnName ] = currentId; // Check if there is a sdescr column available if ( sDescrColumnName != null ) { // Check if this object has a public property with the sdescr name property = item.GetType().GetProperty( sDescrColumnName ); if ( property != null ) { method = property.GetGetMethod(); if ( method != null ) { sDescr = method.Invoke( item, new object[ 0 ] ) as string; // Check if there is a descr column available if ( descrColumnName != null ) { // Check if this object has a public property with the descr name property = item.GetType().GetProperty( descrColumnName ); if ( property != null ) { method = property.GetGetMethod(); if ( method != null ) { descr = method.Invoke( item, new object[ 0 ] ) as string; sDescr = sDescr + " <br > " + descr; } } } } } newRow[ sDescrColumnName ] = sDescr; } bindTable.Rows.Add( newRow ); } return bindTable; }}

Not only does this make for a lengthy method- let alone a getter- it remakes the data source every single time it's accessed, and does so in a way that can only be described as "asinine."

First, it checks the type of the list. If it's one of the types that provides a DataTable out of the box, it uses that- which is fine enough- but failing that, it goes through the whole list, pulling out basic objects and retrieving their IDs via reflection. Oh, and it uses the idColumnName as the ID property name, which allows you to use any column as an ID column... provided, of course, that it hasn't been mangled by any of the previous getters or setters.

Then, it tries to cast each of those IDs as integers- but since a simple explicit cast with error handling would actually be a reasonable solution, it first checks the runtime type of the ID object against a list of types. That has the added benefit of making the code utterly break if the ID is neither an Int32 or a decimal. In this case, it just goes with -1 as the default value. After the ID is retrieved, a six-level-deep if-chain gets the two properties containing a description (again via reflection, and again using the previously reset property names), and finally inserts a new row with the ID and description set.

All this work is, of course, repeated whenever anything tries to access the data source. It serves as a perfect example of how a feature created to simplify the life of the user of your class ends up abused so badly, it makes things harder for everyone involved.

[Advertisement] Use NuGet or npm? Check out ProGet, the easy-to-use package repository that lets you host and manage your own personal or enterprise-wide NuGet feeds and npm repositories. It's got an impressively-featured free edition, too!