2
\$\begingroup\$

StigJan 1

Stigjan 2

What I have tried to accomplish with the code is to make some sort of partner budget where 2 people can have 2 separate budgets with separate expenses but which are still intertwined with eachother in some instances.

The code refers to the range A36:H160 and K36:R76. The first range being Fixed expenses and the 2. range being Other Expenses. Each range have 4 columns with the Amount Spend, date, category, if it is a Common expense (Fælles) or Paid By (Lagt Ud) and description.

So if one person pays for something that is consumed by both(Food), Fælles will be chosen in the 3. column. And if one pays for the whole amount for the other person, Lagt Ud(Paid By) will be chosen.

If Fælles, there will be a -SUMIFS/2 in the expense. And if Lagt Ud it will just minus it from the budget but add it to the other persons budget. The other half of the expense is then added to a box with a sum of all Fælles and Lagt Ud that month to conclude who owes who and what amount. This number is changing the whole time according to who pays most.

The Fælles and STIGS code are just 2 sides of the same code and makes sure that when STIGS is added in the other persons budget it will subtract half of the amount of the expense.

The bolding part of the code is made in order to not add the expenses more than once.

The STIGS and STIG code also have the function to give a good overview through conditional formatting who has made the expenses. So STIGS would be green in my sheet but red in hers.

This is outlines why the code should copy the data in the columns, make the 3. column bold, add STIG or STIGS to the description and remove the 3. column in the other sheet (so that it doesn't look like the other person did the expense)

Hope all this makes sense.

Also i used to have a interior color part in the code, but have since removed this, as this works better with conditional formatting.

Private Sub CommandButton1_Click()

Dim wsSource, wsTarget As Worksheet
Dim i, iLastSource, iRowTarget, count As Long
Dim cell As Range

Set wsSource = Worksheets("Stig Jan")
iLastSource = wsSource.Cells(Rows.count, 1).End(xlUp).Row

Set wsTarget = Worksheets("Laura Jan")

count = 0
With wsSource
iRowTarget = wsTarget.Cells(Rows.count, 1).End(xlUp).Row + 1
For i = 36 To iLastSource
  Set cell = .Cells(i, 4)
  If cell.Font.Bold = False Then
    If cell.Value = "Fælles" Then
      cell.Offset(, 1).Value = "STIGS " & cell.Offset(, 1).Value
      wsTarget.Range("A" & iRowTarget & ":H" & iRowTarget).Value = .Range("A" & i & ":H" & i).Value
      wsTarget.Range("D" & iRowTarget).ClearContents
      iRowTarget = iRowTarget + 1
    count = count + 1
     End If
  End If
  If cell.Font.Bold = False Then
    If cell.Value = "Lagt Ud" Then
      cell.Offset(, 1).Value = "STIG " & cell.Offset(, 1).Value
      wsTarget.Range("A" & iRowTarget & ":H" & iRowTarget).Value = .Range("A" & i & ":H" & i).Value
      wsTarget.Range("D" & iRowTarget).ClearContents
      iRowTarget = iRowTarget + 1
      count = count + 1
    End If
  End If
    If cell.Value = "Fælles" Or cell.Value = "Lagt Ud" Then
    wsSource.Rows(i).Columns("D").Font.Bold = True
    End If
    Next

iRowTarget = wsTarget.Range("K76").End(xlUp).Row + 1
For i = 36 To iLastSource
  Set cell = .Cells(i, 14)
  If cell.Font.Bold = False Then
    If cell.Value = "Fælles" Then
      cell.Offset(, 1).Value = "STIGS " & cell.Offset(, 1).Value
      wsTarget.Range("K" & iRowTarget & ":R" & iRowTarget).Value = .Range("K" & i & ":R" & i).Value
      wsTarget.Range("N" & iRowTarget).ClearContents
      iRowTarget = iRowTarget + 1
      count = count + 1
    End If
  End If
    If cell.Font.Bold = False Then
     If cell.Value = "Lagt Ud" Then
      cell.Offset(, 1).Value = "STIG " & cell.Offset(, 1).Value
      wsTarget.Range("K" & iRowTarget & ":R" & iRowTarget).Value = .Range("K" & i & ":R" & i).Value
      wsTarget.Range("N" & iRowTarget).ClearContents
      iRowTarget = iRowTarget + 1
      count = count + 1
    End If
  End If
 If cell.Value = "Fælles" Or cell.Value = "Lagt Ud" Then
    wsSource.Rows(i).Columns("N").Font.Bold = True
End If
Next
End With
MsgBox "Done : " & count & " rows copied"

End Sub
\$\endgroup\$
9
  • 1
    \$\begingroup\$ Why do you want to shorten it? If I take a look at it, I'd cut it into multiple subs instead. \$\endgroup\$
    – Mast
    Commented Jan 30, 2020 at 20:37
  • \$\begingroup\$ Hi Mast, thank you for taking time to review it. The code has been developed with lots of help from a couple of programmers. And then i have cut/copied/pasted it in to place using the trial and error method. :) And i dont think i think like a programmer yet, so was just thinking that maybe there are some unnecessary lines in there? \$\endgroup\$
    – Strune
    Commented Jan 30, 2020 at 20:59
  • 1
    \$\begingroup\$ I actually just deleted the last part. With the interior color because i think it will be better with conditional formatting. :) \$\endgroup\$
    – Strune
    Commented Jan 30, 2020 at 21:00
  • \$\begingroup\$ I have read abit about subs and cut it up with the best of my abilities. But most likely i am doing it wrong since the code stops at the beginning. i have addded a picture. \$\endgroup\$
    – Strune
    Commented Jan 30, 2020 at 21:55
  • 1
    \$\begingroup\$ That looks like a breakpoint. Remove all breakpoints before execution. Does the code work or not? \$\endgroup\$
    – Mast
    Commented Jan 31, 2020 at 19:49

