|
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!
|
|
|
|
|
I **really** like the second one.
|
|
|
|
|
Maybe it's just because its a Friday afternoon but I had to laugh at the following code I just found for removing the first 7 chars of a string....
s = s.Replace(s.Substring(0,7), "");
... See the funny side? or is it just me?
Life goes very fast. Tomorrow, today is already yesterday.
|
|
|
|
|
|
That's a particular funny way of doing it wrong yes
|
|
|
|
|
s = "badcoder you creazy badcode?"
s = s.Replace(s.Substring(0,7), "");
(it's 1 am here)
Greetings - Jacek
|
|
|
|
|
dont you mean...
Life goes very fast. Tomorrow, today is already yesterday.
|
|
|
|
|
bool hasWarnings = false;
if (!hasWarnings)
{
hasWarnings = true;
}
|
|
|
|
|
I just saw a piece of javascript today which effectively looked like this:
function check_somethingOrOther()
{
someVariable = false;
setTimeout("check_somethingOrOther()", 500);
} Now, I know that someVariable was obviously meant to do something, but this is the only place in the code that it's actually used. Optimising this code was really simple.
|
|
|
|
|
Yes, but what if it changes. You have to make sure it is really false!
|
|
|
|
|
I actually removed this one from a piece of code this week. And believe it or not, it was actually being called.
private bool IsValid()
{
return true;
}
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
If it were a polymorphic function that could be overridden by a derived class then it could make sense. However, in your context it does seem very strange.
|
|
|
|
|
Unfortunately, no. This class was never inherited. I honestly don't think the guy who wrote it originally even knew what that was.
I wasn't, now I am, then I won't be anymore.
|
|
|
|
|
PogoboyKramer wrote: I actually removed this one from a piece of code this week. And believe it or not, it was actually being called.
You fool! Now *nothing* will be valid. Wasn't it better when it was ALL valid? How is anything supposed to work unless it's valid?
He said, "Boy I'm just old and lonely,
But thank you for your concern,
Here's wishing you a Happy New Year."
I wished him one back in return.
|
|
|
|
|
|
So, yeah, spent a few days (at night, as this is not for my day job) looking for a bug in this code...yep, the bug? Needed to place the return command "ret" after white space, ie. not at the start of a line. Side-effect? It doesn't return, just keeps running the next instruction in memory after ret.
DisplaySelectList:
cls()
;========================================
;
; Input:
; AppSelectListControl.TopPosition
; AppSelectListControl.ListItemsIndirect
; AppSelectListControl.LineStartAddys
;
;
; Start reading the LineStartAddys
; at the TopPosition position
;
; Set the TextInverse if the value in
; ListItemsIndirect at position is equal to
; SelectedItem value.
;
; Determine if TopPosition ptr is the first
; line of a list item, if so, then print
; that number.
;
; Print starting at the LineStartAddys[x]
; until NULL reached.
ld a, (AppSelectListControl.TopPosition)
ld (AppSelectListControl.PositionCount), a ; save to variable
;.breakpoint check_a_for_top_position
nop
LoopPositions:
;=======================================
; Check that PositionCount - TopPosition
; is not equal to 9, else stop printing
;=======================================
ld a, (AppSelectListControl.TopPosition)
ld b, a
ld a, (AppSelectListControl.PositionCount)
sub b
cp 9
.breakpoint check_z_for_reached_9_a_vs_b
;==============Base case displayed == 9 ? then stop
jp z, donePrintingSelectList
;=============================================
; Get the addy of the string and save to stack
;
;
;=============================================
ld de, AppSelectListControl.LineStartAddys ;calculate offset of array start
.breakpoint de_has_LineStartAddy
;calculate offset using TopPosition
xor a
ld h, a
ld a, (AppSelectListControl.PositionCount)
ld l, a
add hl, hl ;double because array is of 2byte items
add hl, de ; offset calculated
;.breakpoint check_hl_for_addy_of_LineStartAddys_of_index
;==============
;-->ld (hl), hl
;==============
ld c, (hl)
inc hl
ld b, (hl)
push bc ; Save the addy to list item's string
; to be used by print string.
;==========================
; Get next position's addy,
; could be 0000
;
;==========================
inc hl
ld a, (hl)
ld (AppSelectListControl.NextAddy), a
inc hl
ld a, (hl)
ld (AppSelectListControl.NextAddy+1), a
;reset hl back to ptr to addy of position
dec hl
dec hl
dec hl
.breakpoint just_pushed_bc_as_addy_string
;;===========================
;; Read value of addy into hl
;;===========================
;ld a, (de)
;ld l, a
;inc de
;ld a, (de)
;ld h, a
;;.breakpoint check_hl_has_value_of_listItem_string
;================================================
; if addy is null, then stop printing select list
;================================================
ld a, b
or a
;.breakpoint check_z_for_NULL_listitem_addy1
jp nz, {+} ; don't quit
ld a, c
or a
;.breakpoint check_z_for_NULL_listitem_addy2
jp nz, {+} ; don't quit
pop bc
;.breakpoint check_z_for_1
jp donePrintingSelectList
+:
;=====>base case: TopPosition == 0, consider this
; line the start so skip to that section
ld a, (AppSelectListControl.PositionCount)
;.breakpoint a_has_position_count
ld b, a
xor a
or b
jp nz, NextPosition
xor a
push af ; push 0 as list item number
jp printFormattedNumber
; b has current Position (initially was TopPosition)
NextPosition:
;check the value at PositionCounter - 1 in ListItemsIndirect
dec b
;.breakpoint check_b_for_positionCounter_minus_1
ld de, AppSelectListControl.ListItemsIndirect
;calc offset using TopPosition
xor a
ld h, a
ld l, b ; b has TopPosition - 1
add hl, de
ex de, hl
;.breakpoint check_de_for_addy_of_position_minus_1
; de has the address of the TopPosition - 1 index of
; the ListItemsIndirect array
ld a, (de) ; ld last position's list item number
;.breakpoint check_a_for_lastPositions_listItem_number
push af ; save last list item number
inc de
ld a, (de) ; de is at Position now
ld (AppSelectListControl.ListItemPtr), a ; save for later checking highlight status
;.breakpoint check_listItemPtr_and_de
ld b, a ; ld Position's list item number
;.breakpoint check_b_for_listItem_number
pop af ; get last position's list item number
cp b ; are they the same?
;.breakpoint check_z_for_are_they_same
jp z, printString ; jump past code for making formatted number
push bc
printFormattedNumber:
;===============================================
; Print a formatted number based on list item on
; the stack. (upper-byte)
;===============================================
pop af ; get list item number
.breakpoint check_a_for_listCount1
inc a
;.breakpoint check_a_for_listCount2
;.breakpoint a_has_listItem_number
call Convert.AToDecimalText ; hl will point to decimal text w/ null
;.breakpoint check_hl_addy_has_formatted_number
;======================
; Print each character
;======================
res textInverse, (iy + textFlags) ; do not print highlight
-:
ld a, (hl)
; Check for NULL
xor b
cp b
jp z, {+}
ld a, (hl)
bcall(_VPutMap)
inc hl
jp {-}
+:
;.breakpoint printed_formatted_number
ld a, '.'
bcall(_VPutMap)
ld a, ' '
bcall(_VPutMap)
ld a, ' '
bcall(_VPutMap)
ld a, ' '
bcall(_VPutMap)
printString:
;==============================
; set the text inverse
; if listCount == selectedItem
;==============================
ld a, (AppSelectListControl.ListItemPtr)
ld b, a
ld a, (AppSelectListControl.SelectedItem)
;.breakpoint check_a_for_listcount_and_b_for_selectedItem
cp b
jp z, SetHighlight
res textInverse, (iy + textFlags)
jp doneHighlight
setHighlight:
nop
;.breakpoint Setting_highlight
set textInverse, (iy + textFlags)
doneHighlight:
;=================================
; Print the string starting at the
; addy found on the stack
;=================================
pop hl
;.breakpoint printString_start_hl_has_addy
nop
;========================================
;Get the next line break to know when to
; stop printing the string.
;========================================
-:
ld a, (hl)
xor b
cp b
jp z, {+} ; quit if null reached
;========
; quit if hl matches addy of next line break
ld a, (AppSelectListControl.NextAddy)
ld e, a
ld a, (AppSelectListControl.NextAddy+1)
ld d, a
;.breakpoint check_de_for_next_addy
ld a, l
cp e
jp nz, {++}
ld a, h
cp d
jp nz, {++}
jp {+}
++:
ld a, (hl)
;.breakpoint check_hl_as_chr_ptr
bcall(_VPutMap)
inc hl
jp {-}
+:
;.breakpoint done_printing_this_line
;==================
; NewLine operation
;==================
ld a, (penrow)
ld b, 7
add a, b
ld (penrow), a
xor a
ld (pencol), a
;========================
; Increment PositionCount
; then check if addy
;========================
inc_byte(AppSelectListControl.PositionCount)
;.breakpoint moved_to_next_position
jp LoopPositions
donePrintingSelectList:
.breakpoint done_printing_select_list
nop
ret < ------- NEEDS SPACE IN-FRONT OF IT TO WORK...WTF
|
|
|
|
|
I've seen a similar bug caused by a compiler not behaving correctly when the last line in a file doesn't end in a newline character.
Steve
|
|
|
|
|
Had that with early Visual C++ and .h files: no newline == massive fail by compiler.
Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together.
|
|
|
|
|
Ah! You have brightened up a dull grey Thursday - it has been a few years since I looked at any Z80 code.
It's a bugger when you miss that an instruction is being treated as a label.
Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together.
|
|
|
|
|
Its interesting to see how my suffering helps others.
|
|
|
|
|