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. _
xlAscending, DataOption:=xlSortNormal
With .AutoFilter. _
Sort
.MatchCase = False
.Orientation = xlTopToBottom
.SortMethod = xlPinYin
.Apply
End With
Sheets("Model").Range("A1:R700").Clear
.Range("B1:R" & .Range("B500000").End(xlUp).Row).Copy
End With

With Sheets("Model")

.Range("A1").PasteSpecial

'Take the order no.
OrderNo = .Range("D2").Value

'Go through machine rows
For i = 2 To .Range("A50000").End(xlUp).Row + 1

If .Cells(i, 4) = OrderNo Then

'At first loop, we dont know the shift for sure.
If i <> 2 Then
temp = shift
End If

'Decide the shift
If (Format(.Range("G" & i), "hh:mm") >= "07:30" And Format(.Range("G" & i), "hh:mm") <= "16:59") Then 'And (Format(.Range("H" & i), "hh:mm") > "07:30" And Format(.Range("H" & i), "hh:mm") <= "17:00")
shift = 1
ElseIf (Format(.Range("G" & i), "hh:mm") >= "17:00" Or Format(.Range("G" & i), "hh:mm") < "00:30") Then ' And (Format(.Range("H" & i), "hh:mm") <= "00:31" Or Format(.Range("H" & i), "hh:mm") > "17:01"))
shift = 2
Else
shift = 3
End If

'At first loop, temp is loaded with first shift after idenfying the shift. Temp is used to know when shift changes
If i = 2 Then
temp = shift
End If

If temp = shift Then 'If shift didnt change, continue
sayim = 1

'Counting unplanned, planned stops and production time
If .Cells(i, 6).Value = "D01" Or .Cells(i, 6).Value = "D02" Or .Cells(i, 6).Value = "D03" Or .Cells(i, 6).Value = "D04" Or .Cells(i, 6).Value = "D05" Or .Cells(i, 6).Value = "D06" Or .Cells(i, 6).Value = "D07" Or .Cells(i, 6).Value = "D08" Or .Cells(i, 6).Value = "D09" Or .Cells(i, 6).Value = "D12" Or .Cells(i, 6).Value = "D13" Or .Cells(i, 6).Value = "D14" Or .Cells(i, 6).Value = "D15" Or .Cells(i, 6).Value = "D16" Or .Cells(i, 6).Value = "D17" Or .Cells(i, 6).Value = "D18" Or .Cells(i, 6).Value = "D19" Or .Cells(i, 6).Value = "D20" Or .Cells(i, 6).Value = "D21" Or .Cells(i, 6).Value = "D22" Or .Cells(i, 6).Value = "D23" Or .Cells(i, 6).Value = "D24" Or .Cells(i, 6).Value = "D25" Or .Cells(i, 6).Value = "D26" Or .Cells(i, 6).Value = "D27" Or .Cells(i, 6).Value = "D28" Or .Cells(i, 6).Value = "D29" Or .Cells(i, 6).Value = "D30" Then
unplanned = unplanned + (.Cells(i, 8) - .Cells(i, 7)) * 1440
ElseIf .Cells(i, 6).Value = "D00" Or .Cells(i, 6).Value = "D10" Or .Cells(i, 6).Value = "D11" Then
planned = planned + (.Cells(i, 8) - .Cells(i, 7)) * 1440
ElseIf .Cells(i, 6).Value = 0 Then
prodTime = prodTime + (.Cells(i, 8) - .Cells(i, 7)) * 24
End If

'getting scrap amounts
If .Cells(i, 11).Value = "F0A" Or .Cells(i, 11).Value = "F0B" Then
beginingScrap = beginingScrap + .Cells(i, 10)
ElseIf .Cells(i, 11).Value <> "" Then
ppm_scrap = ppm_scrap + .Cells(i, 10)
End If

'Getting production amount
production = production + .Cells(i, 9)

Else 'If shift has changed

