5
\$\begingroup\$

I have some info. that looks like this:

Header  Debit   Credit

aaaa    20  0

bbbb    0   60

cccc    30  0

xxxx    10  0
xxxx    0   40
xxxx    40  0
xxxx    0   10

oooo    500 0
oooo    0   52
oooo    0   500

pppp    10  
pppp    0   10

(Note this starts in col. H)

The idea is that I want to go through this range, and where there are "blocks" of data (see the "xxxx" group), check for offsetting amounts.

I was able to put this together, to loop through the groups, but I'm not sure if it's the best way to do so. I'm afraid there will be a time it skips a line which is unintended - any ideas/tips on improving it?

Sub strike_out_Matching_Debit_and_Credits()
Dim debitCol&, credCol&, lastRow&, startRow&, endRow&, i&
Dim rng As Range, cel As Range
Dim myWS As Worksheet

Set myWS = Sheets("Raw - edited")
With myWS
    debitCol = .Rows(1).Find(what:="Debit").column
    credCol = .Rows(1).Find(what:="Credit").column
    lastRow = .Cells(.Rows.Count, credCol).End(xlUp).row

startRow = 2
    For i = 2 To lastRow
        endRow = .Cells(i, debitCol).End(xlDown).row
        Set rng = .Range(.Cells(startRow, debitCol), .Cells(endRow, debitCol))

        rng.Select

        If rng.Cells.Count - WorksheetFunction.CountA(rng) = 0 And rng.Cells.Count > 1 Then
            ' We now have a block to work through
            check_Values rng, rng.Offset(0, 1)
            check_for_Offset_Values rng, rng.Offset(0, 1)
        End If
        startRow = .Cells(endRow, debitCol).End(xlDown).row
    Next i

End With
End Sub

Thanks for any ideas/advice/tips!

Edit: I appreciate the help so far! I've made notes on the naming conventions, etc. Does anyone have any tips for tightening up the actual function of the code loop? That's what I'm mainly interested in.

\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to Code Review! I hope you enjoy the ride, good job with your first post! =) \$\endgroup\$ – Mathieu Guindon Jun 28 '16 at 15:32
3
\$\begingroup\$

Following on your (shared) need of tightening up your code:

Get rid of unused variables

As per your code you don't use neither creditCol nor cel, so get rid of them

Avoid useless variables

You don't need myWS and can simply go

With Worksheets("Raw - edited")

where you also want to use Worksheets instead of Sheets since the former is the collection of sheets with actual data in cells while the latter is the collection that takes "chart" sheets too.

You don't need any lastRow, startRow, endRow and i variable.

Just loop through a "well" selected range using SpecialCells property of Range object and one of your already declared variable of Range type for the task.

Use Areas property of Range object

