|
Yeah, you get what you reward for, but rewarding for bug fixes is really bad.
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
It would be okay if either the development was done in the past (so not influenced by the reward), or you only got a reward if the bug you fixed wasn't introduced by your team. But yeah otherwise that is a really stupid thing to measure.
I know it's really vague but basing rewards on customer satisfaction is really the way to go. Basing it on any particular code-related metric will just result in developers working to that metric and ignoring the actual end product.
|
|
|
|
|
A friend of mine used to work at a place that had a similar situation. The powers that be had decided to tie the group's bonus structure to the number of cases/bugs that were closed. So what did that encourage? Of course, all the project managers were closing bugs left, right, and center (fixed or not), and QA would just open a brand new one ... pretty much a copy/paste of the old.
It was a great team building exercise that everybody could get excited about.
|
|
|
|
|
My choice: payment per hour + a realistic manhour estimation + a boss who plays fair and understands that things change + trusted, honest employees + a pizza-day at least once a week.
Greetings - Jacek
|
|
|
|
|
I would not be surprised if this code actually ran faster than most other schemes. But, for a battery gauge?
|
|
|
|
|
While debugging a legacy stored proc from one of our systems, I've come across this:
CREATE PROCEDURE p_create_po_serial (
IN param_reception_ids VARCHAR(255),
IN param_serial_nos VARCHAR(1023),
OUT serial_no_ids VARCHAR(1023))
BEGIN
DECLARE id_to_parse VARCHAR(255);
DECLARE temp_reception_ids VARCHAR(255);
DECLARE c*** INT;
SET temp_reception_ids = param_reception_ids;
SET serial_no_ids = '';
SET c*** = 0;
WHILE (length(temp_reception_ids) > 0) DO
SET id_to_parse = SUBSTRING_INDEX(temp_reception_ids, ';', 1);
SET serial_no_ids = concat(serial_no_ids, '-', id_to_parse);
<Some 200 more lines, in which 'c***' makes absolutely no appearance whatsoever>
SET temp_reception_ids = SUBSTRING(temp_reception_ids, length(id_to_parse) + 2, length(temp_reception_ids));
SELECT id_to_parse, temp_reception_ids;
SET c*** = c*** + 1;
END WHILE;
END;
No variable or method names have been replaced. That would've taken away all the beauty of it.
Now, what I've actually been amused exactly, is that besides the funny (intended or not) misspelling of 'count' - and yes, I know CP will replace the variable name with '*', is that absolutely no-one knows what the counter was supposed to be used for. Counting the while steps maybe?
However, on the grander scheme of things, this proc is actually quite ok, and it even has comments (and yes, they were useful)!! And what it was supposed to do was this:
- Take a string parameter that contained some IDs separated by ';'
- Take each id from that string and do some selects using that id as parameter
- Extract data from those selects and do some inserts / updates with it, depending on some other conditions
But again, that count, and its associated comment, just made my day!
Full-fledged Java/.NET lover, full-fledged PHP hater.
Full-fledged Google/Microsoft lover, full-fledged Apple hater.
Full-fledged Skype lover, full-fledged YM hater.
|
|
|
|
|
That is defensive coding; adding a "just in case you need it" counter.
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
|
CIDev wrote: defensive offensive coding
FTFY
|
|
|
|
|
Andrei Straut wrote: Counting the while steps maybe That would be my guess. I have used counters when debugging code to count the number of times I went through a loop and append that number to a string so I could track the progress. It made it easier to find the offending record.
The only thing I can think of is that it was suggested to this particular developer to use a counter for this reason but the developer didn't understand why they were being told to do this.
It was broke, so I fixed it.
|
|
|
|
|
Hi all,
I was already reading this forum when it was still called the "Hall of shame".
I've got a product to maintain that was created by a developer who is no longer working for us.
Even though I've already seen plenty of "hall of shame worthy" code there, I've have never posted it here, because it would have been my first post on CP ever and I thought it is a bad idea to say hello to a community while bashing an ex colleague.
However, I came across a piece of code that I can't keep for myself and so I'll do what I didn't want to do and bash in my first post:
At the end of a long method I found this one:
try
{
return true;
}
catch (Exception)
{
return false;
}
Cheers,
cmger
|
|
|
|
|
Welcome to posting on CP.
I hope that there used to be more code in the try catch that would give it a reason to be there. Even if there used to be code that justified the try, when that code was removed the try/catch should have also been removed.
This is another fine example of code that might work, but really isn't good code, hence my sig.
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
Thanks for the welcome!
Nope, there was never anything more in there. At least as far as I can tell from the source control system.
I found out that there is only one place where the method is used and there the return value is just ignored. I re-factored the method to return void an removed the block completely. Now there is only the code that preceded the try catch.
|
|
|
|
|
Maybe it was some kind of skeleton for logic to be inserted in the try-catch block later? However, it should've been preceded by a comment at least.
cmger wrote: I found out that there is only one place where the method is used and there the return value is just ignored.
Rejoice, for you got away easily! It could've been a lot worse
Full-fledged Java/.NET lover, full-fledged PHP hater.
Full-fledged Google/Microsoft lover, full-fledged Apple hater.
Full-fledged Skype lover, full-fledged YM hater.
|
|
|
|
|
I am not surprised that there was never any useful code there, just disappointed.
I really hate it when programmers return a value that serves no purpose.
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
Oh, then you would really love the code base where the above stems from,...
Not!
|
|
|
|
|
cmger Wrote: Oh, then you would really love the code base where the above stems from,...
Not!
Sounds like you have a lot more potential entries for this forum.
Just because the code works, it doesn't mean that it is good code.
|
|
|
|
|
Looks more like a stub - that was never filled out.
|
|
|
|
|
My guess is that this was "skeleton" code that the coder copy pasted but forgot to fill.
|
|
|
|
|
Just saw this posted on another forum
List<char> _chars = new List<char>();
for (int i = 0; i < 2000; i++)
{
if ((i >= 48 && i <= 57) || (i >= 97 && i <= 122))
{
_chars.Add((char)i);
}
}
The author claimed that the code "work efficiently". I wonder how the upper limit of 2000 was determined? Why not int.MaxValue? Then you could maximise the inefficiency. Of course, you could do better with a long, or a lower bound of int.MinValue but that's getting silly.
The way _chars was used later all that was required was
string _chars = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
|
|
|
|
|
Reminds me of the old classic QA response "My code is correct, but it's not working"
|
|
|
|
|
Char.IsLetterOrDigit might have been a good starting point. Could have wasted hours of the authors time testing to see which was the more efficient
Interested though if it was also a bug, 97 to 122 are all lowercase, not uppercase.
"You get that on the big jobs."
|
|
|
|
|
Well obviously it is there "for future expansion".
Of the alphabet, I assume...
Ideological Purity is no substitute for being able to stick your thumb down a pipe to stop the water
|
|
|
|
|
¥ɛѦ, tĥѦτ ѡĭ└└ ɳɛvɛr ĥѦῤῤɛɳ‼
|
|
|
|
|
I'm gonna need a bigger keyboard...
Ideological Purity is no substitute for being able to stick your thumb down a pipe to stop the water
|
|
|
|