Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

Your code has tremendously improved since the first time I saw it first time I saw it - good job!

Your code has tremendously improved since the first time I saw it - good job!

Your code has tremendously improved since the first time I saw it - good job!

Source Link
Mathieu Guindon
  • 75.5k
  • 18
  • 194
  • 467

Your code has tremendously improved since the first time I saw it - good job!


This particular line is hard to parse:

wsImport.Range(source.Offset(1), wsImport.Cells(Rows.Count, source.Column).End(xlUp)).Copy _
 wsMain.Cells(Rows.Count, dest.Column).End(xlUp)(2)

Literally: it's crashing the parser!

You could introduce a local variable here:

Dim target As Range
Set target = wsMain.Cells(Rows.Count, dest.Column).End(xlUp)
wsImport.Range(source.Offset(1), wsImport.Cells(Rows.Count, source.Column).End(xlUp)) _
 .Copy target(RowIndex:=2)

Notice how the line continuation is placed so that no instruction is split; doesn't care about line continuations, but it's much easier for the human eye to see what function calls return the arguments for which procedure if you don't split an instruction between the name of a procedure and its arguments - vertically lining up .Range and .Copy also make it clearer that .Copy operates on the result of .Range.


Dim wsImport As Worksheet, wsMain As Worksheet

Is this really buying you anything? Multiple declarations on a single line make it harder to locate declarations at a glance. Compare to:

Dim wsImport As Worksheet
Dim wsMain As Worksheet

My eye sees Dim, my brain sees "variable declaration here" - two Dims, two variables. And I read the variable name at pretty much the exact same millisecond as the one I notice the Dim statement, because I don't need to mentally scroll horizontally and locate the comma. Two variables isn't too bad, but more than that would be problematic. Better avoid multiple declarations on a single line.


There's a redundant reference to wsMain in this With block:

With wsMain
 .Columns("A:AO").AutoFit
 .Cells.ClearFormats
 .Rows(1).Font.Bold = True
 .Cells.Font.Name = "Georgia"
 .Cells.Font.Color = RGB(0, 0, 225)
 .Cells.Resize(wsMain.Rows.Count - 1).Offset(1).Interior.Color = RGB(216, 228, 188)
End With

See it? Right here:

.Cells.Resize(wsMain.Rows.Count - 1).Offset(1).Interior.Color = RGB(216, 228, 188)

Could be

.Cells.Resize(.Rows.Count - 1).Offset(1).Interior.Color = RGB(216, 228, 188)

The extraneous empty lines before End With should be removed, too.


Your indentation isn't consistent.

Dim ws As Worksheet
 For Each ws In Worksheets
 ws.Select
 ActiveWindow.Zoom = 85
 Next ws

The only thing that should be at the same indentation level as Public Sub/End Sub, is line labels (which the VBE forces to start at column 1 anyway).

lang-vb

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