# Requesting A Review



## afwelch (Jul 1, 2015)

Hello,

For the last several months I have been developing an application for my company to record injures at our various facilities.

Since this workbook will eventually be shared on network drive, I have included in this workbook is the login page. I have created this since multiple users will be using this workbook and each will have a different level of access, and that level of access also determines what fields are visible to the user on the main pivot table. To prevent unauthorized access to a user account, after the user enters their username, the program looks at the application user name and if they match, then the user may proceed to enter their password, the only person with full access of course is myself. I intend on incorporating a forgot password button as well as a contact admin. 

This report contains 27 pivottables and 11 charts. 

I am requesting a review of my application, and any comments that you have. 

To access the workbook, set your application user name to "awelch" as that will grant you full access. 

One feature I would like to add is when an injury is being added to the database, is to have a progress bar form pop up while the data is being added, so as to prevent the user from doing any unnecessary clicking, and thus possibly causing excel to crash. 

Thank you and I am looking forward to hearing your comments. 

Here is the link to the file on my google drive folder - Injury Reporting Application


----------



## sous2817 (Jul 1, 2015)

This is just my opinion...so take it with a grain of salt:

Wow, that's a lot of walls of code!  You should take the time to format your code as it will make maintaining a lot easier.  For instance, you have this sub:


```
Private Sub TabStrip2_Change()
Application.ScreenUpdating = False
If Me.TabStrip2.Value = 0 Then
Me.CommandButton1.Visible = True
Me.CommandButton2.Visible = True
Me.CommandButton3.Visible = True
Else
Me.CommandButton1.Visible = False
Me.CommandButton2.Visible = False
Me.CommandButton3.Visible = False
End If
If Me.TabStrip2.Value = 1 Then
Me.cbtnprintcharts.Visible = True
Me.CommandButton7.Visible = True
Me.CommandButton8.Visible = True
Else
Me.cbtnprintcharts.Visible = False
Me.CommandButton7.Visible = False
Me.CommandButton8.Visible = False
End If
If Me.TabStrip2.Value = 2 Then
Me.cbtnaddinjury.Visible = True
Me.cbtnplantifo.Visible = True
Me.CommandButton5.Visible = True
Else
Me.cbtnaddinjury.Visible = False
Me.cbtnplantifo.Visible = False
Me.CommandButton5.Visible = False
End If
If Me.TabStrip2.Value = 3 Then
Me.Label3.Visible = True
Me.Label4.Visible = True
Me.Label5.Visible = True
Me.ListBox1.Visible = True
Call populatelistbox
Me.Shapes("ReportFilters").Visible = msoTrue
Me.CommandButton4.Visible = True
Me.ListBox3.Visible = False
Me.ListBox2.Visible = False
Else
Me.Shapes("ReportFilters").Visible = msoFalse
Me.Label3.Visible = False
Me.Label4.Visible = False
Me.Label5.Visible = False
Me.ListBox1.Visible = False
Me.CommandButton4.Visible = False
End If
If Me.TabStrip2.Value = 4 Then
Me.ListBox2.Visible = True
Me.ListBox3.Visible = False
End If
If Me.TabStrip2.Value = 5 Then
Me.ListBox2.Visible = False
Me.ListBox3.Visible = True
Else
Me.ListBox3.Visible = False
End If
End Sub
```

which I'm sure works fine, but (in my opinion) formatting the code makes it a lot easier to follow:


```
Private Sub TabStrip2_Change()
    Application.ScreenUpdating = False
    If Me.TabStrip2.Value = 0 Then
        Me.CommandButton1.Visible = True
        Me.CommandButton2.Visible = True
        Me.CommandButton3.Visible = True
    Else
        Me.CommandButton1.Visible = False
        Me.CommandButton2.Visible = False
        Me.CommandButton3.Visible = False
    End If
    
    If Me.TabStrip2.Value = 1 Then
        Me.cbtnprintcharts.Visible = True
        Me.CommandButton7.Visible = True
        Me.CommandButton8.Visible = True
    Else
        Me.cbtnprintcharts.Visible = False
        Me.CommandButton7.Visible = False
        Me.CommandButton8.Visible = False
    End If
    
    If Me.TabStrip2.Value = 2 Then
        Me.cbtnaddinjury.Visible = True
        Me.cbtnplantifo.Visible = True
        Me.CommandButton5.Visible = True
    Else
        Me.cbtnaddinjury.Visible = False
        Me.cbtnplantifo.Visible = False
        Me.CommandButton5.Visible = False
    End If
    
    If Me.TabStrip2.Value = 3 Then
        Me.Label3.Visible = True
        Me.Label4.Visible = True
        Me.Label5.Visible = True
        Me.ListBox1.Visible = True
        Call populatelistbox
        Me.Shapes("ReportFilters").Visible = msoTrue
        Me.CommandButton4.Visible = True
        Me.ListBox3.Visible = False
        Me.ListBox2.Visible = False
    Else
        Me.Shapes("ReportFilters").Visible = msoFalse
        Me.Label3.Visible = False
        Me.Label4.Visible = False
        Me.Label5.Visible = False
        Me.ListBox1.Visible = False
        Me.CommandButton4.Visible = False
    End If
    
    If Me.TabStrip2.Value = 4 Then
        Me.ListBox2.Visible = True
        Me.ListBox3.Visible = False
    End If
    
    If Me.TabStrip2.Value = 5 Then
        Me.ListBox2.Visible = False
        Me.ListBox3.Visible = True
    Else
        Me.ListBox3.Visible = False
    End If
End Sub
```

