ToString() and ToUpper() are like, so cool!

[Nathan D Cook]

(some meaningless code left out)

if (combo.SelectedItem.ToString() != null && combo.SelectedItem.ToString().ToUpper() == "APPROVED")
{
/*do some stuff*/
}

else if (combo.SelectedItem.ToString() != null && combo.SelectedItem.ToString().ToUpper() == "UNAPPROVED")
{
/*do some stuff*/
}

else if (combo.SelectedItem.ToString() != null && combo.SelectedItem.ToString().ToUpper() == "PAID")
{
VoidPayment(combo.SelectedItem.ToString().ToUpper());
}

else if (combo.SelectedItem.ToString() != null && combo.SelectedItem.ToString().ToUpper() == "ADJUSTED")
{
VoidPayment(combo.SelectedItem.ToString().ToUpper());
}

VoidPayment(string paytype)
{
if (paytype.ToUpper() == "PAID")
else if (paytype.ToUpper() == "ADJUSTED")
}

I am just a novice programmer, but I saw 3 things that seemed strange to me. First, why check for null every time and a ToString().ToUpper() every time? Why not just check it once and set a string that we can check? :confused: Second, in the paid and adjusted if-else sections, why not just pass PAID or ADJUSTED into VoidPayment? In this particular function, the combo box doesn't change. You already know what section you are in due to the if-else logic. (Or we could pass that same string I mentioned a second ago). Third, in the VoidPayment function why are we doing yet another ToUpper when we know we are passing in either a capitalized PAID or ADJUSTED string? VoidPayment is only called from those two spots, and it's not really a function that should be called elsewhere or added anywhere else later. And they wonder why our code is slow....... Cheers, Nathan

[Lost User]

Nathan D Cook wrote:

First, why check for null every time and a ToString().ToUpper() every time? Why not just check it once and set a string that we can check? :confused:

Absolutely correct. That kind of code comes from people who have no idea what the compiler will be able to make out of this. String operations do not translate to single machine instructions. They usually are loops that go through an array of characters. Only nested loops can slow down the processor even more than having to go through one loop after another. So avoid string operations if you can do the job another way (like using an enum for example) and otherwise try to use them as sparingly as possible.

Nathan D Cook wrote:

Third, in the VoidPayment function why are we doing yet another ToUpper when we know we are passing in either a capitalized PAID or ADJUSTED string? VoidPayment is only called from those two spots, and it's not really a function that should be called elsewhere or added anywhere else later.

Here we have a tough choice. You are right, checking and adjusting the string may be redundant here. We might get away with doing it your way. For now. In a year another programmer may have to work on this code and may not know about your assumption that the parameter has already been checked and adjusted. If he calls the function without making sure of that precondition, he will introduce a bug without knowing it. The whole mess could be avoided if you could use a more suitable datatype. One that's not nullable and can be tested cheaply. An enumeration.

At least artificial intelligence already is superior to natural stupidity

[OriginalGriff]

To add to CDP1802's response, the if...else if...else part could be nicely replaced with a switch, which gets rid of the need to do it multiple times anyway:

if (combo.SelectedItem.ToString() != null)
{
switch (combo.SelectedItem.ToString().ToUpper())
{
case "APPROVED":
...
break;
case "UNAPPROVED":
...
break;
...

Which is a lot more readable, and may be a lot more efficient when compiled.

Ideological Purity is no substitute for being able to stick your thumb down a pipe to stop the water

[VallarasuS]

You have a point and I would also suggest to replace the strings with constants to avoid introducing bugs by misspelling them. "APPROVED", "ADJUSTED" and more...

Regards Vallarasu S | FSharpMe.blogspot.com

[Gary R Wheeler]

string selection = null;
if (combo.SelectedItem != null) selection = combo.SelectedItem.ToString().ToUpper();
switch (selection)
{
case "APPROVED":
//...
break;
case "UNAPPROVED":
//...
break;
default:
//...
break;
}

Software Zen: delete this;

[V 0]

in addition: 1: combo.SelectedItem might be null and thus the call to ToString() will die in a less elegant way. I don't event think the ToString to return null if selectedItem is not null. Worst case is will return the object name. 2: Should not use mystring == null but string.isNullOrEmpty(mystring) (unless "" is allowed.) 3: Personal, but I genuinly hate if - else if code. I prefer switch in that case. switch also allows for a fall through so you can pass on combo.SelectedItem.ToString().ToUpper() as is to the VoidPayment where it is, in this case checked again. 4: a nice variable assignment like     string combostringvalue = combo.SelectedItem.ToString().ToUpper() ; would have been nice idd.

V.

[Nagy Vilmos]

Are there a fixed number of values? This sounds like a job for an enumerator[^].

---

Panic, Chaos, Destruction. My work here is done. Drink. Get drunk. Fall over - P O'H OK, I will win to day or my name isn't Ethel Crudacre! - DD Ethel Crudacre I cannot live by bread alone. Bacon and ketchup are needed as well. - Trollslayer Have a bit more patience with newbies. Of course some of them act dumb - they're often *students*, for heaven's sake - Terry Pratchett

[OriginalGriff]

Oops! :-O I forgot to take off the ToString when I copy-n-pasted from the OP...

Ideological Purity is no substitute for being able to stick your thumb down a pipe to stop the water

[Gary R Wheeler]

This is a petard I've been hoisted by too many times :-O.

Software Zen: delete this;

[PIEBALDconsult]

Definitely a place to use an enumeration.

[TNCaver]

Rated 5 for your use of that humorous and archaic phrase. :)

If you think 'goto' is evil, try writing an Assembly program without JMP.