Abuse of properties - Part 2
-
So our company has this massive web framework that most of our software runs through. Each part of the application is coded into modules, and clients license the modules they want to use. My coworker was trying to figure out why a data grid wasn't showing up correctly in his module, but works in the main framework homepage, so he went diving through the code. The first thing he found was this in one of the fundamental base classes in the framework:
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 apparently was why his code was broken, because the module he was working on doesn't use a PIN column name, and thus by retrieving the selected object, it completely fubars the ID list. He looked at this, went wtf and looked around at related code. He quickly came across this gem after it:
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"; } } }I especially like the converting an int to an int, among a lot of other things. And horray for that useful comment in the setter. So now confronted with this code, my Coworker has stopped working for a bit, because who knows what other modules will break when you fix all the wtfs in this code. Being the masochist he is, he looked at related code and came across another gem.
///
/// 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; } }
-
Yichk! Every getter is a setter! If this guy could make the setters return something other than void, he would, wouldn't he?
I especially enjoy the exception-based flow control.
This part isn't too bad:
[code]if ( idList is LiteCollection )[/code]
But then, like an Alzheimer's patient, he regresses to an infantile state:
[code]switch ( objId.GetType().Name.ToLower() )[/code]
And what the fuck is up with his use of reflection? He makes the assumption that a property matching the name that came down from the controller is an indexer (which is an extremely wierd assumption), but refuses to use an interface to enforce the contract (and support early binding) or even wrap it in a try to catch the parameter mismatch exception.
He doesn't like dynamics either, apparently.
Thanks for posting this, but this is the kind of intellectually-stimulating OO WTF that belongs on the front page; it's better than anything we've had in a long time and would have provoked quality discussion.
-
@hoodaticus said:
Thanks for posting this, but this is the kind of intellectually-stimulating OO WTF that belongs on the front page; it's better than anything we've had in a long time and would have provoked quality discussion.
+1
-
@hoodaticus said:
Yichk! Every getter is a setter! If this guy could make the setters return something other than void, he would, wouldn't he?
I especially enjoy the exception-based flow control.
This part isn't too bad:
<font face="Lucida Console" size="2">if ( idList is LiteCollection )</font>
But then, like an Alzheimer's patient, he regresses to an infantile state:
<font face="Lucida Console" size="2">switch ( objId.GetType().Name.ToLower() )</font>
And what the fuck is up with his use of reflection? He makes the assumption that a property matching the name that came down from the controller is an indexer (which is an extremely wierd assumption), but refuses to use an interface to enforce the contract (and support early binding) or even wrap it in a try to catch the parameter mismatch exception.
He doesn't like dynamics either, apparently.
Thanks for posting this, but this is the kind of intellectually-stimulating OO WTF that belongs on the front page; it's better than anything we've had in a long time and would have provoked quality discussion.
I like how it supports ints and decimals, but not longs. Also, apparently the original author believed that interfaces are for the weak and reflection fixes everything. I love reflection as well, but in some cases - as this one - it's like using a sledgehammer on a tack.
-
@The_Assimilator said:
I love reflection as well, but in some cases - as this one - it's like using a sledgehammer on a tack.
Reflection can be awesome, especially in custom data-binding scenarios or debug tools, but, well, he's doing it wrong.
-
@hoodaticus said:
It's not even that he's doing it wrong, it's that he doesn't understand... I find the funniest/wierdest (and also usually harmless) code from junior developers who don't know the library and built-in functions, but they get the shit working because they test the hell out of it. On the flip-side, I find the most dangerous code from intermediate or senior (read old, not good) developers who write "thread safe" and other scary as shit code, such as the piece of code I posted a coulpe of weeks ago about the public global user object which is shared between all concurrent users... They also tend to test their changes maybe 2-3 times.@The_Assimilator said:
I love reflection as well, but in some cases - as this one - it's like using a sledgehammer on a tack.
Reflection can be awesome, especially in custom data-binding scenarios or debug tools, but, well, he's doing it wrong.
-
@C-Octothorpe said:
On the flip-side, I find the most dangerous code from intermediate or senior
Hello. I have one of those. (Well he's good in general, but not in one respect...)
(read old, not good) developers who write "thread safe" and other scary as shit
code,
Context: C language shop. And not to do with threads, but with passing pointers to/from functions, and whether they should be const or not.
Functions he's written, for example, take strings always as foo(char* bar). If 'bar' is a literal string, then foo (should/must) fails. Or it does now since I increased the warnings on gcc for various modules. He fails to see the problem.
-
@PJH said:
@C-Octothorpe said:
I hate to steriotype (I don't really, but anyhoo), but I'm guessing he's a bit older? Maybe mid 50s? I always found that these are the same types to hold on to their ways with a death grip, and even once they got management involved (about naming conventions, style, etc.)... That was actually quite funny because it backfired spectacularly: management ended up making him change his ways because he couldn't provide a single argument as to why he shouldn't change, other than "that's the way I've always done it". Too fucking bad, fatass.On the flip-side, I find the most dangerous code from intermediate or senior (read old, not good) developers who write "thread safe" and other scary as shit code,
Hello. I have one of those. (Well he's good in general, but not in one respect...)
Context: C language shop. And not to do with threads, but with passing pointers to/from functions, and whether they should be const or not.
Functions he's written, for example, take strings *always* as foo(char* bar). If 'bar' is a literal string, then foo (should/must) fails. Or it does now since I increased the warnings on gcc for various modules. He fails to see the problem.
