1

I have a equation string and when I split it with a my pattern I get the folowing string array.

string[] equationList = {"code1","+","code2","-","code3"};

Then from this I create a list which only contains the codes.

List<string> codeList = {"code1","code2","code3"};

Then existing code loop through the codeList and retrieve the value of each code and replaces the value in the equationList with the below code.

foreach (var code in codeList ){

 var codeVal = GetCodeValue(code);

  for (var i = 0; i < equationList.Length; i++){
     if (!equationList[i].Equals(code,StringComparison.InvariantCultureIgnoreCase)) continue;
                        equationList[i] = codeVal;
                        break;
     }
}

I am trying to improve the efficiency and I believe I can get rid of the for loop within the foreach by using linq.

My question is would it be any better if I do in terms of speeding up the process?

If yes then can you please help with the linq statement?

akd
  • 6,538
  • 16
  • 70
  • 112
  • 1
    the linq would still need to iterate over your collection – yohannist Jun 24 '15 at 11:55
  • LINQ doesn't improve computational efficiency. It is normally slower than hand made code. Unless by "efficiency" you mean "human programmer efficiency"... then yes, it can be done. – xanatos Jun 24 '15 at 11:56
  • 1
    You should use Linq to simplify code/readability, not for speeding up, because in most cases, it won't – Jeroen van Langen Jun 24 '15 at 11:56
  • possible duplicate of http://stackoverflow.com/questions/355062/is-there-a-string-math-evaluator-in-net – Hamid Pourjam Jun 24 '15 at 11:56
  • GetCodeValue return a number as string – akd Jun 24 '15 at 11:57
  • Try it & see, is what I suggest - I have found that depending on usage LINQ can speed things up, but other times can be significantly slower. – PaulF Jun 24 '15 at 11:57
  • If you really care about this kind of micro-optimization, you should keep the for-loop; it is the most efficient. Linq will create an iterator object. You should also change `foreach (var code in codeList)` to a for-loop. – Dennis_E Jun 24 '15 at 11:58
  • 1
    As Eric Lippert said: "If you have two horses and you want to know which of the two is the faster then race your horses." [Fabulous adventures in coding](http://ericlippert.com/2012/12/17/performance-rant/) – Patrik Jun 24 '15 at 11:59
  • You have a for loop inside a for loop. That leads to O(n^2) in runtime. You should create a dictionary of all codes and GetCodeValue-values and than iterate over the equationlist and replace it by the values of the dictionary. So you'll get a runtime of O(n). That will be much faster than your code but only if you've much more than these 4 items. – Sebastian Schumann Jun 24 '15 at 12:01
  • As mentioned, LINQ would still need to iterate over your collection. I'd stick with the code you have. This seems to be falling into the realms of premature micro optimisation. See here: http://programmers.stackexchange.com/questions/80084/is-premature-optimization-really-the-root-of-all-evil – sr28 Jun 24 '15 at 12:07

3 Answers3

1

Before jumping to LINQ... which doesn't solve any problems you've described, let's look at the logic you have here.

  1. We split a string with a 'pattern'. How?
  2. We then create a new list of codes. How?
  3. We then loop through those codes and decode them. How?
  4. But since we forgot to keep track of where those code came from, we now loop through the equationList (which is an array, not a List<T>) to substitute the results.

Seems a little convoluted to me.

Maybe a simpler solution would be:

  1. Take in a string, and return IEnumerable<string> of words (similar to what you do now).
  2. Take in a IEnumerable<string> of words, and return a IEnumerable<?> of values.

That is to say with this second step iterate over the strings, and simply return the value you want to return - rather than trying to extract certain values out, parsing them, and then inserting them back into a collection.

//Ideally we return something more specific eg, IEnumerable<Tokens>
public IEnumerable<string> ParseEquation(IEnumerable<string> words)
{
    foreach (var word in words)
    {
        if (IsOperator(word)) yield return ToOperator(word);
        else if (IsCode(word)) yield return ToCode(word);
        else ...;
    }
}

This is quite similar to the LINQ Select Statement... if one insisted I would suggest writing something like so:

var tokens = equationList.Select(ToToken);
...
public Token ToToken(string word)
{
    if (IsOperator(word)) return ToOperator(word);
    else if (IsCode(word)) return ToCode(word);
    else ...;
}

If GetCodeValue(code) doesn't already, I suggest it probably could use some sort of caching/dictionary in its implementation - though the specifics dictate this.

The benefits of this approach is that it is flexible (we can easily add more processing steps), simple to follow (we put in these values and get these as a result, no mutating state) and easy to write. It also breaks the problem down into nice little chunks that solve their own task, which will help immensely when trying to refactor, or find niggly bugs/performance issues.

NPSF3000
  • 2,421
  • 15
  • 20
0

If your array is always alternating codex then operator this LINQ should do what you want:

    string[] equationList = { "code1", "+", "code2", "-", "code3" };
    var processedList = equationList.Select((s,j) => (j % 2 == 1) ? s :GetCodeValue(s)).ToArray();

You will need to check if it is faster

PaulF
  • 6,673
  • 2
  • 18
  • 29
0

I think the fastest solution will be this:

var codeCache = new Dictionary<string, string>();
for (var i = equationList.Length - 1; i >= 0; --i)
{
    var item = equationList[i];
    if (! < item is valid >) // you know this because you created the codeList
        continue;

    string codeVal;
    if (!codeCache.TryGetValue(item, out codeVal))
    {
        codeVal = GetCodeValue(item);
        codeCache.Add(item, codeVal);
    }

    equationList[i] = codeVal;
}

You don't need a codeList. If every code is unique you can remove the codeCace.

Sebastian Schumann
  • 3,204
  • 19
  • 37