|
...if you replace the "magic numbers" with variables, this will make sense to do this way...in a readability sort of way.
|
|
|
|
|
Well. Not if it's the SAME variable!
int x = z + y - z;
|
|
|
|
|
What if z is volatile?
This would mean that the value of z could change between the add and the subtraction.
Learn from the mistakes of others, you may not live long enough to make them all yourself.
|
|
|
|
|
What if z is volatile?
If Z is volatile, then the code would go from being an idiosyncrasy to a horror, since the two reads would not be equivalent but there would be no sequence point to guarantee which would happen first.
In some cases, it's fine to read a volatile variable twice without an intervening sequence point (e.g. using something like while (*mem != *mem); to determine whether a flash chip is busy). If the order in which the reads take place won't matter, then it won't matter if the compiler rearranges them. On the other hand, in an expression like foo = vol + something - vol, the effect on foo if vol changes will be indeterminate. Far better would be something like foo = vol + something; foo -= vol;. Under that scenario, the semantics if vol changes would be clear.
|
|
|
|
|
In this case, it was for setting the position for modifying a string, so it could certain make sense for clarifying exactly where the update is inserted/replaced. But still, the "magic numbers" were there, not variables.
|
|
|
|
|
This may be a code signature
|
|
|
|
|
SELECT @ValueList = ''''+REPLACE(ISNULL(CAST([Amount] as varchar(8000)),'Null'),'''','''''') + ''',''' + REPLACE(ISNULL(CAST([ActualCost] as varchar(8000)),'Null'),'''','''''') + ''',''' + REPLACE(ISNULL(CAST([Notes] as varchar(8000)),'Null'),'''','''''') + ''',''' + REPLACE(ISNULL(CAST([Qty1] as varchar(8000)),'Null'),'''','''''') + ''' FROM Documents
I'm supposed to edit this, then place it in an update statement - it's the contents of a table field, not a literal query! I would rather write a quote doubler/divider than take my chances, but I was crafty and pasted my edit back into the field in Management Studio, now I just script the data of the table using the Publishing Wizard.
|
|
|
|
|
Why kind of monster can write such SQL?
|
|
|
|
|
It gets worse. When I escape the quotes to script an update of that field, I get the SQL below. The reason for all the casts is historical, and the reason for all the quotes is that the SQL gets passed through several layers of stored proc before actually being executed.
UPDATE [csForm] SET [CopyCommand] = N'SELECT @ValueList = ''''''''+REPLACE(ISNULL(CAST([Amount] as varchar(8000)),''Null''),'''''''','''''''''''') + '''''','''''' + REPLACE(ISNULL(CAST([DocTitle] as varchar(8000)),''Null''),'''''''','''''''''''') + '''''' FROM Documents WHERE DocID =|SELECT ''''''&IDENTITY&'''','''''' + REPLACE(ISNULL(CAST([WorkType] as varchar(8000)),''Null''),'''''''','''''''''''') + '''''','''''' + REPLACE(ISNULL(CAST([WorkTypeID] as varchar(8000)),''Null''),'''''''','''''''''''') + '''''','''''' + REPLACE(ISNULL(CAST([Amount] as varchar(8000)),''Null''),'''''''','''''''''''') + '''''','''''' + REPLACE(ISNULL(CAST([Notes] as varchar(8000)),''Null''),'''''''','''''''''''') + '''''','''''' + REPLACE(ISNULL(CAST([LineType] as varchar(8000)),''Null''),'''''''','''''''''''')+'''''''' AS ValueList FROM DocItems WHERE DocID =|'
|
|
|
|
|
Brady Kelly wrote: The reason for all the casts is historical
...
Don't you mean hysterical 
|
|
|
|
|
this is why they created linq !
|
|
|
|
|
You have my utmost condolences... 
|
|
|
|
|
Hi, I saw this some time ago:
for(int i = 0; i < 2; i++)
{
if(i==0)
if(i==1)
}
this is pretty much a horror
|
|
|
|
|
Your are correct, he/she should have used a switch instead of the if statements. Like this:
for(int i= 0; i < 2; i++)
{
switch(i)
{
case 0:
break;
case 1:
break;
};
}
Learn from the mistakes of others, you may not live long enough to make them all yourself.
|
|
|
|
|
I guess there is no need for a loop at all.
Nothing is being repeated here.
«_Superman_»
I love work. It gives me something to do between weekends.
|
|
|
|
|
«_Superman_» wrote: Nothing is being repeated here.
We don't know that.
|
|
|
|
|
I believe the point was that without the loop and the if's the (equivalent) code becomes
// do something
// do something else
which of course is simply normal program flow! So the whole looping business seems a bit convoluted.
|
|
|
|
|
Probably, but do //Do something and //Do something else alter the value of i?
|
|
|
|
|
PIEBALDconsult wrote: Probably, but do //Do something and //Do something else alter the value of i?
I don't see how they can...
|
|
|
|
|
{
i=i-2;
Console.WriteLine("once more");
}
Luc Pattyn [Forum Guidelines] [My Articles]
- before you ask a question here, search CodeProject, then Google
- the quality and detail of your question reflects on the effectiveness of the help you are likely to get
- use the code block button (PRE tags) to preserve formatting when showing multi-line code snippets
|
|
|
|
|
Such code may be entirely reasonable and practical if there is common code before or after the case-specific code, and if variable scoping would make it impractical for the common code to be pulled out into a separate routine.
My guess would be that there had at some point been some code which was executed every time through the loop, but that such code has been eliminated leaving the now-useless loop structure behind.
|
|
|
|
|
This is for sure a Coding Horror, a good one.
Does a loop for only two predefined values. If i (counter) is changed inside for, this is another coding horror!. And guess what, the developer is interesting for the values 0 and 1. (reminds you something like true/false maybe?)
|
|
|
|
|
I agree with the poster who said it depends.
If this is the loop's entire construct - yes I think it's not ideal. (Not a horror though). I would just have two statements "do something" and "do something else" and MAYBE block them in braces to be clear its a "unit" or make a function if warranted.
If the person planned to expand on this loop - where it cycled thru ten iterations, but was suppose to do something special for iteration 1 and 2, then a case statement might be ideal.. still not a horror.
--Jason P Sage
Know way to many languages... master of none!
|
|
|
|
|
The fact is that it was on a "finished" code, so the programmer didn't want to expand it. I agree with the idea that if you have a two iterations for and have two different actions on each, then there shouldn't be any for at all!
|
|
|
|
|
Do something absolutely is critical here to know whether it is horrible or not.
The following sample indicates what you saw is not horrible:
See, I have two controls: Label0 and TextBox1. I can assign values to them:
for (int i = 0; i < 2; i++)
{
if (i == 0)
{
Label lable = (Label)this.FindControl("Label" + i.ToString());
if (lable != null)
lable.Text = "Test Label " + i.ToString();
}
if (i == 1)
{
TextBox txt = (TextBox)this.FindControl("TextBox" + i.ToString());
if (txt != null)
txt.Text = "Test TextBox " + i.ToString();
}
}
See it? It is not a usual way, but, not as horrible as you saw.
modified on Monday, March 2, 2009 4:04 PM
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
