Note to community

Quote:

---

Originally Posted by FunkyDexter

Ay, there's the rub. My comments should be taken in an "ideal world" scenario which, of course, isn't very realistic, particularly when we're talking refactoring rather than writing from new.

I'll take working code delivered on time before I'll take elegant code delivered late and I'm also a firm believer in "if it aint broke, don't fix it". There was a thread floating around about a year or so ago which proposed that you should be constantly refactoring your code. I thought that was insane and just a recipe for introducing instability. You should refactor as rarely as possible and, when you do, keep your touch as light as possible. If adding some comments is a shortcut to clarifying archaic but stable code then I'd take the comments in preference to refactoring.

Fight! Fight! Fight! Fight!:)

---

Consider unit testing.

Quote:

---

Originally Posted by jmcilhinney

Consider unit testing.

---

Indeed, you should never be afraid to refactor. Unit tests are your safety net.

Quote:

---

Consider unit testing.

---

For new code, yes, definitely.

The problem is when you come across an undocumented method written several years previously and it's not obvious what it does or what side effects it has. As you gain some understanding of it adding a few explanatory comments is useful to the next developer (or possibly yourself) but refactoring it, even if you precede that with writing a unit test for what you think it's doing, is a bad idea unless there's going to be an end benefit that outweighs the risk.

I remember refactoring an ugly method in one of my first development jobs. I can't remember the details now but basicaly this method was meant to return information about an order or customer or some such. The original implementation set and used some global variable which it really didn't need to set to achieve it's stated goal so when I rewrote it I left that out. I bet you can guess what happened next...:blush:

Quote:

---

Indeed, you should never be afraid to refactor

---

You should always be afraid to refactor. Unit tests, TDD etc. help but there's always a risk.

Quote:

---

Originally Posted by dunfiddlin

If only MS had given us something like a flow layout panel or a table layou .... oh!

---

fwiw - those are useless controls if you are going to do something like "add" or "remove" content during runtime.

I had to re-invent my own with x/y and size-fitting logic that actually worked and worked smoothly...

Quote:

---

Originally Posted by FunkyDexter

The original implementation set and used some global variable which it really didn't need to set to achieve it's stated goal so when I rewrote it I left that out. I bet you can guess what happened next...:blush:

---

Well that's half your problem right there. What you did wasn't refactoring, it was rewriting. And you also deliberately changed the externally visible behaviour of the unit (by not writing to the global state).

Quote:

---

Originally Posted by Evil_Giraffe

Indeed, you should never be afraid to refactor. Unit tests are your safety net.

---

Not here...changing working code to refactor it is not that easy. First you would have to justify why you are spending corporate resources on something that works. After you get it unit tested then the fun begins. At least an hours’ worth of turnover documentation starts it out. Write up the test plans for the business side and get them to test in UA. Do all the testing in staging again. Then schedule and perform post production move testing.

I don't understand what all the resistance to comments I seem to be hearing is. Post #60 seems to bring us full circle. We have all voiced some personal feelings about comments and that's fine. During this thread I've been researching what the industry "experts" say about it. The majority consider them desired. Many of the points brought up here about useless comments can be seen and writing code to be self-documenting also. I think across the board comments are recommended.

At any hoot sometimes I get the feeling there are more people here writing code as contractors, consultants, entrepreneurs and not that many firmly entrenched in corporate day to day IT support. It seems like me, me, me versus what is good for the ones paying for us. I might be all wet about that but it is my impression after many years at this site.

Please don't take that the wrong way. I love this site and have gotten out more than I'll ever put back.

I think that we may have got a bit carried away with the refactoring. I would interpret the original statement:

Quote:

---

you should be constantly refactoring your code

---

to mean the code you're currently working on. I wouldn't consider opening up the project for a production application just to see if I could refactor it and I'm not sure that many people would recommend it. Current projects are a different matter though.

Quote:

---

Well that's half your problem right there. What you did wasn't refactoring, it was rewriting. And you also deliberately changed the externally visible behaviour of the unit (by not writing to the global state).

---

