Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

6.) Use String returning functions - Most of the string utility functions have a version that returns a Variant and a version that returns a String (which ends with $). Unless you are storing the result in a Variant, you should use the String version to avoid implicitly casting the return value. There's a full list herehere.

6.) Use String returning functions - Most of the string utility functions have a version that returns a Variant and a version that returns a String (which ends with $). Unless you are storing the result in a Variant, you should use the String version to avoid implicitly casting the return value. There's a full list here.

6.) Use String returning functions - Most of the string utility functions have a version that returns a Variant and a version that returns a String (which ends with $). Unless you are storing the result in a Variant, you should use the String version to avoid implicitly casting the return value. There's a full list here.

Source Link
Comintern
  • 4.2k
  • 1
  • 18
  • 26

Overall not bad. Most of the issues are minor and stylistic, so don't let my brain dump below overwhelm...


1.) Comments - Limit your comments to code that isn't obvious as to what it does. The goal should be to explain the reason for code, not just to describe how it functions. For example...

'Ask for year to export
yearNum = InputBox("Please specify the year you intend to export.")

...and..

'Cycle through each worksheet of the workbook
For Each ws In Worksheets

...are commenting on things that are completely obvious from reading the line below them. A good rule of thumb is "would I trust the person who needs this comment to alter this code"? If the answer is no, leave it out. Otherwise it just adds noise.


2.) Duplicate dereferences - Capture references to objects that you repeatedly reference. I see 5 calls to Sheets("Output"), and the worksheet that it returns is never going to change. Get a reference before you start the loop and use it instead:

Dim target As Worksheet
Set target = Worksheets("Output")
'...
target.Cells(x, 1)...

3.) Deterministic lookups - Along the same lines as #2, use the references that you do have. You get the name of each worksheet here:

Worksheet = ws.Name

Then the only place it's used is to index into the Sheets collection like this...

Sheets(Worksheet)

...which always returns ... wait for it ... ws. Just use ws.


4.) You have a bug. This line of code...

If IsNumeric(Cells(x, y).Value) Then

...will always test the value of the active worksheet because you're using the global Cells collection. You need to fully qualify all of your references to Cells with a worksheet:

If IsNumeric(ws.Cells(x, y).Value) Then

I'd also qualify this reference to Rows, although Rows.Count will return the same value for any given worksheet. This should be habitual:

NextRow = Sheets("Output").Cells(Rows.Count, 1).End(xlUp).Row + 1

5.) Use constants - You have a bunch of magic number columns that you were compelled to comment, like this one:

'MACHINE
'Copy machine value from row 19 of current column
Sheets("Output").Cells(NextRow, 3) = Sheets(Worksheet).Cells(19, y)

You can make your code more readable and maintainable you defining and using constants instead of numbers:

'Module level
Private Const MACHINE_COL As Long = 3
Private Const SOURCE_MACHINE_COL As Long = 19
'...
target.Cells(NextRow, MACHINE_COL) = ws.Cells(SOURCE_MACHINE_COL, y)

Also, FinalRow is always 165 - there's no reason for it to be a variable.


6.) Use String returning functions - Most of the string utility functions have a version that returns a Variant and a version that returns a String (which ends with $). Unless you are storing the result in a Variant, you should use the String version to avoid implicitly casting the return value. There's a full list here.


7.) Use With blocks - Repeated dereferencing of objects to access members isn't free. Find sections of your code where you use the same object repeatedly, and then wrap it in a With ThatObject block. In this case, Sheets("Output") would be a good candidate.


8.) Get rid of superfluous tests - Unless I'm mistaken (and hopefully someone will correct me in the comments if I am), there isn't any way for Excel to return Null as the .Value of a cell:

Sub Nope()
    Sheet1.Cells(1, 1).Value = Null
    Debug.Print IsNull(Sheet1.Cells(1, 1).Value) 'Prints False
End Sub

You can remove the If IsNull(Cells(x, y).Value) = False Then test.


9.) Remove unneeded variables - Cyear will always be the same value as yearNum, although it does need to be validated and trimmed. See below.


10.) Don't trust user input - You'll get subtle errors with this code...

'Ask for year to export
yearNum = InputBox("Please specify the year you intend to export.")

'Load current year as string
Cyear = CStr(yearNum)
'Load next year as string, export all future months despite year entered
Nyear = CStr(yearNum + 1)

...if the user enters some non-numeric value like "this year". Validate user input to make sure you're getting a number. There are good examples of this over on SO.


11.) Useless lines - There's not really much point to the Exit Sub statement here...

PROC_ERR:
MsgBox "Error: " & Err.Number & "; " & Err.Description
Exit Sub

End Sub

...because... wait for it ... it's already the last statement in the Sub. I'd remove it.


Conclusion

Taking all of the above, the main loop should look more like this...

With Worksheets("Output")
    For Each ws In Worksheets
        If Right$(ws.Name, 4) = yearNum Or Right$(ws.Name, 4) = Nyear Then
            For y = 3 To 104 Step 6
                For x = 20 To 165 Step 5
                    If IsNumeric(ws.Cells(x, y).Value) Then
                        NextRow = .Cells(.Rows.Count, JOB_NUMBER_COL).End(xlUp).Row + 1
                        Sheets("Output").Cells(NextRow, JOB_NUMBER_COL).Value = Sheets(Worksheet).Cells(x, y).Value
                        'RUN DATE
                        'Set variable to schedule date range value; Absolute column A. Column A is merged vertically
                        WeekEndDate = ws.Cells(x, 1).MergeArea.Cells(1, 1).Value
                        'Find the week end date, strip the range from the string
                        WeekEndDate = Mid$(WeekEndDate, InStr(1, WeekEndDate, "-") + 2, _
                                           Len(WeekEndDate) - InStr(1, WeekEndDate, "-"))
                        .Cells(NextRow, RUN_DATE_COL) = WeekEndDate
                        .Cells(NextRow, MACHINE_COL) = ws.Cells(SOURCE_MACHINE_COL, y)
                        .Cells(NextRow, QUALITY_COL) = ws.Cells(x, y + 4)
                    End If
                Next x
            Next y
        End If
    Next ws
End With

...assuming the following constants:

Private Const JOB_NUMBER_COL As Long = 1
Private Const RUN_DATE_COL As Long = 2
Private Const MACHINE_COL As Long = 3
Private Const QUALITY_COL As Long = 4
Private Const SOURCE_MACHINE_COL As Long = 19

You can probably get a decent performance gain by switching to array processing instead of direct worksheet access, but it would be hard to recommend how to do that without seeing the actual workbook given the presence of merged cells.