|
For shame! String is a class and all class instances get their references passed around (so there is no large copy when using ByVal). When you do ByRef, you actually pass a reference to that reference, allowing the source variable (used by the caller) to be assigned something different. Take this for example:
Public Class Form1
Private small As String = "a"
Private large As String = "This was a really long string I shortened so I could paste it here easily..."
Private Const MAX As Integer = 9999999
Private startTime As DateTime
Private endTime As DateTime
Private smallDuration As Integer
Private largeDuration As Integer
Dim sb As System.Text.StringBuilder
Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click
sb = New System.Text.StringBuilder()
GC.Collect()
GC.WaitForPendingFinalizers()
startTime = DateTime.Now
For i As Integer = 0 To MAX
sb.Append(FirstChar(large))
Next
endTime = DateTime.Now
largeDuration = CInt(endTime.Subtract(startTime).TotalMilliseconds)
sb = New System.Text.StringBuilder()
GC.Collect()
GC.WaitForPendingFinalizers()
startTime = DateTime.Now
For i As Integer = 0 To MAX
sb.Append(FirstChar(small))
Next
endTime = DateTime.Now
smallDuration = CInt(endTime.Subtract(startTime).TotalMilliseconds)
MessageBox.Show(String.Format("Small: {0}, Large: {1}", smallDuration.ToString(), largeDuration.ToString()))
End Sub
Private Function FirstChar(ByRef str As String) As String
Return str.Substring(0, 1)
End Function
End Class
The timings in that example will always be very similar, whether you use a large string, a small string, ByRef, or ByVal. Using ByRef when you don't actually intend to treat that variable as an output from the function would be a coding horror!
But don't stop there. There are plenty more horrors in the IsNullOrEmpty function.
|
|
|
|
|
You are right, I can only plead stupidity and not bothering to think before I commented!
Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together.
|
|
|
|
|
I fail to see the great horror in your code... If you are posting in the Hall of Shame with a code snippet, should there not be at least one subtle horror in there? We are coming here to be amused, not educated...
(Nicely explained, by the way. I actually ran into a few senior developers in my time who did not understand this concept)
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
Indeed, I'm in a VB shop now and it seems that someone thinks reference types always have be passed byref .
|
|
|
|
|
Ain't that the truth...
Ironically, I have a couple QA's who I'm mentoring in C# for web automation testing that have a better grasp... They have never taken any courses... (They also write better code, but I'll pretend that is because of me)
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
Let's imagine that string.IsNullOrEmpty does not exist. Let's imagine that extension methods do not exist either. Let's imagine that a method like this might be useful. With all of these premises, I still think that someone should invent a computer which slapped anyone who wrote an "If" statement just to return the result of the condition evaluated by that "If". I have seen it countless times, and I still don't understand how they do not notice it.
|
|
|
|
|
If string.IsNullOrEmpty did not exist, I would create it in my source because it is a heavily used utility function, even if the the method only returns the results of an "if" evaluation.
My reasoning for this is that the usage of stringVariable.IsNullOrEmpty() is very clear as to what is being evaluated while if( stringVariable == null || stringVariable.Length == 0 ) is not as clear.
From a sustainability standpoint I would definitely create it.
As a note, I always add this utility to all solutions that do not have the support natively.
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
Yes, sure, but what I meant was more about the implementation itself:
public static bool IsNullOrEmpty(string str)
{
return str == null || str == "";
}
No need of an "if" statement since we are actually returning the result of the condition itself. This is what I meant.
|
|
|
|
|
But do you want to write the entire "if" statement in hundreds of places in a large solution, or one that can be issued a test-case for coverage and validation that will then be validated from intellisense. Any equality statement is dangerous in the old "str = null" vs "str == null" world and thus should be encapsulated into a testable case.
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
Marcus Kramer wrote: But do you want to write the entire "if" statement in hundreds of places in a large solution, or one that can be issued a test-case for coverage and validation that will then be validated from intellisense. Any equality statement is dangerous in the old "str = null" vs "str == null" world and thus should be encapsulated into a testable case.
Uh, what? Are you saying that the if statement should be encapsulated in a method rather than sprinkling the if statement over several places in the code? Erik never said otherwise. He just said that rather than using an if statement, a single return statement can be used (yes, inside a method).
|
|
|
|
|
I stand corrected. I misread that last post from Erik....
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
|
I can't see the problem...
return lst == null || lst.Count == 0;
Becouse if a class implements IList it must also implement ICollection.
|
|
|
|
|
Yes, but String isn't an IList -- but not to worry, I wrote... I wrote my own String class today.
|
|
|
|
|
Instrument = instrument.Accessor;
Accessor = GetAccessorForInstrument(instrument.Accessor);
Accessor.Instrument = instrument.Accessor;
Has anybody seen a more confusing piece of code???
|
|
|
|
|
Is this in a case sensitive language?
|
|
|
|
|
Yes, it is case-sensitive C#.
I forgot to mention that all "instrument" variables are of various "xxxAccessor" types, and all "accessor" variables are of "xxxInstrument" types, so the code is way way worse than it seems.
I swear, the variables are declared something like this:
aaaAccessor Instrument;
bbbInstrument Accessor;
cccAccessor instrument;
dddInstrument accessor;
Instrument = instrument.Accessor;
Accessor = GetAccessorForInstrument(instrument.Accessor);
Accessor.Instrument = instrument.Accessor;
I won't even mention there are also InstrumentFactory class, and AccessorFactory class, and InstrumentWrapper class, and AccessorWrapper class, and IInstrument interface, and IAccessor interface, and InstrumentHelper, and AccessorHelper, and ...
Do you think that deserves the Guinness book? I mean, this is probably the only program where not the values but THE MEANINGS of variables are being swapped all the time. This is not programming, this is meta-programming.
modified on Wednesday, December 29, 2010 4:31 PM
|
|
|
|
|
it's kind of an obfuscation-tactic!
After at least 10 lines you're totally confused and want to drain yourself in your coffee mug.
regards
Torsten
I never finish anyth...
|
|
|
|
|
Yeah, it is probably about job security. Everybody who has to work with his code will quit, and he will be forever the sole maintainer of that garbage.
What about XAML like this?
<Textbox Text="{Binding xxxViewModel.xxxModel.xxxData.xxxModel.DataContext.xxxUnderlying.xxxModel.xxxViewModel.Model....}" />
Yes, Binding paths with 8 dots and more !!!
modified on Friday, January 7, 2011 1:16 PM
|
|
|
|
|
I think something got lost in translation
|
|
|
|
|
What translation? I have to maintain and fix bugs in his code. It is all like that.
Variables have wrong names, and their meaning changes al the time. Variable named "exchange" may actually mean "exchange_code", "exchange_id", "IExchange", "ExchangeConfig". "string exchange" may be used to keep "exchange_id", "exchange_code", and later on "exchange_name"... "instrument" may be of type "IInstrumentWrapper", "Ric", "Occ", "IDataAccessor", or whatever, and "accessor" means exactly the same...
What about his unique approach to multi-threading?
GUI thread: Click
start background thread
do something
synchronous post to GUI Dispatcher
wait until GUI is done
do something
synchronous post to GUI Dispatcher
wait until GUI is done
do something
synchronous post to GUI Dispatcher
wait until GUI is done
do something
Thread synchronization? Done using bunch of boolean flags, so it is one big race condition waiting to happen.
There is no way to stop any background thread of course, so usually you have to use TaskManager to kill the application after you click "Close". Forget about memory leaks and such.
|
|
|
|
|
<code>
string EnableLogging = string.Empty;
string WSURL = string.Empty;
string TransformXSLPath = string.Empty;
....
try
{
EnableLogging = ConfigurationSettings.AppSettings["EnableLogging"].ToString();
}
catch( Exception ex )
{
EnableLogging = string.Empty;
}
try
{
WSURL = ConfigurationSettings.AppSettings["WSURL"].ToString();
}
catch( Exception ex )
{
WSURL = string.Empty;
}
try
{
TransformXSLPath = ConfigurationSettings.AppSettings["TransformXSLPath"].ToString();
}
catch( Exception ex )
{
TransformXSLPath = string.Empty;
}
....
</code>
like this for all variables that are read from config file.
|
|
|
|
|
Assuming this is C#... exception handling is probably not necessary (though better safe than sorry), ToString is not necessary on something that is already a string, the initial assignments are not necessary if they're going to get assigned in the catches, catching a particular exception is not necessary if the exception variable is not going to be used (there are some nuances to that, but for this example it seems unnecessary), and the repeated functionality is not encapsulated in a function. Maybe this would be more appropriate:
private string GetConfigSetting(string key)
{
string val;
try
{
val = ConfigurationSettings.AppSettings[key];
}
catch
{
val = string.Empty;
}
return val;
}
string EnableLogging = GetConfigSetting("EnableLogging");
string WSURL = GetConfigSetting("WSURL");
string TransformXSLPath = GetConfigSetting("TransformXSLPath");
Does that about do it, or was there something else you noticed that was horrific/shameful?
|
|
|
|
|
Agreed, no need of .ToString() as AppSettings[] will return string only.
But this line
EnableLogging = ConfigurationSettings.AppSettings["EnableLogging"].ToString();
will throw only Objectrefnotset exception since .ToString() is used and if the key is not found in the config file. I dont find a reason for any exception.
This should be the right way
if (ConfigurationSettings.AppSettings["EnableLogging"] != null ) <br />
EnableLogging = ConfigurationSettings.AppSettings["EnableLogging"]
or
EnableLogging = ConfigurationSettings.AppSettings["EnableLogging"] ?? string.Empty;
|
|
|
|
|
Yeah, I can't get it to throw an exception (even when I use an invalid key), though the MSDN documentation says an exception can be thrown. One more thing I noticed is that ConfigurationSettings.AppSettings is deprecated in favor of ConfigurationManager.AppSettings , though it may be fine if the code resides in an older version of the .Net Framework in which that was not yet deprecated.
|
|
|
|