Code You Should Not Be Writing – Part 6

 Languages  No Responses »
Jan 072010
 

I want to bring in the new year with this code that doesn’t seem all very harmful, but could be written in a more elegant way. I just hope this will educate people on some of the new functional additions to C#.

[sourcecode language="csharp"]
public List<int> GetListOfIntegers()
{
List<int> list = new List<int>();
for(int i = 0; i < 24; i++)
{
list.Add(i);
}
return list;
}
[/sourcecode]

At its very core essence, this is simply creating a list of numbers from 0 to 23. However, try to find out the various possibilities of mistakes one can make with this code. I’ll leave it as an exercise of the reader to figure out all the possibilities of mistakes in this code. (side note: In fact, there was an error made in the original code. Yes, it looks simple, yet error-prone.)

Here’s a simpler and elegant way to do something like the above.

[sourcecode language="csharp"]
public List<int> GetListOfIntegers()
{
Enumerable.Range(0, 24).ToList();
}
[/sourcecode]

One of the interesting things with Enumerable.Range is it returns an IEnumerable. This effectively generates the numbers lazily. However in this example, in order not to change the signature of the method, ToList() is called to generate the numbers instantly, instead of lazily.

With ranges, you can create various interesting sets of numbers with Linq in a more functional way. I hope this week’s “Code You Should Not Be Writing” helps someone out there.

Code You Should Not Be Writing – Part 5

 Languages  2 Responses »
Dec 312009
 

This is my last post of the year, and I hope to make it special by giving you a piece of code that just amazes me. Staying within the whole “new year” feel, this code is for validating a date.

[sourcecode language="csharp"]
string strMessage = string.Empty;
int index = 0;
bool IsInputValid = true;

string strDate = TextBox_Date.Text; // TextBox_Date is just a textbox to get the date from.
DateTime date;
if (strDate.Length == 10)
{
int year = Int32.Parse(strDate.Substring(6, 4));
if ((year > 1900) && (year < 3000))
{
string strMonth = strDate.Substring(3, 2);
string strDay = strDate.Substring(0, 2);
date = Convert.ToDateTime(strMonth + "-" + strDay + "-" + strDate.Substring(6, 4));
}
else
{
index++;
strMessage += index + ". Date format(dd/mm/yyyy) is not correct.<br/>";
IsInputValid = false;
}
}
else
{
index++;
strMessage += index + ". Date format(dd/mm/yyyy) is not correct.<br/>";
IsInputValid = false;
}
[/sourcecode]

Remember the code you saw in Part 2 about the try-catch, using exceptions to do your data validation? Yes, that code surrounds this code. Imagine the double whammy I got when I saw this code, and the satisfaction of deleting it all.

I don’t know if you realise this, but do you have to take time to actually try to figure out what this developer is trying to do? As in understanding the code? How long do you take to figure out this code and what it is doing, and how flawed it is?

I hope you enjoy this edition of Code You Should Not Be Writing. Till next year, I have more to share! :)

Code You Should Not Be Writing – Part 4

 Languages  2 Responses »
Dec 242009
 

This is a special Christmas Eve “Code You Should Not Be Writing” post that I think is special enough to flip over your chair.

This is a common mistake C# developers make, which is forgiveable. You should use string.IsNullOrEmpty() method instead, which was introduced only in .NET Framework 2.0. It is understandable that most developers haven’t gotten up to speed yet.

[sourcecode language="csharp"]
string duration;
// … some code assignment for duration
if (duration == string.Empty || duration == null)
{
// … some error handling code
}
[/sourcecode]

But this just blew my mind. Can anyone guess why this is so very wrong?

[sourcecode language="csharp"]
string finalTitle;
// … some code assignment for finalTitle
if (finalTitle == string.Empty == null)
{
// … some error handling code
}
[/sourcecode]

Yes. You should never write code like this. Heaven’s me, this is valid code! The condition is just WRONG.

Merry Christmas everyone, and I hope you like this edition of “Code You Should Not Be Writing”.

Code You Should Not Be Writing – Part 3

 Languages  3 Responses »
Dec 172009
 

Well, this is part 3 already. I hope you’ve enjoyed and appreciated the last 2 part and understand my pain. Well, here’s something that doesn’t seem all too convoluted, but could have been achieved with just 1 line of code. By the way, just for those who don’t know, there isn’t really anything syntactically wrong with any of these code.

[sourcecode language="csharp"]
for (int i = 0; i < 24; i++)
{
string strIndex;
if (i < 10)
{
strIndex = "0" + i.ToString();
}
else
{
strIndex = i.ToString();
}
// … do more stuff with strIndex
}
[/sourcecode]

It’s slightly tougher to understand why this is bad code for those inexperienced. For those you understand why you should not write this code, good for you! You’ve got some developer sense in you. Till the next part, I hope you enjoy this code. Learn not to write like that, please.

Code You Should Not Be Writing – Part 2

 Languages  3 Responses »
Dec 102009
 

I hope this series becomes popular enough for bad developers to notice code like this. Because these code is really what’s out that! I’m serious! Next up, abuse of try-catch. This is how type validation is done. Wonderful, isn’t it? And look at the wonderful booleans associated with the try-catch logic? Doesn’t that just make you cringe?

[sourcecode language="csharp"]
DateTime newDateStart;
DateTime newDateEnd;
string errorMessage;
int index = 0;
bool isInputValid = true;
bool isSingleDayLeave = true;

try
{
// dateStart is a string assigned from above this code snippet
newDateStart = Convert.ToDateTime(dateStart);
}
catch
{
index++;
errorMessage += index + ". Date start should be given.<br />";
isInputValid = false;
}

try
{
// dateEnd is a string assigned from above this code snippet
newDateEnd = Convert.ToDateTime(dateEnd);
isSingleDayLeave = false;
}
catch
{
isSingleDayLeave = true;
}
// … more of similar code below.
[/sourcecode]

Code You Should Not Be Writing – Part 1

 Languages  10 Responses »
Dec 032009
 

I’ve enough fixing crap like this and I hope these series will give you a sense of what bad code is. This is what I encounter EVERYDAY in my life right now and I’m getting very sick and tired of this. You’ve seen me lament about it on twitter, posted some code on Facebook. This time, I’m going to log this down as a blog post series. Here’s the first code snippet from my long list of other code snippets. Having fun with booleans.

[sourcecode language="csharp"]
bool x;
bool y;
// … some code to assign y
if (y == true)
{
x = true;
}
else
{
x = false;
}
[/sourcecode]

This is ACTUAL CODE. I’m NOT KIDDING YOU. This is what I see in production code. I didn’t leave out any additional code except those in comments.

[sourcecode language="csharp"]
bool isReady = false;
if(isReady)
{
// … do a bunch of stuff
}
else
{
// … do other bunch of stuff
}
[/sourcecode]

You want more? I almost flipped when I saw this code.

[sourcecode language="csharp"]
if(1 == 1)
{
// … do a bunch of stuff
}
else
{
// … do a bunch of other stuff
}
[/sourcecode]