1 Answer 1

3
\$\begingroup\$

Turn on Option Explicit. From the menu at the top Tools>Option>Editor tab>Code Settings group>Require Variable Declaration check box. Make sure that is checked. This mandates that you use Dim foo as Bar before you can use any variables and will save you needless headaches. Why? myRange is not declared anywhere and a simple typo can mean a half hour or more debugging to find that typo. This will add Option Explicit automatically to any new code modules. It's on you however to add it any existing code modules.


Multiple variables declared on one line. Dim i, iLastSource, iRowTarget, count As Long only count is declared as a Long type. The rest are Variant. Likewise for Dim wsSource, wsTarget As Worksheet only wsTarget has the type of Worksheet, wsSource if of type Variant. Fix this by declaring each variable on its own line. Also declare them just before you use them. This makes refactoring easier and avoids unused variables.


Your Stig Jan worksheet isn't created in the click event which means it's available at design time. Reference that worksheet by using the Worksheet.CodeName property. In the VBIDE under View>Properties Window (Hotkey: F4). Rename the CodeName, shown in properties window as (Name) property, to a descriptive name. I've rename it to StigJan.

enter image description here

Then you can reference that worksheet directly. This way if the name of the worksheet changes your code won't break.

enter image description here


Hungarian notation isn't needed. Because you have your variables declared with a type As Worksheet you don't need the ws prefix. Place your cursor on a variable name and from the menu at the top Edit>Quick Info (Hotkey: Ctrl+I) you can display the variables type.

enter image description here


Your For ... Next statement logic can be simplified. You're looping with the counter i but within that loop setting a cell variable. This is a candidate for a For Each ... Next statement.

For i = 36 To iLastSource
    Dim cell As Worksheet
    Set cell = .Cells(i, 4)

Becomes the code below. This clarifies the intent that you're looping through each cell in the area.

Dim checkArea As Range
Set checkArea = source.Range(source.Cells(36, 4), source.Cells(iLastSource, 4))
Dim checkCell As Range
For Each checkCell In checkArea

Once you've done that the next step is to consolidate the if checks. cell.Font.Bold = False can be rewritten as Not cell.Font.Bold. Combine that into a single check with the value check of the cell below it.

If cell.Font.Bold = False Then
    If cell.Value = "Fælles" Then

Becomes

If Not checkCell.Font.Bold And checkCell.Value = "Fælles" Then

The body within the If statement can then be reviewed. The parts that change are "STIGS ", "A", "H", "D".

