|
It is still bad though!
------------------------------------
I will never again mention that I was the poster of the One Millionth Lounge Post, nor that it was complete drivel. Dalek Dave
CCC League Table Link
CCC Link[ ^]
|
|
|
|
|
Your post the other day was part of my inspiration for my point about the square root of a number... so you're partly to blame for my silliness! 
|
|
|
|
|
That was one of the reasons I immediately posted it - the coincidence of seeing yours so recently.
I almost wish there was something clever like that going on, but no, flag is declared as an Integer.
As DD pointed out though, even if that was the case its still a horror. I'm coming round to the view that operator overloading maybe a bad idea because of the potential for abuses like this. Although I note that D provides a better implementation, where defining opCmp covers the whole family of <, <= etc. preventing abuses like that (to an extent).
|
|
|
|
|
I unfortunately must disagree.. no amount of language feature or design constraint can save you from a programmer lacking in programming skills. The only solution for THAT problem is good code review and a team willing to point out ways to improve (and enforce them) to the souls who are in this occupation but shouldn't be.
And.. your original post is hilarious and sad at the same time. For the same reasons.
|
|
|
|
|
A simple kind of integration between two application is that one application calls the other application via the command line, passing a few parameters. A customer wanted to call our application from their main application. On the configuration form, he entered the parameters to be passed:
OurProgram.exe /param1=[InternalValue1] /param2=[InternalValue2]
When calling OurProgram from their MainApplication, MainApplication parses the command line parameters and replaces internal values with their present values, i.e. it eventually calls:
OurProgram.exe /param1=John /param2=Doe
It works, but there is a catch: the user cannot work now in MainApplication because it waits for OurProgram to exit.
Our customer called the manufacturer of MainApplication, and they told him to add another parameter dontblock, i.e.
OurProgram.exe /param1=[InternalValue1] /param2=[InternalValue2] /dontblock
When MainApplication finds the dontblock parameter while parsing the command line, it knows that it need not wait for the called application to exit.
It works, but again, there is a catch. It now calls
OurProgram.exe /param1=John /param2=Doe /dontblock
and OurProgram complains that it does not know the parameter dontblock.
Great design!
|
|
|
|
|
Sounds like [Manufacturer] is passing the buck instead of having to deal with it...
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
Perhaps a better way would have been to use the start command. Do they not know DOS?
|
|
|
|
|
Everyone knows that if you are doing a lot of string concatenation in a loop, you absolutely must use a StringBuilder for performance!
I found something like this today (the original is in VB.net). It formats some data for a fixed-width text file.
StringBuilder result = new StringBuilder() ;
foreach ( DataRow dr in datatable.Rows )
{
result.AppendLine ( "".PadRight ( ' ' , 9 ) + dr [ 0 ].ToString().PadRight ( ' ' , 9 ) + "".PadRight ( ' ' , 9 ) + dr [ 1 ].ToString().PadRight ( ' ' , 9 ) ... et cetera et cetera et cetera ... the statement is nearly 800 characters long ... ) ;
}
return ( result.ToString() ) ;
I suspect that each resultant line is also nearly 800 characters long. I'm also fairly sure that the data table doesn't usually contain many rows, so percentage-wise not many concatenations are eliminated here.
As I'm new to the company, I chose the "it ain't broke, don't fix it" option. But I added comments suggesting that:
0) new string ( ' ' , 9 ) might perform better, is easier to read, and doesn't make the developer look stoooopid.
1) Proper use of a StringBuilder would definitely be a wise course of action.
Now, I would also like to ask your opinion... This code builds one big-A string in memory and passes it back where it is simply written to a file. Personally, I would pass in a stream and write to it as I go, eliminating the memory hoggage. Another option would be to use events -- raise an event for each line (or field) and the calling code can write it to the file, but I don't really like that here.
If you were writing a data to a fixed-width text file, what technique would you choose? ("Whatever the boss specifies" doesn't count.)
P.S. Don't get me started on how they generate XML... 
modified on Tuesday, November 2, 2010 12:05 AM
|
|
|
|
|
Agreed, but I usually like to see something like the following in these situations.
StringBuilder result = new StringBuilder() ;
foreach ( DataRow dr in datatable.Rows )
{
result.AppendLine( string.Format( "{0}{1}[2}",
new string( ' ', 9 ),
dr[0].ToString().PadRight( ' ', 9 ),
dr[1].ToString().PadRight( ' ', 9 ) );
}
return ( result.ToString() ) ;
However, if each data block is a fixed 9 character length, with a statement that is 800+ characters long, I would want to put some serious thought into an internal overridden StringBuilder that could simply handle each piece of data in a method call so that the redundant .ToString().PadRight... code could be cleaned up for readability. I would definitely fix this for a fixed-width text file to protect it.
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
PogoboyKramer wrote: .ToString().PadRight... code could be cleaned up for readability
Perhaps the following would work 
StringBuilder result = new StringBuilder();
foreach (DataRow dr in datatable.Rows)
{
result.AppendFormat("{0,9}{1,9}{2,9}", string.Empty, dr[0], dr[1]);
result.AppendLine();
}
return result.ToString();
Look at the Alignment Component section of this MSDN page
|
|
|
|
|
Definitely an improvement.
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
Or, you know, a for loop. Or maybe a function that accepts a data reader and an index. Or a data reader, an index, and a padding length (if it's not always 9). Or create an anonymous delegate to capture the data reader to avoid passing the same parameter over and over, then each function call would only require an index and a padding length. Or a function that takes an array of indexes ({0, 1, 2}) and a padding length. Or create a data type that allows indexes and padding lengths to be paired together, then functions applied to that data in one call. Or since they're all pairs and they're all the same data type (int), use a 2D array with an inline initializer rather than a custom data type.
|
|
|
|
|
Depends on the variability of the format and data, but I might go with something like this:
StringBuilder result = new StringBuilder();
int fieldCount = 40;
foreach (DataRow dr in datatable.Rows)
{
for(int i = 0; i < fieldCount; i++)
{
result.Append(" ");
result.Append(dr[i].ToString().PadRight(' ', 9 );
}
result.AppendLine();
}
return result.ToString();
If the data/format is more variable, I might come up with a more interesting solution. Also, I might change some minor things, depending on context. As a simple example, I might make the number of spaces (in the example shown, nine) a parameter in a function or a class-level constant if the scenario warrants it.
|
|
|
|
|
No, it's not always nine, and it's worse than what I presented.
|
|
|
|
|
PIEBALDconsult wrote:
If you were writing a data to a fixed-width text file, what technique would you choose?
I would definitely pass a TextWriter into that function. This way the caller could pass either a StringWriter (which uses a StringBuilder internally) or a StreamWriter depending on the target.
Together with some helper functions it would look like this:
public string GetAsText()
{
using (var writer = new StringWriter())
{
WriteTo(writer);
return writer.ToString();
}
}
public void WriteToFile(string path)
{
using (var writer = File.CreateText(path))
{
WriteTo(writer);
}
}
public void WriteTo(TextWriter writer)
{
foreach ( DataRow dr in datatable.Rows )
{
writer.WriteLine(...);
}
}
|
|
|
|
|
What I did, just for the sake of my own sanity, was write a FixedFileFormatter class. It can be used like so:
using
(
FixedFileFormatter f
=
new FixedFileFormatter
(
System.Console.Out
,
new FixedFileField ( 10 , Alignment.Left )
,
new FixedFileField ( 15 , Alignment.Right )
,
new FixedFileField ( 15 , Alignment.Center )
{
Format="X8"
}
,
new FixedFileField ( 10 , Alignment.Left )
)
)
{
f.WriteLine ( new object[] { "Hello, World!" , "Hello, World!" , 255 , 42} ) ;
}
|
|
|
|
|
Careful there, you are introducing seriously useful O-O concepts to a piece of code that was written by someone channeling a 'way-back' machine (a la Bullwinkle). And I totally agree.
|
|
|
|
|
PIEBALDconsult wrote: As I'm new to the company
PIEBALDconsult wrote: and doesn't make the developer look stoooopid.
No matter how right you are, you need to consider the social implications of what you say.
“If you want to build a ship, don't drum up people together to collect wood and don't assign them tasks and work, but rather teach them to long for the endless immensity of the sea”
- Antoine de Saint-Exupery
|
|
|
|
|
Which is why I said it here. 
|
|
|
|
|
Oh, sorry, I thought you said you made a comment in the code to that effect. Something that would surely have been noticed eventually.
“If you want to build a ship, don't drum up people together to collect wood and don't assign them tasks and work, but rather teach them to long for the endless immensity of the sea”
- Antoine de Saint-Exupery
|
|
|
|
|
The comment I added was much much tamer.
|
|
|
|
|
Some production VB.Net code I happened across and fixed (variable names and such changed to protect the innocent):
If number > 10 Or number < 30 Then
allowed = 1
End If
You should notice two things strange about that code. Also, that code to check a range was hardcoded right after code that looks up ranges in a database and performs the same check. Perhaps it was added by somebody without database permission? 
|
|
|
|
|
It could have been written before the db contained the validation or without knowledge of the previous. (I know I'm making assumptions and such. It's late and I'm ready to go to bed.)
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
The fact that an integer is being set to 1 or 0 rather than using a bool is a pretty good indicator that some of this code was copied from the database. Here is what my best guess is as to how this code evolved:
*Only single numbers were checked, so they were put in a database table.
*Next, certain ranges needed to be checked, and they were hard coded in the SP that checked single numbers.
*Somebody thought the SQL code looked ugly or needed to use the SP without ranges, so they moved that to the VB.Net.
*Somebody thought the VB.Net looked ugly, so they added ranges to the database table.
*At some point, there was some VB.Net copy/pasting so that hard coded logic stayed.
Result: 
|
|
|
|
|
aspdotnetdev wrote: integer is being set to 1 or 0 rather than using a bool is a pretty good indicator that some of this code was copied from the database
I would have agreed with you until I say some javascript web page logic a while back that was hand coded to work with 1s and 0s instead of booleans. Now I just think there are some pretty thick junior devs out there being allowed to write code that gets to prod without review.
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
