Requesting A Review

A

afwelch

New Member
Joined
Oct 16, 2014
Messages
5
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

 
Sort by date Sort by votes
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:

Code: 
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:

Code: 
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:
Code: 
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.
 
Upvote 0
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:
Click to expand...

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)!
Click to expand...

Hmm so this would possibly be another select case statement then?
 
Upvote 0
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!
 
Upvote 0
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.
 
Upvote 0
You must log in or register to reply here.

Similar threads

B
  • Question Question
Looking for Solution for User Authorization to An Excel Workbook
Replies
0
Views
274
bisel
B
M
  • Question Question
Excel Time Keeping "Application"
Replies
0
Views
623
Mrock1
M
P
  • Solved
writing a macro to copy an entire xlsm workbook to a users desktop as xlsx values only.
Replies
9
Views
1K
Alex Blakenburg
A
H
  • Question Question
Macro to download zip, unpack and copy data
Replies
0
Views
853
HeinrichPaul
H
A
  • Question Question
Take screenshots & simultaneously capture the link, place it into an Excel file which is linked to a FileMaker/Claris db.
Replies
3
Views
586
Dan_W
D

Share this page

We've detected that you are using an adblocker.

We have a great community of people providing Excel help here, but the hosting costs are enormous. You can help keep this site running by allowing ads on MrExcel.com.
Allow Ads at MrExcel

Which adblocker are you using?

Disable AdBlock

Follow these easy steps to disable AdBlock

1)Click on the icon in the browser’s toolbar.
2)Click on the icon in the browser’s toolbar.
2)Click on the "Pause on this site" option.
Go back

Disable AdBlock Plus

Follow these easy steps to disable AdBlock Plus

1)Click on the icon in the browser’s toolbar.
2)Click on the toggle to disable it for "mrexcel.com".
Go back

Disable uBlock Origin

Follow these easy steps to disable uBlock Origin

1)Click on the icon in the browser’s toolbar.
2)Click on the "Power" button.
3)Click on the "Refresh" button.
Go back

Disable uBlock

Follow these easy steps to disable uBlock

1)Click on the icon in the browser’s toolbar.
2)Click on the "Power" button.
3)Click on the "Refresh" button.
Go back
Back
Top