Hacker News Reader

Comment by DogLover_ | Hacker News Reader

Same old recommendations.

Some things to consider imo:

1. Using x as variable name can sometimes increase readability. For generic functions that are close to mathematics it is not a sin to use such variable names. Though way to often I get pointed out to rename it to something more descriptive. Longer variable names can also leads to worse readability since a linter will now make 1-liner split into a 4-liner.

2. Tests: "With increasing complexity your codebase will become much harder to maintain and control — tests will do it for you." Good intentions here but be careful. Tests can cause people not wanting to refactor/fix because now they have to fix the structure of the tests. Refactor means more than changing the implementation of functions.

3. "Commit daily,... . Give meaningful commit messages." I am starting to see a trend where PRs get squashed into one commit. Thus making such advice not very useful.

View on HN

1. The number of cases where a one-character variable name is a good choice is small, and those are usually limited to the scope of a single loop, a lambda parameter, etc. A good linter should understand that.

Though naming is hard, and coming up with a descriptive and short name is often not easy. It's a worthy exercise, though, it makes you think about what this thing really is, and can even uncover bugs.

2. Test the public interface. Don't write, rewrite, or remove tests that test specific implementation details, to say nothing of trivial things like getters / setters. In a a good test suite, 20% of tests break during a big refactoring, but the remaining 80% show you that you're still on the right track. Most of the broken tests are often easy to fix mechanically, and those that need thinking, would need thinking anyway. They test the actually changed stuff.

3. Commit more than daily; commit with every change worth looking at, or reverting to. Spend these 15 seconds thinking up a good commit message.

Squashing on development branches is both counter-productive, hostile to code reviewers, and needless. Git can auto-squash commits on a merge for ages. Github has a checkbox for it, so does Bitbucket. Use that.

1. I did say sometimes that single letter variable are okay not always. The example you mentions are good cases. Especially lambda functions. If its not mentioned some will interpret that single letter variables are always bad.

2. My experience says that is too static way of thinking. Maybe I work in too fast paced place, but usually we learn that previous written software is not good today and we need to rewrite the code to make it better. Rarely have I seen that interfaces stays the same forever.

3. Whats the difference here? Squashing on a development branch vs on merge? Btw I do commits daily with good messages, but I am just wondering what I get from this if I loose it later anyway.

If you have to severely rework interfaces very often, your area may indeed be very fast-paced; e.g. so fast where reasonable design and planning is impossible. (I can name a few areas like that, e.g. trading algorithms.)

If you, in your very fast-paced environment, ever do code reviews, you likely noticed that showing the thought process, or separation of concerns, in separate commits helps reviewers wrap their heads around your changes. Adding commits that address feedback also helps the review process. Another feature of multi-commit development is when you notice that e.g. a particular feature won't fit in the next release, and you take only these changes to a different branch.

I guess its subjective/individual what is considered fast-paced. I would consider it normal web-development in agile environment. There is time for design and planning but you also have to be practical and get work done. Good design is preferred but change will happen, otherwise you can end up shoehorning yourself.

We already have practices about keeping PRs small so one could argue having to focus on crafting good individual commits is not that important in the grand scope of things. Especially if those carefully crafted messages will go lost after a merge. Some PRs require heavy reviews, while others just require a quick look through. From my experience people usually look at the changes and not the commit messages (myself including).

> Another feature of multi-commit development is when you notice that e.g. a particular feature won't fit in the next release, and you take only these changes to a different branch.

If your PRs are small you could also just move the changes yourself manually. Or you could do as you say and move each 'WIP' commit if necessary.

Regarding your first point, in side projects, I've started doing things like this:

  for c in customers:
    return c.first_name + c.last_name

or:

  customers.map(c => c.first_name + c.last_name)

I've started doing this because I didn't like having "customers" and "customer", I found this very un-readable. It also makes it simpler to follow what's going on since I have less code to parse.

