Fork me on GitHub
#off-topic
<
2018-04-27
>
sveri11:04:57

Disclaimer: some might argue that this is nitpicking, but thats not the point here 😉 So today at work I did a code review, for context, this is Java, using the SWTBot library (which is a framework for GUI testing SWT applications). I found this line:

next(wizardBot); // -> init
My complain here is that the comment tells me absolutely nothing and that it should be removed or be more clear, so he came up with this:
// switch to default input mapping page
next(wizardBot);
This is arguably way better, but still I was bugged and we talked about it. I suggedestd to just create a function and call that like this:
openInputMappingPage() {
    next(wizardBot);
}
and remove the comment completely as I argued, in the bigger picture and reading flow it is easier to read the line: openInputMappingPage() instead of the two lines: // switch to default input mapping page next(wizardBot). In general I am a fan of speaking function names instead of comments. He argued against it, saying that I would replace two lines with three lines and that the reading flow is way better like his version, because one first reads the comment and then the function. Opinions? Is there any scientific data available on these types of problems?

danielstockton11:04:49

I'm with you @sveri, had a similar argument a couple of days ago. I strive for 0 comments honestly, as I think if code is written right it should be readable in itself. Sometimes they're necessary when something is really magic, or you've left something for compatibility reasons, but they're always a last resort for me.

danielstockton11:04:54

People usually reply that I will forget my code in 2 months, or it's hard for newcomers but a) I rarely read comments, even if it's not my code and b) if its hard for newcommers without comments, it's a code smell

gklijs11:04:57

@sveri depends a bit on the context, but in general it's a good idea keep methods not to big, and put parts in separate methods.

gklijs11:04:42

I would say keeping complexity down is more important then less loc.

danielstockton11:04:44

An exception might be if you're using some doc generation tool to generate API documentation or something

danielstockton11:04:03

But I definitely would have had to same objection to you, in the above example

danielstockton12:04:29

To present what I would consider a good example: https://github.com/tonsky/datascript/blob/master/src/datascript/btset.cljc Big comment at the top explaining main concepts, and then hardly any comments in the actual code. I've read every line of this code and was able to understand things just fine.

danielstockton12:04:27

When there is a comment e.g. ;; using defn instead of declare because of it's related to a weird bug that meant some non-idiomatic code had to be written

sveri12:04:29

So we are on the same page here. The question is, how do I argue this. In the end he said it comes down to personal preference, which I dont think.

borkdude12:04:02

Comments can get out of sync

borkdude12:04:16

Uncle Bob has a chapter on this in Clean code

borkdude12:04:25

where he gives his reasons for this

sveri12:04:48

Yea, I brought that up too, he was not convinced.

borkdude12:04:08

I’m not sure if I would introduce a wrapping function for every little thing, it’s a balancing act

danielstockton12:04:09

Usually, as long as it isn't too superfluous, I let it go. Just highlight every time you find a non-informative comment, and also every time a comment says something different to the code (happens a lot) to emphasize your point. Get people to think about it.

☝️ 4
danielstockton12:04:05

Comments get out of sync precisely because people don't read them, because they're overused.

sveri12:04:53

Thanks for your responses. It's good to see there is consensus on the comments 🙂

troglotit12:04:08

I have non-common opinion on things like code-review/quality. I think speed of development and time-to-master should be prioritized over quality. When code is easy come-easy go - you don't have kind of attachment to it, and you don't make big of a deal arguing over silly thing, but just merge the PR, and then fix it with your commit - if your collegue think it was offensive - you just revert it but say your thoughts, that you wanted to improve that aspect of code quality.

danielstockton12:04:01

The problem is that if quality is not maintained, speed of development suffers in the long term .

troglotit12:04:44

I haven't been in a team where it was easy (from politics p-o-v) to push-to-master and with bad quality

danielstockton12:04:45

I also think that it's a slippery slope. Similar to how NY supposedly solved its crime epidemic by focusing on petty vandals, maintaining pride in your code has a knock on effect.

danielstockton12:04:27

I definitely prefer working in teams that take pride in their work.

troglotit12:04:49

Yeah, I find this common line of thought convincing and kinda right, but still, your work is not code quality per se, but business value - delivering features. Also, giving up code quality - means that you trust your collegues higher, and you are humble with your opinions on code quality

danielstockton12:04:33

I think it depends on the caliber of the team and whether the work is deemed disposable. It shouldn't prevent features getting shipped, but the team should at least regret letting bad code past and have a plan to rectify it.

danielstockton12:04:29

If you're building a prototype, then sure, but I'm not sure (e.g.) cognitect would have much success taking that approach to clojure or datomic.

danielstockton12:04:10

Good developers will be able to distinguish when something is just matter of personal preference vs just bad

troglotit12:04:17

that is just anecdata, and there's another: http://hintjens.com/blog:106

sveri12:04:52

For context. We build a product and have been working on that for almost ten years with approx 7-12 developers over the time. So its a massive codebase, it is here to stay and we will keep on working on that for the next upcoming years. Also in some places we hit already the big bad muddy balls, thats why I am so keen on code quality. Also we have next to zero feature pressure and code quality is demanded from the very top, among other things.

danielstockton12:04:56

I think we agree, you're just advocating for code review after merge, rather than before.

borkdude12:04:25

@sveri Can I ask which kind of product this is?

