0

Basically i need to get a list of each different type of issue from the first sheet and then display on the second.

The first sheet has 1 row which is just titles for the columns and then 4 rows with data.

It was wokring but I think i accidently changed something i cant work out whats wrong now, if someone can see an issue or if theres a better way of doing this then im all ears.

Sub ListQueryTypes()

'Create Variables'
Dim numberOfIssues As Integer
Dim numberOfMacroIssues As Integer
Dim numberOfReportIssues As Integer
Dim numberOfTechnicalIssues As Integer
Dim numberOfTrendIssues As Integer
Dim cellValue As String

'Set query register as active worksheet'
Sheets("Query Register").Activate

'set range for search'
Set myRange = Range("G:G")

'Minus 1 for the first row being column name'
numberOfIssues = Application.WorksheetFunction.CountA(myRange)

'Do, for as many issues as were found previously'
For i = 2 To numberOfIssues + 1
    cellValue = ActiveSheet.Cells(i, 7).Value
        Select Case cellValue
            Case "Macro"
                numberOfMacroIssues = numberOfMacroIssues + 1
            Case "Report"
                numberOfReportIssues = numberOfReportIssues + 1
            Case "Technical"
                numberOfTechnicalIssues = numberOfTechnicalIssues + 1
            Case "Trend"
                numberOfTrendIssues = numberOfTrendIssues + 1

        End Select
Next i
Sheets("Inventory").Activate
ActiveCell = Cells(2, 2)
ActiveCell.Value = numberOfMacroIssues
ActiveCell.Offset(1).Value = numberOfReportIssues
ActiveCell.Offset(2).Value = numberOfTechnicalIssues
ActiveCell.Offset(3).Value = numberOfTrendIssues
Community
  • 1
  • 1
