Looking to improve efficiency of code

DonEB

Board Regular
Joined
Apr 26, 2016
Messages
77
I was preparing to send you a stripped down version of the workbook. And as I was testing the code to verify the error still exist... to my surprise it worked!!! Not sure why... I'm using the exact same data. I simply copied from one workbook to another.

I'm going to have to re-examine the code to see what I must have done wrong.

As soon as I figure that particular issue out... I will be able to take the next major step in revamping this program. Once again... thank you for all your help. From where I'm standing... it was priceless.
 

Some videos you may like

Excel Facts

How can you automate Excel?
Press Alt+F11 from Windows Excel to open the Visual Basic for Applications (VBA) editor.

DonEB

Board Regular
Joined
Apr 26, 2016
Messages
77
Stephen.... here is a link to a stripped down version of my project. It will contain only two worksheets and only one subroutine... your code and some misc stuff. Let me know if the link worked.
Code Not Summing Correctly
  • Worksheet("Courts1) - the worksheet that receives the information from your subroutine
  • Worksheet("CommonData") - the worksheet that contains the data the your subroutine uses to populate Worksheet("Courts1")
  • PlayerCombo - the subroutine you provided
I would first bring your attention to the CommonData worksheet.
  1. Down the left column "B", locate players 2, 3 and 4.
  2. Look at the combinations here 2 & 3, 2 & 4, 3 & 4 and the notice the results
  3. Example: Column B find 2 (at B5) and look across Row 3 until you find player number 3 (at M3). You will notice that there is a 1 in the M5 cell.
  4. The same will be true for the other two combinations above.
  5. Then go to macros and run the PlayerCombo subroutine
  6. I would like you to find the following results: players 5 - 2 - 4 - 3 by sorting column C by ascending order
  7. When you find the above series of numbers... you should find that there are all zeros on the fields where I would expect to see at a few 1's.
If I messed something up... I would appreciate you shedding some light on this for me. If there is something in the code.... then I know I would not be able to tweak it to make it work.

Thanks for taking the time to review this for me.

Don
 

StephenCrump

Well-known Member
Joined
Sep 18, 2013
Messages
3,634
Office Version
365
Platform
Windows
Maybe it's time to dig into the code, understand what it does? ;)

Try (in a junk copy of your workbook):
- putting some random numbers into CommonData!E5:X24 (perhaps =RANDBETWEEN(1,200) and copy/paste values so there are likely to be few duplicates)
- putting a breakpoint on the bold line below, and stepping through the first six iterations

VBA Code:
inarray = Sheets("CommonData").Range("E5:X24").Value2

For i = 1 To UBound(lCombinations)
    For j = 1 To UBound(lPairs)
[B]       dScores(i, j) = inarray(lCombinations(i, lPairs(j, 1)), lCombinations(i, lPairs(j, 2)))[/B]
    Next j
- testing the values of lPairs, lCombinations and dScores at each iteration.

This is the first combination, i.e. 1,2,3,4 (out of the Combin(20,4) combinations 1,2,3,4 | 1,2,3,5 | ... 17,18,19,20)
and the six iterations test the six pairs 1-2, 1-3, 1-4, 2-3, 2-4 and 3-4

Where (in inarray) is the code expecting to find the values for dScores? I gave you a clue in Post #2:

Your code does a lot of looping through rows and columns, because you are trying to locate player numbers, e.g. 6, 18, 17 and 29 for the first combination. If you think instead of this first combination as 1,2,3,4, the second as 1,2,3,5 ... and the 4,845th as 17,18,19,20, then you can work direct with these numbers as row and column numbers.

Do see why your layout is the problem?
 

StephenCrump

Well-known Member
Joined
Sep 18, 2013
Messages
3,634
Office Version
365
Platform
Windows
This is the bold line:

dScores(i, j) = inarray(lCombinations(i, lPairs(j, 1)), lCombinations(i, lPairs(j, 2)))

Maybe this new Forum doesn't let you change fonts inside code tags?
 

DonEB

Board Regular
Joined
Apr 26, 2016
Messages
77
Oh... my apologies. I misunderstood and thought the code already returned the combinations back to the actual values since "NameList" references those player numbers. So I may understand, what is NameList used for?
NameList.png

At the risk of asking too much of you, if you look at that chart in CommonData, you'll notice that I do have numbers 1 - 20 in E2:X2 and the actual players numbers are in E3:X3. The only way I know how to convert the numbers generated from your code back to the actual player numbers is by using a lookup. However, my guess is that there is a much more efficient way of handling this. Might you have one more suggestion for that?

I appreciate you helping understand what is happening within the code you provided.

Don
 

DonEB

Board Regular
Joined
Apr 26, 2016
Messages
77
Maybe it's time to dig into the code, understand what it does? ;)

Try (in a junk copy of your workbook):
- putting some random numbers into CommonData!E5:X24 (perhaps =RANDBETWEEN(1,200) and copy/paste values so there are likely to be few duplicates)
- putting a breakpoint on the bold line below, and stepping through the first six iterations

VBA Code:
inarray = Sheets("CommonData").Range("E5:X24").Value2

For i = 1 To UBound(lCombinations)
    For j = 1 To UBound(lPairs)
[B]       dScores(i, j) = inarray(lCombinations(i, lPairs(j, 1)), lCombinations(i, lPairs(j, 2)))[/B]
    Next j
- testing the values of lPairs, lCombinations and dScores at each iteration.

This is the first combination, i.e. 1,2,3,4 (out of the Combin(20,4) combinations 1,2,3,4 | 1,2,3,5 | ... 17,18,19,20)
and the six iterations test the six pairs 1-2, 1-3, 1-4, 2-3, 2-4 and 3-4

Where (in inarray) is the code expecting to find the values for dScores? I gave you a clue in Post #2:

Your code does a lot of looping through rows and columns, because you are trying to locate player numbers, e.g. 6, 18, 17 and 29 for the first combination. If you think instead of this first combination as 1,2,3,4, the second as 1,2,3,5 ... and the 4,845th as 17,18,19,20, then you can work direct with these numbers as row and column numbers.

Do see why your layout is the problem?
OK... my mind is exploding so please let me try this again.... I really didn't understand some of what you were asking me to try and do.

I have created the random sample of values as you suggested and I pasted the values in to the chart in CommonData!E5:x24.

I had to look up "breakpoint" to see what that was and how to do it... so I got figured out how to set the breakpoint by placing the cursor to the left side of the line of code and pressing F9. But, I'm not sure how to go about "testing the values of lPairs, lCombinations and dScores at each iteration".

Yes... I understand that the code is in a loop (For i and For j) but I'm not sure how to view the values of IPairs, ICombinations and dScores at each iteration. I would assume any code that would allow me to view the values would be placed right below the "dScores(i, j) = inarray(lCombinations(i, lPairs(j, 1)), lCombinations(i, lPairs(j, 2)))" code but that is simply my best guess.

And yes... I would like to try and understand how your code works.

Don
 

StephenCrump

Well-known Member
Joined
Sep 18, 2013
Messages
3,634
Office Version
365
Platform
Windows
Oh... my apologies.
No problem, and no apologies necessary.

And yes... I would like to try and understand how your code works.
Great! You may need to invest a bit of time, but it should give you the confidence to start making changes to the code ...

I suggest you do a bit of Googling - something like How to use the Visual Basic Editor should bring up a range of articles/video clips that you could work through depending on how you like to learn.

You've worked out how to insert a break-point (another way is to put the cursor into a code line and use F9 to toggle on/off, or use the menu Debug/Toggle Breakpoint).

You should also find out about:
- F5 (to run the code up to the next breakpoint)
- F8 or Shift-F8 (to step through the code line by line)
- Using the Immediate Window

For example, if you put in a breakpoint and step through the code using F8, you should be able to hover the cursor over particular terms j, i, lPairs(j, 1), dScores(i, j) and see the VB Editor evaluate these for you?

So I may understand, what is NameList used for?
NameList = Sheets("CommonData").Range("$E$3:$X$3").Value2

This is just a quick way to get the values of a range into a VBA array. NameList is a misnomer - It's actually your player numbers, rather than names, i.e. 14, 15, 19 ,5.

If you put a breakpoint in after this line, you can use the immediate window to see what you've got:
e.g. try
? Ubound(NameList)
? Ubound(NameList,2)

... will let you know you have a 1 x 20 array.
? NameList(1,3)
will return 19.

So if you step through this next bit of code for the first four iterations (i.e. i =1 and j = 1 to 4) you'll see how my first combination (1,2,3,4) is converted to your numbering (14,15,19,5). The results are stored in the first row of the lNumbers array, which is later dumped into Excel for viewing.

Code:
r = 4
'...
For i = 1 To UBound(lCombinations)
    '...
    For j = 1 To r
        lNumbers(i, j) = NameList(1, lCombinations(i, j))
    Next j
Next i
 

DonEB

Board Regular
Joined
Apr 26, 2016
Messages
77
No problem, and no apologies necessary.



Great! You may need to invest a bit of time, but it should give you the confidence to start making changes to the code ...

I suggest you do a bit of Googling - something like How to use the Visual Basic Editor should bring up a range of articles/video clips that you could work through depending on how you like to learn.

You've worked out how to insert a break-point (another way is to put the cursor into a code line and use F9 to toggle on/off, or use the menu Debug/Toggle Breakpoint).

You should also find out about:
- F5 (to run the code up to the next breakpoint)
- F8 or Shift-F8 (to step through the code line by line)
- Using the Immediate Window

For example, if you put in a breakpoint and step through the code using F8, you should be able to hover the cursor over particular terms j, i, lPairs(j, 1), dScores(i, j) and see the VB Editor evaluate these for you?



NameList = Sheets("CommonData").Range("$E$3:$X$3").Value2

This is just a quick way to get the values of a range into a VBA array. NameList is a misnomer - It's actually your player numbers, rather than names, i.e. 14, 15, 19 ,5.

If you put a breakpoint in after this line, you can use the immediate window to see what you've got:
e.g. try
? Ubound(NameList)
? Ubound(NameList,2)

... will let you know you have a 1 x 20 array.
? NameList(1,3)
will return 19.

So if you step through this next bit of code for the first four iterations (i.e. i =1 and j = 1 to 4) you'll see how my first combination (1,2,3,4) is converted to your numbering (14,15,19,5). The results are stored in the first row of the lNumbers array, which is later dumped into Excel for viewing.

Code:
r = 4
'...
For i = 1 To UBound(lCombinations)
    '...
    For j = 1 To r
        lNumbers(i, j) = NameList(1, lCombinations(i, j))
    Next j
Next i
Stephen... thanks to your persistence and willingness to work with me... I believe I found my problem.

It turns out that the chart that the table that was being referred to when you use this code
VBA Code:
inarray = Sheets("CommonData").Range("E5:X24").Value2
was not exactly as you had expected it to be. Let me explain.... while the player numbers in row E3:X3 were used to provide a reference to the above chart were in random order, I had actually sorted the player numbers in an associated column (top to bottom, ascending order) because I was using those numbers to conduct a LOOKUP... and that required them to be sorted.

However, in the code you provided, you performed all the summing for each group of players which took away the requirement for me to do any LOOKUPS... so I was able to get rid of the sort and now the appropriate values are being grouped together and summed. I hope that makes sense to you. But all is good now.

So... once again... thanks for your help and guidance. It was all much appreciated.

Don
 

StephenCrump

Well-known Member
Joined
Sep 18, 2013
Messages
3,634
Office Version
365
Platform
Windows
Great, I am glad you figured it out ...

... and maybe learned a bit of coding in the process.
 

DonEB

Board Regular
Joined
Apr 26, 2016
Messages
77
Great, I am glad you figured it out ...

... and maybe learned a bit of coding in the process.
Stephen.... I have been able to figure out quite a bit now. While I'm beginning to understand how certain code is working, I'm having some difficulty trying to modify the code to make it work when trying to fill all 5 courts. I need to be able to change the "E5:X24" reference below depending on which court I'm working on.

inarray = Sheets("CommonData").Range("E5:X24").Value2

I attempted to create a new public variable called cRange as variant. Then as I progressed down each step, I would reference the appropriate "range" of cells that will be used by your code to perform the proper summing of the values found within that chart. In other subroutines, I was setting cRange = Sheets("CommonData").Range("E5:X24").Value2 or cRange = Sheets("CommonData").Range("AS5:BH2024").Value2, etc. However, when I do this, then I get a "Run-Time Error '13'; Type Mismatch.

This confuses me a little since you defined inarray as variant and I defined Public cRange as variant.

I'm sure it is something simple... but even though I tried to google this... I must be asking the wrong question. Any thoughts? I think once I get this linked up properly then all the values I'm expecting to be summed... will be correct.

Thanks,
Don
 

Watch MrExcel Video

Forum statistics

Threads
1,089,877
Messages
5,410,923
Members
403,334
Latest member
SH2020

This Week's Hot Topics

Top