According to Jan Norgreen (See also JWalk’s post):
To read other’s code is a way to improve one’s own. I would love to find a site on the Internet where one could have one’s code criticised. It is a common service amongst Go and chess players and is an excellent way to improve ones strength.
What a good idea. I’ll start. Here’s some code I wrote to protect Data Validation from being erased when the user copies and pastes. It’s the whole sheet module, so I’m putting it on a separate page.
Criticize away!
Then, send me your code, I’ll post it, and everyone can criticize it. If we don’t learn something from this, then I don’t know what.
Dick, I’d happily criticize your code but I’ll need to study a lot more more to know what I should be looking at :)
Anyway, thanks for the tip. I’ve entered it into My Favorites for future reference!
The code looks good and is easy to read, but if you want it to be foolproof, I can work around it in a few ways:
1. Drag and drop moving of a cell
2. Paste values
3. Paste formulas
4. Just hit escape after getting the error message
That beats my method of just turning cutcopy mode off on selection change. However, I’m just an accountant, so my modules are free to be as user-hostile as I want them to be.
Seriously, though, I always do assume that anyone using a template of mine is determined to find a way to put bad data into a cell no matter how I protect it.
This is a bug in Excel as far as I’m concerned; some cell properties just shouldn’t be changed during copy and paste operations.
The code looks good and is easy to read, but if you want it to be foolproof, I can work around it in a few ways:
1. Drag and drop moving of a cell
2. Paste values
3. Paste formulas
4. Just hit escape after getting the error message
That beats my method of just turning cutcopy mode off on selection change. However, I’m just an accountant, so my modules are free to be as user-hostile as I want them to be.
Seriously, though, I always do assume that anyone using a template of mine is determined to find a way to put bad data into a cell no matter how I protect it.
This is a bug in Excel as far as I’m concerned; some cell properties just shouldn’t be changed during copy and paste operations.
OK, I shouldn’t have hit post twice.
Here’s my code to try to build versioning (is that a word) into my templates. My versioning file is a tabbed text file with program name in column A and version number in column B. The reference to range B12 in my code means (hate to build in absolute references, but I’m stumped for an alternative) it’s the 12th program in my versioning file.
Private Sub Workbook_BeforeClose(Cancel As Boolean)
On Error Resume Next
Application.CommandBars(1).Controls(“CC”).Delete
End Sub
Private Sub Workbook_Open()
Const sngVer As Single = 2003
‘Check for new version
Application.ScreenUpdating = False
Application.DisplayAlerts = False
Application.EnableCancelKey = xlDisabled
On Error Resume Next
‘open versioning file on network and save to c:
Workbooks.Open FileName:=”G:deptsprovrembprovreimversions.txt”
If Err.Number <> 1004 Then ActiveWorkbook.SaveAs FileName:=”c:program filesmicrosoft officeofficeversions.txt”
If Err.Number = 1004 Then ‘not on network, used saved file
Err.Clear
Workbooks.Open FileName:=”c:program filesmicrosoft officeofficeversions.txt”
End If
If Err.Number <> 1004 Then
If Range(“B12?) <> sngVer Then
If MsgBox(“Version ” & Range(“B12?) & ” is now available.” & vbCrLf & “You are currently running version ” & sngVer & “.” & vbCrLf & vbCrLf & “You will need to update to the newest version to make sure you have the most current bug fixes, formatting changes, and revised forms.” & vbCrLf & vbCrLf & “If, for some odd reason, you need to use this old version then click “”YES”” below. Note, though, that no further support is available for this version”, vbInformation + vbYesNo + vbDefaultButton2, “Update Available”) = vbNo Then
Workbooks(“versions.txt”).Close SAVECHANGES:=False
ActiveWorkbook.Close SAVECHANGES:=False
Application.EnableCancelKey = xlInterrupt
Exit Sub
End If
End If
Else ‘couldn’t open either file (this should never happen, but just in case)
MsgBox “The version number could not be verified. Please be sure you are using the most current version before continuing.”, vbInformation, “Version Check Error”
End If
Workbooks(“versions.txt”).Close SAVECHANGES:=False
Application.EnableCancelKey = xlInterrupt
Application.ScreenUpdating = False
Range(“A1?).Select
ActiveWindow.Zoom = True
End Sub
(I do have indents, BTW, but I don’t think they will show in the post)
Thanks for opening your blog for code critique!!!
When I launched the idea in the group microsoft.public.excel.programming I got only one response, so I am double grateful.
I think it is beautiful to learn from other’s mistakes. :)
I have spent the last half hour studying and testing your code.
I have to admit that I did not know that Excel has data validation built in, so it took me some time to realise which problem you were solving.
As I understand it, copy and paste, destroys data validation of a range, and that is the problem you are dealing with.
My first comment will be as a user. The program does not behave the way I would like it to behave.
I gave a cell data validation: integer between 12 and 45. When I tried to copy 333 into the cell, I was told ‘Value is not valid’ and sent into edit mode. I pressed Enter (being dumb or non-thinking), and 333 stayed in the cell.
As a user, when I try to paste 333, I would like to be treated the same way as if I type 333 into the cell. That is, I would like to see the error message I originally created for not valid data, and under no circumstance should 333 be accepted.
The code was very nicely written with useful comments and easy structure to follow. It taught me about worksheet events and sensible use of global variable.
Thanks!
Regards,
Jan
The error handling could catch a bug…
Error handling only gets switches off if there is no error, ie after the IF, otherwise it remains on for the rest of the routine, which in this case doesn’t matter. If you added code to the end, errors would be silently handled.
I keep On Error statements aligned, like If/End If or For loops. That is, wrap the On Error statements around the code to be tested, and set a status flag. Later, you can take this little chunk and move it to a function of its own with little effort.
Congratulations on doing your error code like that, I think it makes more sense.
You could move the ‘With Target.Valuation’ a few lines higher, eg to include the .Add statement.
Other than that, most people use the prefix m for public variables, not v. I was wondering why they were called Variants, but Dimmed as Strings!
From a style point-of-view, I’d suggest changing the module-level variable names. If “s” indicates a string, should not “v” indicate a Variant, and not the mixture of types shown? I’f also suggest a name modifier for variables of greater than local scope, say an “m” for modules or class members and a “g” for globals, if you must have them.
The big guard clause in Worksheet_Change might be better pulled up to the top thus:
Dim errno As Long
On Error Resume Next
sTemp = Target.Validation.ErrorMessage
errno = Error.Number
On Error Goto 0
If errno <> 1004 Then
Exit Sub
End If
… which then allows the code inside to un-indent a level. The same might then be done to get:
If Not bCopyReady Then
Exit Sub
End If
…which puts the main purpose of the routine outside of any indentation.
Those two guard clauses could then perhaps move into another function, perhaps called ReadyToCopyValidation(). There’s probably a better name ;)
I’d look to do something similar with the Worksheet_SelectionChange Sub.
While I recognise that there have been vocal schools of thought that demanded single entry/exit points, I’d suggest that short routines with almost trivial amounts of indentation are not compromised by early exits.
How about this method?
http://j-walk.com/ss/excel/tips/tip98.htm
I haven’t really analyzed Dick’s code, and I haven’t revisited mine in quite a while. But I do remember that it worked very well. And it’s a lot simpler.
The method described in
http://j-walk.com/ss/excel/tips/tip98.htm
is not bad, but very basic.
E.g. paste a valid text into one validation field. in the above described method it is not allowed. With Jans code this entry is allowed, as it is a valid entry.
EK