New answers tagged

2

A rule of thumb for me is to comment on code that looks wrong. It's very easy for another developer, or yourself in the future, to look at such code and think, "Obviously, this should be changed." If you already tried that, and found a hidden problem, leave a comment. This might be due to a bug in a third-party library (including a reference to your bug ...


1

Think about comments as one possible form of documentation. Arguably, they're the preferred form because they're immediately accessible when you're looking at the code. Some developers say code should be self-documenting, but I contend that self-documenting code is impossible. To understand why, ponder this: Without documentation, there can be no such thing ...


6

I agree with many of the other answers: comments can be a very good thing, if used well.  But here's a slightly different way of looking at it: “Should I write comments?” is the wrong question. The right question is: “How can I make this code clear?” You want to make it as easy as possible for whoever reads the code in future.  (That may be other members ...


-1

Comment to describe the intention, not the implementation. // Needs to start disabled to avoid artifacts the first frame. Will enable after frame complete could/should be just // Avoid artifacts the first frame How that is accomplished is irrelevant. Also, if the way that's done changes, the comment won't. Also, // Will enable after frame complete ...


1

tl;dr– Comments themselves can be good! However, comments shouldn't be relied on to enforce program structure when the code could do this more reliably. It's unnecessarily relying on comments as a substitute for program structure that can be bad. read that comments are almost always a bad idea for future maintainability. They probably meant that ...


2

A "why" comment is one of the better uses of comments, and is certainly better than a "why" function name. However, the "why" in your particular example is temporal coupling, and that is definitely worth seriously trying to fix. Temporal coupling in a nutshell is when you have dependencies in time of what order things in a module must be called or set. How ...


0

As many have said, comments that explain why rather than what are generally a good idea. Your example is a perfect example of a comment that explains why an non-obvious bit of code exists. If you have this exact line of code, and a copy of the comment, in multiple places, then it would make sense to make a one-liner function in order to avoid repetition. ...


0

Just a suggestion I haven't seen here yet. I usually write my initial code without many comments, but every time I have a reason to revisit/re-read code (I usually do a few times when I'm initially writing it) if I come across any code that's hard to understand I either refactor it, add comments or improve comments--whatever it takes to ensure that next ...


-4

I feel that most of the time a comment isn't necessary. Most, if not all, information comments should be obvious to the surrounding code. If it isn't obvious then the code around it might also need to be rearranged so that it's more obvious. Now in your specific case this is kind of hard: clearly it doesn't depend on the "unit" that the code resides in but ...


48

... read that comments are almost always a bad idea for future maintainability. It's great that you found that. You now know one "reference source" which is written by someone who is utterly incompetent and whose opinion is less than worthless. They're not only wrong, they're actively damaging code for people like yourself who believe them. Or possibly ...


5

The best comment explains something that the code does not express: why it is not done a more obvious or standard way Starting off with a flag disabled is not unusual or non-standard. Inside of implementation, we use flags and flows and checks. That's normal As long as the function that uses this code is well-named and does one and only one thing (which ...


7

Among these other good answers, I would suggest that the right way to express this requirement is with one or more unit tests: LineRendererShouldStartDisabledToAvoidArtifactsFirstFrame() LineRendererFirstFrameShouldBeFreeFromArtifacts() et cetera ...with your IDE being helpful enough to show the connection between your line of code and the related unit ...


184

and read that comments are almost always a bad idea for future maintainability And now you are reading that the above is total and absolute BULL____. Use comments. Put in as many as you think are necessary. A lot of people think that comments are used to just describe what the code is doing. Basically narrating the steps. That itself isn't usually very ...


14

I hope that the sentence you are referring to comments are almost always a bad idea taken out of context has lost a long list of caveats and exceptions, otherwise the first advice I would give you is to find another guru. As for that comment, that is shown in the question, the best thing to do is to leave it as it is.


75

You answered this yourself in the very first sentence of your question - comments are almost always a bad idea for maintainability. There are times when a quick note in the code is a good thing to include so that way a future developer can understand why something was done that way it was. This could prevent a change that introduces a problem. Generally, I ...


1

This is an addition to the accepted answer written by @Christophe. Compilers want clean code, too Except it's not the Clean Code by Robert Cecil Martin. The clean code that compilers want to read are called Static Single Assignment form (SSA). Modern compilers use SSA as the intermediate representation. Beliefs vs. optimizer vs. profilers Optimizer ...


7

Beliefs vs. optimizer There are a lot of beliefs out there that have been made obsolete by progress of optimizers. So you’d better write the code, then profile it, and if it appears that the optimizer could not get rid of a bottleneck, then only should you have a second thought at it. Instance vs local variables Again, the driving force should be class ...


1

Thanks everyone for the insights. As pointed out by the comments and posted answers above, we had multiple problems (some of which were not even visible till I read these answers). Although we are still in a long running refactor mode now, but for the sake of completion, I am posting here some hints for the reference. Hopefully it may help someone in the ...


1

