foreach statement not running

[Jfisher387]

Hello,

Working through a problem, I cannot quite grasp why this nested foreach does not run. While I am at it, I would like to think its not the fastest way to sift through the data, maybe someone has a suggestion for another approach? My data comes from a sql database with 10's of thousands of entries. I have 3 tables I am trying to grab data from. It just takes a couple seconds to loop through but its enough to bother me.

private void Active_Jobs_Drill_Down_Load(object sender, EventArgs e)
        {
          
            //TODO: Highlight due dates that are past due in red, yellow if within 7 days from being due?
            DataList.Clear();
            string jobID = job_Label.Text;
            try
            {
               this.job_OperationTableAdapter.Fill(this.pRODUCTIONDataSet2.Job_Operation);
            }
            catch(Exception ex)
            {
                //MessageBox.Show(ex.Message);
            }

            foreach (DataRow row in pRODUCTIONDataSet2.Tables["Job_Operation"].Rows)
            {
               if(row["Job"].ToString() == jobID)
                {
                    DrilldownData entry = new DrilldownData();
                    entry.Job = Convert.ToString(row["Job"]);
                    entry.Run_Qty = Convert.ToString(row["Run_Qty"]);
                    entry.Description = Convert.ToString(row["Description"]);
                    entry.Job_Operation = Convert.ToString(row["Job_Operation"]);
                    entry.Vendor = Convert.ToString(row["Vendor"]);
                    entry.Work_Center = Convert.ToString(row["Work_Center"]);
                    entry.Est_Total_Hrs = Convert.ToString(row["Est_Total_Hrs"]);
                    entry.Act_Run_Labor_Hrs = Convert.ToString(row["Act_Run_Labor_Hrs"]);
                    entry.Sched_Start = Convert.ToString(row["Sched_Start"]);
                    entry.Promised_Date = Convert.ToString(row["Due_Date"]);
                    entry.Status = Convert.ToString(row["Status"]);

                    string topJob;
                    topJob = entry.Job.Substring(0,6);
                    foreach (DataRow deliveryrow in pRODUCTIONDataSet2.Tables["Delivery"].Rows)
                    {
                        if (deliveryrow["Job"].ToString() == topJob)
                        {
                              
                            entry.Promised_Date = Convert.ToString(deliveryrow["Promised_Date"]);
                        }
                      
                    }
                  
                    DataList.Add(entry);
                }
            }
            dataGridView1.DataSource = DataList;
            dataGridView1.Refresh();

        }

 

[jmcilhinney]

Have you actually used the debugger? I'm assuming not. Place a breakpoint at the top of the code and step through it line by line, examining the state at each step. Then you will be able to see exactly what happens and why.

 

[Jfisher387]

I have tried debugging before and cannot figure out how it works. I usually just put a messagebox.show("Test"); somewhere to see if it makes it to a point in the program but it doesn't obviously show me why it won't get there. I have tried following steps online for debugging and I get lost.

 

[jmcilhinney]

Then you should stop what you're doing and learn how to debug first, because you need to know how. The basics are pretty simple. Click on a line and press F9 to set a breakpoint. When execution breaks press F10 to step to the next line. At each step, use the Autos, Locals, Watch and Immediate windows to evaluate variables and other expressions of interest, or just hover the mouse over them in many cases. That's all you really need to start with but also may attentions to the Debug toolbar.

 

[Jfisher387]

I will give it a shot.

 

[Skydiver]

From what I can see from the last screenshot on post #1, the "Job" column is empty. Therefore, I find it highly unlikely that line 37 will be true, thereby making it look like the foreach loop on lines 35-43 are being skipped.

 

[Skydiver]

Personally, I would let the database do all the searching and joining for me instead of loading all the data into a DataSet and then doing all the searches and joins through the DataSet. Basically the other loop of lines 16-47 is doing a search (eg. a WHERE condition in SQL), and the inner loop on lines 35-43 is doing a join with line 37 being the JOIN condition.

 

[Jfisher387]