borkdude12:04:18

One always has to keep in mind the lifecycle of a software product. Some things are one time scripts, some products last only one or two years and others are built for a decade or longer.

danielstockton12:04:39

I don't have such a problem with that and I agree that it's best not to be too militant about what gets through. The most important thing is that code review and discussion happens.

sveri12:04:42

@borkdude It's the business rules part of the Software AGs webMethods Suite: https://www.softwareag.com/de/products/process/rules_management/default.html

sveri12:04:58

Btw. our CR process is like this: 1. Create PR 2. usually during the next 24 hours one of our team has worked on it, mostly it happens the same day a few hours later. 3. Comments are made withing the tool 4. The PR createor fixes them or we start a discussion about specific topics face to face. 5. We decide on one outcome, if we dont agree, and it is important to one of us we include a third person to decide. In my specific case I just left it with the comment as I think the difference did not justify including a third person. It would just be nice to have some facts handy next time.

borkdude13:04:46

I’m considering buying the Sennheiser PXC 550 bluetooth noise cancelling headphone. Does anyone here have it?

dominicm14:04:01

@borkdude in our office I see the Bose and the Sony ones. The bose ones are slightly better than the Sony ones, but Sony has a killer "ambient noise only" feature, so you can hear voices but not the ambient sounds. Perfect for listening to announcements on the train for example.

dominicm14:04:19

The bose quiet comfort definitely have the best noise cancellation though.

borkdude14:04:01

yeah, but I don’t like Bose’s sound overall, I’m more into Sennheiser

4
bronsa14:04:45

most bose I’ve tried have been overpriced pieces of shit, that sound massively muffled

borkdude14:04:29

I’m willing to sacrifice the quality of the noise cancelling over the overall sound quality

4
3Jane15:04:07

…following up on the above, I’ve been looking into noise-cancelling earphones, with a greater need for noise cancelling rather than high sound quality (my commute is NOISY), do you have preferences / recommendations?

dominicm15:04:03

@lady3janepl bose quietcomfort are the absolute best.

dominicm15:04:17

I'm considering some for reading on my commute.

dominicm15:04:55

@lady3janepl the 35 are the ones in the office

3Jane15:04:31

oh the puffy ones

3Jane15:04:19

they’d be better for sound isolation, but I can’t use them for too long, my ears hurt 😞

dominicm15:04:23

High comfort & best noise cancellation. Audio quality suffers a little though, but my hearing cannot tell the difference.

borkdude15:04:59

Someone told me it’s a cultural difference: Bose sound is more geared toward American preferences, Sennheiser more European.

borkdude15:04:28

Maybe it also depends on the genre you’re listening to

dominicm15:04:35

https://headphonesaddict.com/the-best-noise-canceling-bluetooth-headphones/ > Noise cancelling of Sennheiser PXC 550 competes very well with the Bose QC35 and Sony MDR1000X. There’s almost no difference. > Some users might notice that the Bose and Sony models provide a slightly better noise cancelling experience, but an average user will likely not notice any difference (including myself). I noticed that the Sony didn't handle voices as well as Bose when switching between them.

danielglauser15:04:03

They’re expensive but the build quality and audio quality is amazing. The noise cancelling is great too though not quite a good as the Bose.

danielglauser15:04:37

@borkdude The audio quality is what sold me on the headphones.

borkdude15:04:16

cool, thanks!

valtteri16:04:26

I’ve had Bose QC-35 about a year and I use them daily. They’re awesome ❤️ and battery doesn’t show any fatigue yet.

borkdude16:04:58

How easy is it to switch devices on a bluetooth headphone like that? Say I want to switch from my iPhone to my Macbook to my iPad

alexeiz17:04:17

Why is it that I can use my Java class directly i.e. (com.my.Class. params), but if I try to (require '[http://com.my]), I get "could not locate com/my__init.class"?

alexeiz17:04:06

Sorry, wrong channel

mg17:04:55

ironically that’s off topic for this channel

😂 16
tbaldridge17:04:27

And I now remember why so few datalog engines implement negation.

😄 8
fellshard18:04:46

It's one of the weird assumptions/perceptions we have as humans about propositional logic that ends up being way off base - that proving negation is not necessarily the same as just flipping a positive proposition around

4
valtteri18:04:18

@borkdude with QC-35 you can have 2 connected devices simultaneously. I keep them usually connected to phone and laptop. If you have more devices, switching can be done with an app on your smartphone or using controls on the headphones. The latter is a bit wonky.

dominicm20:04:16

https://github.com/cs-au-dk/dk.brics.automaton/blob/master/src/dk/brics/automaton/BasicAutomata.java#L68-L76 Why can this code set accept on the State object? But when I get a state object I can neither read nor write it.

scriptor20:04:03

hah, these are great

😄 4
Graham Seyffert20:04:24

@dominicm do you mean when you do State s = new State() in your own code?

dominicm20:04:01

@gseyffert yes, but the equivalent from clojure

Graham Seyffert20:04:00

I’ve found I usually have to use aget/`aset` to read/write from classes/objects in Java/Script

Graham Seyffert20:04:21

With the .-{fieldName} syntax

Graham Seyffert20:04:15

Or

(doto (Object.)
  (-> (. -field1) (set! value1))
  (-> (. -field2) (set! value2)))