I've tried to follow this, it looks wonderful but I'm having a hard time getting it to run. You are correct in your assessment, I've moved past recording coding to writing script but obviously have a long ways to go to writing the way you do. There were tons of wonderful things you added here and I'll be referring myself to it frequently until I understand it all. In the mean time could you be so kind as to put an example of all that you suggested that could be copied and pasted to run? When I attempted to do that based on your comment I got "Compile error: Invalid inside procedure"

Ah, the Type must be defined in the module's declarations section, above everything else, near where you would have Option Explicit ;-)

This is how I have it starting 'Private Function IsNonEssentialRow(ByVal sheet As Worksheet, ByVal rowIndex As Long) As Boolean End Function Public Sub DeleteNonEssentialRows() Dim sheet As Worksheet Set sheet = ThisWorkbook.Worksheets("My New Worksheet") it errors on Public Sub row'

I've edited my answer to give a better idea, hope it helps

Sorry now it is saying, Complie Error: Sub or Function not defined. I found this link but I'm not sure what is wrong with it. It's spelled right. Is it because the Function "IsNonEssentialRow" is private?

The Private modifier makes the function only visible to code within the same module. If you implemented it in a separate module it needs to be Public.

hi!

This still has the same error

show me your code

I've tried declaring the type and functions as public

Option Explicit
Public Type TRowItem
    AmountPaid As String
    CompoundCode As String
    ClaimType As String
    ClaimStatus As String
    CategoryCode As String
    PreAuth As String
    GroupId As String
End Type

Public Sub DeleteNonEssentialRows()

    ToggleWaitMode

    Dim r As Long
    For r = Sheet2.UsedRange.Rows.Count To 1 Step -1
        If IsNonEssentialRow(Sheet2, r) Then
            Sheet2.Rows(r).EntireRow.Delete
        End If
    Next

    ToggleWaitMode False

End Sub

Public Function IsNonEssentialRow(ByVal sheet As Worksheet, ByVal rowIndex As Long) As Boolean

you have ToggleWaitMode and StringMatchesAny anywhere?

um I don't know to be honest

I have my original code in a different module

that's fine

if I declare all those as private would that fix it?

doesn't matter if it's calling functions that don't exist.

you need to have StringMatchesAny somewhere, and implement your ToggleWaitMode helper procedure

if VBA can't find them, the code won't compile

oh I see in the Function it's there twice

well 3 times

Public Sub ToggleWaitMode(Optional ByVal wait As Boolean = True)
    Application.ScreenUpdating = Not wait
    Application.Calculation = IIf(wait, xlCalculationManual, xlCalculationAutomatic)
End Sub

^^ could look like this

wait isn't a valid value

and I gave you StringMatchesAny in my answer

sorry, forgot the Optional keyword

sure it's valid :)

Public Sub ToggleWaitMode(ByVal wait As Boolean = True) is red

^oh I see

sorry, context-switching from C# :)

^^ and I wondered why I had a parser error lol

I think it's still hanging up on the StringMatchesAny part

well did you define the function anywhere?

I'm not sure of the syntaxes for defining fuctions

no I didn't

Public Function StringMatchesAny(ByVal source As String, ParamArray values()) As Boolean

    Dim find As String, i As Integer, found As Boolean

    For i = LBound(values) To UBound(values)
        find = CStr(values(i))
        found = (source = find)
        If found Then Exit For
    Next

    StringMatchesAny = found

End Function

I just tested it, seems to work.

^oh yeah each language does it's own thing, it's weird how close so many are though, SQL isn't too terribly hard to read once one has learned vba

do you know about Rubberduck?

type mismatch

the type of AmountPaid is.. oh, a string.

no I don't know about Rubberduck, I just barely joined this one

your original had something different but it wasn't an option

*something else

try result = result Or CCur(item.AmountPaid) <= 0

"CCur" converts a string to a currency value

you want to be comparing numbers vs. numbers

if the string value contains an illegal / non-numeric value, conversion might fail though

it failed, on the orignal data it is just a general number

basically you need to validate your inputs, especially if it's a user typing it in :)

it's a conversion from a huuuge parser. would it be better if it was currency?

depends. if the value is "xyxyyagfuwdho" it doesn't matter what the type is

try Val(item.AmountPaid) <= 0 then.

Rubberduck is my side project, you may have seen this ad in the side bar:

rubberduck-vba.com

ok i had to do that for a couple other things

but the end result is now that other sheet is blank

lol, it deleted everything?

what you can do, is extract the logic that creates the item, and make the function take that TItem value as a parameter instead - then make the function public, and write unit tests for it to validate the logic

well, with Rubberduck I mean

otherwise the only way to test VBA code is to run it :)

I would use that for other things but I work in the medical field, so I can't really share any of the data I'm running

HIPAA

