Cast/Convert/WTF?



  • Not a huge WTF, but pretty crap code either way.

    Got the joy of having to fix the following code.  Simple enough, loading data from a DB and feeding it into objects.  The guy who wrote it felt the need to 'ToString()' the integer typed data only to pass it to int.Parse.  He did the same for all datatypes, such as 'DateTime'... <?xml:namespace prefix = o ns = "urn:schemas-microsoft-com:office:office" /><o:p></o:p>

    Sigh.<o:p></o:p>

     <o:p></o:p>

    orReg.ItemType = (ItemTypes) int.Parse(odrRow["ItemTypeId"].ToString());<o:p></o:p>

    orReg.EventId = int.Parse(odrRow["EventId"].ToString());<o:p></o:p>

    if (odrRow.IsNull("LastModifiedDate"))<o:p></o:p>

    orReg.LastModifiedDate = DateTime.Now;<o:p></o:p>

    else<o:p></o:p>

    orReg.LastModifiedDate = DateTime.Parse(odrRow["LastModifiedDate"].ToString());<o:p></o:p>

    <o:p></o:p>

    <o:p> </o:p>

     



  • @kswanton said:

    Got the joy of having to fix the following code.

    It's a lazy implementation, I'll give you that.  But I'm not sure I'd even bother to take the time to fix it.

    A "better" solution, if properly implemented, is going to result in more lines of code, with little in the way of performance improvement.



  • @bullseye said:

    @kswanton said:
    Got the joy of having to fix the following code.

    It's a lazy implementation, I'll give you that.  But I'm not sure I'd even bother to take the time to fix it.

    A "better" solution, if properly implemented, is going to result in more lines of code, with little in the way of performance improvement.


    No.  A better solution will result in the same lines of code, but they will be better, safer lines.  This code is a common WTF I come across.  It's akin to writing:
    object foo = 5;
    int bar = int.Parse(foo.ToString());

    Those that actually understand the language and libraries know that the last line can be written as:
    int bar = (int) foo;

    This WTF has an insiduous effect on the system as a whole.  If the type of a particular value changes in the database, this code may still function until a parse fails.  If you went the casting route, it would be immediately obvious that you've got the datatype wrong and that the code must be fixed.


  • @bullseye said:

    @kswanton said:
    Got the joy of having to fix the following code.

    It's a lazy implementation, I'll give you that.  But I'm not sure I'd even bother to take the time to fix it.

    A "better" solution, if properly implemented, is going to result in more lines of code, with little in the way of performance improvement.


    Maybe, maybe not, but it will also be more readable.

    The difference between a readable codebase and an unreadable one is that the former is a breeze to maintain, and the other a breeding ground for all sorts of bugs and WTFs hidden in the arcane nonsense.


  • @Chris F said:


    This WTF has an insiduous effect on the system as a whole.  If the type of a particular value changes in the database, this code may still function until a parse fails.  If you went the casting route, it would be immediately obvious that you've got the datatype wrong and that the code must be fixed.

    Casts can also fail at runtime, but the ToString()-detour is of course crap.

    A little anecdote about a similar issue, especially for the Orable fans here:

    11 years ago, I had taken over a rather crappy application made with Oracle Forms, embedded SQL  (SQL*C) etc. The system went live in Dec. 1984, and went horrible. After two very stressfull months of bugfixing (on the running system), it worked, kind of. Then came March 1st, 1985 - and the whole thing crashed again. Reason: The clever guys that made the original system had taken the current date, converted it to string, and later converted it back to date. Unfortunately, this didn't work in March, probably because the language setting was German and March is "März" in German, note the Umlaut "ä". I guess that was the reason because the default conversion format (that they used) was something like "01. MÄR 1985 12:23:44".



  • @Chris F said:

    No. A better solution will result in the same lines of code, but they will be better, safer lines.

    Without error handling (i.e. more lines of code), how is an explicit cast safer?

    Those that actually understand the language and libraries know that the last line can be written as: int bar = (int) foo;

    At the risk of being pedantic, those that *know* the libraries know that the line can be written as:

    IDataRecord.GetInt32( index );
    

    This WTF has an insiduous effect on the system as a whole. If the type of a particular value changes in the database, this code may still function until a parse fails. If you went the casting route, it would be immediately obvious that you've got the datatype wrong and that the code must be fixed.

    I have visions of global meltdown and world destruction...

    I'm also having a hard time thinking of an example where this renders a system inoperable. In all seriousness, feel free to write one up, and I may change my mind. In the mean time, let me clarify. I don't advocate the method used to retrieve DB values. I also don't slit my wrists when I see it. If you have idle time, and want to pretty up the code, by all means, rewrite it.

    @RayS said:
    The difference between a readable codebase and an unreadable one is that the former is a breeze to maintain, and the other a breeding ground for all sorts of bugs and WTFs hidden in the arcane nonsense.

    In most cases, I'd agree. But this example is not one of them. I don't find Int32.Parse( string ) any harder to read/understand than (int)object. I actually dislike both methods for this purpose. Again, I wasn't advocating this method, but I was advocating against a mandatory refactoring.

     



  • @bullseye said:

    @Chris F said:
    No. A better solution will result in the same lines of code, but they will be better, safer lines.

    Without error handling (i.e. more lines of code), how is an explicit cast safer?

    This WTF is not about error handling.  It's about error hiding, lack of knowledge, and unnecessary complexity.  You have no idea if the posted snippet is wrapped in a try/catch block and properly handles errors.


    @bullseye said:
    @Chris F said:
    Those that actually understand the language and libraries know that the last line can be written as: int bar = (int) foo;

    At the risk of being pedantic, those that *know* the libraries know that the line can be written as:

    IDataRecord.GetInt32( index );

    What's your point?  That someone can write bad code of a different sort?

    @bullseye said:
    @Chris F said:
    This WTF has an insiduous effect on the system as a whole. If the type of a particular value changes in the database, this code may still function until a parse fails. If you went the casting route, it would be immediately obvious that you've got the datatype wrong and that the code must be fixed.

    I have visions of global meltdown and world destruction...

    I'm also having a hard time thinking of an example where this renders a system inoperable. In all seriousness, feel free to write one up, and I may change my mind.

    I don't know how you go from "an insiduous effect on the system" to "visions of global meldown and world destruction" and "this renders a system inoperable".  Maybe you just have a penchant for ludicrous exaggeration.  Hard to say.

    Anyway, I clearly stated my problem with this anti-idiom and its lack of safety, but I'll throw in an example for good measure: Let's say EventId has changed in the database.  For some reason, the type is now bigint instead of int.  With this code, the system would still run right up to the point where you load data that is outside the bounds of a normal integer.  At this point you could have 100% test coverage and your system would still fail given bad data.  I don't know about the standards of quality you hold yourself to, but to me that is not the hallmark of a well designed system.


  • @Chris F said:

    You have no idea if the posted snippet is wrapped in a try/catch block and properly handles errors.

    We'll assume that the original author went to great lengths to add proper error handling, in spite of using a lazy method to retrieve the data in the first place.  However, if you are advocating a cast from any SqlType, I would at least hope you are going to implement some error handling of your own.  Explicit casts are problematic when returning values from some system functions, and can cause failures in otherwise reasonable scenarios.  Since we don't have all the information, he may have been returning a value from such a function, which would cause a cast to fail.

    @Chris F said:

    What's your point?  That someone can write bad code of a different sort?

    That's nonsense.  I use this method above the others to avoid cast problems as the one stated above.  You may find it "unnecessarily complex", but I think it is both explicit and clean.

    @Chris F said:

    I don't know how you go from "an insiduous effect on the system" to "visions of global meldown and world destruction" and "this renders a system inoperable".  Maybe you just have a penchant for ludicrous exaggeration.  Hard to say.

    IMO, "insiduous effect on the system" was bordering on drama.  Global meltdown and world destruction was a kind way of hinting that you are wound way too tight over this.

    @Chris F said:

    For some reason, the type is now bigint instead of int.

    OK... if you are in an environment that allows arbitrary changes to the database, then I'll admit casting will catch some errors in a proper test environment that the Parse method wouldn't.  But if this is a common occurrence, then catching cast errors could be the least of your problems.

    @Chris F said:

    I don't know about the standards of quality you hold yourself to, but to me that is not the hallmark of a well designed system.

    Neither is a system that experiences uncoordinated schema changes. If you are going to bump the field-size, or change it altogether, there is hopefully more planning involved than just a regression test after the fact.



  • @bullseye said:

    @Chris F said:
    What's your point?  That someone can write bad code of a different sort?

    That's nonsense.  I use this method above the others to avoid cast problems as the one stated above.  You may find it "unnecessarily complex", but I think it is both explicit and clean.

    The code you pasted used an ordinal rather than an actual name from the resultset.  That is usually inferior to value access by name, as it couples a value with position in a resultset - something that almost never has any semantic value whatsoever.  Nevertheless, I don't find that line of code to be "unnecessarily complex".  Unsafe, confusing, and silly yes.  But not complex.

    @bullseye said:

    @Chris F said:
    I don't know how you go from "an insiduous effect on the system" to "visions of global meldown and world destruction" and "this renders a system inoperable".  Maybe you just have a penchant for ludicrous exaggeration.  Hard to say.

    IMO, "insiduous effect on the system" was bordering on drama.  Global meltdown and world destruction was a kind way of hinting that you are wound way too tight over this.

    Might I remind you that you're the one who came up with ridiculous exaggerations.  The word insidious is entirely accurate given the nature of the stealthy bug.

    @bullseye said:

    @Chris F said:
    For some reason, the type is now bigint instead of int.

    OK... if you are in an environment that allows arbitrary changes to the database, then I'll admit casting will catch some errors in a proper test environment that the Parse method wouldn't.  But if this is a common occurrence, then catching cast errors could be the least of your problems.

    @Chris F said:

    I don't know about the standards of quality you hold yourself to, but to me that is not the hallmark of a well designed system.

    Neither is a system that experiences uncoordinated schema changes. If you are going to bump the field-size, or change it altogether, there is hopefully more planning involved than just a regression test after the fact.

    Hahahah.  Now where did I say the change was arbitrary or part of an uncoordinated schema change?  I didn't.  That's an assumption you made.  Your above statements are nothing more than fallacies of distraction that focus on external forces instead of the WTF code in question.

    The best of processes will still yield code with bugs.  You can not excuse poor code such as this no matter how fabulous your change process.



  • @Chris F said:

    The code you pasted used an ordinal rather than an actual name from the resultset.  That is usually inferior to value access by name, as it couples a value with position in a resultset - something that almost never has any semantic value whatsoever.  Nevertheless, I don't find that line of code to be "unnecessarily complex".  Unsafe, confusing, and silly yes.  But not complex.

    I did use a magic number in the example, which I agree is poor in practice, so I'll clarify.  As with any constant value, I use constants to represent the columns.  Should the columns be reordered, it's as simple as renumbering the constant.  This is no different than having to change a string name for a column.  This is also much more preferable than doing a GetOrdinal(name) every time.

    As for unsafe and confusing.. it all depends on the wielder, I suppose.  I find it to be "safer" than any other method discussed.

    @Chris F said:

    Your above statements are nothing more than fallacies of distraction that focus on external forces instead of the WTF code in question.

    Focusing on the code in question, why would you believe it to be a WTF without taking external forces into consideration?

    @Chris F said:

    The best of processes will still yield code with bugs.  You can not excuse poor code such as this no matter how fabulous your change process.

    I'll wholeheartedly agree, starting from scratch, that this is a sloppy tactic to employ. I never said it was brilliant.  So for the record, don't do it.  There...  we agree on that.

    However, as I said from the start and in every post after that, I see the code as lazy, not insidious.  I don't like it, I don't use it, but the level of risk it poses is minimal.  I don't like explicit casts for that matter, which I've already covered.  The simple point I was trying, and perhaps failed, to make before we began this long discussion is that when it comes to refactoring, you have to pick your battles.  I think everyone that has been in software for a while has come across systems with sections of code that they would just love to rewrite for the sake of "purity".  Giving in to this predilection every time makes for low productivity, especially given the fact that most of the time, it will not make a difference.

     



  • @bullseye said:

    I did use a magic number in the example, which I agree is poor in practice, so I'll clarify.  As with any constant value, I use constants to represent the columns.  Should the columns be reordered, it's as simple as renumbering the constant.  This is no different than having to change a string name for a column.

    I disagree, I think it's very different.  If a query is restructured to return the same results with different ordinals, your code must change while one that uses string names does not.  Similarly, the names in a resultset are all that changes then ordinal access code does not need to be modified.

    @bullseye said:

    @Chris F said:
    Your above statements are nothing more than fallacies of distraction that focus on external forces instead of the WTF code in question.

    Focusing on the code in question, why would you believe it to be a WTF without taking external forces into consideration?


    I think you misunderstand.  I'm talking about this snippet of code in isolation - that includes its solution to a common problem, and its possible inputs and outputs.  You're talking about external factors like organizational processes.  The WTFs with this code have already been clearly documented.

    @bullseye said:
    However, as I said from the start and in every post after that, I see the code as lazy, not insidious.  I don't like it, I don't use it, but the level of risk it poses is minimal.

    That's really extraordinarily difficult to say, as this code may be part of a critical section that you're unaware of.  Without context it's impossible to say.

    @bullseye said:
    I don't like explicit casts for that matter, which I've already covered.

    From a type safety perspective, there is no difference between (int) reader["foo"] and reader.GetInt32(reader.GetOrdinal("foo")).  At least per the SqlClient implementation, both will perform a cast.  One is not safer than the other.


  • Some quick responses, because I am *supposed* to be working.

    @Chris F said:

    @bullseye said:
    This is no different than having to change a string name for a column.

    I disagree, I think it's very different..

    C'mon Chris...  Even a basic user knows there is a difference in use.  I was referring to the level of effort involved for changing the ordinal, which is the same level of effort for changing a column name.  Column names can change just as easily as column order, coming from a query.  You touted readability, which I agree with.  Using Getxxx( GetOrdinal( string ) ) stretches that a bit.  If you still disagree, then just chalk it up to personal preference, since this is a very trivial point.

    @Chris F said:

    @bullseye said:
    I don't like explicit casts for that matter, which I've already covered.
    From a type safety perspective, there is no difference between (int) reader["foo"] and reader.GetInt32(reader.GetOrdinal("foo")).  At least per the SqlClient implementation, both will perform a cast.  One is not safer than the other.

    In the case of the SqlClient implementation, GetInt32 handles casting between SqlTypes, Int32 does not.  As I have already pointed out, there are scenarios where this comes into play, in which an explicit cast to a CLR type is problematic.



  • @bullseye said:

    @Chris F said:
    @bullseye said:
    This is no different than having to change a string name for a column.

    I disagree, I think it's very different..

    C'mon Chris...  Even a basic user knows there is a difference in use.  I was referring to the level of effort involved for changing the ordinal, which is the same level of effort for changing a column name.  Column names can change just as easily as column order, coming from a query.  You touted readability, which I agree with.  Using Getxxx( GetOrdinal( string ) ) stretches that a bit.  If you still disagree, then just chalk it up to personal preference, since this is a very trivial point.

    I disagree again.  Of course there is a difference in use, but I disagree that the differences are as trivial as you make them out to be.

    Ordinals are more likely to change than column names.  Take, for example, a standard mapping function that maps some row of data onto an instance of an object.  If you use ordinal access, you must ensure that the same data appears in the same position no matter what query returns it.  This is extremely difficult if not impossible.

    I do not consider Getxxx(GetOrdinal(columnName)) to be an access by ordinal, as the information you are supplying is the column name.

    @bullseye said:

    In the case of the SqlClient implementation, GetInt32 handles casting between SqlTypes, Int32 does not.  As I have already pointed out, there are scenarios where this comes into play, in which an explicit cast to a CLR type is problematic.

    All accesses handle conversions between the SqlType structures and native types.  I'm not sure what you mean here.  In what circumstances will GetInt32 yield different behavior?


Log in to reply