cell.Offset(, 1).Value = "STIGS " & cell.Offset(, 1).Value
wsTarget.Range("A" & iRowTarget & ":H" & iRowTarget).Value = .Range("A" & i & ":H" & i).Value
wsTarget.Range("D" & iRowTarget).ClearContents
iRowTarget = iRowTarget + 1
count = count + 1

We extract that into its own dedicated Sub and supply the arguments that let it requires. The parameters targetRow, and copiedRowCount have the ByRef modifier because we want any changes to be reflected in the calling member after this Sub finishes.

Private Sub UpdateOffsetCellAndClearContents(ByVal checkCell As Range, _
                                                ByVal sourceWorksheet As Worksheet, _
                                                ByVal targetWorksheet As Worksheet, _
                                                ByRef targetRow As Long, _
                                                ByVal leftTargetColumn As Variant, _
                                                ByVal rightTargetColumn As Variant, _
                                                ByVal columnOfCellToClear As Variant, _
                                                ByVal offsetPrefixValue As String, _
                                                ByRef copiedRowCount As Long)
    checkCell.Offset(ColumnOffset:=1).Value = offsetPrefixValue & " " & checkCell.Offset(ColumnOffset:=1).Value

    Dim destinationArea As Range
    Set destinationArea = targetWorksheet.Range(targetWorksheet.Cells(targetRow, leftTargetColumn), targetWorksheet.Cells(targetRow, rightTargetColumn))
    Dim sourceArea As Range
    Set sourceArea = sourceWorksheet.Range(sourceWorksheet.Cells(checkCell.Row, leftTargetColumn), sourceWorksheet.Cells(checkCell.Row, rightTargetColumn))
    destinationArea.Value2 = sourceArea.Value2

    targetWorksheet.Cells(targetRow, columnOfCellToClear).ClearContents

    targetRow = targetRow + 1
    copiedRowCount = copiedRowCount + 1
End Sub

The call sites where this is used.

UpdateOffsetCellAndClearContents checkCell, source, target, targetRow, "A", "H", "D", "STIGS ", count
... 
UpdateOffsetCellAndClearContents checkCell, source, target, targetRow, "A", "H", "D", "STIG ", count
...
UpdateOffsetCellAndClearContents checkCell, source, target, targetRow, "K", "R", "N", "STIGS ", count
...
UpdateOffsetCellAndClearContents checkCell, source, target, targetRow, "K", "R", "N", "STIG ", count

Now if/when you need to make an update to the logic you change it within the Sub and all sites where it's called are now updated.


Instead of If cell.Value = "Fælles" Or cell.Value = "Lagt Ud" Then to bold the font or If myCell Like "*STIG*" Then to color a cells interior use conditional formatting. That way you set it for the entire range and it will automatically be applied whenever the cell changes. For their respective parts I came up with the below for bolding and