Having a composite function name is acceptable, so which naming scheme you use depends on your context. There are purists who will tell you to break it up, but I will argue otherwise. There are two reasons to try to avoid such things: Purity - A function which does two things should be converted to two functions which do one thing each. Lexical size - a ...


0

I would use guard clauses as in this example so parameter checks can be reused. In addition, you can use the builder pattern so more than one validation can be chained. It will look a little bit like fluent assertions.


0

What is the ultimate intent of this method? Why add these transformed ids to the DB, is it for the purpose of caching? I'll work under that assumption. It sounds like the intent of the method is really just to get the transformed response ids (either doing the transform or getting them from a cache), and so there's your method name: ...


0

What we like to do for business logic errors (not neccessarily argument errors etc.) is to have a single enum that defines all potential types of errors: /// <summary> /// This enum is used to identify each business rule uniquely. /// </summary> public enum BusinessRuleId { /// <summary> /// Indicates that a valid body weight ...


-2

You say that the function returns something "from a transformation, not from any db action". This suggests that the function behaves more like a constructor than a getter. However, constructors also shouldn't cause side-effects (thanks for the link, @MechMK1). Factory methods, on the other hand, already presuppose some level of messiness. For example, one ...


13

As others have mentioned, using and in a function name, automatically implies that a function is doing at least two things, which usually indicates that it's doing too much or is doing something at the wrong place. Using the and clause in a function name however, in my experience, can make sense when there are some steps in a certain flow that need to be ...


2

Your function does two things: Gets a set of IDs via a transform and returns them to the caller, Writes that set of IDs to a database. Remember the single responsibility principle here: you should aim for a function to have one responsibility, not two. You have two responsibilities, so have two functions: getResponseIds - Gets a set of IDs via a transform ...


9

I would say it's fine in general, but isn't acceptable in all cases. For example, one could argue against and in a function name based on the single responsibility principle aka the S in SOLID - because the and could imply multiple responsibilities. If the and really does mean the function is doing two things, then you probably want to think carefully about ...


56

A function name that contains and is at the wrong level of abstraction. I lean towards addResponseIdToDB() because otherwise the ‘side effect’ is a complete surprise. However: responseIds = addResponseIdToDB(); doesn’t leave anyone surprised. The command query responsibility segregation principle argues that this should not be the only way to get the ...


0

Are you the only one who is concerned about the code organization? Are you prepared to drive a discussion on coding standards with your team? What about management, have they been made aware of the risk of coming to a standstill because of tech debt? In a first round, automated refactorings like extract function or move class to file are your friend. ...


1

UserSearchVO is just a search expression Your UserSearchVO object represents a search expression. So it seems reasonable that the UserSearchVO class would know how to convert itself into an Expression<Func<User,bool>>, which you could then provide to the Where clause. A simple example might look like this (thanks to this answer): class ...


1

I would go for a builder pattern: var users = SearchUserWhich .IsActive() .FirstNameIs('Joe') .LastNameIs('Random') .Fetch(); Now you can combine the filters internally the way you want until you call .Fetch. Basically you are wrapping SQL in a (limited) DSL because you're not using a nice SQL DSL for whatever reason (e.g. you do need that ...


3

As a relatively senior guy in my org, I just plainly reject PRs that have insane code structure, like 30k lines in a single file (I bet much of that is copy-paste). My suggestion would be to talk to the team and make them aware of the best, or at least reasonably idiomatic, React practices, and then enforce them. Make sure you actually can afford doing ...


0

A cache is the correct solution. I would keep the signature of the cache the same as the conversion function. If a conversion function converts a string to a string, I would keep the cache a Map. You'll want to pick a fixed size LRU cache so infrequently used items don't clog the cache.


2

Assumed you don't want to change the existing User object significantly, you could use reflection to iterate over the properties of UserSearchVO check for the equally named properties of User implement a generic comparison of those properties for the data types required This approach can be extended by providing custom attributes to the properties of ...


4

I find this much more readable: // some other layer SearchUser(UserSearchVO searchVO) { var users = from user in allusers select user; users = MatchIsActive(users, searchVO); users = MatchFirstname(users, searchVO); users = MatchLastname(users, searchVO); users = MatchId(users, searchVO); users = MatchSomeFutureValue(...


2

Let's discuss options. The option you seem to be dismissing is the one that is actually really clean: public void resolveChanges() { boolean isChanged = false; isChanged = isChanged || oldTrans.getfOrder().equals(newTrans.getfOrder()); isChanged = isChanged || oldRight.getfRight().equals(newRight.getfRight()); // etc. compound....


1

If you like detailed line information in your exceptions: bool changeDetected = false; changeDetected = changeDetected || oldTrans.getfOrder().equals(newTrans.getfOrder()); changeDetected = changeDetected || oldTrans.getgOrder().equals(newTrans.getgOrder()); if (changeDetected) { compound.setIsStateChanged(true); LOGGER.info("major change detected"...


Top 50 recent answers are included