Is there ANYTHING wrong with this code?

[alarian]

Sub ReFileContacts()
On Error GoTo ErrHandler:
Dim items As items, item As ContactItem, folder As folder
Dim contactItems As Outlook.items
Dim itemContact As Outlook.ContactItem
Dim Count As Long

Set folder = Session.GetDefaultFolder(olFolderContacts)
Set items = folder.items
Count = items.Count
If Count = 0 Then
MsgBox "Nothing to do!"
Exit Sub
End If

'Filter on the message class to obtain only contact items in the folder
Set contactItems = items.Restrict("[MessageClass]='IPM.Contact'")

For Each itemContact In contactItems 'Loop through all contacts
itemContact.Email1Address = LCase(Trim(Replace(itemContact.Email1Address, " ", "")))
itemContact.Save 'Save the contact
Next

MsgBox "Your contacts have been converted to lowercase without whitespaces."
Exit Sub
ErrHandler:
MsgBox (itemContact.Email1Address)
Resume Next
End Sub

I'm having issues with a client... Going through an arbitration
Is there anything that should prevent this from running?
Please try it out, say if you get any errors

[westconn1]

i see nothing wrong with it, but can not test
it would be possible to put some more logging or much better error handling
do you know if they are getting any errors?

[alarian]

i see nothing wrong with it, but can not test
it would be possible to put some more logging or much better error handling
do you know if they are getting any errors?

For this task, I don't think there's any reason to log or handle anything.
The client has problems with corrupt contact data and get synchronization errors.
It's unrelated to the code, as in, it would happen if the client would try to update all email addresses manually.
Hence the arbitration...

[si_the_geek]

Just because it would not work properly manually, that doesn't mean it is OK for your code to not handle things in a graceful manner.

As it stands, you can have absolutely no idea what the error is or where it occurred etc (and it could well be your code that is failing, it is foolish to pretend it isn't until you have fully proven otherwise), the error message you show is just an email address with no explanation whatsoever, and if there are multiple failures during the process you bombard the user with a series of message boxes that they need to click on one at a time (rather than just one single message at the end with a list of all the failures, preferably one that they could copy+paste so that they can attempt manual fixes etc).

On top of all that, your error handler assumes that errors can only ever occur inside the itemContact loop. If they occur elsewhere, your code is likely to totally crash.


Looking at it from the point of view of a user, I would not be happy with a program that acted that way (if I'd paid for it in a shop, I'd expect a refund), so as a programmer I would not consider releasing something like that to user (unless they demand a cheap/rush job, and accept the consequences after you have explained them).

The amount of effort needed to add good error handling is quite small, but it will have a dramatic effect on how professional your code seems to be - and how likely the users are to get annoyed at you (errors are annoying, but badly handled errors add an insult on top of it).

[alarian]

Looking at it from the point of view of a user, I would not be happy with a program that acted that way (if I'd paid for it in a shop, I'd expect a refund), so as a programmer I would not consider releasing something like that to user (unless they demand a cheap/rush job, and accept the consequences after you have explained them).
The amount of effort needed to add good error handling is quite small, but it will have a dramatic effect on how professional your code seems to be - and how likely the users are to get annoyed at you (errors are annoying, but badly handled errors add an insult on top of it).

There are some factors to this job:

1. The client has some understanding of VBA and don't care about proper error handling. The error handler here is just to indicate which contact is corrupt for manual handling. This was agreed with the client and the solution he found himself and was satisfied with was to delete the few contacts that were corrupted. He would even have been satisfied with "On Error Resume Next" (which he suggested), but I wanted to at least sort out the corrupt data. The guy is just using screenshots of a Synchronization error he got while running the code to get away from paying.
2. It's an extremely cheap job with an extremely cheap client. AND as it turns out, he has no white hat purposes with the way he spend his time on the internet...
3. The customer is using custom forms
4. The client is running windows on a mac with a cracked version of outlook 2007
5. The client has a very bad attitude and is trying to get away without paying
6. The code has converted all his contact's e-mail addresses as he requested and has also helped him with another issue of corrupt contacts
7. He accepted the job as complete on Skype, but argued on the internet freelance site when I reported work as complete. The site policy states that Skype conversation is not valid in arbitration since it can be faked.

I'm sorry that I wasn't more clear with the circumstances and I do understand your critique. But this guy is a "says a lot of very harsh curse words"...
Currently, I've worked many hours for free (if he wins arbitration) and on top of that it also gives me an incredibly low rating.
Out of 72 freelance projects I have received 71 10/10 ratings and one 9/10 rating.

So back to my original question... Please.
Is there anything _wrong_ about this code?

[westconn1]

For this task, I don't think there's any reason to log or handle anything.

Is there anything _wrong_ about this code?

the first may tell you the second

[alarian]

the first may tell you the second

This is the job description:
I've got 20K contacts in Outlook. Many of the Email1Address are in uppercase, and have blank spaces in them. It confuses my applications.
I need a macro that will convert all Email1Address to lowercase and remove blank spaces.
This is a quick job, I don't expect any fancy code just something simple that loops through the contacts and change Email1Address

During the job he wrote this over Skype:
Naah, you don't have to do that, just use "On Error Resume Next"? I don't care if a few contacts aren't converted.


Turns out, these applications he is talking about are spam applications that can import outlook exports.
AND in the arbitration, Skype conversations are NOT cared for.

SO PLEASE!
Back to my initial question... with clarification
And could the next answer PLEASE be less reluctant!? I'm asking for help here not some smart/short disparaging reply with NO intention to actually help out but just point fingers. I have enough of that.

What does this code do? Does it loop through all contact items and convert the Email1Address to lowercase and without white spaces?
Please, could you test it in your independent Outlook?

[si_the_geek]

It sounds like the person you did the work for is rather dodgy, it's not a nice situation to be in.


westconn1 was alluding to something that I mentioned in a bit more detail - unless you have proper error handling, you cannot tell whether your code is valid or not.

All you know is that it worked in your tests, which of course means that you can only be sure that it works for the mix of software/versions/settings/addons/data/etc that you tested it against. Just one minor variation in any of those items could expose failings in your code (in addition to the flawed 'errors can only happen in the loop' assumption, which is the only obvious issue).


It is almost pointless for anybody to test your code, because whether it works for them or not has no implication that it would be the same for anyone else. The best you can hope for is that somebody gets problems with it, as then you would know for sure that it has issues.

Unless somebody has written very similar code before, the only way to verify your code is to put in far more effort than you already have... and even then you cannot be sure that it will work for everyone.


As things stand, I think the best thing you can do is deal with the arbitration, then just let the whole thing go.