|
... when you start to code like this
var splittedKey = Key_.Split(new Char[]{'.'});
(yes|no|maybe)*
|
|
|
|
|
I like. I like very much!
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
|
|
|
|
|
That gave me a chuckle I really needed.
|
|
|
|
|
Place it randomly in your code... 
(yes|no|maybe)*
|
|
|
|
|
Oh, I have to copy this some day 
Soren Madsen
|
|
|
|
|
This is SO much going into my code, one way or another... Epic win!
|
|
|
|
|
Just found this little gem in a live application that has been wrote (for us) and maintained by an outsourcing partner we use...
paraphrased to make it easier to read.
string keyToFind = "asdfgsd44rtgr";
Dictionary<string, object> dctTemp = GetDictionary();
object theObject = dctTemp.ToList().FirstOrDefault(x => x.key == keyToFind).Value;
This is in a live piece of code, and is run on every application login.
As far as I'm aware, the compiler would not be able to optimise this to use dctTemp[keyToFind] and therefore would not be using a HashCode
|
|
|
|
|
This is "wise foresight": In the next update, they can say that they increased their performance by about 20%.
I've the feeling, some companies introduce bugs on purpose in their own application to boost the sale for their future updates.
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...
|
|
|
|
|
|
Martin Thwaites wrote: run on every application login
So, how often is this?
Unless there are a lot of logins/second, don't sweat the small stuff.
IMO, the use of ToList (which can simply be deleted) and FirstOrDefault is less of an issue than the fact that they're storing objects. That's a bigger WFT than the optimization from the dictionary specific methods.
Don't optimize for speed until you have confirmation it's a hotspot.
Optimize first for robustness and maintainability.
And be aware, it can't be replaced it with:
object theObject = dctTemp[keyToFind];
For the same behavior, it'd have to be:
object theObject = dctTemp.ContainsKey(keyToFind) : dctTemp[keyToFind] : null;
|
|
|
|
|
I wasn't saying it was a hotspot, just that it's poor use of code. It's actually nowhere near to being a hotspot (they have 1 particular click in the application that takes 15 minutes to load the page, and therefore only works when there is a decent internet connection and a high timeout set!).
The object thing is FAR worse, however, in the context of how they've wrote the application it does make sense. That is actually one of the things I'm tackling at the moment.
This is one of many issues I've found that they've put in (before I started reviewing the code), and I suspect that they will be doing less now.
I also appreciate it can't be replaced by exactly what I've said based on the code sample, and in the application itself it would actually need to be different than you've suggested.
|
|
|
|
|
Harley L. Pebley wrote: object theObject = dctTemp.ContainsKey(keyToFind) : dctTemp[keyToFind] : null;
Shouldn't that be
object theObject = dctTemp.ContainsKey(keyToFind) ? dctTemp[keyToFind] : null;
?
EDIT: And why was this downvoted?
Bill Gates is a very rich man today... and do you want to know why? The answer is one word: versions.
Dave Barry
Read more at BrainyQuote[ ^]
modified 10-Jul-12 14:45pm.
|
|
|
|
|
Yep! No red underline to alert me to the typo. 
|
|
|
|
|
Aren't we in a sad state when we trust our tools more than our minds? Don't feel bad, I am also guilty of such offenses. (I am using MS Word to type this to check may spelling and grammar errors as well as use the thesaurus.)
|
|
|
|
|
And the reliance on tools bites you as you comment on it ...
|
|
|
|
|
Martin Thwaites wrote:
string keyToFind = "asdfgsd44rtgr";
Dictionary<string, object> dctTemp = GetDictionary();
object theObject = dctTemp.ToList().FirstOrDefault(x => x.key == keyToFind).Value;
Should be:
object theObject = GetDictionary()[keytoFind];
will return null if it's not in there, and with a dictionary you can only add one unique Key per...See:
using System.Collections.Generic;
namespace ConsoleApplication2
{
class Program
{
static void Main(string[] args)
{
Class1.Sample();
}
static class Class1
{
private static Dictionary GetDictionary()
{
var retval = new Dictionary();
retval.Add("asdfgsd44rtgr", "1");
retval.Add("asdfgsd44rtgr", "throws error");
return retval;
}
public static void Sample()
{
string keyToFind = "asdfgsd44rtgr";
Dictionary dctTemp = GetDictionary();
object theObject = GetDictionary()[keyToFind];
}
}
}
}
or did I miss Something ?
I'd blame it on the Brain farts.. But let's be honest, it really is more like a Methane factory between my ears some days then it is anything else...
-----
"The conversations he was having with himself were becoming ominous."-.. On the radio...
|
|
|
|
|
class WinCheckButtonProxy
{
public bool IsChecked()
{
return this.IsChecked();
}
}
modified 9-Jul-12 3:17am.
|
|
|
|
|
No problem. At first glance, I thought of a stack overflow error. But the missing ";" will prevent that from being succesfully compiled.
|
|
|
|
|
If C#, yes; but maybe it isn't, we don't know. 
|
|
|
|
|
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
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