'Write the results of the shift to Dashboard
If sayim <> 0 Then
Sheets("Dashboard").Range("A" & Sheets("Dashboard").Range("A10000").End(xlUp).Row + 1) = temp
Sheets("Dashboard").Range("B" & Sheets("Dashboard").Range("b10000").End(xlUp).Row + 1) = machine
Sheets("Dashboard").Range("c" & Sheets("Dashboard").Range("c10000").End(xlUp).Row + 1) = OrderNo
Sheets("Dashboard").Range("d" & Sheets("Dashboard").Range("d10000").End(xlUp).Row + 1) = unplanned
Sheets("Dashboard").Range("e" & Sheets("Dashboard").Range("e10000").End(xlUp).Row + 1) = planned
Sheets("Dashboard").Range("g" & Sheets("Dashboard").Range("g10000").End(xlUp).Row + 1) = production
Sheets("Dashboard").Range("h" & Sheets("Dashboard").Range("h10000").End(xlUp).Row + 1) = beginingScrap
Sheets("Dashboard").Range("I" & Sheets("Dashboard").Range("I10000").End(xlUp).Row + 1) = ppm_scrap
ppm = (ppm_scrap / production) * 1000000
Sheets("Dashboard").Range("j" & Sheets("Dashboard").Range("j10000").End(xlUp).Row + 1) = ppm
Sheets("Dashboard").Range("f" & Sheets("Dashboard").Range("f10000").End(xlUp).Row + 1) = prodTime
sayim = 0
End If

'set counters to zero
beginingScrap = 0
ppm_scrap = 0
ppm = 0
production = 0
prodTime = 0
planned = 0
unplanned = 0

'Start counting for the new shift
If .Cells(i, 6).Value = "D01" Or .Cells(i, 6).Value = "D02" Or .Cells(i, 6).Value = "D03" Or .Cells(i, 6).Value = "D04" Or .Cells(i, 6).Value = "D05" Or .Cells(i, 6).Value = "D06" Or .Cells(i, 6).Value = "D07" Or .Cells(i, 6).Value = "D08" Or .Cells(i, 6).Value = "D09" Or .Cells(i, 6).Value = "D12" Or .Cells(i, 6).Value = "D13" Or .Cells(i, 6).Value = "D14" Or .Cells(i, 6).Value = "D15" Or .Cells(i, 6).Value = "D16" Or .Cells(i, 6).Value = "D17" Or .Cells(i, 6).Value = "D18" Or .Cells(i, 6).Value = "D19" Or .Cells(i, 6).Value = "D20" Or .Cells(i, 6).Value = "D21" Or .Cells(i, 6).Value = "D22" Or .Cells(i, 6).Value = "D23" Or .Cells(i, 6).Value = "D30" Then
unplanned = unplanned + (.Cells(i, 8) - .Cells(i, 7)) * 1440
ElseIf .Cells(i, 6).Value = "D00" Or .Cells(i, 6).Value = "D10" Or .Cells(i, 6).Value = "D11" Then
planned = planned + (.Cells(i, 8) - .Cells(i, 7)) * 1440
ElseIf .Cells(i, 6).Value = 0 Then
prodTime = prodTime + (.Cells(i, 8) - .Cells(i, 7)) * 24
End If

If .Cells(i, 11).Value = "F0A" Or .Cells(i, 11).Value = "F0B" Then
beginingScrap = beginingScrap + .Cells(i, 10)
Else
ppm_scrap = ppm_scrap + .Cells(i, 10)
End If

production = production + .Cells(i, 9)

End If

Else 'If order no has changed

'Write last order no's last shift's results
Sheets("Dashboard").Range("A" & Sheets("Dashboard").Range("A10000").End(xlUp).Row + 1) = shift
Sheets("Dashboard").Range("B" & Sheets("Dashboard").Range("b10000").End(xlUp).Row + 1) = machine
Sheets("Dashboard").Range("c" & Sheets("Dashboard").Range("c10000").End(xlUp).Row + 1) = OrderNo
Sheets("Dashboard").Range("d" & Sheets("Dashboard").Range("d10000").End(xlUp).Row + 1) = unplanned
Sheets("Dashboard").Range("e" & Sheets("Dashboard").Range("e10000").End(xlUp).Row + 1) = planned
Sheets("Dashboard").Range("g" & Sheets("Dashboard").Range("g10000").End(xlUp).Row + 1) = production
Sheets("Dashboard").Range("h" & Sheets("Dashboard").Range("h10000").End(xlUp).Row + 1) = beginingScrap
Sheets("Dashboard").Range("I" & Sheets("Dashboard").Range("I10000").End(xlUp).Row + 1) = ppm_scrap
ppm = (ppm_scrap / production) * 1000000
Sheets("Dashboard").Range("j" & Sheets("Dashboard").Range("j10000").End(xlUp).Row + 1) = ppm
Sheets("Dashboard").Range("f" & Sheets("Dashboard").Range("f10000").End(xlUp).Row + 1) = prodTime
beginingScrap = 0
ppm_scrap = 0
ppm = 0
prodTime = 0
production = 0
planned = 0
unplanned = 0

