1

I was mildly surprised to find this function in our code base:

public static double ToDouble( object value )
{
   if ( value is double )
   {
      return (double)value;
   }
   else if ( value is decimal )
   {
      return decimal.ToDouble( (decimal)value );
   }
   else
   {
      return double.Parse( 
        value.ToString(),       
        System.Globalization.CultureInfo.InvariantCulture );
   }
}

It is called from several places in the project, including:

  1. with the ScannedQty column from a deserialised DataTable,
  2. with the Qty and ScannedQty columns in an unrelated DataTable received from an ADO.NET connection.

I consider it typically unsafe, dangerous, and fraught with evasive errors, whereas the author, and my colleague, disagrees. He wrote it as a universal one-for-all device to read a double from an object regardless of the source and type of the underlying data and uses it while reading an incoming XML and while loading data from the database. Sometimes, he said, the actual value may be a double, sometimes a decimal, and sometimes (e.g. when it comes from an .XML file) a string.

I proposed that he rewrite access to the various data sources using explicit casting to the appropriate type, which is stable for every use case, e.g.:

public static double DblFromDblObj( object value )
{  return ( double )value;  }

public static double DblFromDecObj( object value )
{  return ( double )( decimal )value;  }

public static double DblFromStrObj( object value )
{  return Double.Parse( ( string )value, CultureInfo.InvariantCulture );  }

and replace all invocations of ToDouble() with one of these, depending on the actual data type involved, but he is loth to do so because

a. it will be a waste of time since the code works as is, and

b. if a data type changes in the database or the incoming XML file the program will break.

In my opinion, a strict and unambiguous protocol shall be established, and enforced in the implementation by means of clean access functions that shall fail in case of type mismatch, indicating an error in the data and a violation of the protocol. This kind of fragility is the basis of reliability.

If the programmer cannot decide the type of a field in the payload and writes dynamic type checking for so trivial a task, there is clearly something amiss, such as lack of attention to the data exchange protocol or mere laziness on part of the programmer that prevented him from studying which type is returned where. For example, when querying a database via ADO.NET or another SQL-based mechanism, it is dead wrong to sweep the dust under the carpet:

object valueObj = myDataRow["ScannedQty"];
double value =  Convert.ToDouble( value );

Direct type-casting is cleaner, faster, and more transparent (because it shows the unambiguous internal type to the reader of the code):

double value = ( double )( decimal )myDataRow["ScannedQty"]; // NOT NULL field

Therefore, the refactoring that I propose is not a waste of time but a useful improvement that makes the code cleaner and the program more reliable. It is also very well that the program will stop working if the underlying type of the object changes, because by this token we shall know that somebody broke the protocol.

These arguments of mine, however, failed to convince my colleague, so I should be grateful for others' opinions to help us come to a common understanding. How do you resolve such conflicts at your job?

P.S.: This seeming a more informal forum than StackOverflow, I hope it will not be downvoted and closed as opinion-based. Please, let me know if I am wrong and then be so good as to suggest a better place to ask my question.

P.P.S.: Please, be polite to both of us, for I will show this question to my colleague.

  • 7
    Ugh, neither solution is good. Convert exists. Serializers exist. Don’t reinvent the wheel. – Telastyn Nov 30 '17 at 23:35
  • 6
    The question you want answered is how to resolve conflicts at work? – Erik Eidt Dec 1 '17 at 1:50
  • 1
    Ultimately it depends on whether you can do this change without causing massive breakage or massive code changes that disrupt others' development work. When processing legacy data, slightly malformatted data (such as using Decimal when the spec says double) is to be expected. Therefore the ability to correct such malformatted data is typically part of business requirement, which has higher priority than recommended programming practices such as type safety. In other words it's just having a correct understanding of priorities. – rwong Dec 1 '17 at 3:37
  • 1
    To clarify what @Telastyn said, this ToDouble function looks like an incomplete reimplementation of the built-in System.Convert.ToDouble(object) function. – Sebastian Redl Dec 1 '17 at 13:48
  • Convert.ToDouble() does not accept a culture and uses the current system culture instead of CultureInfo.InvariantCulture. – Ant_222 Dec 1 '17 at 16:19
4

Here is my opinion about both of your conversion approaches. In short, both can be "the correct" solution, it depends on the case. The approach of your colleague has certain use cases where it fits clearly better, but same is true for yours (and the gist of your conflict is probably that each of you only sees "his" use case).

Your approach is IMHO more appropriate when

  • the input data is very strictly typed
  • it is a clear error when the input data does not have the exact expected type, and there is no room for variance

Of course, you already told us that you prefer to establish such protocols, and in an ideal world, every data source provides enough and 100% correct meta data to allow this. Unfortunately, we are not working in an ideal world, software is not perfect, and it is not always in our own hands how protocols and meta data look like (especially when something like a database or xml is provided by a 3rd party).

So for cases where some variance is needed and tolerable, the approach of your colleague is IMHO more appropriate. It actually follows the well-known robustness principle - be liberal in what you accept (and conservative in what you do). And that is not error-prone, quite the opposite, it is often necessary for keeping bigger systems manageable.

So my advice is: look at the data source!

  • do you have it full under your control?
  • does it provide all the meta data for the required type checking?
  • is it really a hard error when the provided types do not exactly match the expected ones? Or can a value be accepted as double when it is technically not a double, but convertible by, for example, a string conversion?
  • are you sure a slight change in a third party component will not cause a huge cascade of changes in your type conversion code, which could be avoided by using your colleagues approach?

If you can answer all those 4 questions honestly with "yes", then your approach will fit better, otherwise I would recommend the one of your colleague.

  • "it is a good strategy, often lethal for building bigger systems." - That doesn't sound good to me. Did you mean "vital"? – Sebastian Redl Dec 1 '17 at 13:45
  • Its worth pointing out that the robustness principle is a contradiction of the "Fail Early, Fail hard" concept. (refer stackoverflow.com/questions/2807241/…) – mattnz Dec 6 '17 at 2:23
  • @mattnz: this "contradiction" (if it is one) IMHO arises just because of people misunderstand the different preconditions of those principles. "Fail early" is the correct approach when there is unexpected input data to a component and the component cannot continue its function in a stable manner. "Robustness principle" is the correct approach when there is a foreseeable variance in the input data (a variance which does not introduce ambiguity), and the component can perfectly continue its function in a stable manner. – Doc Brown Dec 6 '17 at 6:53
  • ... The problem with these two principles is, to apply them correctly, one has to check the preconditions before, which means it is necessary to make a proper analysis of the case, and it is not possible here to follow a braindead "best practice" rule. – Doc Brown Dec 6 '17 at 6:59

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.