Also, since your tabstrip can only have 1 value at a time, there's no reason to check if it's 0, then 1, then 2, then 3. etc.  Perhaps a Case statement would declutter it a bit.

Another point.  Take a look at what your code is doing and thing "hey...did I repeat myself a bunch of times?"  If the answer is yes, you can probably simply it.  For instance this:

```
Private Sub TabStrip1_Change()
Dim ws As Worksheet
Set ws = ThisWorkbook.Sheets("Charts")
If Me.TabStrip1.Value = 0 Then
ws.ChartObjects(1).Visible = True
ws.ChartObjects(2).Visible = False
ws.ChartObjects(3).Visible = False
ws.ChartObjects(4).Visible = False
ws.ChartObjects(5).Visible = False
ws.ChartObjects(6).Visible = False
ws.ChartObjects(7).Visible = False
ws.ChartObjects(8).Visible = False
ws.ChartObjects(9).Visible = False
ws.ChartObjects(10).Visible = False
ws.ChartObjects(12).Visible = False
End If
If Me.TabStrip1.Value = 1 Then
ws.ChartObjects(1).Visible = False
ws.ChartObjects(2).Visible = False
ws.ChartObjects(3).Visible = False
ws.ChartObjects(4).Visible = False
ws.ChartObjects(5).Visible = False
ws.ChartObjects(6).Visible = False
ws.ChartObjects(7).Visible = False
ws.ChartObjects(8).Visible = False
ws.ChartObjects(9).Visible = False
ws.ChartObjects(10).Visible = False
ws.ChartObjects(12).Visible = True
End If
If Me.TabStrip1.Value = 2 Then
ws.ChartObjects(1).Visible = False
ws.ChartObjects(2).Visible = True
ws.ChartObjects(3).Visible = False
ws.ChartObjects(4).Visible = False
ws.ChartObjects(5).Visible = False
ws.ChartObjects(6).Visible = False
ws.ChartObjects(7).Visible = False
ws.ChartObjects(8).Visible = False
ws.ChartObjects(9).Visible = False
ws.ChartObjects(10).Visible = False
ws.ChartObjects(12).Visible = False

End If
If Me.TabStrip1.Value = 3 Then
ws.ChartObjects(1).Visible = False
ws.ChartObjects(2).Visible = False
ws.ChartObjects(3).Visible = True
ws.ChartObjects(4).Visible = False
ws.ChartObjects(5).Visible = False
ws.ChartObjects(6).Visible = False
ws.ChartObjects(7).Visible = False
ws.ChartObjects(8).Visible = False
ws.ChartObjects(9).Visible = False
ws.ChartObjects(10).Visible = False
ws.ChartObjects(12).Visible = False
End If
If Me.TabStrip1.Value = 4 Then
ws.ChartObjects(1).Visible = False
ws.ChartObjects(2).Visible = False
ws.ChartObjects(3).Visible = False
ws.ChartObjects(4).Visible = True
ws.ChartObjects(5).Visible = False
ws.ChartObjects(6).Visible = False
ws.ChartObjects(7).Visible = False
ws.ChartObjects(8).Visible = False
ws.ChartObjects(9).Visible = False
ws.ChartObjects(10).Visible = False
ws.ChartObjects(12).Visible = False
End If
If Me.TabStrip1.Value = 5 Then
ws.ChartObjects(1).Visible = False
ws.ChartObjects(2).Visible = False
ws.ChartObjects(3).Visible = False
ws.ChartObjects(4).Visible = False
ws.ChartObjects(5).Visible = True
ws.ChartObjects(6).Visible = False
ws.ChartObjects(7).Visible = False
ws.ChartObjects(8).Visible = False
ws.ChartObjects(9).Visible = False
ws.ChartObjects(10).Visible = False
ws.ChartObjects(12).Visible = False
End If

If Me.TabStrip1.Value = 6 Then
ws.ChartObjects(1).Visible = False
ws.ChartObjects(2).Visible = False
ws.ChartObjects(3).Visible = False
ws.ChartObjects(4).Visible = False
ws.ChartObjects(5).Visible = False
ws.ChartObjects(6).Visible = True
ws.ChartObjects(7).Visible = False
ws.ChartObjects(8).Visible = False
ws.ChartObjects(9).Visible = False
ws.ChartObjects(10).Visible = False
ws.ChartObjects(12).Visible = False
End If
If Me.TabStrip1.Value = 7 Then
ws.ChartObjects(1).Visible = False
ws.ChartObjects(2).Visible = False
ws.ChartObjects(3).Visible = False
ws.ChartObjects(4).Visible = False
ws.ChartObjects(5).Visible = False
ws.ChartObjects(6).Visible = False
ws.ChartObjects(7).Visible = True
ws.ChartObjects(8).Visible = False
ws.ChartObjects(9).Visible = False
ws.ChartObjects(10).Visible = False
ws.ChartObjects(12).Visible = False
End If

If Me.TabStrip1.Value = 8 Then
ws.ChartObjects(1).Visible = False
ws.ChartObjects(2).Visible = False
ws.ChartObjects(3).Visible = False
ws.ChartObjects(4).Visible = False
ws.ChartObjects(5).Visible = False
ws.ChartObjects(6).Visible = False
ws.ChartObjects(7).Visible = False
ws.ChartObjects(8).Visible = True
ws.ChartObjects(9).Visible = False
ws.ChartObjects(10).Visible = False
ws.ChartObjects(12).Visible = False
End If

If Me.TabStrip1.Value = 9 Then
ws.ChartObjects(1).Visible = False
ws.ChartObjects(2).Visible = False
ws.ChartObjects(3).Visible = False
ws.ChartObjects(4).Visible = False
ws.ChartObjects(5).Visible = False
ws.ChartObjects(6).Visible = False
ws.ChartObjects(7).Visible = False
ws.ChartObjects(8).Visible = False
ws.ChartObjects(9).Visible = True
ws.ChartObjects(10).Visible = False
ws.ChartObjects(12).Visible = False
End If
If Me.TabStrip1.Value = 10 Then
ws.ChartObjects(1).Visible = False
ws.ChartObjects(2).Visible = False
ws.ChartObjects(3).Visible = False
ws.ChartObjects(4).Visible = False
ws.ChartObjects(5).Visible = False
ws.ChartObjects(6).Visible = False
ws.ChartObjects(7).Visible = False
ws.ChartObjects(8).Visible = False
ws.ChartObjects(9).Visible = False
ws.ChartObjects(10).Visible = True
ws.ChartObjects(12).Visible = False
End If
End Sub
```

