|
Hmm.. OK fine, I'll accept that excuse.
This time.
|
|
|
|
|
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
|
|
|
|
|
Misspelled identifiers.
Inconsistent naming for related items.
C++ header files that group things by their access method (public:, protected:, private:) rather than putting related items together.
Hungarian notation should die in a fire.
Software Zen: delete this;
|
|
|
|
|
|
Hmm. I agree with Joel but I stand by my dislike of the common Hungarian notation, despite Simonyi's original intent.
Software Zen: delete this;
|
|
|
|
|
Thank you, that's a very interesting article - I never new the Hunagrian notation was meant to be like that.
And I actually like it: It could be an easy way to improve the legibility of old legacy code by means of a simple Rename, and it could be done incrementally, at our own pace, without need to set aside extra time for refactoring. 
GOTOs are a bit like wire coat hangers: they tend to breed in the darkness, such that where there once were few, eventually there are many, and the program's architecture collapses beneath them. (Fran Poretto)
|
|
|
|
|
Comments like this:
i=6; //set i to six
modified 20-Oct-19 21:02pm.
|
|
|
|
|
My main bug bears are
1. Excessive whitespace between code lines
2. In code comment,for that complex method that are so short they are as useful as the var keyword.
Every day, thousands of innocent plants are killed by vegetarians.
Help end the violence EAT BACON
|
|
|
|
|
1. incorrect bracket placement
2. if (2 == var)
3. not enough whitespace
4. globals
|
|
|
|
|
Currently dealing with a codebase that uses magic numbers and tucks them into the window's hMenu member (and uses them from there)! Ghhgh!
The other nightmare offense in the code is using "m_" for:
a) Some, but not all, member variables.
b) Some, but not all, in function variables, including loop indexes.
c) Some, but not all, passed function parameters.
That was fun to get sorted - NOT. There were even a couple places where the same name was used in two or more different ways, so a project-wide replace would blow up.
|
|
|
|
|
|
-2. Swallowing an exception
-1. Throwing an exception with no message and just the generic "Exception" class.
0. Using try-catch for normal program flow..
(And these are just the rules to the exception )
|
|
|
|
|
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 
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 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. 
You'll never get very far if all you do is follow instructions.
|
|
|
|
|
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, use SingleOrDefault not FirstOrDefault - you're only getting one value back. Secondly, learn how to use the power of LINQ, that Where statement is redundant so the statement can become myList.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 no On Error Resume Next behaviour 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 Where statement is redundant so the statement can become myList.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!
1.
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?
|
|
|
|
|
I'm not sure what you mean - the timings were all on the same machine.
PooperPig - Coming Soon
|
|
|
|
|
I was just curious on the performance of SingleOrDefault vs FirstOrDefault as per Petes suggestion.
|
|
|
|
|
Oh - I see. I didn't do that - the figures were from a test I did some time ago when someone kept going through our code base changing all the .where(predicate).something() to .something(predicate) because they said it was more efficient - which I didn't think was the case.
PooperPig - Coming Soon
|
|
|
|
|
Willy nilly #including in large code bases. Then you get...
Q) Why does my build take an hour?
A) Because every source now indirectly includes every header
|
|
|
|
|
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
|
|
|
|
|
Team work often brings out the best (and the worst) in people. My peeves are about devs who indulge in:
1. Sending an email to the dev in the next cubicle instead of simply having a chat
2. Refusing to commit code until it is "perfect"
3. Making working code not work in the name of "refactoring"
4. Spending a week perfecting the latest LINQ statement and being unable to debug or optimise the thing
5. Deciding mid-project to change data access
6. Bitching about FXCop
7. Logging? What logging?
8. Hubris
9. Read the spec - real devs don't read specs!
My two fav's are: Inline braces and swallowing exceptions
|
|
|
|
|
Charl wrote: 2. Refusing to commit code until it is "perfect"
There's another side to this. Excessive check ins. You don't need to check in every single change every time you make the change - wait until you have a logical break and then check it in. I've seen people check in every 5 minutes, just checking in things like colour or border thickness changes. It wouldn't be so bad if the next checkin wasn't for the next colour.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