I may have changed it's extrenal behaviour but I didn't change it's contract with it's consumer. It's contract was to return the information and setting the global was not part of that, it was originally just an implementation detail. The problem was that successive developers had used the incidental change in global state, when that change ceased to occur their assumptions, and code, subsequently failed.

Now think of that in the context of TDD. The purpose for which the method was originally written did not include setting that variable so the developer would not have written a test for that. Why would they? It was not part of the contract their method was writing for the consumer. TDD applied to the method would not have caught the change. Applied to the methods of the consumers it might have, but that may be outside your scope as a developer for the core method.

And that's the point about refactoring, it's always dangerous. it always has been and it always will be. The whole arc of the growth of software development best practices is about minimising that risk. Not using IN/OUT variables, scoping variable as low as possible, separating update functionality from access functionality (subs vs functions), TDD, systems testing, commenting code... you name it. We do all those things in an attempt to minimise the risk of code breaking due to change. They all help but none is a silver bullet. And not refactoring code "for the hell of it" is just another of those measures. If there will be a genuine benefit from the refactoring and if it will outweigh the risk introduced then it's worthwhile, but refactoring for the purpose of one's own ego (which seems to happen a staggering amount) is bad practice.

Quote:

---

I wouldn't consider opening up the project for a production application just to see if I could refactor it and I'm not sure that many people would recommend it.

---

I think we're basically on the same page. If I was being really pedantic I'd say that I wouldn't refactor my current code without applying the "risk vs reward" calculation either. Actually, that's not strictly true. I do it all the time on my own home spun projects because, well, that would be my ego creeping in. But I never seem to finish any of my home projects and I think that's one of the main reasons. On a client's project my impetus is to deliver a functional system in a reasonable time frame. Elegant, well written code is, of course, still important but it's secondary.

