PDA

View Full Version : Can someone critique my code please



Larbec
11-09-2015, 05:54 AM
I have written code that resets a sheet called "Input". What is happening is when I go to run it all it wants to do is sit and spin and I finally give up after 30 minutes and kill the file. Now, this same code will run in a different book I have. Perhaps something is not correct. My books should be identical. Any suggestions would be greatly appreciated.

How this works is I place the Game number I want to reset and all the numbers move down a row. So, If I place a 48 in Q5 the numbers will move from Game 50 to Game 48

Thanks



Sub ResetBackToGame()
Dim d As Double:
d = Application.WorksheetFunction.CountIf([i2:i101], "?")
Dim i As Integer:
'make sure user entered positive number 1 to 50 into Q5
If IsEmpty(Range("Q5")) _
Or Range("Q5") < 1 _
Or Range("Q5") > 50 Then
'do nothing if is empty or value not between 1 and 50, inclusive
Exit Sub
End If
i = 50 - [q5]
Application.ScreenUpdating = False
[I2:N101].Copy Cells(3 + i - d, 9) 'copies I2:O101 to position (3+i-d) rows down.
Range("i2", Cells(2 + i, 15)) = "?" 'puts ? in I2 over & down to cleared row in N.
[i102:n152].CLEAR: [q5].ClearContents
Application.ScreenUpdating = True 'not required, auto-resets to True at End Sub or Exit Sub
End Sub

Here is a snip of the Input sheet

14723

SamT
11-09-2015, 10:04 AM
If you use the "Go Advanced" button, below the Advanced Editor, the "Manage Attachments" button will let you upload workbooks.

The first thing to do is change this line
Application.ScreenUpdating = False

To:
'Application.ScreenUpdating = False 'Uncomment after Testing

Important! Insure that "d" is never greater than "i" + 2, else
[I2:N101].Copy Cells(3 + (50 - 48) - 4, 9) 'Pastes to Row 1. Or worse, there is no Row 0

"Range("i2", Cells(2 + i, 15))" Column "15" is Column "O" (Oh)

"...: [q5].ClearContents" Row 5 has shifted down. Also, you have two commands on one line = confusing

"d" is a Double, but can never be a decimal number. "i" is an Integer, but Row numbers are Variants, (Strings or Longss.). Neither of these should affect the code, the VBA compiler is very forgiving.

Use of the Evaluate Method, "[...]", instead of the Range Object can slow down the Code, but usually not by any noticeable amount. "Range("i2", Cells(...))" also requires the Compiler to evaluate, (guess,) what you mean by "i2".



Many of my comments are regarding coding style, but your situation is one reason I use a very explicit style and avoid all "Evaluations." "Range(Range("i2"), Cells(...))"

Larbec
11-09-2015, 10:31 AM
I finally received an error. Can you tell me what to check. The book is too large to upload

Thanks

14725

SamT
11-09-2015, 11:03 AM
I was in the process of editing my post when you last read it.

My first suspect in that error is d => i + 2

SamT
11-09-2015, 11:14 AM
The book is too large to upload
We would only need that one sheet with the code in it.

Paul_Hossler
11-09-2015, 11:20 AM
Not checking the logic, but I think that the [...] notation means evaluate




[I2:N101].Copy Cells(3 + i - d, 9) 'copies I2:O101 to position (3+i-d) rows down.
Range("i2", Cells(2 + i, 15)) = "?" 'puts ? in I2 over & down to cleared row in N.
[i102:n152].CLEAR: [q5].ClearContents


should be



Range("I2:N101").Copy Cells(3 + i - d, 9) 'copies I2:O101 to position (3+i-d) rows down.
Range("i2", Cells(2 + i, 15)) = "?" 'puts ? in I2 over & down to cleared row in N.
Range("i102:n152").CLEAR
Range ("q5").ClearContents


The second line might be more understandable using .Resize





Range("i2", Cells(2 + i, 15)) = "?" 'puts ? in I2 over & down to cleared row in N.

SamT
11-09-2015, 11:35 AM
@ Paul, The A/A Icon lets you paste CODE Tagged code without the formatting

Paul_Hossler
11-09-2015, 12:22 PM
@ Paul, The A/A Icon lets you paste CODE Tagged code without the formatting


Well that'll be easier than the way I usually do it:thumb

Larbec
11-09-2015, 12:42 PM
Thanks Sam, there is no numbers above a 9 and it is column "O" oh

To not confuse myself basically the code is fine as is except for changing the
'Application.ScreenUpdating = False 'Uncomment after Testing

Could you rewrite the code (if you have time and do not mind) with the changes so I can see exactly the changes you are speaking about. I am new to VBA but trying and learing

Thanks

Larbec
11-09-2015, 12:51 PM
I have taken out quite a bit and thin it will still run once its fixed. I REALLY appreciate all your help with this

Paul_Hossler
11-09-2015, 01:49 PM
Changing the macro like I did in #6 runs without errors and puts "?" in I2:N52




Option Explicit