What if, at the start of the sub, you hide all of the chart objects.  Then you just have to set 1 object to be visible depending on what the value is.  Your 150 or so lines of code got reduced down to about 20. This will also make maintaining your code a lot easier.  Imagine the edits you'd have to make if you wanted to hide and show ChartObjects(11)!

You can *greatly* simplify your code base...you should start there before adding new features.


----------



## afwelch (Jul 1, 2015)

Thank you, and this what I was looking for. 



> Wow, that's a lot of walls of code! You should take the time to format your code as it will make maintaining a lot easier. For instance, you have this sub:



THe reason it looked like that was that I used the "Code Cleaner" Addin with the eliminate white space checked. 




> What if, at the start of the sub, you hide all of the chart objects. Then you just have to set 1 object to be visible depending on what the value is. Your 150 or so lines of code got reduced down to about 20. This will also make maintaining your code a lot easier. Imagine the edits you'd have to make if you wanted to hide and show ChartObjects(11)!



Hmm so this would possibly be another select case statement then?


----------



## sous2817 (Jul 1, 2015)

1) White space isn't always an evil thing.  Again, just my preference.  

2) yes, you can use a select case.  You don't *have* to use a select case, but it will make things easier.  Keep in mind that a select case is only evaluated once, so it's perfect when you need to do something based on a specific value of something else.  If the value is 2, there's no reason to check if it's also 3, 4, or 5 because it can only be 2.  The lager lesson I was trying to impart was how to identify a certain "code smell", DRY (Don't Repeat Yourself).  It's not always possible to completely eliminate it and a lot of times you don't even realize you're doing it until the 4th or 5th time.  But with practice you get better at it...

Hope that helped!


----------



## zuki01 (Sep 25, 2015)

Wow.

I created a simple model with macros running a hidden sheet like a database and emailing to managers to record accidents at my firm so I thought I'd be quite good at looking at yours.

It's all way above my head. Very impressive !!!!

*Steals ideas*

The red is a bit dark is the only thing I could think of haha. It's very cool spreadsheet.


----------