Private Sub AddBoldConditionalFormattingTo(ByVal formatArea As Range, ParamArray values())
    If formatArea.FormatConditions.count > 0 Then
        formatArea.FormatConditions.Delete
    End If

    Dim topLeftAddress As String
    topLeftAddress = formatArea.Cells(1, 1).Address(False, False)
    Dim orArguments As String
    orArguments = topLeftAddress & "=""" & Join(values, """," & topLeftAddress & "=""") & """"
    Dim formulaForTopLeftCell As String
    formulaForTopLeftCell = "=OR(" & orArguments & ")"

    Dim addedCondition As FormatCondition
    Set addedCondition = formatArea.FormatConditions.Add(XlFormatConditionType.xlExpression, Formula1:=formulaForTopLeftCell)
    addedCondition.Font.Bold = True
End Sub

Private Sub AddInteriorColorConditionalFormattingTo(ByVal formatArea As Range, ByVal interiorColor As Long, ByVal valueToSearchFor As String)
    If formatArea.FormatConditions.count > 0 Then
        formatArea.FormatConditions.Delete
    End If

    Dim formulaForTopLeftCell As String
    formulaForTopLeftCell = "=NOT(ISERROR(SEARCH(""" & "STIG" & """," & formatArea.Cells(1, 1).Address(False, False) & ")))"

    Dim addedCondition As FormatCondition
    Set addedCondition = formatArea.FormatConditions.Add(XlFormatConditionType.xlExpression, Formula1:=formulaForTopLeftCell, TextOperator:=XlContainsOperator.xlContains)
    addedCondition.Interior.Color = interiorColor
End Sub

Their respective call sites as below

AddBoldConditionalFormattingTo checkArea, "Fælles", "Lagt Ud"

and

AddInteriorColorConditionalFormattingTo target.Range("A36:S1000"), "STIG", RGB(255, 220, 220)

Static cell ranges. Range("K76") will break whenever a row above or column to the left is inserted/deleted, as will Range("A36:S1000"). Make these named ranges and reference them through the named ranges because named ranges don't break with insertions/deletions. I have no clue what these cells represent and can't begin to offer a suggestion.


Magic numbers. 36 has what significance? It's in the code for some reason. Why is it there? Use a name to describe why its there and/or its siginificance. If this number will never ever change convert it to a Const statement with a descriptive name like Const StartRow As Long = 36. If it may vary at run-time determine its value and assign it

dim startRow As Long
startRow = source.Cells(1,4).End(xlDown).Row + 1

The refactored code below reflect these changes

Option Explicit

Private Sub CommandButton1_Click()
    Dim source As Worksheet
    Set source = StigJan
    Dim lastSourceRow As Long
    lastSourceRow = source.Cells(Rows.count, 1).End(xlUp).Row

    Dim target As Worksheet
    Set target = LauraJan

    Dim targetRow As Long
    targetRow = target.Cells(Rows.count, 1).End(xlUp).Row + 1

    Const StartRow As Long = 36

    Dim count As Long
    Dim checkArea As Range
    Set checkArea = source.Range(source.Cells(StartRow, 4), source.Cells(lastSourceRow, 4))
    Dim checkCell As Range
    For Each checkCell In checkArea
        If Not checkCell.Font.Bold And checkCell.Value = "Fælles" Then
            UpdateOffsetCellAndClearContents checkCell, source, target, targetRow, "A", "H", "D", "STIGS ", count
        End If

        If Not checkCell.Font.Bold And checkCell.Value = "Lagt Ud" Then
            UpdateOffsetCellAndClearContents checkCell, source, target, targetRow, "A", "H", "D", "STIG ", count
        End If
    Next
    AddBoldConditionalFormattingTo checkArea, "Fælles", "Lagt Ud"

    targetRow = target.Range("K76").End(xlUp).Row + 1
    Set checkArea = source.Range(source.Cells(StartRow, 14), source.Cells(lastSourceRow, 14))
    For Each checkCell In checkArea
        If Not checkCell.Font.Bold And checkCell.Value = "Fælles" Then
            UpdateOffsetCellAndClearContents checkCell, source, target, targetRow, "K", "R", "N", "STIGS ", count
        End If

        If Not checkCell.Font.Bold And checkCell.Value = "Lagt Ud" Then
            UpdateOffsetCellAndClearContents checkCell, source, target, targetRow, "K", "R", "N", "STIG ", count
        End If
    Next
    AddBoldConditionalFormattingTo checkArea, "Fælles", "Lagt Ud"

    AddInteriorColorConditionalFormattingTo target.Range("AdequatelyNamedArea"), "STIG", RGB(255, 220, 220)

    MsgBox "Done : " & count & " rows copied"
End Sub

Private Sub UpdateOffsetCellAndClearContents(ByVal checkCell As Range, _
                                                ByVal sourceWorksheet As Worksheet, _
                                                ByVal targetWorksheet As Worksheet, _
                                                ByRef targetRow As Long, _
                                                ByVal leftTargetColumn As Variant, _
                                                ByVal rightTargetColumn As Variant, _
                                                ByVal columnOfCellToClear As Variant, _
                                                ByVal offsetPrefixValue As String, _
                                                ByRef copiedRowCount As Long)
    checkCell.Offset(ColumnOffset:=1).Value = offsetPrefixValue & " " & checkCell.Offset(ColumnOffset:=1).Value

    Dim destinationArea As Range
    Set destinationArea = targetWorksheet.Range(targetWorksheet.Cells(targetRow, leftTargetColumn), targetWorksheet.Cells(targetRow, rightTargetColumn))
    Dim sourceArea As Range
    Set sourceArea = sourceWorksheet.Range(sourceWorksheet.Cells(checkCell.Row, leftTargetColumn), sourceWorksheet.Cells(checkCell.Row, rightTargetColumn))
    destinationArea.Value2 = sourceArea.Value2

    targetWorksheet.Cells(targetRow, columnOfCellToClear).ClearContents

    targetRow = targetRow + 1
    copiedRowCount = copiedRowCount + 1
End Sub

Private Sub AddBoldConditionalFormattingTo(ByVal formatArea As Range, ParamArray values())
    If formatArea.FormatConditions.count > 0 Then
        formatArea.FormatConditions.Delete
    End If

    Dim topLeftAddress As String
    topLeftAddress = formatArea.Cells(1, 1).Address(False, False)
    Dim orArguments As String
    orArguments = topLeftAddress & "=""" & Join(values, """," & topLeftAddress & "=""") & """"
    Dim formulaForTopLeftCell As String
    formulaForTopLeftCell = "=OR(" & orArguments & ")"

    Dim addedCondition As FormatCondition
    Set addedCondition = formatArea.FormatConditions.Add(XlFormatConditionType.xlExpression, Formula1:=formulaForTopLeftCell)
    addedCondition.Font.Bold = True
End Sub

Private Sub AddInteriorColorConditionalFormattingTo(ByVal formatArea As Range, ByVal interiorColor As Long, ByVal valueToSearchFor As String)
    If formatArea.FormatConditions.count > 0 Then
        formatArea.FormatConditions.Delete
    End If

    Dim formulaForTopLeftCell As String
    formulaForTopLeftCell = "=NOT(ISERROR(SEARCH(""" & valueToSearchFor & """," & formatArea.Cells(1, 1).Address(False, False) & ")))"

    Dim addedCondition As FormatCondition
    Set addedCondition = formatArea.FormatConditions.Add(XlFormatConditionType.xlExpression, Formula1:=formulaForTopLeftCell, TextOperator:=XlContainsOperator.xlContains)
    addedCondition.Interior.Color = interiorColor
End Sub

\$\endgroup\$
5
  • \$\begingroup\$ I am just completely blown away and overwhelmed by the amount of help I have received in this community. This must have taken ages to come up with! I am also very surprised how you are able interpret what i want to do with the information i have put out. I got the impression that you have to be very concise and precise about the the questions on Stack Overflow, so this is why I haven't written too much info about what I wanna do. I can't wait to check this out and I will try to write what i am trying to do and why when i get back home! Thank you from the bottom of my heart! \$\endgroup\$
    – Strune
    Commented Feb 4, 2020 at 13:02
  • \$\begingroup\$ Hi Ivenbach, I have read your answer a couple of times now and to be honest it is not everything i understand yet. Will definitely have to revisit it a few times more. I have tried to do the instructions now and the code seems to be running fine except for this part:Set addedCondition = formatArea.FormatConditions.Add(XlFormatConditionType.xlExpression, Formula1:=formulaForTopLeftCell) \$\endgroup\$
    – Strune
    Commented Feb 4, 2020 at 17:32
  • \$\begingroup\$ Also I have rewritten the whole question in order for it to make more sense of what i have tried to do to you and others that might see it. \$\endgroup\$
    – Strune
    Commented Feb 4, 2020 at 17:36
  • \$\begingroup\$ Just wanna say that i am very grateful for your elaborate answer. This is very advanced for me at this point. But i will revisit it many times and try to grasp every step of it! \$\endgroup\$
    – Strune
    Commented Feb 5, 2020 at 9:22
  • \$\begingroup\$ The Set addedCondition = ... is adding a format condition. Have a look at the documentation for FormatConditions.Add. It uses xlExpression which is a member of XlFormatConditionType enumeration as the first argument. The argument for Formula1 is built up and saved to a variable. Work on a simplified format that you want to add first then add complexity until it matches your original. That should get you to resolving that issue. \$\endgroup\$
    – IvenBach
    Commented Feb 26, 2020 at 22:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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