2
\$\begingroup\$

I would like some advice to shorten down this snippet, give it more efficient flow and look. Review should focus on keeping it as a compact single method (except the two public props). The DataType and MeasureStatus is an enum. Readability is of course more important than compactness.

....
/// <summary>
/// Helper that hold methods for evaluate and calculate data 
/// that will be serialized/deserialized in Serializer/SerializerBase.
/// Please don't put serializing routines in here and also understand 
/// that some decisions here are based on protocol specifikations.
/// </summary>
....
public Unit MeasureType { get; set; }
public ModeType Aggregate { get; set; }
.....

public Tuple<DataType, byte> EvaluateDataType(double value, MeasureStatus? status = null)
{
    int quantityOfDecimals = 0;
    DataType dataType = DataType.MISSING_LONG;
    byte nibbled = 0;
    bool statusIsMissing = false;

    if (status != null)
        if (status == MeasureStatus.Missing)
            statusIsMissing = true;

    quantityOfDecimals = 
      (this.MeasureType == Unit.kWh || this.MeasureType == Unit.ºC) ? 1 : 2;

    // Qty decimals affect limit size  of 'value'
    if (quantityOfDecimals == 1)
    {
        if (value < 3276 && value > -3276)
            if (!statusIsMissing)
                dataType = DataType.SHORT_1DEC;
            else
                dataType = DataType.MISSING_SHORT;
        else
            if (!statusIsMissing)
                dataType = DataType.LONG_1DEC;
            else
                dataType = DataType.MISSING_LONG;
    }

    // Qty decimals affect limit size  of 'value'
    if (quantityOfDecimals == 2)
    {
        if (value < 327 && value > -327)
            if (!statusIsMissing)
                dataType = DataType.SHORT_2DEC;
            else
                dataType = DataType.MISSING_SHORT;
        else
            if (!statusIsMissing)
                dataType = DataType.LONG_2DEC;
            else
                dataType = DataType.MISSING_LONG;
    }

    nibbled = (byte)(Aggregate | dataType);
    return new Tuple<DataType, byte>(dataType, nibbled);
}
\$\endgroup\$
  • \$\begingroup\$ If there are design decisions that are driving this, then they need to be referenced in comments, probably more than just a 'look here for why' (unless you have a good referencing system with tool...). Magic numbers are still bad, perhaps especially if specified externally - you only want to have to change ONE value if the specification changes, not look for a million hard-coded values (and in your case, they aren't written as the same value anyways!). And never rely on a developer reading documentation before looking at isolated code... \$\endgroup\$ – Clockwork-Muse Nov 28 '11 at 16:37
  • \$\begingroup\$ Thank's. I think this fell outside of the scoop of question. Also we came to goal of the question already, \$\endgroup\$ – Independent Nov 29 '11 at 7:41
3
\$\begingroup\$

There are some (potentially bizarre to me) things going on here. A slightly larger view of the system may be helpful in further refining this method. In any case, here's how I'd restructure this. I've also included notes about some things I found odd...

public Tuple<DataType, byte> EvaluateDataType(double value, MeasureStatus? status = null) 
{ 

    // As much as possible, initialize a local variable to the proper value
    // when you declare it (as if it had Java's *final* attatched).
    // This isn't always possible, of course
    DataType dataType;
    // Really, these are the only two datatypes that use 1 decimal place?
    int decimalPlaces = (MeasureType == Unit.kWh || MeasureType == Unit.ºC) ? 1 : 2;
    // Structure your code so that it's values and executions give the -reasons-
    // or the -why-.  The code itself says -what- is happening, but it's usually -why-
    // that's actually important.  
    // Among other things, this means you should (usually) avoid magic numbers. 
    bool valueOutOfRange = Math.Abs(value) >= (Int16.MaxValue / (10 * decimalPlaces)); 

    // status == null is considered -present-?
    if (status != null && status == MeasureStatus.Missing) 
    {
    // Note, never leave off brackets - {} - you run a nasty risk of
    // someone writing indented code that isn't actually part of the block.   
    // Also, as much as possible, test for -affirmative- results, not negative ones.
    // Although, multiple negatives should still be avoided.         
        if (valueOutOfRange) 
        {
            dataType = DataType.MISSING_LONG;
        } 
        else
        {
            dataType = DataType.MISSING_SHORT;
        }
    }
    else
    {
        if (valueOutOfRange) 
        {
            if (decimalPlaces == 1) 
            {
                dataType = DataType.LONG_1DEC;
            }
            else
            {
                dataType = DataType.LONG_2DEC;
            }
        }
        else
        {
            if (decimalPlaces == 1)
            {
                dataType = DataType.SHORT_1DEC;
            }
            else 
            }
                dataType = DataType.SHORT_2DEC;
            }
        }
    }

    // Are you sure you don't mean "byte nibbled = (byte) (Aggregate & dataType);"?
    // I'm assuming you're attempting to mask Aggregate for the cast, and that 
    // you haven't done anything -too- out there in the operator override.
    byte nibbled = (byte) (Aggregate | dataType);
    return new Tuple<DataType, byte>(dataType, nibbled);
}

Although, I'm a little suspicious of anything that's moving some sort of DataType around like this (especially because you're only storing a byte). Normally it's best to prefer some sort of implemented generic, so you can get compile time checking of your datatypes. Nothing like mismatches at runtime to crash your probe into Mars (granted, that was a mismatch in the measure type, which you're attempting to cover here. However, you still need to deal with value mis-matches...).

\$\endgroup\$
  • \$\begingroup\$ ermm "never leave off brackets". I think your missing a bracket after the else statement just after this comment :) Otherwise good feedback. To some degree I think your use of spacing helps the readability more than the changes. \$\endgroup\$ – dreza Nov 25 '11 at 20:37
  • \$\begingroup\$ @dreza - Thanks. This is another reason why - your compile can catch that you've put something in the wrong place. I don't have a C# compiler here. \$\endgroup\$ – Clockwork-Muse Nov 25 '11 at 21:41
  • \$\begingroup\$ Updated! I hope the edits are clear enough, at last. \$\endgroup\$ – Independent Nov 28 '11 at 13:31
  • \$\begingroup\$ One super-minor-pity comment, decimalPlaces = number of ,? \$\endgroup\$ – Independent Nov 28 '11 at 15:06
  • \$\begingroup\$ @Jonas - decimalPlaces is equivalent to your quantityOfDecimals, which is a somewhat counter-intuitive name. Usually, 'decimal places' is used to refer to how many places (digits) are present to the right of the decimal-point - tenths, hundreths, thousandths, and so on. \$\endgroup\$ – Clockwork-Muse Nov 28 '11 at 16:41

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.