|
Yes, ";" is missing. I fixed it.
|
|
|
|
|
Why? Just delete the code - don't try to fix it!
Ideological Purity is no substitute for being able to stick your thumb down a pipe to stop the water
|
|
|
|
|
Tell the compiler/computer to execute each new "IsChecked" invocation as double as fast as the current "IsChecked" invocation. Since 1 (the time of the first IsChecked) + 1/2 (the time of the second IsChecked) + 1/4 + ... + 1/2^n converges to 2 for n -> Inf, you have the result of "IsChecked" in 2 time units
But Achilles wishes from Zeus, who executes every wish, that his wish mustn't come true and the world crashes... (that was an insider from the same book)
If you find spelling- or grammer-mistakes, please let me know, so that I can correct them (at least for me) - english is not my first language...
modified 9-Jul-12 14:10pm.
|
|
|
|
|
(some meaningless code left out)
if (combo.SelectedItem.ToString() != null && combo.SelectedItem.ToString().ToUpper() == "APPROVED")
{
}
else if (combo.SelectedItem.ToString() != null && combo.SelectedItem.ToString().ToUpper() == "UNAPPROVED")
{
}
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?
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
|
|
|
|
|
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?
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
modified 5-Jul-12 17:38pm.
|
|
|
|
|
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
|
|
|
|
|
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;
|
|
|
|
|
Oops! 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
|
|
|
|
|
This is a petard I've been hoisted by too many times .
Software Zen: delete this;
|
|
|
|
|
Rated 5 for your use of that humorous and archaic phrase.
If you think 'goto' is evil, try writing an Assembly program without JMP.
|
|
|
|
|
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.
|
|
|
|
|
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
|
|
|
|
|
Definitely a place to use an enumeration.
|
|
|
|
|
public class Field
{
private readonly string _field;
private readonly string _table;
Field()
{}
public Field(Field s)
{
_table = string.Copy(s.Table);
_field = string.Copy(s.Field);
}
}
string.Copy for what?
|
|
|
|
|
So that the immutable string value can be changed without affecting this class, obviously!
Just because you make something that can't be changed readonly doesn't mean you should take chances...
Ideological Purity is no substitute for being able to stick your thumb down a pipe to stop the water
|
|
|
|
|
Yeah... original dev didn't took chances, but fore sure I have to. I'll kick on his a** next time i see him.
|
|
|
|
|
Someone told me that String.Copy is faster than raw assignment...
So
_field = string.Copy(s.Field);
might actually perform better than
_field = s.Field
Is he crazy or am I mad to not have known this??
|
|
|
|
|
That guy used to be a C/C++ programmer. He wants to make sure that he copies the value, instead of referencing the original value. Just think of those terrible char * . C--!
|
|
|
|
|
What if I tell you he is an experienced C# developer
|
|
|
|
|
Only I noted the private default constructor that make this class impossible to instantiate? or there are more constructors?
I'm brazilian and english (well, human languages in general) aren't my best skill, so, sorry by my english. (if you want we can speak in C# or VB.Net =p)
|
|
|
|
|
FYI The default accessor is internal not private.
"You get that on the big jobs."
|
|
|
|
|
Verify again, it's private, i just tested to make sure. ALL members of a class are private for default, there is no exception to constructors, i just tested like this:
My class:
public class Form
{
Form() {
}
}
i used a project that i was working on, so this is a MVC 3 project, my controller:
public class SurveyController : Controller
{
public SurveyController() {
Form form = new Form();
}
}
this yields this compiler error:
Error 1 'FormularioAvaliacaoSaude.Models.Formulario.Formulario()' is inaccessible due to its protection level C:\workspace\FormularioAvaliacaoSaude\FormularioAvaliacaoSaude\Controllers\PesquisaController.cs 22 31 FormularioAvaliacaoSaude
(sorry for don't translating the paths on the error message, but i fell like i'll screw it up if i do it...)
by this error i make the conclusion that the default Access Modifier.
...
Just run into this on MSDN:
The declaration of the empty constructor prevents the automatic generation of a default constructor. Note that if you don't use an access modifier with the constructor it will still be private by default. However, the private modifier is usually used explicitly to make it clear that the class cannot be instantiated.
I'm brazilian and english (well, human languages in general) aren't my best skill, so, sorry by my english. (if you want we can speak in C# or VB.Net =p)
|
|
|
|
|
Sorry my bad. I thought it was internal. You are right
"You get that on the big jobs."
|
|
|
|
|
In Java it is package-visible (pretty close to internal) ... and bizarrely, there is no explicit way to specify this access level!
|
|
|
|
|
This has got me a little interested. By not making a copy, could that stop the GC from disposing of Field s until this instance is also ready for collection?
"You get that on the big jobs."
|
|
|
|