Hmm. Interesting. The no-commenters produce the most comments in support of their decision not to comment. Great paradox in this I see! (Why haven't we got a Yoda smiley?)

I'd like to respond to this point made in EG's code response to TG's example:

Code:

---

11.' "If there are unsuppressed messages, ..."

12.' Well, how about writing the code exactly saying that, instead of obscuring it with counting and then
13.' comparing that count with some other value?

14.If unsuppressedMessages.Any Then

---

As an answer to the question, how about: Because using .Any is about 15x slower than comparing .Count >0. Perhaps it doesn't matter since they are both quite fast, but here's a variation on a saying you have probably heard: Look after the milliseconds and the seconds will look after themselves.

Quote:

---

Originally Posted by Shaggy Hiker

I'd like to respond to this point made in EG's code response to TG's example:

Code:

---

11.' "If there are unsuppressed messages, ..."

12.' Well, how about writing the code exactly saying that, instead of obscuring it with counting and then
13.' comparing that count with some other value?

14.If unsuppressedMessages.Any Then

---

As an answer to the question, how about: Because using .Any is about 15x slower than comparing .Count >0. Perhaps it doesn't matter since they are both quite fast, but here's a variation on a saying you have probably heard: Look after the milliseconds and the seconds will look after themselves.

---

Not only that but Any didn't exist before .NET 3.5 and some people may not know what Any does anyway. It may be obvious to some but it may not to others. That's part of what commenting is about: things seem obvious to you when you write them so you don't comment them but they may not seem obvious to others or even yourself some time later. I'm sure everyone has written what they thought was obvious code at the time and has then come back later and wondered what it was all about. Trying to write all good, self-documenting code is something to aspire to but not something that always happens in practice. If you comment your code then it's purpose is less likely to be obscured.

Quote:

---

Originally Posted by Shaggy Hiker

As an answer to the question, how about: Because using .Any is about 15x slower than comparing .Count >0. Perhaps it doesn't matter since they are both quite fast, but here's a variation on a saying you have probably heard: Look after the milliseconds and the seconds will look after themselves.

---

Disagreed. The Any function captures the intent of the logic much more clearly (which is why it was the language used in the comment). I would rather capture intent than speculatively micro-optimise.

Before I micro-optimise at the expense of readability, I need to see performance data showing that there is a performance problem, showing that this particular piece of code is the bottleneck and have exhausted opportunities for macro-optimisation.

But if you really must have those micro-seconds back, here:

Code:

---

If ThereAreAny(unsuppressedMessages) Then

' ...

Private Function ThereAreAny(Of T)(list As List(Of T)) As Boolean
    Return list.Count > 0
End Function

---

Quote:

---

I would rather capture intent than speculatively micro-optimise.

---

I'd certainly agree with that. There are, of course, occasions where performance trumps readability but, in business software at least, they're pretty few and far between. The obvious exception is database queries. The database is ALWAYS a bottleneck so I'll aim for performance every time in my queries, even if that does mean using obscure tricks (a quirky update to avoid a triangular join is the one that immediately springs to mind).

Quote:

---

Originally Posted by jmcilhinney

That's part of what commenting is about: things seem obvious to you when you write them so you don't comment them but they may not seem obvious to others or even yourself some time later. I'm sure everyone has written what they thought was obvious code at the time and has then come back later and wondered what it was all about.

---

This is what ALL my commenting is about. The number of times I have come back to what I thought was perfectly clear only to find it much less so is disturbingly high. I've tried to improve the quality of my comments, but to some extent I find that to be a fools game. You can never be certain what perspective the new viewer will come from, even if that new viewer is a later version of yourself. Therefore, it is nearly impossible to know what comment will be the key point that will crystalize understanding in the brain of the new viewer. Worse, writing too much is often as bad as writing too little, as it can be hard to wade through, so you can't write absolutely everything. This is a point I constantly wrestle with.


@EG: I found that it did not capture the intent very well at all. Though I generally know what .Any does, I was a bit surprised to find that I wasn't sure what it meant in that particular case for a fraction of a second. Count > 0 is actually more clear to me than .Any, though I see no particular reason why that would be so, which suggests it is more due to the peculiarities of the individual or individual experience. Nonetheless, it is so.

However, would you write slow code rather than fast simply because it is easier to read? That would be an argument for implicit conversions and option Strict OFF. After all, if you can be certain that a certain string holds a number, then MyString = 5 is considerably more readable than CInt(MyString) = 5, or any other proper conversion. Anybody can read the first variation without problem, but somebody new to programming would likely be unsure what CInt(MyString) was doing in the second variation. Option strict does prevent certain bugs from slipping in, but probably not bugs that you would allow anyways.

Furthermore, if you are in the habit of reaching for the slower tool because it looks better to you, wouldn't you expect that performance is more likely to be an issue than it would for a person who is in the habit of reaching for the fastest tool? A fraction of a millisecond here and there usually makes no difference at all, until you end up working on a program where it does matter and you have to go back and re-work it because you neglected to do so on the first pass. In this case, I feel that .Any is less clear than .Count >0, but that's just a matter of opinion. Still, knowing that there is a difference of opinion there (and I'm clearly not alone in my view, since we aren't talking about code I initially wrote), wouldn't it be better to reach for the tool that works better?

Quote:

---

Originally Posted by dunfiddlin

Hmm. Interesting. The no-commenters produce the most comments in support of their decision not to comment. Great paradox in this I see! (Why haven't we got a Yoda smiley?)

---

WOW...that is sure turning out to be true. I wonder if there is something Freudian involved :p

This thread has gotten a bit long in the tooth, as have I, so I have forgotten who the non-commentors are? FD said he was pretty minimalist, but TG, DataMiser, JMC, and myself all said we comment fairly heavily. EG doesn't appear to have explicitly endorsed either camp, though he appears to be leaning towards minimal comments with some of his commentary on TG's comments. Who else falls into the minimal camp?

I'm not a "non-commenter", I'm a "if your code needs comments I'd prefer that - instead of writing the needed comments - you rewrote the code so it didn't need comments". This does, unfortunately, make it sound like I'm a "non-commenter" which is why I've spent most of this thread clarifying my position. :) [Edit2: If you want to bucket people into Non/Minimal/Heavy then I'm in Non, but only because I write my code in a certain way.]

[Edit: I'm also the opposite to jmc in that I write far more comments in code posted on VBF than I do in my day job]

Quote:

---

At any hoot sometimes I get the feeling there are more people here writing code as contractors, consultants, entrepreneurs and not that many firmly entrenched in corporate day to day IT support. It seems like me, me, me versus what is good for the ones paying for us. I might be all wet about that but it is my impression after many years at this site.

---

I thought this was worth addressing. I really hope nobody here takes that attitude and think they're probably creating a rod for their own back if they do.

I work as a contractor and I generally take the aproach that I want to make myself redundant as quick as possible. I don't mean I want to get laid off because the client doesn't want me anymore, but I want to get to a position where the client can let me go because they don't need me anymore. I want to have closed a gap for them and know it will stay closed in my absence. I want them to hire me back when they want a new project done and they remember my success from last time rather than because they feel they've got no choice because I left a mess so bad I'm the only person who can clean it up again. you can only hold a client hostage for so long before they wise up.

Quote:

---

you can only hold a client hostage for so long before they wise up

---

Indeed, if the rumoured updates to W8 are anything to go by, even if you're Microsoft!

Quote:

---

Originally Posted by Shaggy Hiker

This thread has gotten a bit long in the tooth, as have I, so I have forgotten who the non-commentors are? FD said he was pretty minimalist, but TG, DataMiser, JMC, and myself all said we comment fairly heavily. EG doesn't appear to have explicitly endorsed either camp, though he appears to be leaning towards minimal comments with some of his commentary on TG's comments. Who else falls into the minimal camp?

---

I'm with EG. Let the code be the commenting. So.....when can we start persecuting each other on a bloody field of battle ;)

The commutes too long for that.

I'm going to resort to using snide commenting in my code, instead.

lol....;)

Well on the bright side looks like I've created a good topic to discuss. I've learned a little here and I hope everyone else has picked up something as well. I really joy these discussions. So nice to debate and learn new stuff through debate. I will try to limit my commenting in the future.

Quote:

---

Originally Posted by Vladamir

I agree with the OP that comments are important but there is a time and a place for everything and sometimes just posting a quick snippet will suffice without comments. What I can complain about is a block of code which requires an Imports statement and those are left off. My increase in experience with VB has made this less of a problem than when I first started. But I can tell you that in the beginning this was a real problem for me. Code will run fine with or without comments. But without the correct Imports it will not run and the IDE doesn't always indicate which of the many thousands of available options are needed.

---

The MSDN documentation for every single type includes what assembly it's defined in and what namespace it's a member of so it's never more than a 10 second job to determine what Imports are required.

Quote:

---

I will try to limit my commenting in the future.

---

It's not so much about limiting it, it's abut targetting it. I think JM hit the nail pretty much on the head for me when he said commenting is about explaining "why" rather than "what". The next developer, if he is even half way decent, can see the "what" at a glance. The "why" is much harder to spot.

I think the debate, really, is about how you should explain the why. Us minimalists believe our primary method of doing that should be the code itself. Maximalists (if there is such a thing) believe that explicit commenting is better and/or adds an extra level of clarity above and beyond what the code itself can. Ultimately, it doesn't really matter which of us is right because it's not about the comments or the code, it's about the clarity. As long as clarity is achieved than it doesn't really matter how.

Quote:

---

I found that it did not capture the intent very well at all

---

I must admit that I found the .Any jarring as well and would probably have gone with .Count > 0. I guess that's only because it's a property that was new to me though. I thought the really good stuff EG demonstrated was breaking out the helper methods. Because that gives you a chance to explicitely name the helper method it also gives you a chance to encapsulate a great deal of information in that name. It's really the High-Cohesion principle applied at a method rather than class level and is, IMO, a much better way of writing your code.

Like I said, though, I personally still struggle with it. My ingrained tendancy is to have the problem mapped out in my head and just want to dump it down onto the screen. That tends to lead me to writing one big long method. Decomposing it into tiny parts is very much a concious, and not very comfortable, step for me. Still, I think it's one worth taking.

On the performance vs readability issue, I think you're being a little unfair when you say "one day you have to rewrite the whole thing" (I'm paraphrasing because I'm on the next page of the thread) because that's not what happens. What does sometime happen is that a particular part of the program becomes a bottleneck. So then you just refactor that particular part to optimise it and then, yes, you will sacrifice readability for speed.

I think this discussion has pretty much run its course. We've basically got to the point where we're just restating our positions now, but a couple of final points worth addressing:

Quote:

---

Originally Posted by Shaggy Hiker

However, would you write slow code rather than fast simply because it is easier to read? That would be an argument for implicit conversions and option Strict OFF. After all, if you can be certain that a certain string holds a number, then MyString = 5 is considerably more readable than CInt(MyString) = 5, or any other proper conversion. Anybody can read the first variation without problem, but somebody new to programming would likely be unsure what CInt(MyString) was doing in the second variation. Option strict does prevent certain bugs from slipping in, but probably not bugs that you would allow anyways.

---

No, I wouldn't code with implicit conversions, because it's not about "easier to read" it's about "easier to maintain". It's about the process of going from the code displayed on screen to a sufficiently accurate mental model of the intent of the code in the programmer's head. Implicit conversions may transfer the textual representation into your brain more easily, but then you need to figure out where all the converting occurs, which makes it harder to understand.

Quote:

---

Originally Posted by Shaggy Hiker

Furthermore, if you are in the habit of reaching for the slower tool because it looks better to you, wouldn't you expect that performance is more likely to be an issue than it would for a person who is in the habit of reaching for the fastest tool? A fraction of a millisecond here and there usually makes no difference at all, until you end up working on a program where it does matter and you have to go back and re-work it because you neglected to do so on the first pass. In this case, I feel that .Any is less clear than .Count >0, but that's just a matter of opinion. Still, knowing that there is a difference of opinion there (and I'm clearly not alone in my view, since we aren't talking about code I initially wrote), wouldn't it be better to reach for the tool that works better?

---

If it comes down to a choice between fast or clear then I would definitely choose clear on the first pass. The reason is that it's very easier to sacrifice the clarity for speed if you need to, but it is much harder to understand that fast code later. Since it is much more likely that you need to modify code for bugs or changing requirements than you are to need to improve its performance, it is always worth striving for the clarity over speed.

Also, the question on performance isn't "which is faster", it should be "which is fast enough?". If you have two approaches, and only one meets your performance budget (whether that be speed (with all its sub categories [time-to-first-byte, time-to-last-byte, throughput]), memory, power consumption, etc), then you clearly need to use that approach. If neither meets your performance budget, well then you've got a problem. If both meet your performance budget then you choose the clearest code - even if it isn't the fastest. Note that all this assumes you have a performance requirement you need to meet. If you don't have a performance requirement (and, no, "must be fast" isn't a performance requirement) then you don't have a performance problem - and prematurely optimising before you have any performance data has the potential to make your performance worse. Also, optimising in one area could cause a bigger slowdown somewhere else.

So as another saying you may have heard goes: "Measure twice, cut once". Except that in performance considerations your two measures need to be one before and one afterwards.

Quote:

---

Originally Posted by FunkyDexter

My ingrained tendancy is to have the problem mapped out in my head and just want to dump it down onto the screen. That tends to lead me to writing one big long method.

---

I can't remember who said earlier in the thread that they tend to write pseudo-code comments to define the top level idea of the method. I do something similar in that I don't think through the details when I first approach a method. I know that to do X, i will need to do A, B and C, but I don't particularly worry about how I'll do A, B or C straight off. However, instead of writing comments like this:

vbnet Code:

Public Sub doX()
    ' do A
    ' do B
    ' do C
End Sub

I'll instead write something like this:

vbnet Code:

Public Sub doX()
    doA()
    doB()
    doC()
End Sub

I can use the IDE to insert method stubs for those three methods. Now when I'm thinking about how to implement A, I write that code in the doA method, rather than under the doA comment in the doX method.

Well, that's actually highly simplified to the point where I don't actually recognise that as my approach, but it's kinda how I work.

Quote:

---

Originally Posted by Evil_Giraffe

I think this discussion has pretty much run its course. We've basically got to the point where we're just restating our positions now, but a couple of final points worth addressing:

---

Heck, I thought it had run its course before it got moved to General Developer. I'm surprised at how much, and how well, it has taken off since then. Rather nice, really.

Quote:

---

No, I wouldn't code with implicit conversions, because it's not about "easier to read" it's about "easier to maintain". It's about the process of going from the code displayed on screen to a sufficiently accurate mental model of the intent of the code in the programmer's head. Implicit conversions may transfer the textual representation into your brain more easily, but then you need to figure out where all the converting occurs, which makes it harder to understand.



If it comes down to a choice between fast or clear then I would definitely choose clear on the first pass. The reason is that it's very easier to sacrifice the clarity for speed if you need to, but it is much harder to understand that fast code later. Since it is much more likely that you need to modify code for bugs or changing requirements than you are to need to improve its performance, it is always worth striving for the clarity over speed.

Also, the question on performance isn't "which is faster", it should be "which is fast enough?". If you have two approaches, and only one meets your performance budget (whether that be speed (with all its sub categories [time-to-first-byte, time-to-last-byte, throughput]), memory, power consumption, etc), then you clearly need to use that approach. If neither meets your performance budget, well then you've got a problem. If both meet your performance budget then you choose the clearest code - even if it isn't the fastest. Note that all this assumes you have a performance requirement you need to meet. If you don't have a performance requirement (and, no, "must be fast" isn't a performance requirement) then you don't have a performance problem - and prematurely optimising before you have any performance data has the potential to make your performance worse. Also, optimising in one area could cause a bigger slowdown somewhere else.

---

That all sounds good to me. I recognize that part of my views comes from working on a series of projects that have real potential for running into slowdowns due to the likelihood of having LOTS of iterations, but often unknown numbers of iterations. That situation argues for watching the time taken in any one particular iteration. A type of program that I tend to work on a fair amount where this applies would be Genetic Algorithms, where each individual generation is trivial...but there are so MANY of them. Still, in a general business app, I think I would do it the way you suggest.

Quote:

---

So as another saying you may have heard goes: "Measure twice, cut once". Except that in performance considerations your two measures need to be one before and one afterwards.

---

That's a fine statement for a tailor, but pointless for software development. The cost of the measurement in software development can easily be higher than the cost of cutting incorrectly.

Quote:

---

Originally Posted by Shaggy Hiker

The cost of the measurement in software development can easily be higher than the cost of cutting incorrectly.

---

This is true if you expect the analogy to hold. The measurements are of course completely different for the tailor and the software developer. It's more like expecting the tailor to measure how much cloth he has, and if he doesn't have enough then try cutting up a certain piece of cloth, and then measure all the cloth again. If he now has less cloththan before then he needs to stitch the piece he cut back together and try a different cut, if he now has more cloth but still not enough then he should keep that cut and try cutting a different piece and if he has now has enough cloth then he should stop cutting.

Ye-e-e-eah, I think that saying doesn't really apply in the performance optimisation scenario.

It should be: measure 1 - Requirements; measure 2 - design ... but rather often if falls into a cycle more like this:
Attachment 99411


-tg

That's a painfully accurate diagram. xkcd is pretty awesome.

Quote:

---

Originally Posted by Shaggy Hiker

That's a painfully accurate diagram.

---

I concur.

Quote:

---

Originally Posted by Niya

I concur.

---

I guess it is like following directions but ending up miles away from where you are actually suppose to be.

Yep, that pretty much describes every project I've ever worked on.

Quote:

---

Originally Posted by Shaggy Hiker

This thread has gotten a bit long in the tooth, as have I, so I have forgotten who the non-commentors are? FD said he was pretty minimalist, but TG, DataMiser, JMC, and myself all said we comment fairly heavily. EG doesn't appear to have explicitly endorsed either camp, though he appears to be leaning towards minimal comments with some of his commentary on TG's comments. Who else falls into the minimal camp?

---

I would not say I comment my code heavily, only where it is needed and/or when I am coding something as an example for someone else. Most of my code is written in such a way that you should not need a comment to understand what it does and why it would do it that way.