4
\$\begingroup\$

I am writing some C# that will be running on Linux and will extract values from a kernel generated system file that represents readings from a sensor.

The file contains two lines of text, and I have a method which will extract two values from the file.

The file looks like this:

$ cat /sys/bus/w1/devices/28-00000*/w1_slave
c3 01 4b 46 7f ff 0d 10 2f : crc=2f YES
c3 01 4b 46 7f ff 0d 10 2f t=28187

The method only cares about the crc=2f and YES (can also be NO) fields. The method returns a Nullable<SensorResult> type to indicate the success of parsing the fields:

  • If parsing could not happen at all because the passed parameter (a string) was invalid then a null will be returned.
  • If parsing was successful but the CRC was invalid (set to NO) then return a SensorResult and set appropriate fields to indicate that the CRC value is invalid.
  • If parsing was successful then return a SensorResult and set fields to indicate that the CRC value is valid.
  • Set the CrcValue field in SensorResult to the value extracted from the string. (If the YES/NO field is set to NO then getting the CRC value is still required.)

Because I'm parsing the string and extracting fields from specific positions, I've made a couple of enums to store these positions instead of relying on magic numbers.

Can the code be improved? If so, how? Are there any best practices I'm not following?

Parser:

    // <summary>
    // Parses the given line from the sensor file and determines if the CRC value is valid.
    // </summary>
    // <param name="sensorFileLine">The sensor file line.</param>
    // <returns>A <see cref="Nullable<SensorCrcValue>" /> that is used to indicate whether parsing the sensor data was successful.</returns>
    public static SensorCrcValue? TryParseCrcValue(string sensorFileLine)
    {
        if (string.IsNullOrWhiteSpace(sensorFileLine))
        {
            return null;
        }

        string[] initialSplitString = sensorFileLine.Split(' ');
        if (initialSplitString.Count() != (int)ParserFieldCount.CrcLineFieldCount)
        {
            return null;
        }

        string crcValueField = initialSplitString[(int)ParserFieldPosition.CrcValue].Split('=')[1];
        string crcValidField = initialSplitString[(int)ParserFieldPosition.CrcIsValid];

        SensorCrcValue sensorCrcValue;

        sensorCrcValue.CrcValue = Convert.ToByte(crcValueField);
        sensorCrcValue.IsCrcValid = false;

        if (crcValidField.Equals("YES"))
        {
            sensorCrcValue.IsCrcValid = true;
        }
        else if (crcValueField.Equals("NO"))
        {
            sensorCrcValue.IsCrcValid = false;
        }

        return sensorCrcValue;
    }

SensorResult struct:

public struct SensorCrcValue
{
    public bool IsCrcValid;
    public byte CrcValue;
}
\$\endgroup\$
4
\$\begingroup\$
public static SensorCrcValue? TryParseCrcValue(string sensorFileLine)

A common design pattern for Try[Action] methods is to return a boolean value to indicate if the action were successful or not and let the result be passed back to the caller through a by reference argument.

public static bool TryParseCrcValue(string sensorFileLine, out SensorCrcValue result)

SensorCrcValue sensorCrcValue;

sensorCrcValue.CrcValue = Convert.ToByte(crcValueField);
sensorCrcValue.IsCrcValid = false;

if (crcValidField.Equals("YES"))
{
    sensorCrcValue.IsCrcValid = true;
}
else if (crcValueField.Equals("NO"))
{
    sensorCrcValue.IsCrcValid = false;
}

return sensorCrcValue;

The last part of your code can be shortened since IsCrcValid is not a nullable boolean.

return new SensorCrcValue
{
    CrcValue = Convert.ToByte(crcValueField),
    IsCrcValid = crcValidField.Equals("YES")
};
\$\endgroup\$

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.