6
\$\begingroup\$

Currently all this does is retrieve data from specific columns, the final result will be that I will check the data to make sure that it passes the test cases, but I want to look at this foreach right now and see if I can't make it a little simpler.

A lot of what I have coded I have come back and turned into Linq statements and made things a lot simpler (shorter), and I would really like to do the same with this foreach statement, but not real sure how to do it.

I hate to admit that I am very new to using Linq, I can read it for the most part but still have a little difficulty when writing more complex statements/queries.

Is there something that I can do to make this a lot more simple before I code any more on this Scenario, I would like to simplify so that I don't write extraneous code that will be refactored out when I make this more clear.

I have also been trying to find something that imports the HTML table into a nice neat object that I can query, but everything I have found so far has been similar to what I am doing or more complex than what I am doing here.

[Then(@"see the '(.*)' of '(.*)'")]
public void ThenSeeThe(string columnHeader, string entryName)
{
    var htmlTable = Ie.FindElementByClassName("table");
    var tableHeaders = htmlTable.FindElements(By.TagName("th"));
    var rows = new List<List<string>>();

    var htmlRows = htmlTable.FindElements(By.TagName("tr"));

    foreach (var htmlRow in htmlRows)
    {
        //first row holds the table headers
        if (htmlRow.FindElements(By.TagName("th")).Count > 0)
        {
            var thList = tableHeaders.Select(th => th.Text.ToString()).ToList();
            rows.Add(thList);
            continue;
        }

        var cellsInRow = htmlRow.FindElements(By.TagName("td"));
        var cells = cellsInRow.Select(cell => cell.Text.ToString()).ToList();
        rows.Add(cells);
    }

    var columnIndex = rows.First().IndexOf(columnHeader);
    var field = rows.Where(x => x[0] == entryName).Select(x => x[columnIndex]).First();
    if (columnHeader == "Create UTC Date")
    {
        _CreateUTC = Convert.ToDateTime(field);
    }
    else if (columnHeader == "Modify UTC Date")
    {
        _ModifyUTC = Convert.ToDateTime(field);
    }

}
\$\endgroup\$
2
\$\begingroup\$

If I understand correctly, the role of this procedure (which is far from obvious from the name alone) is to assign either _CreateUTC or _ModifyUTC values (which should be _createUTC and _modifyUTC if they're private fields - always use camelCase for fields and locals/parameters.

You don't need that rows list of lists of strings at all, let alone iterate that monster afterward to retrieve a value.

Just loop and locate the value as you're looping, don't store anything but what you need and stop looping once you got it - I think something like this would do it:

var headerIndex = 0;
DateTime value;
foreach (var htmlRow in htmlRows)
{
    if (htmlRow.FindElements(By.TagName("th")).Count > 0)
    {
        headerIndex = tableHeaders.Select(th => th.Text.ToString()).IndexOf(columnHeader);
        continue;
    }

    var cellsInRow = htmlRow.FindElements(By.TagName("td")).ToArray();
    if (cellsInRow[0] == entryName)
    {
        value = Convert.ToDateTime(cellsInRow[headerIndex].Text.ToString());
        break;
    }
}

if (columnHeader = "Create UTC Date")
{
    _createUTC = value;
}
else if (columnHeader = "Modify UTC Date")
{
    _modifyUTC = value;
}
\$\endgroup\$
  • \$\begingroup\$ This is a very good response to the code given, unfortunately because of what I am coding I actually need to create a way to compare the state of the table before and after certain actions/changes. If I were just pulling in this data once, I would do it this way, and I may still do this depending on how the rest of this feature goes, and how I track the row location. thank you for the review \$\endgroup\$ – Malachi Jul 8 '16 at 13:22
  • \$\begingroup\$ I don't understand. 99% of the data you're collecting is useless and not kept anywhere, it's all local. Or did you not list your actual, real code? \$\endgroup\$ – Mathieu Guindon Jul 8 '16 at 13:45
  • \$\begingroup\$ it is actual, real code, but as I have been coding I have been adding new code and changing code to better fit my requirements and to make the code clearer and less redundant for what I am doing with the scenarios. so, I eventually want to be able to check all the columns, I am not sure whether I want to hold all the rows though, so I think I might be able to get away with only extracting the table header and the actual row that matches the item I want to inspect. \$\endgroup\$ – Malachi Jul 8 '16 at 14:13

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.