Sub ResetBackToGame()
Dim d As Double
d = Application.WorksheetFunction.CountIf([i2:i101], "?")
Dim i As Integer
i = 50 - [q5]
'Application.ScreenUpdating = False 'Uncomment after Testing
Range("I2:O101").Copy Cells(3 + i - d, 9)
Range("i2", Cells(2 + i, 15)) = "?"
Range("i102:o152").CLEAR
Range("q5").ClearContents
Application.ScreenUpdating = True
End Sub




Don't know if the macro does what you want it to, but it does run without errors

SamT
11-09-2015, 02:20 PM
Paul did it the way I do.

Larbec
11-09-2015, 02:51 PM
Changing the macro like I did in #6 runs without errors and puts "?" in I2:N52




Option Explicit

Sub ResetBackToGame()
Dim d As Double
d = Application.WorksheetFunction.CountIf([i2:i101], "?")
Dim i As Integer
i = 50 - [q5]
'Application.ScreenUpdating = False 'Uncomment after Testing
Range("I2:O101").Copy Cells(3 + i - d, 9)
Range("i2", Cells(2 + i, 15)) = "?"
Range("i102:o152").CLEAR
Range("q5").ClearContents
Application.ScreenUpdating = True
End Sub




Don't know if the macro does what you want it to, but it does run without errors

Thanks Guys/Paul,

I am running it now for 3 minutes which is not normal, it must be something I have wrong with an external sheet perhaps that get the counter numbers tot eh Game sheets. I will also try ad run it like I uploaded it for the site and report back

Larbec
11-09-2015, 03:43 PM
I tried to run the code and I do not understand what is going on. It does not run for me just sits and spins but it runs for you guys. I must have another issue someplace in my settings perhaps. errr, thanks for your help and patience. I get the same error as before, method copy error as in post #3

Paul_Hossler
11-09-2015, 04:40 PM
You have links to 3 other workbooks (picture attached), and the only place I could find an external reference was on the hidden sheet 'Unlocked copy of ND'

Can you delete that sheet or at least not need external workbooks?


14727

SamT
11-09-2015, 05:37 PM
Sub ResetBackToGame()
Dim d As Double
d = Application.WorksheetFunction.CountIf([i2:i101], "?")
Dim i As Integer
i = 50 - [q5] '<-- Set BreakPoint here
Dim X
X = 3 + i - d
'Application.ScreenUpdating = False 'Uncomment after Testing
Range("I2:O101").Copy Cells(3 + i - d, 9)
Range("i2", Cells(2 + i, 15)) = "?"
Range("i102:o152").CLEAR
Range("q5").ClearContents
Application.ScreenUpdating = True
End Sub

Run the sub, It will stop at the Breakpoint. Press F8 until the first "Range(..." line is highlighted. Hover the Mouse over the "X" variable to see it's Value. You can also hover over the "i" and "d" variables to see their values.


Seriously, this is an Excel Crash waiting to happen, because you have no control over the relative values of "i" and "d".
Cells(3 + i - d, 9)

Larbec
11-09-2015, 06:29 PM
You have links to 3 other workbooks (picture attached), and the only place I could find an external reference was on the hidden sheet 'Unlocked copy of ND'

Can you delete that sheet or at least not need external workbooks?



14727




I need those books paul, unless I put them all in one which I could do but would take some time i'm sure. They are all linked together

Larbec
11-09-2015, 06:34 PM
Sam,

when I run it , it just sits and looks like this. Are you asking me to keep pressing F8 like I am going in to safe mode? I tried that too and nothing

14728

Larbec
11-09-2015, 06:47 PM
Sam,

I am not sure if this helps but here are all my notes when I originally wrote the code


Sub ResetBackToGame()
Dim d As Double:
d = Application.WorksheetFunction.CountIf([i2:i101], "?")
Dim i As Integer:
'make sure user entered positive number 1 to 50 into Q5
If IsEmpty(Range("Q5")) _
Or Range("Q5") < 1 _
Or Range("Q5") > 50 Then
'do nothing if is empty or value not between 1 and 50, inclusive
Exit Sub
End If
i = 50 - [q5]
Application.ScreenUpdating = False
[I2:O101].Copy Cells(3 + i - d, 9) 'copies I2:O101 to position (3+i-d) rows down.
Range("i2", Cells(2 + i, 15)) = "?" 'puts ? in I2 over & down to cleared row in O.
[i102:o152].CLEAR: [q5].ClearContents
Application.ScreenUpdating = True 'not required, auto-resets to True at End Sub or Exit Sub
End Sub



]

14728

SamT
11-10-2015, 09:07 AM
Run this sub just for grins, then try my advice again


Sub RestoreApplication()
With Application
.DisplayAlerts = True
.Calculation = xlCalculationAutomatic
.EnableEvents = True
.CutCopyMode = False
.ScreenUpdating = True
End With
End Sub

Larbec
11-10-2015, 12:08 PM
Run this sub just for grins, then try my advice again


Sub RestoreApplication()
With Application
.DisplayAlerts = True
.Calculation = xlCalculationAutomatic
.EnableEvents = True
.CutCopyMode = False
.ScreenUpdating = True
End With
End Sub



Sam,

I ran only the above and it runs but nothing happens even when I press F8. What is supposed to take place or perhaps I need to run it differently. I am assigning it to the "reset" button and pressing the "reset button" This is the "only" Macro in the module

Even when I go to the module and hit run, it runs but does nothing?

Thanks