Skip to main content
Code Review

Return to Answer

fixing silly mistake in code
Source Link
Joseph
  • 221
  • 1
  • 3

To supplement Gaffi's suggestions, I think you would benefit from changing this:

'Char mishap replacements
With Workbooks("CompanyBook.xlsm")
.Worksheets("Report1").Cells.SpecialCells(xlCellTypeConstants).Replace What:="&", Replacement:="&", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report1").Cells.Replace What:=""", Replacement:=Chr(34), LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="â€TM", Replacement:="’", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="…", Replacement:="...", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="£", Replacement:="£", LookAt:=xlPart, MatchCase:=False
'.Worksheets("Report2").Cells.Replace What:="†̃", Replacement:="‘L", LookAt:=xlPart, MatchCase:=False
.Worksheets("Company").Select
End With

To this:

Dim r1 As Excel.Range, r2 As Excel.Range
Set r1 = Workbooks("CompanyBook.xlsm").Worksheets("Report1").Cells.SpecialCells(xlCellTypeConstants)
Set r2 = Workbooks("CompanyBook.xlsm").Worksheets("Report2").Cells.SpecialCells(xlCellTypeConstants)
With r1
 r1.Replace What:="&", Replacement:="&", LookAt:=xlPart, MatchCase:=False
 r1.Replace What:=""", Replacement:=Chr(34), LookAt:=xlPart, MatchCase:=False
End With
With r2
  r2.Replace What:="…", Replacement:="...", LookAt:=xlPart, MatchCase:=False
 r2.Replace What:="…", Replacement:="...", LookAt:=xlPart, MatchCase:=False
 r2.Replace What:="£", Replacement:="£", LookAt:=xlPart, MatchCase:=False
End With

This way, you narrow down the selection to only cells that have content for Excel to find/replace. Also, because you set the range to a variable once, Excel doesn't have to search through all cells multiple times like it is now.

To supplement Gaffi's suggestions, I think you would benefit from changing this:

'Char mishap replacements
With Workbooks("CompanyBook.xlsm")
.Worksheets("Report1").Cells.SpecialCells(xlCellTypeConstants).Replace What:="&", Replacement:="&", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report1").Cells.Replace What:=""", Replacement:=Chr(34), LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="â€TM", Replacement:="’", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="…", Replacement:="...", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="£", Replacement:="£", LookAt:=xlPart, MatchCase:=False
'.Worksheets("Report2").Cells.Replace What:="†̃", Replacement:="‘L", LookAt:=xlPart, MatchCase:=False
.Worksheets("Company").Select
End With

To this:

Dim r1 As Excel.Range, r2 As Excel.Range
Set r1 = Workbooks("CompanyBook.xlsm").Worksheets("Report1").Cells.SpecialCells(xlCellTypeConstants)
Set r2 = Workbooks("CompanyBook.xlsm").Worksheets("Report2").Cells.SpecialCells(xlCellTypeConstants)
With r1
 r1.Replace What:="&", Replacement:="&", LookAt:=xlPart, MatchCase:=False
 r1.Replace What:=""", Replacement:=Chr(34), LookAt:=xlPart, MatchCase:=False
End With
With r2
  r2.Replace What:="…", Replacement:="...", LookAt:=xlPart, MatchCase:=False
 r2.Replace What:="…", Replacement:="...", LookAt:=xlPart, MatchCase:=False
 r2.Replace What:="£", Replacement:="£", LookAt:=xlPart, MatchCase:=False
End With

This way, you narrow down the selection to only cells that have content for Excel to find/replace. Also, because you set the range to a variable once, Excel doesn't have to search through all cells multiple times like it is now.

To supplement Gaffi's suggestions, I think you would benefit from changing this:

'Char mishap replacements
With Workbooks("CompanyBook.xlsm")
.Worksheets("Report1").Cells.Replace What:=""", Replacement:=Chr(34), LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="â€TM", Replacement:="’", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="…", Replacement:="...", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="£", Replacement:="£", LookAt:=xlPart, MatchCase:=False
'.Worksheets("Report2").Cells.Replace What:="†̃", Replacement:="‘L", LookAt:=xlPart, MatchCase:=False
.Worksheets("Company").Select
End With

To this:

Dim r1 As Excel.Range, r2 As Excel.Range
Set r1 = Workbooks("CompanyBook.xlsm").Worksheets("Report1").Cells.SpecialCells(xlCellTypeConstants)
Set r2 = Workbooks("CompanyBook.xlsm").Worksheets("Report2").Cells.SpecialCells(xlCellTypeConstants)
With r1
 .Replace What:="&", Replacement:="&", LookAt:=xlPart, MatchCase:=False
 .Replace What:=""", Replacement:=Chr(34), LookAt:=xlPart, MatchCase:=False
End With
With r2
 .Replace What:="…", Replacement:="...", LookAt:=xlPart, MatchCase:=False
 .Replace What:="£", Replacement:="£", LookAt:=xlPart, MatchCase:=False
End With

This way, you narrow down the selection to only cells that have content for Excel to find/replace. Also, because you set the range to a variable once, Excel doesn't have to search through all cells multiple times like it is now.

Source Link
Joseph
  • 221
  • 1
  • 3

To supplement Gaffi's suggestions, I think you would benefit from changing this:

'Char mishap replacements
With Workbooks("CompanyBook.xlsm")
.Worksheets("Report1").Cells.SpecialCells(xlCellTypeConstants).Replace What:="&", Replacement:="&", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report1").Cells.Replace What:=""", Replacement:=Chr(34), LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="â€TM", Replacement:="’", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="…", Replacement:="...", LookAt:=xlPart, MatchCase:=False
.Worksheets("Report2").Cells.Replace What:="£", Replacement:="£", LookAt:=xlPart, MatchCase:=False
'.Worksheets("Report2").Cells.Replace What:="†̃", Replacement:="‘L", LookAt:=xlPart, MatchCase:=False
.Worksheets("Company").Select
End With

To this:

Dim r1 As Excel.Range, r2 As Excel.Range
Set r1 = Workbooks("CompanyBook.xlsm").Worksheets("Report1").Cells.SpecialCells(xlCellTypeConstants)
Set r2 = Workbooks("CompanyBook.xlsm").Worksheets("Report2").Cells.SpecialCells(xlCellTypeConstants)
With r1
 r1.Replace What:="&", Replacement:="&", LookAt:=xlPart, MatchCase:=False
 r1.Replace What:=""", Replacement:=Chr(34), LookAt:=xlPart, MatchCase:=False
End With
With r2
 r2.Replace What:="…", Replacement:="...", LookAt:=xlPart, MatchCase:=False
 r2.Replace What:="…", Replacement:="...", LookAt:=xlPart, MatchCase:=False
 r2.Replace What:="£", Replacement:="£", LookAt:=xlPart, MatchCase:=False
End With

This way, you narrow down the selection to only cells that have content for Excel to find/replace. Also, because you set the range to a variable once, Excel doesn't have to search through all cells multiple times like it is now.

lang-vb

AltStyle によって変換されたページ (->オリジナル) /