5 ms·

This is the coding equivalent of a designer putting appearance over usability. In this case, it is putting code elegance over readability/maintainability. Sure

This is the coding equivalent of a designer putting appearance over usability. In this case, it is putting code elegance over readability/maintainability. Sure, you've reduced the number of if statements, but those if statements explicitly represent the logic behind the calculations. Come back 6 months later, and you'll be scratching your head about what's going on. Unless you add a comment, but comments are also a code smell.

riskable 8y ago
Comments are a code smell? WTF? No. Just... No. Not having comments is a code smell. Everything has context and it is wrong to assume that the next developer to come along will know/understand it well (or at all). Even you as the original coder might not remember why you did something a certain way. I personally comment like I'm going to start suffering from a massive cognitive decline any day now and will need most things re-explained to me. Example: I was looking at some old code this morning... temp.write('\n') temp.close() Why's that newline being written there? From the perspective of the code it serves no propose. Good thing I had a comment right above it... # Add a trailing newline so 'cat' doesn't leave an ugly mess "Ah, yes. That's a good reason to add a newline."
- Retric 8y ago
  You misunderstood. The need for comments means the code is not clear. That need is a code smell.
  - magicalhippo 8y ago
    I think that's too broad. Needing comments to explain what the code does is a smell. Needing comments to explain why the code needs to do what it does is not a smell.
    - Retric 8y ago
      I don’t think zero comments should be the goal, just that minimizing the need for them is often a good idea. Why can generally be included in the code. Total = SubTotal + SalesTax; Needs no real explanation. Code smell is just another way to compare two possible approaches. If one version can include significantly fewer comments without issue then that’s good sign.
      - EpicEng 8y ago
        >Why can generally be included in the code. Total = SubTotal + SalesTax; Needs no real explanation. Sure, and if all the code you ever write is implementing some trivial school assignment then your probably fine. No one with half a brain thinks your example requires a comment, but it's a bad example.
        Retric 8y ago
        No offense, but functionally like that shows up in school assignments and billion dollar companies back ends. However, it’s easy for even that simple idea to end up being something like: Item.FinalPrice = Item.BasePrice * CalculateModifier(Item.ItemCode, Item.OrderContext); Which sacrificed understanding for elegance. Sure, the functionality to find that SalesTax number might take tens of thousands of hours to create and cover a multitude of egde cases. Still with proper context, structure, naming conventions, etc the why’s should be clear. If your thinking “The exceptions function adjusts for tax holidays. So of course it needs to get exceptions based on location and the order date, and then it needs each items metadata etc” then that’s a great sign. Elegant code is elegant when it encapsulates the why’s not just the how’s.
        EpicEng 8y ago
        >However, it’s easy for even that simple idea to end up being something like: Item.FinalPrice = Item.BasePrice * CalculateModifier(Item.ItemCode, Item.OrderContext); Which sacrificed understanding for elegance. Sure, and I'd call that bad code unless it exists because there are far more considerations than sales tax. Either way I don't see how that is an example of when to or not to leave a comment. >Sure, the functionality to find that SalesTax number might take tens of thousands of hours to create and cover a multitude of egde cases. Still with proper context, structure, naming conventions, etc the why’s should be clear. Again, that's just not true. I have a hard time imaging that you're a professional engineer with real world experience if you've never found yourself in a situation where variable names alone could not express the _why_ behind a piece of code. >Elegant code is elegant when it encapsulates the why’s not just the how’s Great, not always possible. For example: // We have a longer than normal backoff period on // timeouts here because device XYZ is a piece of junk // and randonly stops responding for minutes at a time or // version 2 of the spec switched to an XML format and // allows the header to be anywhere above the root // element of the document (as a processing // instruction). We cannot define a reasonable min // header position/length. Just read the whole file. const size_t MinHeaderLength = std::numeric_limits<size_t>::max(); or // Workaround issue caused by .NET 4.6.1 upgrade which // has more restrictive certificate checks for secure // connections. This is currently affecting SignalR // Scaleout connections to Azure Service Bus. AppContext.SetSwitch("Switch.System.IdentityModel.DisableMultipleDNSEntriesInSANCertificate", true); Of course you could suss out the reasoning on your own eventually, but why force people to do that? What variable naming scheme would you use to convey those reasons?
  - finaliteration 8y ago
    This has been a debate I’ve had with a friend for awhile now. I think that clear code consists of good naming, formatting, structuring, etc., and you should only need to comment high-level functionality, weird cases, or where it’s not really possible to clarify things further. They, however, commment nearly every single line/block. Their code isn’t even bad, they just do it “just in case”. To me it feels like a lot of work for not much benefit.
    - dpark 8y ago
      I generally only see this from extremely junior devs. This behavior is a waste of time for negative benefit, as comments have maintenance cost. I consider this behavior to be a sign of an immature dev. If I ever see a senior engineer do this, I'll know he/she's probably over-leveled.
      - finaliteration 8y ago
        > I generally only see this from extremely junior devs. He’s definitely junior in this case. He’s only in his first programming job out of college. I tried to get him to see the error of his ways but not everyone will listen to reason. :)
        dpark 8y ago
        If he’s actually writing good code (so has sufficient skill) and is working somewhere with decent mentorship, he’ll probably get the message soon enough. Code reviews from senior engineers should tell him to cut it out every time.
  - EpicEng 8y ago
    That's simply not at all true. Code can be clear as day in it's purpose, but not in it's intent because it is implementing a requirement that the next reader may not be aware of. In other worda, the comment explains _why_.
    - Retric 8y ago
      What percentage of requirements should you keep in your source code? CSS for example might assign a button to be green. Should you trace back to the specific requirement to say why it’s green or can you trust if somone changes it to blue it’s becase the requirement changed. I would generally say the second. Ideally, the vast majority of requirements can be treated as such. Cases where I have wanted to include the comments generally relates to brittle code where some change likely has knock on effects. Ideally such code should be avoided where possible. IMO, such things are also better cought in unit tests and documented in source control providing more context. That said, including things like design goal can be helpful to get people familiar with the system. But again IMO, specific requirements should rarely sit in comments rather than unit tests, design documents etc.
      - EpicEng 8y ago
        Well, I didn't mean every requirement of course. I meant the code that gets things working the right way when a cursory view of the code doesn't tell the whole story. I wouldn't question a button color without good reason to do so, but what if that single button was green when every other was blue? What if the button we're shaped like a camel, or called an API in a non-standard way?
        Retric 8y ago
        > I wouldn't question a button color without good reason to do so Exactly, and I don’t think a comment about an old requirement would generally do so. Someone just gave you the new requirement which presumably replaces the old one. But, if the comment mentions 508 usability as an issue or as you say it’s shaped like a camel, then that’s going to be an issue for any version of the website you create. Basicly, comments that make it on the minimum list are suck there and don’t become relevant when comparing different possible designs. Code smell is about comparing designs and implementations not high level requirements. If 1/2 your customers uses JAWS then you got to do what you got to do.