OrderNo = .Cells(i, 4).Value 'change order no to new one

'count for the new order no and shift

If .Cells(i, 6).Value = "D01" Or .Cells(i, 6).Value = "D02" Or .Cells(i, 6).Value = "D03" Or .Cells(i, 6).Value = "D04" Or .Cells(i, 6).Value = "D05" Or .Cells(i, 6).Value = "D06" Or .Cells(i, 6).Value = "D07" Or .Cells(i, 6).Value = "D08" Or .Cells(i, 6).Value = "D09" Or .Cells(i, 6).Value = "D12" Or .Cells(i, 6).Value = "D13" Or .Cells(i, 6).Value = "D14" Or .Cells(i, 6).Value = "D15" Or .Cells(i, 6).Value = "D16" Or .Cells(i, 6).Value = "D17" Or .Cells(i, 6).Value = "D18" Or .Cells(i, 6).Value = "D19" Or .Cells(i, 6).Value = "D20" Or .Cells(i, 6).Value = "D21" Or .Cells(i, 6).Value = "D22" Or .Cells(i, 6).Value = "D23" Or .Cells(i, 6).Value = "D30" Then
unplanned = unplanned + (.Cells(i, 8) - .Cells(i, 7)) * 1440
ElseIf .Cells(i, 6).Value = "D00" Or .Cells(i, 6).Value = "D10" Or .Cells(i, 6).Value = "D11" Then
planned = planned + (.Cells(i, 8) - .Cells(i, 7)) * 1440
ElseIf .Cells(i, 6).Value = 0 Then
prodTime = prodTime + (.Cells(i, 8) - .Cells(i, 7)) * 24
End If

If .Cells(i, 11).Value = "F0A" Or .Cells(i, 11).Value = "F0B" Then
beginingScrap = beginingScrap + .Cells(i, 10)
Else
ppm_scrap = ppm_scrap + .Cells(i, 10)
End If

production = production + .Cells(i, 9)

End If

Next

End With

Ending:

Next

End Sub

I write the comments for almost all codes for you to understand. I am not a software engineer you will notice quickly.
I just want to learn if there is way to do the same task with easier, more readable and flexible codes. What I mean with flexible is, easily change the code when number of shifts has changed or etc.

You can find the Excel here
https://www.dropbox.com/s/uaizjqph7ltfx58/Analiz%20v1.3.xlsm?dl=0

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:

1. 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.
2. 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.)

@M.Doerner makes a lot of excellent points. My points are rather pedantic

When you end up with lots of nested if/elseif, you might want to use select case statements instead. Combining these with using variables for your current if statements will make everything more readable.

Stylistically, you can clean things up by using ":" for new lines of code and dimentionalising things all on the same line. e.g.

Dim shift As Integer
Dim OrderNo As String
Dim unplanned As Long : unplanned = 0
Dim planned As Long: planned = 0
Dim prodTime As Double :prodTime = 0
Dim production As Long :production = 0
Dim ppm_scrap As Long :ppm_scrap = 0
Dim beginingScrap As Long: beginingScrap = 0
Dim ppm As Long: ppm = 0

Which shows you that you initalised the ppm to 0 twice, or this:

Dim shift As Integer
Dim OrderNo As String
Dim unplanned As Long, planned As Long ,production As Long, ppm_scrap As Long, beginingScrap As Long, ppm As Long
unplanned = 0: planned = 0 :production = 0 :ppm_scrap = 0: beginingScrap = 0 : ppm = 0
Dim prodTime As Double :prodTime = 0
• Initializing a numeric value to 0 is redundant. Numeric types already initialize to 0. And I'd argue that cramming initialization together with declaration on the same line with an instruction separator hurts readability. A better idea IMO is to declare variables closer to their usage, instead of in a wall of declarations at the top of the procedure. Dec 19, 2016 at 17:00