What are the worst programming habits?
-
I was thinking about the things that bug me and came up with a short list
- No comments. I know - let's have a religious war etc, but I find no comments dangerous.
- using o as a variable name. In fact using anything that's not sensible.
ctx,dr_rfp_ptr,i2 - Bad formatting. It's like walking into a house and being unable to sit down because of empty pizza boxes on the couch
- Mystery side-effects in code.
- Magic numbers
I'm guilty of 2 of these on occasion. What's your list?
cheers Chris Maunder
Things that bug me... 1. Comments. Too often have I seen comments that made no sense, were outdated, wrong or overly obvious. In fact I've learned to ignore comments as they've never helped me in any way. I guess programmers can't write English... 2. Code that is copy-pasted. Often the cause of bugs. 3. Swallowing exceptions. 4. Non-Object Oriented code in Object Oriented languages. 5. Formatting, you've said it. Bonus: 6. Not using brackets in if statements. Very dangerous... I'm working for a company that has worked with VB since the start. The first employees have worked with VB even longer and used Clipper before that. I've seen 1 through 5 all to often :sigh: I've come to hate 6 when we started doing C# and outsourced a project to another company. HORRIBLE!!! I read the code and didn't know if they meant it that way or if they had introduced subtle bugs... A lot of samples I get from the internet have it too. I probably forgot some stuff, but these are a few of my least-favourite things.
It's an OO world.
public class SanderRossel : Lazy<Person>
{
public void DoWork()
{
throw new NotSupportedException();
}
} -
I was thinking about the things that bug me and came up with a short list
- No comments. I know - let's have a religious war etc, but I find no comments dangerous.
- using o as a variable name. In fact using anything that's not sensible.
ctx,dr_rfp_ptr,i2 - Bad formatting. It's like walking into a house and being unable to sit down because of empty pizza boxes on the couch
- Mystery side-effects in code.
- Magic numbers
I'm guilty of 2 of these on occasion. What's your list?
cheers Chris Maunder
I like to mark other's code with the unsafe keyword. That's what it is for, after all. Edit: Really bad things: - Stringly typing - Similar to magic numbers: Literal values and strings all over the place instead of enums, resources or similar. - Spaghetti code, or totally insanely structured code - Global variables, including abuse of the session or caches - managing data in dozens of separate variables instead of using even the most primitive kind of entity - not understanding the framework, being unwilling to learn and contaminating the code with flawed homebrew solutions
The language is JavaScript. that of Mordor, which I will not utter here
I hold an A-7 computer expert classification, Commodore. I'm well acquainted with Dr. Daystrom's theories and discoveries. The basic design of all our ship's computers are JavaScript. -
I was thinking about the things that bug me and came up with a short list
- No comments. I know - let's have a religious war etc, but I find no comments dangerous.
- using o as a variable name. In fact using anything that's not sensible.
ctx,dr_rfp_ptr,i2 - Bad formatting. It's like walking into a house and being unable to sit down because of empty pizza boxes on the couch
- Mystery side-effects in code.
- Magic numbers
I'm guilty of 2 of these on occasion. What's your list?
cheers Chris Maunder
Ooh, good thread. Not ones I'm guilty of, but ones I call out in code reviews, that I haven't seen covered elsewhere in this thread: 0. Implementing code that's already part of the framework 1. Explicit path creation (rather than using Path.Combine) 2. Lines that are too long 3. Overly complex lines; these are normally ones that combine numerous statements together using ternary and coalescing null operators (it's a sure sign that someone has Resharper installed). 4. Talking about R#, converting clear code into a muddied part ordinary syntax, part LINQ abomination that you cannot remember what it does so you have to spend 20 minutes figuring out the damn thing. 5. Oh, and while we're at it with LINQ,
myList.Where(p=> p.Id == aUniqueId).FirstOrDefault();This one's a twofer - first of all, if it's a unique number, useSingleOrDefaultnotFirstOrDefault- you're only getting one value back. Secondly, learn how to use the power of LINQ, thatWherestatement is redundant so the statement can becomemyList.SingleOrDefault(p => p.Id == aUniqueId);6. Not checking inputs into methods for validity. 7. When testing code, not asserting that something has happened. 8. Unnecessary try/catch blocks. 9. Blindly consuming exceptions - I'll have noOn Error Resume Nextbehaviour please. 10. Not checking for null. I once saw a production system go boom because while the main code was properly checking nulls, the code that was reporting out that it couldn't do something because of null values didn't check the value out that it was attempting to log; thus blowing up the system. That's not all, but that should be enough to be getting on with. -
I like to mark other's code with the unsafe keyword. That's what it is for, after all. Edit: Really bad things: - Stringly typing - Similar to magic numbers: Literal values and strings all over the place instead of enums, resources or similar. - Spaghetti code, or totally insanely structured code - Global variables, including abuse of the session or caches - managing data in dozens of separate variables instead of using even the most primitive kind of entity - not understanding the framework, being unwilling to learn and contaminating the code with flawed homebrew solutions
The language is JavaScript. that of Mordor, which I will not utter here
I hold an A-7 computer expert classification, Commodore. I'm well acquainted with Dr. Daystrom's theories and discoveries. The basic design of all our ship's computers are JavaScript.CDP1802 wrote:
mark other's code with the unsafe
You might not be surprised to learn that I compile all my code with
/unsafe. :cool:You'll never get very far if all you do is follow instructions.
-
Dave Kreskowiak wrote:
In C# the default is private while in VB it's Public. I absolutely hate that and really dont want to have to remember what the defaults ars supposed to be when scanning over code for problems.
I hate that it's public in VB.NET too, but it does not change the way I look at C#. Having tried it, for several months, in both languages, to me, the benefit outweighs the possible disadvantage of confusion.
Bastard Programmer from Hell :suss: If you can't read my code, try converting it here[^]
What benefit? Saving 4 keystrokes?
A guide to posting questions on CodeProject
How to debug small programs
Dave Kreskowiak -
mark merrens wrote:
People that tell you their code is 'self-commenting'.
Sometimes, it is though.
// check if user is valid
if(IsUserValid(user))
{
// update the user
UpdateUser(user);
}
else
{
// show a messagebox with an error
MessageBox(error);
}In that snippet, the comments are sorta annoying.
Regards, Nish
Latest article: Using the Microsoft Azure Storage Client Library for C++ Blog: voidnish.wordpress.com
-
I was thinking about the things that bug me and came up with a short list
- No comments. I know - let's have a religious war etc, but I find no comments dangerous.
- using o as a variable name. In fact using anything that's not sensible.
ctx,dr_rfp_ptr,i2 - Bad formatting. It's like walking into a house and being unable to sit down because of empty pizza boxes on the couch
- Mystery side-effects in code.
- Magic numbers
I'm guilty of 2 of these on occasion. What's your list?
cheers Chris Maunder
-
Since I don't do this for a living anymore, my opinion probably doesn't count for much. But back when I did do it, the cost of maintaining code far exceeded the cost of developing it, and I considered a lack of meaningful comments grounds for termination. I still do. Others in my list would include leaving commented-out code in production source, and embedding numeric constants in code for use in calculations. I don't know if that last one is common anymore, but it used to drive me nuts, and I found it in a lot of code.
Will Rogers never met me.
Roger Wright wrote:
I don't know if that last one is common anymore, but it used to drive me nuts, and I found it in a lot of code.
Oh, it's still common! I found some code a while ago that the dev had obviously thought he'd done the right thing...
const int FiveHundred= 500;
Sure, re-factoring is easier (although it was only used in one place anyway) but not the most meaningfull names! (it was for a 1/2 second time out time)
PooperPig - Coming Soon
-
Adding oil to the fire, a practical example;
using System;
using System.Threading;
using System.Windows.Forms;
using System.Timers;namespace ConsoleApplication5
{
class Program
{
Timer t = new System.Threading.Timer(null); // will not compile, as it is unclear which Timer
Timer t2 = new System.Windows.Forms.Timer(); // is declared (as opposed to the type instantiated)
Timer pfld_SysTimrTimrt3 = new System.Timers.Timer(); // using hungarian systems with namespace prefixstatic void Main(global::System.String\[\]\[\] strSrgs) { global::System.Console.ReadLine(); } }}
And yes, the "console application template" has an entry point which is implicitly private.
Bastard Programmer from Hell :suss: If you can't read my code, try converting it here[^]
-
I do #2, when I specifically have to work with an
objectComments are my major bugbear: I enforce XML comments on all public methods (and add them to non-public ones) and have "warnings as errors" on, so I have to comment my methods as a bare minimum. The rest of the time, I reserve comments for where they are needed. 6) I hate comments that explain exactly what the code is telling you it is doing! I can read the code, dammit - I don't need you to putif (customer.IsAnIdiot)
{
// If the customer is an idiot then we need to handle it.- Out of date comments. This gets my goat. Comments are there to help, when the code is complicated and more explanation is needed. So if you change the damn code, change the damn comments! Or you will hear the sound of a soft cough behind you, and it'll be me, with the ClueBat... 8) Variables names that don't reflect the use and / or purpose. Leaving control names at the VS default for example... ClueBat time!
Those who fail to learn history are doomed to repeat it. --- George Santayana (December 16, 1863 – September 26, 1952) Those who fail to clear history are doomed to explain it. --- OriginalGriff (February 24, 1959 – ∞)
-
Not sealing classes by default/crazy overuse of inheritance Just because something needs something else doesn't mean it is a base class of that other thing, I think in a modern programming language you rarely actually need to use inheritance Also, Code that does nothing, but hasn't been taken out of the project, eugh I rarely comment my code unless I am doing something weird, I assume the next developer will be at least as smart as me, if not much much smarter (likely) I might use o as a variable name if I'm maybe inside a for loop inside another for loop (using i for the outer one), everybody should know what for(var i = 0; i < blah; i++) means, anything more descriptive is a waste of keystrokes Everything else I agree with
HomerTheGreat wrote:
I assume the next developer will be at least as smart as me, if not much much smarter (likely)
Not so. You have business knowledge that you will have accumulated over time. You shouldn't need to comment straight forward code to tell the future devs technically what it is doing, but to explain the business reasons behind it.
PooperPig - Coming Soon
-
Ooh, good thread. Not ones I'm guilty of, but ones I call out in code reviews, that I haven't seen covered elsewhere in this thread: 0. Implementing code that's already part of the framework 1. Explicit path creation (rather than using Path.Combine) 2. Lines that are too long 3. Overly complex lines; these are normally ones that combine numerous statements together using ternary and coalescing null operators (it's a sure sign that someone has Resharper installed). 4. Talking about R#, converting clear code into a muddied part ordinary syntax, part LINQ abomination that you cannot remember what it does so you have to spend 20 minutes figuring out the damn thing. 5. Oh, and while we're at it with LINQ,
myList.Where(p=> p.Id == aUniqueId).FirstOrDefault();This one's a twofer - first of all, if it's a unique number, useSingleOrDefaultnotFirstOrDefault- you're only getting one value back. Secondly, learn how to use the power of LINQ, thatWherestatement is redundant so the statement can becomemyList.SingleOrDefault(p => p.Id == aUniqueId);6. Not checking inputs into methods for validity. 7. When testing code, not asserting that something has happened. 8. Unnecessary try/catch blocks. 9. Blindly consuming exceptions - I'll have noOn Error Resume Nextbehaviour please. 10. Not checking for null. I once saw a production system go boom because while the main code was properly checking nulls, the code that was reporting out that it couldn't do something because of null values didn't check the value out that it was attempting to log; thus blowing up the system. That's not all, but that should be enough to be getting on with.Pete O'Hanlon wrote:
Secondly, learn how to use the power of LINQ, that
Wherestatement is redundant so the statement can becomemyList.SingleOrDefault(p => p.Id == aUniqueId);However, using Where(predicate).function rather than Function(predicate) is significantly faster. Incidentally a straight forward While loop is more efficient than either. So if efficiency is a concern (and we're not just talking close here, there's a big enough difference that it counts!) you need to be careful!
var found = collectionClass.FirstOrDefault(i => i.Field == searchValue);
or
2.
var found = collectionClass.Where(i => i.Field == searchValue).FirstOrDefault();or
3.
foreach(item in collectionClass)
{
if (item.Field = searchValue)
{
found = item;
break;
}
}The results for 100,000 collection with 100,000 searches? 1. 'normal' 78.5 1. parallel 32.2 2. 'normal' 51.2 2. parallel 31.9 3. 29.9
PooperPig - Coming Soon
-
I was thinking about the things that bug me and came up with a short list
- No comments. I know - let's have a religious war etc, but I find no comments dangerous.
- using o as a variable name. In fact using anything that's not sensible.
ctx,dr_rfp_ptr,i2 - Bad formatting. It's like walking into a house and being unable to sit down because of empty pizza boxes on the couch
- Mystery side-effects in code.
- Magic numbers
I'm guilty of 2 of these on occasion. What's your list?
cheers Chris Maunder
As part of No. 4 in your strangely numbered list (starting at 1!) 4a) Putting anything but the bare minimum in Setters/Getters - part of the project I am working on lives in permanent side-effect hell because some getters access the database to get values (and don't cache them) and some setters access otehr properties that access properties that access properties - and all of them have side effects 0. Method Names that don't match their function, or do more than their function e.g.
bool IsValid(Entity myEntity)
{
if (myEntity.Property = null) return false;myEntity.OtherProperty = SomeValue; DbService.Save(myEntity); OtherEntity= DbService.GetOtherEntity(myEntity.Property); return true;}
n.) Double-Negatives
if (!notSaved || !isInvalid)
n+1.) Any code that doesn't look like it would if I wrote it on a good day.
PooperPig - Coming Soon
-
Roger Wright wrote:
I don't know if that last one is common anymore, but it used to drive me nuts, and I found it in a lot of code.
Oh, it's still common! I found some code a while ago that the dev had obviously thought he'd done the right thing...
const int FiveHundred= 500;
Sure, re-factoring is easier (although it was only used in one place anyway) but not the most meaningfull names! (it was for a 1/2 second time out time)
PooperPig - Coming Soon
Well, he tried, as you say. I used to program automated test equipment for missile guidance systems, and each test station had to be initiated with a local gravity vector for its physical location. Some programmers simply hard-coded a three-valued constant into the code; DATA 0.00340120, 0.00002101, 32.16254301, or some such. Moving the machine to a new location meant recoding gravity at that point, but nothing in the code told what numbers were gravity. That's just one example, and there were many much worse.
Will Rogers never met me.
-
Well, he tried, as you say. I used to program automated test equipment for missile guidance systems, and each test station had to be initiated with a local gravity vector for its physical location. Some programmers simply hard-coded a three-valued constant into the code; DATA 0.00340120, 0.00002101, 32.16254301, or some such. Moving the machine to a new location meant recoding gravity at that point, but nothing in the code told what numbers were gravity. That's just one example, and there were many much worse.
Will Rogers never met me.
Reminded me of a story I heard about a dev who had written guidance software for tank aiming - the idea being the operator could identify a target and the software would move the barrel to track the object and fire when aimed. The story goes that the first time it was tried out on a real tank, the tank fired almost immediately - in entirely the wrong direction. Turned out that the software was full of literal values, and had been fudged during testing so the devs didn't have to wait while a virtual barrel turned laboriously around - and they'd missed a value when they took out the changes in the real McCoy! Not sure I believe it (as surely there'd need to be some feedback from the tank) but nice image of lots of brass and boffins ducking for cover!
PooperPig - Coming Soon
-
I do #2, when I specifically have to work with an
objectComments are my major bugbear: I enforce XML comments on all public methods (and add them to non-public ones) and have "warnings as errors" on, so I have to comment my methods as a bare minimum. The rest of the time, I reserve comments for where they are needed. 6) I hate comments that explain exactly what the code is telling you it is doing! I can read the code, dammit - I don't need you to putif (customer.IsAnIdiot)
{
// If the customer is an idiot then we need to handle it.- Out of date comments. This gets my goat. Comments are there to help, when the code is complicated and more explanation is needed. So if you change the damn code, change the damn comments! Or you will hear the sound of a soft cough behind you, and it'll be me, with the ClueBat... 8) Variables names that don't reflect the use and / or purpose. Leaving control names at the VS default for example... ClueBat time!
Those who fail to learn history are doomed to repeat it. --- George Santayana (December 16, 1863 – September 26, 1952) Those who fail to clear history are doomed to explain it. --- OriginalGriff (February 24, 1959 – ∞)
OriginalGriff wrote:
Leaving control names at the VS default
I do that sometimes, mostly with container controls I don't reference in the code. I often get weird build errors if I set the 'Create a variable' option (or whatever that option is called, no VS instance open right now to check) to false, so I don't usually do that. I have no idea why, and it only happens with certain types of controls (e.g. Panel, Table Layout Panel, etc.) VS is weird sometimes.
What do you get when you cross a joke with a rhetorical question?
-
Pete O'Hanlon wrote:
Secondly, learn how to use the power of LINQ, that
Wherestatement is redundant so the statement can becomemyList.SingleOrDefault(p => p.Id == aUniqueId);However, using Where(predicate).function rather than Function(predicate) is significantly faster. Incidentally a straight forward While loop is more efficient than either. So if efficiency is a concern (and we're not just talking close here, there's a big enough difference that it counts!) you need to be careful!
var found = collectionClass.FirstOrDefault(i => i.Field == searchValue);
or
2.
var found = collectionClass.Where(i => i.Field == searchValue).FirstOrDefault();or
3.
foreach(item in collectionClass)
{
if (item.Field = searchValue)
{
found = item;
break;
}
}The results for 100,000 collection with 100,000 searches? 1. 'normal' 78.5 1. parallel 32.2 2. 'normal' 51.2 2. parallel 31.9 3. 29.9
PooperPig - Coming Soon
How about the SingleOrDefault on the same machine?
Wrong is evil and must be defeated. - Jeff Ello[^]
-
Well, he tried, as you say. I used to program automated test equipment for missile guidance systems, and each test station had to be initiated with a local gravity vector for its physical location. Some programmers simply hard-coded a three-valued constant into the code; DATA 0.00340120, 0.00002101, 32.16254301, or some such. Moving the machine to a new location meant recoding gravity at that point, but nothing in the code told what numbers were gravity. That's just one example, and there were many much worse.
Will Rogers never met me.
-
PIEBALDconsult wrote:
the developer's intent should be clearly specified.
It IS clearly specified if it is omitted. It is not some arcane trick, it is not something that causes side-effects, and it improves readability. It is as usefull as typing "begin" and "end" instead of the default scope-blocks. It might take some getting used to, but it conveys the same amount of information using less symbols. That's kinda essential, and the reason why we are not programming in COBOL.
PIEBALDconsult wrote:
I don't want to have to guess
If you have to guess at the default access modifier in C#, you should not be writing in C#.
PIEBALDconsult wrote:
and decrease the hit to your own productivity caused by your juniors.
Should I prefix each class with a complete namespace? Otherwise they'd be guessing at which class it will take :D You explain a junior ONCE that everything that does not have a modifier is private. If they come asking, even once, then make them prefix everything. Using "this" and "that", using namespaces, using "global::". Throw in some hungarian systems, so they won't have to guess the type :suss:
Bastard Programmer from Hell :suss: If you can't read my code, try converting it here[^]
-
Reminded me of a story I heard about a dev who had written guidance software for tank aiming - the idea being the operator could identify a target and the software would move the barrel to track the object and fire when aimed. The story goes that the first time it was tried out on a real tank, the tank fired almost immediately - in entirely the wrong direction. Turned out that the software was full of literal values, and had been fudged during testing so the devs didn't have to wait while a virtual barrel turned laboriously around - and they'd missed a value when they took out the changes in the real McCoy! Not sure I believe it (as surely there'd need to be some feedback from the tank) but nice image of lots of brass and boffins ducking for cover!
PooperPig - Coming Soon
I don't have any trouble believing that story, as I've been part of worse. Back in the days when the Phalanx gun system was being developed by my company, at a demonstration session they fired it up in front of a bunch on Navy brass (unloaded, but still noisy as hell), it deployed from its clamshell mount, spun around and locked on the first available target - the bridge of the ship where all the brass stood watching. I suspect a lot of dress whites needed cleaning that afternoon, and I dearly wish I could have been there to see it. Happily, I was still in school when I started working at General Dynamics, and my Control Systems instructor was a part timer whose day job was chief engineer for Phalanx at General Dynamics. He was there, and told us all about it. Then he assigned us a 10 question take home, open book exam that contained all the field measurements, design equations for the control loop, and target classification algorithms, and required us to solve the problem and properly assign constraints and design compensation circuits to stabilize the control system. Apparently we managed to solve the problem for him, as the error never happened again, and I passed the class. That was the single, most difficult test I ever took, as it was all using real world data - not theoretical nonsense that textbooks present - and each question depended upon the answer to the one before it being correct - exactly how the real world works for an engineer. If you get step two wrong, in the real universe, everything that follows will also be wrong, though you might not notice that until step 28, years and million$ later. :-O
Will Rogers never met me.