riskable 8y ago

Comments are a code smell? WTF? No. Just... No. Not having comments is a code smell. Everything has context and it is wrong to assume that the next developer to come along will know/understand it well (or at all). Even you as the original coder might not remember why you did something a certain way. I personally comment like I'm going to start suffering from a massive cognitive decline any day now and will need most things re-explained to me. Example: I was looking at some old code this morning... temp.write('\n') temp.close() Why's that newline being written there? From the perspective of the code it serves no propose. Good thing I had a comment right above it... # Add a trailing newline so 'cat' doesn't leave an ugly mess "Ah, yes. That's a good reason to add a newline."

Retric 8y ago
You misunderstood. The need for comments means the code is not clear. That need is a code smell.
- magicalhippo 8y ago
  I think that's too broad. Needing comments to explain what the code does is a smell. Needing comments to explain why the code needs to do what it does is not a smell.
  - Retric 8y ago
    I don’t think zero comments should be the goal, just that minimizing the need for them is often a good idea. Why can generally be included in the code. Total = SubTotal + SalesTax; Needs no real explanation. Code smell is just another way to compare two possible approaches. If one version can include significantly fewer comments without issue then that’s good sign.
    - EpicEng 8y ago
      >Why can generally be included in the code. Total = SubTotal + SalesTax; Needs no real explanation. Sure, and if all the code you ever write is implementing some trivial school assignment then your probably fine. No one with half a brain thinks your example requires a comment, but it's a bad example.
      - Retric 8y ago
        No offense, but functionally like that shows up in school assignments and billion dollar companies back ends. However, it’s easy for even that simple idea to end up being something like: Item.FinalPrice = Item.BasePrice * CalculateModifier(Item.ItemCode, Item.OrderContext); Which sacrificed understanding for elegance. Sure, the functionality to find that SalesTax number might take tens of thousands of hours to create and cover a multitude of egde cases. Still with proper context, structure, naming conventions, etc the why’s should be clear. If your thinking “The exceptions function adjusts for tax holidays. So of course it needs to get exceptions based on location and the order date, and then it needs each items metadata etc” then that’s a great sign. Elegant code is elegant when it encapsulates the why’s not just the how’s.
        EpicEng 8y ago
        >However, it’s easy for even that simple idea to end up being something like: Item.FinalPrice = Item.BasePrice * CalculateModifier(Item.ItemCode, Item.OrderContext); Which sacrificed understanding for elegance. Sure, and I'd call that bad code unless it exists because there are far more considerations than sales tax. Either way I don't see how that is an example of when to or not to leave a comment. >Sure, the functionality to find that SalesTax number might take tens of thousands of hours to create and cover a multitude of egde cases. Still with proper context, structure, naming conventions, etc the why’s should be clear. Again, that's just not true. I have a hard time imaging that you're a professional engineer with real world experience if you've never found yourself in a situation where variable names alone could not express the _why_ behind a piece of code. >Elegant code is elegant when it encapsulates the why’s not just the how’s Great, not always possible. For example: // We have a longer than normal backoff period on // timeouts here because device XYZ is a piece of junk // and randonly stops responding for minutes at a time or // version 2 of the spec switched to an XML format and // allows the header to be anywhere above the root // element of the document (as a processing // instruction). We cannot define a reasonable min // header position/length. Just read the whole file. const size_t MinHeaderLength = std::numeric_limits<size_t>::max(); or // Workaround issue caused by .NET 4.6.1 upgrade which // has more restrictive certificate checks for secure // connections. This is currently affecting SignalR // Scaleout connections to Azure Service Bus. AppContext.SetSwitch("Switch.System.IdentityModel.DisableMultipleDNSEntriesInSANCertificate", true); Of course you could suss out the reasoning on your own eventually, but why force people to do that? What variable naming scheme would you use to convey those reasons?
