Can someone critique my code please

[Larbec]

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

Pic of Input sheet.jpg

[SamT]

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]

I finally received an error. Can you tell me what to check. The book is too large to upload

Thanks

run time error.jpg

[SamT]

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]

The book is too large to upload

We would only need that one sheet with the code in it.

[Paul_Hossler]

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]

@ Paul, The ^A/A Icon lets you paste CODE Tagged code without the formatting

[Paul_Hossler]

@ 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

[Larbec]

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]

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]

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]

Paul did it the way I do.

[Larbec]

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]

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]

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?


Capture.JPG

[SamT]

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]

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?



Capture.JPG

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]

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

Attachment 14728

[Larbec]

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
   
   
   
  ]

Attachment 14728

[SamT]

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