Summary Data According to Machine No, Order No and Shifts

Here is the code that I write for my work on Excel-vba. I use that code to know shifts' and operators' performances.

Main purpose is to divide machines' work to orders and shifts.

```
Sub Ucshift()

On Error Resume Next
'Cleaning before start

Sheets("Data").Range("A1:R568").AutoFilter
Sheets("Data").Range("A:A,F:I,N:O,S:T").EntireColumn.Hidden = True
Sheets("Dashboard").Range("A2:J1500").ClearContents

Dim shift As Integer
Dim OrderNo As String
Dim unplanned As Long
Dim planned As Long
Dim prodTime As Double
Dim production As Long
Dim ppm_scrap As Long
Dim beginingScrap As Long
Dim ppm As Long
production = 0
prodTime = 0
unplanned = 0
planned = 0
ppm = 0
ppm_scrap = 0
beginingScrap = 0
ppm = 0
Dim sayim As Integer
Dim j As Long
Dim machine As String
Dim i As Long
Dim sorgu() As String
ReDim sorgu(Sheets("Dashboard").Range("K12222").End(3).Row - 1)

Dim temp As Integer

'will loop each machine
For j = 2 To Sheets("Dashboard").Range("K12222").End(3).Row
machine = Sheets("Dashboard").Range("K" & j)

'Keeps how many rows that machine has, to know when it ends
sorgu(j - 1) = (WorksheetFunction.VLookup(machine, Sheets("Data").Range("J:J"), 1, 0))

If (sorgu(j - 1) = "") Then
GoTo Ending 'To pass the next machine
End If

sayim = 0
'Take that machines row to another sheet to work on them easily
With Sheets("Data")
.Range("$A$1:$T$5000").AutoFilter Field:=10, _
Operator:=xlFilterValues
.Range("$A$1:$T$5000").AutoFilter Field:=10, Criteria1:=machine
.AutoFilter.Sort. _
SortFields.Clear
.AutoFilter.Sort. _
SortFields.Add Key:=Range("D1:D5000"), SortOn:=xlSortOnValues, Order:= _
xlAscending, DataOption:=xlSortNormal
With .AutoFilter. _
Sort
.Header = xlYes
.MatchCase = False
.Orientation = xlTopToBottom
.SortMethod = xlPinYin
.Apply
End With

To make the code more readable and more flexible, i.e. more maintainable, you might try to apply some general principles and some useful features of VBA/Excel.

The two most useful, albeit in practise hard to get completely right, principles are good naming and the single responsibility principle. In short, the single responcibility principle tells you that each unit of your code, like e.g. a Sub, should do one thing and only one.

Let me explain these two points a little further with reference to the code.

The array name sorgu for example might be replaced with rowsOfMachine. (At least that is what your comment is saying what is saved in the array.) This would immediately make clear what the array is used for. This would be a great help for the next person looking at the code.

In general, the name of a variable or procedure or function should say what it is or what it does, but not how it does it. Here, a longer name that conveys the meaning better is preferable to a shorter one with less expressive power.

One VBA feature that helps you naming types of things are Enums. These let you give names to numbers. In the code, the shifts are divided into three values. To convey their meaning better you could introduce an Enum as follows.

Enum Shifts
   dayShift = 1
   eveningShift = 2
   nightShift = 3
End Enum

Then you can assign the three constants instead of assigning the numbers directly. Moreover, you can always see what shifts exist by looking at the Enum declaration.

The single responsibility principle can be applied in several ways to your code.

Looking at the shifts again, you can see that your Sub seems to have the responsibility to determine what shift a certain time belongs to. Changes in the shifts would be easier to maintain, if the logic for the shifts were extracted into a function taking the time string as a parameter and returning the Enum. (Enums are of type Long.)

In general, a good guideline what to extract from a large procedure is to extract everything that would otherwise warrent a comment as heading. Then calling the subprocedure or function actually makes the comment obsolete. Extracting procedures and functions has the added benefit that you can reuse the procedures.

One responsibility of the code that is smeared over the entire procedure is knowing where things are saved in the workbook. This is problematic since you will have to change things everywhere whenever the layout changes.

There, are several ways you could handle this. One way would be to extract aptly named functions that return the the ranges. The other way would be to inject them as a parameter into the Sub. However, then you cannot call it anymore as a macro.

No matter how you extract this responsibility, there is one feature of Excel that allows you to not hardcode the ranges at all, named ranges. You can define named ranges in Excel and then get the ranges by name in VBA. This way, you only have to change the named ranges when changing the layout of your workbook, but not the code; at least as long as the changes are not too large.

A further thing you might want to consider is to read the data inta a two dimensional array instead of copying it to another sheet. This is not only faster, at least to my eyes, code handling array indices is easier to read then code referencing cells on a woksheet.
Loading from and writing the array back to a woksheet is very simple. For both you can use the Range.Value method.

Dim rangeArray As Variant
rangeArray = ActiveWorksheet.Range("A3:C12").Value
ActiveWorksheet.Range("H13").Value = rangeArray

Often Value2 is better for reading in the data because it is faster and does not interpret the values in the cells. (See this blog post.) However, in the code the datetimes are actually processed. So Value is probably more appropriate.

Now, let me mention two general programming guidelines which are not followed in the code:

• GoTo is evil: it makes it really complicated to follow the logic of a procedure. The only place it actually makes sense is inside an On Errorstatement or an error handler. (You do not have much choice, there.)

In the code, it could simply be replaced with Continue For.

• Never turn off errors unless you specifically know why you are doing it. Errors get raised in order to make you aware that something is broken. Writing On Error Resume Next at the start of a procedure is a very bad idea.

There are circumstances where you want to continue after an operation that might fail, most of the time because you know how to identify the failure from a return value and furthermore know how to handle it right there. In this case you should turn off errors with On Error Resume Next right before the statement and right after turn it on again with On Error GoTo 0. (GoTo 0 indicates that the error should be raised and the procedure should stop right there.)

StackExchange Code Review Q#150073, answer score: 7