Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have implemented class which manages communication with measuring device, which sends list of assignments, like variableName=value. These assignments are always separated by space. There are two options, the first is simple assignment variableName=value (without space around equal sign), the second is "array assignment", like variableName[num]= value_1 ... value_num (here is space after equal sign and between single values). Here is my code for parser:

    /// <summary>
    /// Parse variable value "y" from assignment pair "x=y"
    /// </summary>
    /// <param name="s">string where "x=y" pairs are separated by space</param>
    /// <param name="varName">left side of assignment</param>
    /// <returns>right side of assignment</returns>
    private static string ParseEqualValue(string s, string varName)
    {
        int startPos = s.IndexOf("=", s.IndexOf(varName)) + 1;
        int endPos = s.IndexOf(" ", startPos);
        return s.Substring(startPos, endPos - startPos);
    }

    /// <summary>
    /// Parse variable values after "=" from string
    /// </summary>
    /// <param name="s">string in format: varName[num]= x1 .. xnum</param>
    /// <param name="varName">left side of assignment</param>
    /// <returns>right side of assignment in the list</returns>
    private static List<string> ParseEqualValues(string s, string varName)
    {
        List<string> list = new List<string>();

        // position of ShapeX..
        int varPos = s.IndexOf(varName);

        // parsing of numPts value in []
        int numStartPos = s.IndexOf("[", varPos) + 1;
        int numEndPos = s.IndexOf("]", numStartPos);
        int num = 0;
        int.TryParse(s.Substring(numStartPos, numEndPos - numStartPos), out num);

        int startPos = s.IndexOf("=", varPos) + 2;

        for (int i = 0; i < num; i++)
        {
            int endPos = s.IndexOf(" ", startPos);
            if (endPos < 0)
                endPos = s.Length;

            list.Add(s.Substring(startPos, endPos - startPos));
            startPos = endPos + 1;
        }

        return list;
    }

And here how I use it:

    /// <summary>
    /// Create ColdTest cut from ProfRoll answer
    /// </summary>
    /// <param name="s">Answer from ProfRoll</param>
    /// <returns></returns>
    public static Cut CreateColdTestCut(string s)
    {
        Cut cut = new Cut();
        cut.pts = new List<PointF>();

        int numShapes;

        double.TryParse(ParseEqualValue(s, "AvgTemp"), out cut.pyroTemp);
        double.TryParse(ParseEqualValue(s, "MinTemp"), out cut.tempMin);
        double.TryParse(ParseEqualValue(s, "MaxTemp"), out cut.tempMax);

        int.TryParse(ParseEqualValue(s, "NumShapes"), out numShapes);

        List<int> numPts = new List<int>();

        for (int i = 0; i < numShapes; i++)
        {
            List<string> xPts = ParseEqualValues(s, string.Format("ShapeX{{0}}", i));
            List<string> yPts = ParseEqualValues(s, string.Format("ShapeY{{0}}", i));

            if (xPts.Count != yPts.Count)
                break;

            numPts.Add(xPts.Count);

            for (int j = 0; j < xPts.Count; j++)
            {
                try
                {
                    cut.pts.Add(new PointF(float.Parse(xPts[j]), float.Parse(yPts[j])));
                }
                catch { }
            }
        }

        cut.isColdTest = true;
        cut.time = DateTime.Now;
        cut.numCurves = cut.numPts.Length;

        return cut;
    }

I will be glad for any critical comments.

share|improve this question

I'm not sure exactly where to start with reviewing this code. There are many things that jump out as unsafe or awkwardly approached.

If my source string contained:

foo1=bar1 foo=bar

Then querying for 'foo'

int startPos = s.IndexOf("=", s.IndexOf(varName)) + 1;

Will incorrectly return the index of 'bar1' not 'bar'.

Use positional data to locate the keys within the string. All keys begin immediately after a space, and end immediately before an equals sign, and contain no spaces or equal signs.

Start by splitting the string on the '='. Your first key will be result[0]. Find the last index of a ' ' character in result[1]. Everything before that is your first value. Iterate your split results to build a Dictionary<string, string> and a Dictionary<string, string[]>, processing the value of key as appropriate based on the assignment type. You can then check if a particular key is defined by looking for it in the dictionaries, and get the value directly.

Is there a purpose to specifying the number of array elements in the key, when you already know the number from the value? How do you propose resolving the difference in the case of an incorrectly constructed string?

"a[1]= x y z"

Should either be a[3]= x y z or a[1]= x, but which takes precedence. If you can control the incoming format, and are not dealing with something beyond your control, use a[]= x y z and don't rely on the count being placed in the key.

share|improve this answer
    
I definitely like idea of using Dictionary here, thanks! Constructing correct string with number of elements relies on measuring device class, so if it constructs a[1]= x y z for some reason, I want to take a[1]= x, maybe it sends me all measured results and tells me that only the first on is valid, who knows. – Majak Nov 24 '15 at 8:18

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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