this will let you loop through the actual "blocks" (as you're naming them) of the parent range, i.e. the groups of contiguos cells that range is made of

Choose proper row counter column

As already pointed out in previous answers, from your data example "Credit " column has blank cells that may prevent you from picking the actual last one, while "Header" column seems to be the best candidate (and of which you already know column index,being it "H")

So choose this latter or loop through all relevant columns and calculate the maximum lastRow between them (make a Function for this task)

Proper use of Find method of Range object

It always "remembers" and uses the last user's choice of some options, were it made from Excel UI or any VBA code.

So you'd better always specify those options and ensure they're those you actually need

Find(What:="Debit", LookIn:= xlValues, LookAt:= xlWhole, MatchCase:= False)

Final result

All what above toghether results in the following code

Sub strike_out_Matching_Debit_and_Credits()
Dim debitCol As Long
Dim block As Range 

With Worksheets("Raw - edited")
    DebitCol = .Rows(1).Find(What:="Debit", LookIn:= xlValues, LookAt:= xlWhole, MatchCase:= False)
    For Each block In .Columns("H").SpecialCells(xlCellTypeConstants, xlTextValues).Areas
        If block.Count > 1 Then
            check_Values block.Offset(, debitCol - 8), block.Offset(, debitCol - 7)
            check_for_Offset_Values block.Offset(, debitCol - 8), block.Offset(, debitCol - 7)
        End If
    Next block 
End Sub

This also assumes that:

  • "Debit" is there for sure in some cell of row 1

  • column "H" has at least one cell with text value

were these assumptions unsafe then you'd add some "entry" checks, possibly wrapped in a specific function returning a boolean to tell your main Sub whether to proceed or exit

\$\endgroup\$
  • \$\begingroup\$ Excellent answer! credCol is referenced in OP's code though. Great point about the assumptions, but I'd say given invalid input, the procedure should just raise an error and leave it to the caller to handle. \$\endgroup\$ – Mathieu Guindon Jun 30 '16 at 2:43
  • \$\begingroup\$ This is great! I'm marking this as the answer since I can only choose one, and I learned to use Block/Areas with "Special Cells" which is a huge help in this macro, and in some others I have. Thanks a lot! (@Mat's Mug - thanks for your help too, I was able to apply all the answers here to my code. I appreciate everyone's help! :D) \$\endgroup\$ – BruceWayne Jul 6 '16 at 14:12
  • \$\begingroup\$ You are welcome. Happy to be of any help \$\endgroup\$ – user3598756 Jul 6 '16 at 15:01
2
\$\begingroup\$

Let's first clear Rubberduck inspections:

  • Local variable cel is never assigned a reference, and never referred to: that declaration is dead code that can be safely removed.
  • Consider renaming variables rng and i - identifier names should indicate what they're used for (not just their type) and should be readable; avoid disemvoweling.
    • Local variable i is counting rows; currentRow would be more descriptive.
    • Local variable rng represents the current block of data; currentBlock would be more descriptive.
  • The assignment of myWS refers to the Sheets collection, which implicitly references Application.ActiveWorkbook - when you probably intended to be referring to ThisWorkbook (this may cause surprising bugs!). Also, the Sheets collection can contain charts and other non-Worksheet objects; consider using the Worksheets collection instead.

    • Consider giving the "Raw - Edited" sheet a meaningful code name. By default, Excel creates globally-scoped SheetX objects (e.g. Sheet1, Sheet2, etc.), but you can rename them in the properties pane, and use that identifier in code to refer directly to a "free" object, instead of indirectly making a copy of an existing object reference. myWS becomes redundant, and assuming you named it RawEditedDataSheet you could do this:

      With RawEditedDataSheet
          debitCol = .Rows(1).Find(what:="Debit").Column
          credCol = .Rows(1).Find(what:="Credit").Column
          lastRow = .Cells(.Rows.Count, credCol).End(xlUp).Row
      End With
      
  • Procedure strike_out_Matching_Debit_and_Credits is implicitly Public. Consider being explicit about access modifiers.

  • Consider declaring variables closer to their usage, in a single declaration per instruction. This makes the code easier to follow, since the maintainer doesn't constantly need to jump back and forth between where they're at and the top of the procedure to locate variables; declaring each variable in its own instruction makes each one easier to spot, too.
  • Consider using an explicit type (e.g. As Long) instead of type hints (e.g. &), which may not be understood by all maintainers and younger devs (after all this language feature predates the "V" of "VB").

The indentation isn't very consistent, and the underscores in the procedure's name are rather unusual - public members in VBA typically use a standard PascalCase.

This leaves us with this:

Public Sub StrikeOutMatchingDebitAndCredits()

    With RawEditedDataSheet

        Dim debitCol As Long
        debitCol = .Rows(1).Find(what:="Debit").Column

        Dim credCol As Long
        credCol = .Rows(1).Find(what:="Credit").Column

        Dim lastRow As Long
        lastRow = .Cells(.Rows.Count, credCol).End(xlUp).Row

        Dim startRow As Long
        startRow = 2

        Dim currentRow As Long
        For currentRow = 2 To lastRow

            Dim endRow As Long
            endRow = .Cells(currentRow, debitCol).End(xlDown).Row

            Dim currentBlock As Range
            Set currentBlock = .Range(.Cells(startRow, debitCol), .Cells(endRow, debitCol))

            currentBlock.Select

            If currentBlock.Cells.Count - WorksheetFunction.CountA(currentBlock) = 0 And currentBlock.Cells.Count > 1 Then
                ' We now have a block to work through
                CheckValues currentBlock, currentBlock.Offset(0, 1)
                CheckForOffsetValues currentBlock, currentBlock.Offset(0, 1)
            End If

            startRow = .Cells(endRow, debitCol).End(xlDown).Row

        Next currentRow

    End With

End Sub

Some observations:

  • It's not clear why you would need to .Select anything here. In any case, you would normally avoid .Select and .Activate, since it makes your code more frail than it needs to be... besides .Selecting a range for no reason at every iteration is only slowing down the execution of the loop.
  • There's a potential problem with using credCol to locate the lastRow of a given block:

    pppp    10  
    pppp    0   10
    

    It seems the credCol column doesn't reliably contain data for each row, which means this would be a possibility:

    pppp    0   10
    pppp    10  
    

    ...and I think it would wreck the row-counting logic, since the lastRow wouldn't be the "real" last row of the block. I'd suggest using the header column instead, if that one's reliable. Or any other column that reliably has data on every row that can have data in either of the two interesting columns.

  • rng.Offset(0, 1) assumes the debitCol is offset by exactly 1 column from the creditCol, however that assumption isn't made when assigning credCol, which means inserting a column between the two will break the code despite all the attention given to ensure the "Credit" header is picked up in whichever column it's under. Either make credCol = debitCol + 1, or compute the actual offset /difference between the two column numbers by passing rng.Offset(0, credCol - debitCol) instead.

  • Assigning startRow at the end of the loop seems fine, but then it's not clear how it affects i and, at a glance, it seems i is making too many iterations and that each "block" gets processed as many times as it has rows; IMO the problem isn't that you're skipping rows, rather that you're possibly processing each row more than once.

  • lastRow is the last row with data on the sheet. endRow is the last row with data in the current block. Their names should make the distinction more obvious.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your post! It's long, so I'll read through carefully. A quick comment is my .Select lines are there just for when I walk through, I can visually see the range, to make sure it's doing what I expect. I usually comment those lines out, I just forgot to here. \$\endgroup\$ – BruceWayne Jun 28 '16 at 15:35
2
\$\begingroup\$

Please feel free to post the code for check_Values and check_for_Offset_Values. If we had the logic for those, we could really transform your code.


Type Hints

They're outdated. Like really outdated. Also unclear and confusing. Use the normal form:

Dim debitCol As Long
Dim credCol As Long
Etc.

Sure, slightly longer, but much, much clearer, and clearer is much more important. Pixels are cheap, human understanding is not.


Naming

Your naming isn't terrible, but it could be a lot better. Names must be descriptive, then unambiguous, and only then concise. As before, pixels are cheap, understanding is not. Make your names as intuitive and easy to understand as possible:

rng --> blockRange  

ws  --> rawDataSheet  

lastRow  --> sheetEndRow

startRow --> blockStartRow

endRow   --> blockEndRow

etc.


Advanced Naming

In addition to giving your variables names, sometimes you should invent variables for the sole purpose of giving them names.

For instance, this check:

    If rng.Cells.Count - WorksheetFunction.CountA(rng) = 0 And rng.Cells.Count > 1 Then
        ' We now have a block to work through

should really be a Boolean variable called blockHasValues or similar. You could even write a Function called BlockHasValues() and extract your block-checking logic into there:

If BlockHasValues(blockRange) Then

    check_values blockRange, blockRange.Offset(0, 1)

    check_for_Offset_Values blockRange, blockRange.Offset(0, 1)

End If

Public Function BlockHasValues(ByRef blockRange As Range) As Boolean

    BlockHasValues = blockRange.Cells.Count - WorksheetFunction.CountA(blockRange) = 0 And blockRange.Cells.Count > 1

End Function

And now you don't even need comments because your code literally speaks for itself.


The closer your code reads to plain English, the easier it is to understand. The easier it is to understand, the easier it is to use, to change, to fix, to extend etc.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your comments! I agree with the "advanced naming" part of your post especially. However, I've always wondered (and would love your thoughts): when should I break out some logic into a separate function or not? I understand it's a lot better to have a sub that calls others within it, than having a sub be like 20000 lines long. However, it's determining what is "worth" putting into a sub, or not, that I struggle with. Also, I didn't know the hints were outdated. To be honest, I did that because it looks neater (read "cooler"), not that it saves time or anything :P \$\endgroup\$ – BruceWayne Jun 28 '16 at 17:25
  • 2
    \$\begingroup\$ It's about abstraction levels, and possibly what logic you want to be able to test all by itself. Code at the top of a module should be a higher level of abstraction than the nitty gritty details closer to the bottom. =) \$\endgroup\$ – Mathieu Guindon Jun 28 '16 at 17:58
  • 2
    \$\begingroup\$ @BruceWayne Any block of code that can be summed up in a single over-arching comment e.g. "print data to sheet" should be its' own sub or function. As a general rule, if you can take 2-3 lines of code and replace it with a one-line function/sub, you should. \$\endgroup\$ – Kaz Jun 28 '16 at 18:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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