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] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!