Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

This is going to make for either a poor UI or a change in behavior later. Better to just change its behavior now.

 Dim ixYear As Long
 For ixYear = 2000 To Year(Now)
 yearBox.AddItem ixYear
 Next ixYear

The problem is that this is always and forever going to start on the year 2000. The list will just continue to grow over time. That's fine if you need the year 2000 to always be there, but you should ask your users if that's the case. I see two options here.

  1. Calculate backwards 15 years.

     Dim thisYear = Year(Now)
     For ixYear = (thisYear - 15) To thisYear
    
  2. Make this year the first option in the list and populate the list in reverse.

     For ixYear = Year(Now) To 2000 Step -1
    

The approach you take will depend on your exact requirements. Of course, if you take the second route, you'll quickly find all the places where you hardcoded the number 2000.

The only other "issue" I see worth mentioning is this use of empty quotes.

 If yearBox.Text <> "" Then

You should use vbNullString wherever possible. It's intent is more clear and it uses less memory than the empty string literal. (Okay, so it's a negligible amount of memory, but still...)

All in all its good code, very self documenting as far as what it does, but some comments explaining why couldn't hurt. For example, why does the selection start at the year 2000?

I've got to say, I'm impressed at how far you've come since your first post here your first post here.

This is going to make for either a poor UI or a change in behavior later. Better to just change its behavior now.

 Dim ixYear As Long
 For ixYear = 2000 To Year(Now)
 yearBox.AddItem ixYear
 Next ixYear

The problem is that this is always and forever going to start on the year 2000. The list will just continue to grow over time. That's fine if you need the year 2000 to always be there, but you should ask your users if that's the case. I see two options here.

  1. Calculate backwards 15 years.

     Dim thisYear = Year(Now)
     For ixYear = (thisYear - 15) To thisYear
    
  2. Make this year the first option in the list and populate the list in reverse.

     For ixYear = Year(Now) To 2000 Step -1
    

The approach you take will depend on your exact requirements. Of course, if you take the second route, you'll quickly find all the places where you hardcoded the number 2000.

The only other "issue" I see worth mentioning is this use of empty quotes.

 If yearBox.Text <> "" Then

You should use vbNullString wherever possible. It's intent is more clear and it uses less memory than the empty string literal. (Okay, so it's a negligible amount of memory, but still...)

All in all its good code, very self documenting as far as what it does, but some comments explaining why couldn't hurt. For example, why does the selection start at the year 2000?

I've got to say, I'm impressed at how far you've come since your first post here.

This is going to make for either a poor UI or a change in behavior later. Better to just change its behavior now.

 Dim ixYear As Long
 For ixYear = 2000 To Year(Now)
 yearBox.AddItem ixYear
 Next ixYear

The problem is that this is always and forever going to start on the year 2000. The list will just continue to grow over time. That's fine if you need the year 2000 to always be there, but you should ask your users if that's the case. I see two options here.

  1. Calculate backwards 15 years.

     Dim thisYear = Year(Now)
     For ixYear = (thisYear - 15) To thisYear
    
  2. Make this year the first option in the list and populate the list in reverse.

     For ixYear = Year(Now) To 2000 Step -1
    

The approach you take will depend on your exact requirements. Of course, if you take the second route, you'll quickly find all the places where you hardcoded the number 2000.

The only other "issue" I see worth mentioning is this use of empty quotes.

 If yearBox.Text <> "" Then

You should use vbNullString wherever possible. It's intent is more clear and it uses less memory than the empty string literal. (Okay, so it's a negligible amount of memory, but still...)

All in all its good code, very self documenting as far as what it does, but some comments explaining why couldn't hurt. For example, why does the selection start at the year 2000?

I've got to say, I'm impressed at how far you've come since your first post here.

Source Link
RubberDuck
  • 31.2k
  • 6
  • 74
  • 176

This is going to make for either a poor UI or a change in behavior later. Better to just change its behavior now.

 Dim ixYear As Long
 For ixYear = 2000 To Year(Now)
 yearBox.AddItem ixYear
 Next ixYear

The problem is that this is always and forever going to start on the year 2000. The list will just continue to grow over time. That's fine if you need the year 2000 to always be there, but you should ask your users if that's the case. I see two options here.

  1. Calculate backwards 15 years.

     Dim thisYear = Year(Now)
     For ixYear = (thisYear - 15) To thisYear
    
  2. Make this year the first option in the list and populate the list in reverse.

     For ixYear = Year(Now) To 2000 Step -1
    

The approach you take will depend on your exact requirements. Of course, if you take the second route, you'll quickly find all the places where you hardcoded the number 2000.

The only other "issue" I see worth mentioning is this use of empty quotes.

 If yearBox.Text <> "" Then

You should use vbNullString wherever possible. It's intent is more clear and it uses less memory than the empty string literal. (Okay, so it's a negligible amount of memory, but still...)

All in all its good code, very self documenting as far as what it does, but some comments explaining why couldn't hurt. For example, why does the selection start at the year 2000?

I've got to say, I'm impressed at how far you've come since your first post here.

lang-vb

AltStyle によって変換されたページ (->オリジナル) /