- finaliteration 8y ago
  This has been a debate I’ve had with a friend for awhile now. I think that clear code consists of good naming, formatting, structuring, etc., and you should only need to comment high-level functionality, weird cases, or where it’s not really possible to clarify things further. They, however, commment nearly every single line/block. Their code isn’t even bad, they just do it “just in case”. To me it feels like a lot of work for not much benefit.
  - dpark 8y ago
    I generally only see this from extremely junior devs. This behavior is a waste of time for negative benefit, as comments have maintenance cost. I consider this behavior to be a sign of an immature dev. If I ever see a senior engineer do this, I'll know he/she's probably over-leveled.
    - finaliteration 8y ago
      > I generally only see this from extremely junior devs. He’s definitely junior in this case. He’s only in his first programming job out of college. I tried to get him to see the error of his ways but not everyone will listen to reason. :)
      - dpark 8y ago
        If he’s actually writing good code (so has sufficient skill) and is working somewhere with decent mentorship, he’ll probably get the message soon enough. Code reviews from senior engineers should tell him to cut it out every time.
- EpicEng 8y ago
  That's simply not at all true. Code can be clear as day in it's purpose, but not in it's intent because it is implementing a requirement that the next reader may not be aware of. In other worda, the comment explains _why_.
  - Retric 8y ago
    What percentage of requirements should you keep in your source code? CSS for example might assign a button to be green. Should you trace back to the specific requirement to say why it’s green or can you trust if somone changes it to blue it’s becase the requirement changed. I would generally say the second. Ideally, the vast majority of requirements can be treated as such. Cases where I have wanted to include the comments generally relates to brittle code where some change likely has knock on effects. Ideally such code should be avoided where possible. IMO, such things are also better cought in unit tests and documented in source control providing more context. That said, including things like design goal can be helpful to get people familiar with the system. But again IMO, specific requirements should rarely sit in comments rather than unit tests, design documents etc.
    - EpicEng 8y ago
      Well, I didn't mean every requirement of course. I meant the code that gets things working the right way when a cursory view of the code doesn't tell the whole story. I wouldn't question a button color without good reason to do so, but what if that single button was green when every other was blue? What if the button we're shaped like a camel, or called an API in a non-standard way?
      - Retric 8y ago
        > I wouldn't question a button color without good reason to do so Exactly, and I don’t think a comment about an old requirement would generally do so. Someone just gave you the new requirement which presumably replaces the old one. But, if the comment mentions 508 usability as an issue or as you say it’s shaped like a camel, then that’s going to be an issue for any version of the website you create. Basicly, comments that make it on the minimum list are suck there and don’t become relevant when comparing different possible designs. Code smell is about comparing designs and implementations not high level requirements. If 1/2 your customers uses JAWS then you got to do what you got to do.