Re: Please Help me Optimize my Code
- From: Sergey Poberezovskiy <SergeyPoberezovskiy@xxxxxxxxxxxxxxxxxxxxxxxxx>
- Date: Wed, 30 May 2007 21:49:00 -0700
1) You should use dataset, not datarow to get the initial dataview:
Dim myView As Data.DataView = myDataSet2.Tables(0).DefaultView
2) To avoid duplicate IDs you do not have to use any of the methods you
described - you could just change your loops as follows:
For compFactor1 = 1 To maxRatio
For compFactor2 = compFactor1 To maxRatio
and this way you will cut the number of iterations, as well as resolve the
problem of possible duplicates.
On May 30, 3:59 pm, Sergey Poberezovskiy.
I did not quite understand your logic with IDs - so if you could explain it
better, I will try to come up with a solution. Meanwhile, we could shrink the
number of calculations that your code performs. Though arithmetic operations
do not seem to be expensive, but considering the number of times you loop
inside your code...
a) consider the following line (executed for every 1000 rows in reader):
compRed1 = myReader1.Item(myReader1.GetOrdinal("redvalue"))
I would declare a redOrdinal variable instead before the loop:
Dim columns As DataColumnCollection = myDS2Row.Tables(0).Columns
Dim redOrdinal As Integer = columns.IndexOf("redvalue")
b) testRed = (compRed1 * compFactor1 + compRed2 * compFactor2) /
(compFactor1 + compFactor2)
(compFactor1 + compFactor2) - in your algorithm is calculated 3 times
instead of 1 for 1000 rows (in reader) * 255 (by brand) * maxRatio * maxRatio
- and now it makes sense to declare a variable:
Dim compFactorSum As Integer = compFactor1 + compFactor2
c) "startRed + maxDiff" and "startRed - maxDiff" could also be calculated
outside the very first (reader) loop
d) myReader1.GetOrdinal("brand")) and myReader1.GetOrdinal("id") - again -
move outside the loops
e) in your inner loop, where you check:
If (testRed < startRed + maxDiff And testRed > startRed - maxDiff) _
AndAlso (testGreen < startGreen + maxDiff And testGreen > startGreen -
AndAlso (testBlue < startBlue + maxDiff And testBlue > startBlue - maxDiff)
ensure that you use AndAlso instead of And - this way you can cut quite a
number of comparisons as the second token in the And statement will not be
evaluated (for AndAlso) if the first one is False.
I know that some of the suggestions look like very minor improvements, but
taken into account the number of iterations, I think every bit counts.
On May 29, 8:56 pm, Sergey Poberezovskiy
TF, you have just a HUGE number of iterations happenning on the page - and
some of them are unnecessary. A few suggestions:
- create a vew on the datatable from the dataset
Dim myView As DataView = myDS2Row.Tables(0).DefaultView
and then on every reader row assign a filter to it:
myView.RowFilter = "brand='" & myReader1("brand").ToString & "' and id<>" &
then instead of going though the whole table loop through the view rows
- instead of StringBuilders to keep IDs I would suggest using generic lists
Dim usedIDs As List(Of Integer) = New List(Of Integer)
then you could easily test whether an ID is already there (Contains method)
and add new ID (Add method)
Try that for a start, I may try to come up with a few more suggestions if I
have some more time...
I made a ASP.NET 2.0 site that shows possible "recipes" for paint
colors stored in an access dbase. Basically, 1000 colors are stored
= " & compFactor1 & "<br>")
= " &
read more »
I really think most of my iterations are looping just to get skipped
with no results. Let me see if I understand your first suggestion.
Create the view on the table before any looping. Then inside the
first/outer loop filter the view so that the inner loop will only see
brands that match the outer loop? Also exclude the id? If that's
what I think it is then that will cut the inner loops by about 70%
since the most records of any brand is about 255. Hmmm. Very
interesting. For the second suggestion, I see how it could be more
efficient but I don't understand how it will tell me what combos were
used. I track the IDs in pairs because it's ok if either component
gets used again with some other recipe. Maybe I'm missing your
point. Thanks very much!
First the IDs. It's just an autonumber field w/no duplicates. When I
started this I found that paint pairs that met the criteria at, say
2:1 ratio, were also displayed at 4:2, 8:4, etc. I want them to only
display the first time so I started concatting strings (later traded
for stringbuilder). I needed the ID pairs to be fwd and rev because
the outer loop would eventually get to the second record of the combo
and it would display again. It's the only workaround I could come up
with to keep used pairs from displaying more than once.
I created lowRed, hiRed, etc variables before the first loop. Good
idea. Would like to tackle an issue with your previous suggestion
before going into the others. I put in the statement...
Dim myView As Data.DataView = myDS2Row.Tables(0).DefaultView
....right after the "dataAdapter.Fill(myDataSet2)" line. VWD made me
change it to...
Dim myView As System.Data.DataView = myDS2Row.Table().DefaultView
....and VWD is now warning me that myDS2 is used before it's been
assigned a value. Also errors out when I try to run it. I think
filtering out most of the rows before the inner loop starts would be a
huge step. Am I missing something?
Thanks for your help, Sergey.