|
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.
|
|
|
|
|
I have seen people use ints with 1s and 0s because they learned programming with a language that did not have a boolean type. I say that is what you get when you have someone whose degree is not in programming write the software 
What is really scary is when folk with a real programming degree do the same thing! 
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
I'm currently ROFL-ing inside, guys!!! A true pearl.
Notice, that ANY int suits the condition )))
|
|
|
|
|
I must have really been tired last night not to catch that. I'm having the same laugh now.
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
Indeed, that was the real bug I fixed and one of the two things that was odd about the code (excluding the fact that it's VB.Net rather than C#, which is obviously a flaw too). 
|
|
|
|
|
aspdotnetdev wrote: VB.Net rather than C#
Very obviously...
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
Heh heh.. messed up boolean conditions are the bane of so many.. I think boolean logic is one of the hardest things to read well, and is one reason why I unit test my code thoroughly. Misused AND and OR is so easy to miss on code inspection.. but good tests never lie.
|
|
|
|
|
With that kind of range checking, the coder is more suited to checking the range where the burgers are being cooked.
Chris Meech
I am Canadian. [heard in a local bar]
In theory there is no difference between theory and practice. In practice there is. [Yogi Berra]
|
|
|
|
|
Either that, or being brought to the range to be slaughtered. Which wouldn't be much different, because he'd end up at the burger place anyway.
|
|
|
|
|
Shhh!!! Don't give John any ideas!!!
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
I was digging around some code I wrote three years ago, looking for the cause of a bug some reported, when I found this. And no wonder - the full code is so full of holes that I'm surprised it works as expected.
private static bool CheckSpecialCases(List<List<int>> adjancies, Dictionary<int, Region> regions)
{
if (regions.Count < 2) return true;
if (regions.Count == 2)
{
int key1 = 0, key2 = 0, inc = 0;
foreach (KeyValuePair<int, Region> kvp in regions)
{
if (inc == 0)
{
inc++;
key1 = kvp.Key;
}
else
key2 = kvp.Key;
}
lock (listLock)
{
if (adjancies[key1].Contains(key2))
return true;
adjancies[key1].Add(key2);
adjancies[key2].Add(key1);
return true;
}
}
return false;
}
Hmm, where to start. How about that comment "True if it was a special case". Yes, because it is obvious what a special case is. Then the whole looping over a collection of two items, with a flag to set two items to specific values (instead of say, asking for the keys of the Dictionary). Oh, and the whole reason for the reported bug is that the indices of key1 and key2 were not checked for being in bounds somewhere prior in the code, which would have saved user trouble from this. Plus, the redundancy of using lists to specify connections, while matching the underlying implementation of data that I was working with, makes things unnecessarily complicated.
|
|
|
|
|
Harsh! I hope the guy or gal who wrote this can take a little constructive criticism. 
|
|
|
|
|
jamie550 wrote: unnecessarily complicated
In OOP they call it "extendable". 
Greetings - Jacek
|
|
|
|
|
A company hired me for cleaning up the PHP-code the CEO created. The whole code reads like a how-not-to of programming, so I decided to post some of the gems here. This is the first one (I preserved all the formatting):
MyCart::IsContentvalid() checks items in the cart against a database. Each call does 3 queries (per item!) of tables with around 500.000 entries. Note also the clever use of quotation marks.
if(MyCart::IsContentvalid()==1)
{
Helper::Redirect("confirmation.php");
}
if(MyCart::IsContentvalid()==2)
{
$this->setErrorMsg("$curlang[error_msg1]");
}
if(MyCart::IsContentvalid()==-12)
{
$this->setErrorMsg("These items can not be sold.");
}
if(MyCart::IsContentvalid()==15)
{
$this->setErrorMsg("Your account doesn't allow you to buy these items.");
}
else
{
$this->setErrorMsg("$lang[error_msg2]");
}
Oh, and the use of else is only for advanced programmers:
if ($result == 1)
{
}
if ($result != 1)
{
}
More will come soon!
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
