0
\$\begingroup\$

I am writing a C# script to count all the 'if...then' statements present in 4000 lines of SAS code. I am taking the SAS code as a text file and then performing my operation. Here's my C# code:

class Program
{

static void Main(string[] args)
{

    List<String> list = new List<string>();
    list = File.ReadAllLines(@"C:\Users\Dey\Documents\Trucks\file.doc").ToList();
    int count = CountIfs(list);
    Console.ReadKey();
}

static int CountIfs(List<String> list)
{
    int count = 0;
    var index = new List<int>();
    var value = new List<String>();

    for (int i = 0; i < list.Count; i++)
    {
        if (list[i].Contains("if"))
        {
            if (list[i].Contains("then") || list[i + 1].Contains("then"))
            {
                count++;
                index.Add(i);
                String temp = null;

                if(list[i].Contains(";"))
                temp = list[i].Substring(0, list[i].IndexOf(";"));

                if (temp != null)
                {
                    value.Add(temp);
                }  
            }

        }
    }
    return count;
}
}

I'm essentially checking for "if" per line and if that is found, i'm checking for "then". And finally I'm also making sure its one line of code by checking for ";" . My logic is that if all these conditions satisfy, then I am indeed able to extract all the if-statements from the code. The list "value" would contain all the "if" statements found in the code. Now I've output "value" to a textfile in notepad and have manually cross-checked about 20 entries. And so far it seems to be working alright. I was hoping someone can proof-read my code and tell me if I can improve it somewhere perhaps? Or is this logic sufficiently accurate?

\$\endgroup\$
  • 3
    \$\begingroup\$ Have you really named your method DoSomething? \$\endgroup\$ – t3chb0t Feb 15 '18 at 11:34
  • 3
    \$\begingroup\$ Is DoSomething your actual method name, or is it pseudo-code? Note that questions about pseudo-code are off-topic on Code Review. \$\endgroup\$ – Raimund Krämer Feb 15 '18 at 11:35
  • \$\begingroup\$ You might want to take a look at I accidentally created two accounts; how do I merge them? \$\endgroup\$ – t3chb0t Feb 15 '18 at 11:44
3
\$\begingroup\$

The code in question has some flaws which I will address here:

  • var index = new List<int>(); isn't really used and should be removed
  • var value = new List<String>(); isn't really used and should be removed
  • The method parameter List<String> list isn't used as a List<T> but rather like an array hence it should be an array.

Fixing these points leads to

private static int CountIfs(string[] linesOfCode)
{
    var count = 0;

    for (int i = 0; i < linesOfCode.Length; i++)
    {
        var line = linesOfCode[i];
        if (line.Contains("if"))
        {
            if (line.Contains("then") || line[i + 1].Contains("then"))
            {
                count++;
            }
        }
    }
    return count;
}

Now the code looks much cleaner and smaller.


  • An if will always be the first statement in a line of code after removing any leading whitespace and will have a trailing whitespace. Using Contains() can lead to many false positives. Assume a line of code contains the following text:
    "I received the tif-file of this invoice from my partner in athen."

  • A then will always have a leading whitespace and either have a trailing whitespace or will be the first part of a line of code after removing leading whitespace.

  • Like mentioned in @Paparazzi's answer you could run into an IndexOutOfRangeException by accessing list[i + 1].

Let us introduce a new method which just handles the check for the if statement.

private static bool HasIfStatement(string line)
{
    return line.Trim().StartsWith("if ");
}

No we need a method to check for a then.

private static string[] spaceArray = new string[] { " " };
private static bool HasThenStatement(string line)
{
    return line.Split(spaceArray , StringSplitOptions.RemoveEmptyEntries).Any(w => w == "then");
}

Well this method is some kind of quick & dirty because it will create a lot of strings. I will leave it to you to make it better.

By adding a flag for the case that the line had an if and doesn't had an then we can't get an IndexOutOfRangeException anymore. As a sideeffect we could now use a foreach loop and have an IEnumerable<string> as the methods parameter type.

Putting all together could look like so

private static int CountIfs(IEnumerable<string> linesOfCode)
{
    var count = 0;
    var lastLineHadIf = false;
    foreach (var line in linesOfCode)
    {
        if (lastLineHadIf)
        {
            lastLineHadIf = false;
            if (HasThenStatement(line))
            {
                count++;
            }
        }
        else if (HasIfStatement(line))
        {
            if (HasThenStatement(line))
            {
                count++;
            }
            else
            {
                lastLineHadIf = true;
            }
        }
    }
    return count;
}

private static bool HasIfStatement(string line)
{
    return line.Trim().StartsWith("if ");
}

private static string[] spaceArray = new string[] { " " };
private static bool HasThenStatement(string line)
{
    return line.Split(spaceArray , StringSplitOptions.RemoveEmptyEntries).Any(w => w == "then");
}  

I don't know about case-sensitivity of if and then so you may adjust the code to take this into account.

This code doesn't take comments into account, so you should adjust the code for comments as well.

\$\endgroup\$
  • \$\begingroup\$ OKay so by this logic, I get a count of 398. Whereas with my code I was getting 452. I tried to find the distinct values by using List.Except but that returns a value of 401 instead of 96 (452-356) that is expected. I manually checked the output of this code and indeed some if statements are being ignored. PS. I used case-insensitivity. \$\endgroup\$ – Richeek Dey Feb 19 '18 at 11:33
  • \$\begingroup\$ I realise what is going wrong. If it does find "if" then its checking for "then". However "then" may be one line after. Is there a way to do that "if" is found then it will take all the values from "if" till ";" into one string? Because sometimes "then" exists a few lines below and there ultimately is always a ";" I suppose on way to do this would be to use a for loop and then to use the i+1th value? i+n rather, n being until it finds ";" \$\endgroup\$ – Richeek Dey Feb 19 '18 at 12:41
  • \$\begingroup\$ Without seeing different version of the code (if and then on the same line, if on one line and then on the next line, if on one line and then some lines below) its hard to answer. \$\endgroup\$ – Heslacher Feb 19 '18 at 12:57
  • \$\begingroup\$ Hmm.. Thanks. perhaps I can parse the entire code command by command rather than line by line? \$\endgroup\$ – Richeek Dey Feb 19 '18 at 13:06
0
\$\begingroup\$

You can get an index out of range on this

list[i + 1]

Why are you using string to store integer?

var value = new List<String>();

Could just do IndexOf and look for >=0

\$\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.