Ugly, ugly, ugly!
-
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.
/// <summary>
///
/// </summary>
/// <param name="adjancies"></param>
/// <param name="regions"></param>
/// <returns>True if was a special case.</returns>
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.
-
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.
/// <summary>
///
/// </summary>
/// <param name="adjancies"></param>
/// <param name="regions"></param>
/// <returns>True if was a special case.</returns>
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. ;)
-
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.
/// <summary>
///
/// </summary>
/// <param name="adjancies"></param>
/// <param name="regions"></param>
/// <returns>True if was a special case.</returns>
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.