Summary Data According to Machine No, Order No and Shifts

Question

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
        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

M.Doerner · Accepted Answer · 2016-12-20 23:24:43Z

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

REdim.Learning · Accepted Answer · 2016-12-19 14:09:57Z

@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. — Mathieu Guindon♦, Dec 19 '16 at 17:00

Stack Exchange Network

current community

your communities

more stack exchange communities

Summary Data According to Machine No, Order No and Shifts

2 Answers 2

Your Answer

Not the answer you're looking for? Browse other questions tagged vba excel or ask your own question.

Hot Network Questions

Summary Data According to Machine No, Order No and Shifts

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged vba excel or ask your own question.

Related

Hot Network Questions