And because I never use 1-lettered variable names (unless it's a counter like i, j, k), I find it very easy to read when this convention occurs in my code.

Yes, I personally like this style.

Its not a lack of thinking or laziness. Its just easier to read.

Using single char variables is fine and IMHO leads to more concise code without sacrificing readability. When you are looping over a set, it's very common to think about it like `for all x in Xylophones do {...}`. For example in a nested loop, I would much rather prefer `i` and `j` over `iterativeAuxiliaryVariable1` and `iterativeAuxiliaryVariable2`. Never using single character variables as a "best practice" is just bad advice. As with pretty much everything in programming, the real answer is "it depends".

Meaningful commit messages on the feature branch make it easy to compile a list of changes for the PR; in the case of GitHub, the squashed commit's message for the PR lists the individual commits that make it up in the larger text box, which can be nicely formatted into bullet points.

> Thus making such advice not very useful.

Even with a squash, I think there is value in committing frequently and with good commit messages.

The Clojure world has a set single-letter naming conventions that, since they are widely accepted, greatly improve readability. If you see an argument named `f`, you can be sure it is a function. `m` is a map, while `k` and `v` are used for keys and values, respectively. `x` and `y` are numbers and `n` is always an integer.

I prefer a convention of semantically relevant variable names prefixed/suffixed by a letter indicating type.

Didn't we already collectively decide that Hungarian notation is a bad idea? About 20 years ago?

No. It's alive and particularly pragmatic in dynamic languages. The type of sigil, @ or $ or I or Array(space) are all equal to the interpreter. The difference is how offended you might be that type annotations are semantically identical.

To be clear, the above examples are used when the function is designed to accept any such argument and only relevant info is the type.

I bet the scope of such names is pretty narrow, no more than 3-4 lines? The narrower the scope, the less descriptive your names may be, and such math-like conventions make sense.

In a 50-line function (which you should avoid but sometimes can't because of long lines wrapped for readability), using a name like `f` throughout is a harder sell.

> 2. Tests: "With increasing complexity your codebase will become much harder to maintain and control — tests will do it for you." Good intentions here but be careful. Tests can cause people not wanting to refactor/fix because now they have to fix the structure of the tests. Refactor means more than changing the implementation of functions.

Isn't the entire point of tests that you can refactor the code under test without changing the tests themselves? Otherwise how do you ensure that you have not introduced bugs in the code by doing your refactor because you've changed the thing that tested the code in the first place.

A refactor of the tests should be done independently of the code under test.

Yes, ideally. Usually though you have to make changes because something is not working as it should or you found a better way. If you never change the interface the code often times rots. Most of the time we don’t make the correct design decisions from the start. We learn from previous understanding and that might require to do things over.

I agree and I think this is why OP said to be careful -- there are so many engineers out there who do not separate the ideas of code and functionality. When these people write unit tests, they are essentially just checking that the unit behaves exactly as it currently does (white-box style), instead of checking that the unit behaves as desired (black-box style). Every subsequent change to the codebase requires a corresponding change to the tests, even if the external behavior of the unit remains the same.

> Using x as variable name can sometimes increase readability. For generic functions that are close to mathematics it is not a sin to use such variable names. Though way to often I get pointed out to rename it to something more descriptive. Longer variable names can also leads to worse readability since a linter will now make 1-liner split into a 4-liner.

I've rarely seen cases where abstract short variable names have improved readability on their own.

Where it has helped, either there is a comment explaining what the variable is for, or there is a strong convention already in the domain, like using x,y,z in graphics programming to indicate position.

Also... I haven't really encountered linter issues with longer variable names. Do you mean linters that enforce maximum line widths? That seems increasingly rare these days.

> "Commit daily,... . Give meaningful commit messages." I am starting to see a trend where PRs get squashed into one commit. Thus making such advice not very useful.

As a general point of practice, squashes should merge together into semantically meaningful packages, and big multi-purpose commits are the actual no-no. So for example I'll squash together two commits that say "Fixed ABCD issue by doing XYZ" and "Fixed accidental regression caused by XYZ" to just the first message. The fact that I also needed to fix a regression is fine being documented in the bug tracker only, since someone looking at a commit diff will spot the extra XYZ related changes anyway.

Are you sure its considered a no-no? Gitlab has it integrated as part of the PR interface. https://docs.gitlab.com/ee/user/project/merge_requests/squas...

> Do you mean linters that enforce maximum line widths? That seems increasingly rare these days.

Yes, that is what I mean, and I see it more and more because some think passing a linter test will make your code good.

>some think passing a linter test will make your code good.

Who thinks this?

At first I read this and thought "well nobody probably thinks that exact thought, but lots of people think they help you avoid mistakes". Then I scrolled down and read a comment that said:

> A couple of those points could be boiled down to "use a strict linter with most of the rules turned on".

Now I'm not so sure.

I saw someone else on HN use the word "glib", which I think applies here. It has a kernel of truth and sounds good, but there are very obvious reasons why "it passes the linter" is an insufficient condition for good code.

> Are you sure its considered a no-no? Gitlab has it integrated as part of the PR interface.

Good point. I was careful to say "big" as well as "multi-purpose", because I think most of us are more comfortable with "big" commits than "multi-purpose" commits, and a commit that is both should send off red flags.

Specifically, it's hard to code review multi-purpose commits because you have to think more extensively about the side effects, risks, regressions, etc. for unrelated changes that are lumped together. If it's a small commit, that tends to not be an issue, but if you're touching several unrelated areas in a big commit, it gets really tough. And as a general best practice, you want your code to be easy to review.

Higher level tests require less refactoring

Reply on news.ycombinator.com