From what I can see from the last screenshot on post #1, the "Job" column is empty. Therefore, I find it highly unlikely that line 37 will be true, thereby making it look like the foreach loop on lines 35-43 are being skipped.

theres 20901 lines of data. there are some where job is empty. the vast majority have job numbers associated.

 

[Skydiver]

I see. Well then as suggested by @jmcilhinney in post #2 and #4, use breakpoints. Set a breakpoint on line 34 and 37. The breakpoint on line 34 should be hit only once, but the one on line 37 should be hit 20901 times.

 

[cjard]

Noo... you've ended up getting it all wrong. You put all that effort into making a strongly typed datatable, and you've even filled it with a table adapter like you should..

..and then you've looped through it as though it's a weakly typed, plain DataTable.

If you take away nothing else from this advice, take away this:

If you have a table that you filled with a XxxTableAdapter, and you then ever access the table's .Rows property, it is highly (highly) likely youre Doing It Wrong

It started going wrong at this point:

foreach (DataRow

But by the time it got to here, you were really railroaded into the hard way of doing things:

foreach (DataRow row in pRODUCTIONDataSet2.Tables

---

When you use the dataset designer to create visually represented data tables with nicely typed and named columns etc, though it's true that those things are DataTables, because they inherit from System.Data.DataTable, visual studio has put thousands of lines of code together for you to provide an inherited version that is vastly easier to use. Because this new version inherits from DataTable it still exhibits the properties of its parent, like Rows, but those things from the parent are hard work to the extent that you should never use them without good reason. All the easy stuff is on the inherited custom version of the datatable that VS made

I mentioned earlier that it went really wrong as soon as you accessed .Tables on a dataset. Without wanting to get too deep into inheritance specifics in C#, accessing a datatable via someDataSet.Tables gets you your easy to use custom data table but dressed up as a plain DataTable. From then on you'll find yourself accessing Rows, which gives you a plain DataRow and that really hides all the magic..

What magic? Well, every row in a datatable is an object, some different manifestation of related object data. If your datatable is called people, your data row might have Name, Age, SSN etc columns.. In plain DataRow you'd access them like this:

  var name = myDataRow["Name"] as string;
  var age = (int)myDataRow["Age"];

and so on. The magic though, is that most of the code visual studio wrote for you was in adding properties to a new class that inherits from data row. The properties behave like:

  public int Age{
    get{ return (int)myDataRow["Age"]; }
    set{ myDataRow["Age"] = value; }
  }

It's small, but it means that intellisense can really help you out so long as you don't drop to basic DataTable/DataRow mode

Your code should look like:

//use var, not DataRow
//Do not access Tables, which returns a basic datatable
//access the property named of the table name, which returns the customised table instance
foreach (var row in pRODUCTIONDataSet2.Job_Operation)
{
  //do not access the row by string indexer
  //access the named property of the column you want
  //The property is already a string. Do not convert it
  if(row.Job == jobID)
  {
                    DrilldownData entry = new DrilldownData();
                    entry.Job = row.Job; //access the property, the property is a string

Further, you've got that huge block of code that does all those conversions to convert row["Something"] and store Something from the row into the Something from the DrillDownData class..

..but the customised row class that VS wrote for you already has all those properties, named and typed to be usable.. so actually you don't even need your DrillDownData class any more, because you can just use a (the) Job_OperationRow class that VS wrote for you; it's a class with properties just like DrillDownData or any other even though its data is backed by a data row

Let this sink in a while and have an experiment with it; stop accessing column names by "string"- they're all available by property, which intellisense can see. If you end up with them not available by property you've accidentally dropped to the basic data row by some implicit cast (or accessed something that returns basic rows, like the Rows property)

(Whatever tutorial you were following, was not targeted at effective use of strongly typed tables, and was basically no good for you)

 

[cjard]

The next thing I want to talk about is your Fill commands

You've probably created your datatables and table adapters by giving a query of "SELECT * FROM Job" or similar..

You seldom, if ever, want to do this. I typically make the first query (the one that defines the table adapter and the schema of the associated data table) take the form SELECT * FROM Table WHERE PrimaryKeyColumn = @primaryKey - the @xxx is a sql variable, and the query looks up a single row with a particular primary key value. This is often useful for loading related data but you don't usually use it to load data by some search term. When the adapter wizard asks you what to call the query, I call it FillByXxxId or similar, something that indicates it is the primary key column that is searched. In the c# code we don't call Fill(table), we call FillByXxxId(table, "some xxx id here")

For those scenarios like loading all 100 jobs with status active or all 300 jobs created by John Smith, we add *another* query to the tableadapter. Right click, add query, enter SELECT * FROM jobs WHERE Status = @status , call it FillByStatus
In c# we might say FillByStatus(table, "Active") and only the rows with Active in the status column are filled. Similarly we can then add yet another query with "where creator = @creatorName" etc. there isn't a limit to the number of different queries you can put in a table adapter so make many for all your different searching requirements

Don't download your entire database into the datatable and then implement some slow search yourself; Microsoft spent millions on the cleverest beards in the industry, to make sure SQLServer can join and filter data as fast as possible. Anything you do in half an hour in C# will perform worse, so strive to use the tools at your disposal for the purpose intended. Your logic is nested loops, which is about as slow as you can make a correlation betwen two sets of data; a thousand rows in each set would need c# to make approximately half a million comparisons just to associate 2000 data items together

You don't have to stick to just one table in these additional filtering queries you put in a table adapter. For example, returning all jobs that had an operation in the related JobOperation table, named Blahblah:

Add a query to the Job TableAdapter like SELECT * FROM Job WHERE Job IN(SELECT Job FROM Job_Operation WHERE Name = @operationName) - and call it FillByOperationName

You can do anything you like with the sql query, so long as you return a set of results that has the same number and type and name of columns as the original set that were returned when you set up the tableadapter, and that means you can perform joins and grouping and other advanced query functions in the name of filtering or supplementing the rows returned

 

[cjard]

Hopefully now that you understand all that, we can take a look at your actual requirements.

So it seems you're searching the operation table for a job id and you want to add on just one bit of info from the Delivery table.

There are plenty of ways to skin this cat. Perhaps the first, straight forward approach:

1) you pull one job operation by job id -
* add a query to the JobOperation table adapter of "SELECT * FROM JobOperation WHERE Job = @jobId",
* you call it FillByJobId and GetDataByJobId,
* you invoke var jo = myJobOpTableAdapter.GetDataByJobId(jobId) (getdata gives you a new table, it can be easier than filling an existing)
you copy the data out and into your entry if you want

2) you pull one delivery based on the 6 chars of the job -
* add a query to the Delivery table adapter like "SELECT TOP 1 * FROM Delivery WHERE job = @job ORDER BY sometjing_sensible_here_to_determine_the_best_row DESC"
* invoke var d = myDeliveryTA.GetDataByJobId(jobId.Remove(6))


Now you have two data tables jo and d, you can fill your "entry" from. Remember; do not use .Rows property! Just access them like an array if you want the first row: jo[0].Name, jo[0].Whatever etc

 

[cjard]

And what might be a second, more cheaty way to do it? Well remember I said that you can use any SQL that returns any data so long as the columns are matched? Slight tweak; I'm reasonably certain (it's been a long time) you can return a subset of columns so long as the names match and the columns you don't return allow nulls, so a query like:

SELECT jo.Name, jo.Whatever, d.PromisedDate as JobOperationCreationDate
FROM
JobOperation jo
INNER JOIN Delivery d ON SUBSTRING(d.Job, 1, 6) = jo.Job
WHERE jo.Job = @job

Could be added as a query to the job operation, you'd get a job op row in the c# side whose creation date was the promised delivery date, and so long as the other columns in a JO are Nullable, anything not mentioned in the query would just be null. There is also nothing stopping you making more table adapters that show the results of join queries (but they aren't updatable) and other tricks..