:(

huh? it's not about sharing anything...

I'm not following..

^opensourcing?

lol, the project is open-source. it's an add-in for the VBA Editor.

requires admin privs to install though

but then with it you can write unit tests.. hold on...

    '@TestMethod
    Public Sub ZeroAmountPaid_ReturnsTrue()

        'arrange
        Dim item As TItem
        item.AmoundPaid = 0
        item.CompoundCode = "1"
        item.ClaimType = "P"
        item.ClaimStatus = "BLA"
        item.CategoryCode = "Z"
        item.PreAuth = 0
        item.GroupId = "ZZ"

        'act
        Dim result As Boolean
        result = IsNonEssentialRow(item)

        'assert
        Assert.IsTrue result
    End Sub

^^ something like this

you can make a test module that validates every possible execution path for the IsNonEssentialRow function, and Rubberduck evaluates the Assert.IsTrue call and reports your test results in a test explorer window

then when you make a change to the logic, you have a "safety net" that tells you when you broke something

the key is to decouple the logic from the spreadsheet-handling code

that makes sense

I have 742 tests right now in Rubberduck itself... and it's not nearly enough :)

it says type mismatch there

yeah, your function wants a Worksheet and a Long parameter

why would that code you post delete everything?

I don't know, maybe I messed up some of the conditions

anyway you'll want to extract another function, something like this:

Private Function GetRowItem(ByVal sheet As Worksheet, ByVal rowIndex As Long) As TItem
    Dim item As TRowItem
    item.AmountPaid = sheet.Cells(rowIndex, "C").Value
    item.CompoundCode = sheet.Cells(rowIndex, "AE").Value
    item.ClaimType = sheet.Cells(rowIndex, "L").Value
    item.ClaimStatus = sheet.Cells(rowIndex, "J").Value
    item.CategoryCode = sheet.Cells(rowIndex, "S").Value
    item.PreAuth = sheet.Cells(rowIndex, "N").Value
    item.GroupId = sheet.Cells(rowIndex, "AD").Value
    GetRowItem = item

not sure if that's legal... at that point I'd make TItem an actual class/object

but with that part extracted out of IsNonEssentialRow, now all IsNonEssentialRow needs is that TItem value

hi @Insane!

so IsNonEssentialRow can then look like this:

Public Function IsNonEssentialRow(ByVal item As TItem) As Boolean

and now it's fully decoupled from worksheets, and you can supply it any random made-up value and see what it returns and whether the output is as expected

Option Explicit
Private Type TRowItem
    AmountPaid As String
    CompoundCode As String
    ClaimType As String
    ClaimStatus As String
    CategoryCode As String
    PreAuth As String
    GroupId As String
End Type

Private Type TItem
    AmountPaid As String
    CompoundCode As String
    ClaimType As String
    ClaimStatus As String
    CategoryCode As String
    PreAuth As String
    GroupId As String
End Type

Public Sub DeleteNonEssentialRows()

    ToggleWaitMode

    Dim r As Long
    For r = Sheet2.UsedRange.Rows.Count To 1 Step -1

^That's what I have in the Module

It is catching on result = IsNonEssentialRow(item)

specifically the item part

of course

your signature says "IsNonEssentialRow(ByVal sheet As Worksheet, ByVal rowIndex As Long)"

it wants a worksheet and a long

and you're giving it a TItem

if I were the VBA compiler I'd be kicking and screaming too!

I don't follow?

but hey, nevermind the ZeroAmountPaid_ReturnsTrue() test method, the compiler doesn't know of any Assert object if you're not running Rubberduck

@MadChadders the logic has two parts: the first part uses sheet and rowIndex to collect information from the worksheet and populate the fields of a TItem value

the second part evaluates that TItem value

what I'm saying is, if you want to be able to test it, you needd to factor out the part that's coupling it with the worksheet, by extracting another function:

and then you can change the signature of IsNonEssentialRow to take a TItem value instead of a sheet and a rowIndex

leaves you with a function responsible for collecting the row values, and another responsible for evaluating these values

separation of concerns is a good thing

so instead of calling IsNonEssentialRow(sheet, r) you do IsNonEssentialRow(GetRowItem(sheet, r))

There is way too much data to care to evaluate all that I want left over. I just want the non essential rows deleted whatever the fastest way to do that

it would take forever to try and figure out all the items that could be in the rows I want to stay

...

that was why I had looped originally

it's not about the data, it's about the correctness of the conditional logic

the test methods should be in a separate module, and there should be as many tests as you have specs. in other words, if you specifications say "delete any row where 'amount paid' is zero" then you have a test method for that; specs say "delete any row where claim type is not P" then you have a test method for that, etc.

ok I think I understand, so you're saying isolate each one, until I figure out which one is ending up deleting everything?

if everything gets deleted, certainly at least one of the tests will fail

I can just put the ' in front of parts of the code and do the same thing

lol, sorry I'm bombarding you with what's probably quite foreign concepts here... object-oriented programming, test-driven development, ...

yeah little bit, I'm a script writer

hi mat, just lurkin

sure. and then 6 months down the line you need to change that logic (because specs change all the time right?) and you dive back into that code and you're 100% certain you didn't break anything with that little modification? if you have 20 green tests, you are.

I'm aware of some of those but I certainly don't understand them

well usually I'm the end user so some of the things are unnecessary but I understand the logic of why declaring everything is the better way to do it

I usually create scripts piece by piece until it is perfect

I was once in your shoes :)

yeah I want to be better, I really am trying to follow things but I know i have a crazy ton to learn still

^for this the specs shouldn't change but I trust you know more than me

I've read a lot of things that recommend the same things you are doing

stick around and keep posting your working code on CR, you'll learn a ton! :)

probably the better way would be just copy over what I actually need from another sheet?

What I have been doing before this code, is copy everything, then remove all the items I don't need

Ideally you wouldn't be coding any logic directly against worksheets - worksheet interactions are THE slowest thing VBA does

lucky you @MadChadders taking advice from the king of VBA reviews ;)

lol

I wish the top guns over from Stack Overflow would venture onto this side of Stack Exchange to steal my throne :)

I'll try to figure out which item is accidentally deleting out all the rows, and let you know