A.Cassin
  • 41
  • 1
  • 6
  • 3
    "*Stopped working*" is a useless error description. You must describe what is going wrong and where, and what you expect instead: "*Questions seeking debugging help ("why isn't this code working?") must include the desired behavior, a specific problem or error and the shortest code necessary to reproduce it in the question itself.*" – Pᴇʜ Feb 15 '18 at 10:34
  • Sorry!, im still new to the whole asking questions stuff, ill reword it – A.Cassin Feb 15 '18 at 10:44
  • 1
    **Welcome to Stack Overflow!** Check out the [tour] as well as "[ask]", and there are important tips about **providing examples** at "[mcve]". It's important to show that you made an **effort** to find a solution yourself **before** asking for help (on a _specific_ problem), so you can always [edit] your question to include details about what you've tried so far. More info: [help/on-topic]. Properly **tagging** your question is also helpful; more on that [here](https://stackoverflow.com/help/tagging). See also [these great tips](https://codeblog.jonskeet.uk/writing-the-perfect-question). – ashleedawg Feb 15 '18 at 10:49
  • 1
    Well done for posting your code. Now you need to explain exactly the nature of your problem. None of us is familiar with your file, data or what you are trying to achieve. – SJR Feb 15 '18 at 12:30
  • @SJR i thought i did in the title of the question?, i then went to explain that it does indeed have 4 rows of data so i dont understand why the code is return 0 when i feel like it should be returning 4 – A.Cassin Feb 15 '18 at 12:36
  • 1
    @A.Cassin Try to fully qualify the sheet for every range `Set myRange = Worksheets("MySheetName").Range("G:G")` otherwise Excel guesses which sheet you mean and that could easily fail. Also use `Long` instead of `Integer` Excel has more rows than `Integer` can handle. Also there is no advantage in using `Integer` at all. I recommend you to go and find a tutorial on how to avoid `.Select` and `.Activate` and follow it, this will prevent many issues and is a good practice to avoid them. – Pᴇʜ Feb 15 '18 at 12:39
  • 1
    Another error source: apparently you wanted to say "my active cell is B2 now", whereas `ActiveCell = Cells(2, 2)` assigns the current value of B2 to your active cell value. By the way: `Select` and `Activate` should be avoided in most cases (see https://stackoverflow.com/questions/10714251/how-to-avoid-using-select-in-excel-vba). – T.M. Feb 15 '18 at 12:50
  • @Pᴇʜ Tahnks! Did't realise it was such bad practice to use .Select and .Activate. This is my first Macro made using VBA so im still getting to grips with it :) – A.Cassin Feb 16 '18 at 09:08

1 Answers1

0

To collect all the issues mentioned in the comments, to improve your code. Have a look at the comments in the code

Option Explicit 'force to declare all variables correctly (this way you don't forget any)

Public Sub ListQueryTypes()

    'Create Variables'
    Dim numberOfIssues As Long 'was Integer but we can always use Long (only advantages) and Excel has more rows than Integer can handle
    Dim numberOfMacroIssues As Long 
    Dim numberOfReportIssues As Long 
    Dim numberOfTechnicalIssues As Long 
    Dim numberOfTrendIssues As Long 
    'Dim cellValue As String  'you don't need that variable see below

    'Set query register as active worksheet'
    'Sheets("Query Register").Activate 'instead of activate we define that sheet as variable which is more save
                                       'and we use Worksheets
    Dim WsQueryReg As Worksheet
    Set WsQueryReg = ThisWorkbook.Worksheets("Query Register")

    'set range for search'
    Dim myRange As Range 'we should to declare all variable properly
    Set myRange = WsQueryReg.Range("G:G") 'we need to define in which sheet the range is

    'Minus 1 for the first row being column name'
    numberOfIssues = Application.WorksheetFunction.CountA(myRange)

    'Do, for as many issues as were found previously'
    Dim i As Long 'declare every variable first
    For i = 2 To numberOfIssues + 1
        'cellValue = WsQueryReg.Cells(i, 7).Value 'no need to write this in a variable we can use it directly in Select Case
        Select Case WsQueryReg.Cells(i, 7).Value 'was cellValue before
            Case "Macro"
                numberOfMacroIssues = numberOfMacroIssues + 1
            Case "Report"
                numberOfReportIssues = numberOfReportIssues + 1
            Case "Technical"
                numberOfTechnicalIssues = numberOfTechnicalIssues + 1
            Case "Trend"
                numberOfTrendIssues = numberOfTrendIssues + 1
        End Select
    Next i

    'we can use with instead of activating a sheet and selecting a cell
    'this also specifies in which sheet and workbook the cell is
    With ThisWorkbook.Worksheets("Inventory").Cells(2, 2)
        .Value = numberOfMacroIssues
        .Offset(1).Value = numberOfReportIssues
        .Offset(2).Value = numberOfTechnicalIssues
        .Offset(3).Value = numberOfTrendIssues
    End With
End Sub

Instead of looping through your rows For i = 2 To numberOfIssues + 1 you could probably count Macro, Report, etc. like

With WsQueryReg
    numberOfMacroIssues = Application.WorksheetFunction.CounfIf(.Range(.Cells(2, 7), .Cells(numberOfIssues + 1, 7)), "Macro")
    numberOfReportIssues  = Application.WorksheetFunction.CounfIf(.Range(.Cells(2, 7), .Cells(numberOfIssues + 1, 7)), "Report")
    '… others here
End With
Pᴇʜ
  • 56,719
  • 10
  • 49
  • 73
  • Thank you! I'll get to work on making some of those changes. That was my first ever bit of VBA written so I'm still learning. However i dont understand that last bit, I guess i'll google the "CountfIf" function and see how it works but it seems like its a much nicer way than what i came out with. – A.Cassin Feb 16 '18 at 09:10
  • @A.Cassin countif counts all cells in a range that contain a specific value. It e.g. means count all cells in the given range that contain "Macro". It might be faster than a loop. – Pᴇʜ Feb 16 '18 at 09:15
  • Yeah I see that, but how comes there is ".Range" and ".Cells" rather than just "Range" or "Cells". Is that just how that function works or is there another reason? – A.Cassin Feb 16 '18 at 09:30
  • the range is defined as `.Range(firstCell, lastCell)` so it means from firstCell to lastCell. – Pᴇʜ Feb 16 '18 at 09:32
  • Ah okay, wasnt sure if you could use either .Range or just simple Range without the '.'. Thanks for clarifying that up. – A.Cassin Feb 16 '18 at 09:43
  • the difference between `.Range` and `Range` is that `.Range` triggers the `With WsQueryReg` statement it is the same as `WsQueryReg.Range` just shorter in combination with `With`. But `Range` without the dot doesn't use the `With` statement (and VBA has to guess which sheet you mean, which is bad. Never let VBA guess). – Pᴇʜ Feb 16 '18 at 10:36
  • Ahh okay, thanks! Also, im getting an Invalid Quaifier Compile Error on .CountIf – A.Cassin Feb 16 '18 at 10:38
  • remove the `.Value` in the end :( my bad – Pᴇʜ Feb 16 '18 at 10:39