code-reviews

pithyless 2020-09-02T10:36:11.030600Z

@admin055 the one thing that bugs me is it was hard to skim the code and understand which parts were "important". I find it nice to split out the logic from the parts that touch I/O (because you probably would want to add more error-handling around that later on anyways). And you should definitely avoid trying to do too much in your main function. The way it's written, it will be hard to test (automated or in the REPL). I took your code and tried to implement it as I would - step by step in the REPL - and this is what I got: https://gist.github.com/pithyless/5ce137958be0c6a8411b403885c99665 Maybe a side-by-side comparison will be useful to you. Notice that most of the work can actually be tested in the REPL without writing anything to disk (or running -main).

3šŸ‘1ā˜ļø
2020-09-02T12:22:18.031Z

@admin055 I left some comments in github.

1šŸ‘
2020-09-02T12:23:48.031100Z

Yeah I couldnā€™t disagree with this comment more.

2020-09-02T12:24:02.031300Z

Adding a bajillion names only hinders readability.

2020-09-02T12:25:27.031500Z

The argument about running in the REPL or in test is valid, but the only chunk of code worthy of running in isolation is your parse-product ā€” which I agree is something clearly worth pulling out.

2020-09-02T12:31:26.031800Z

Let me be more concrete: Reading @admin055ā€™s code, I immediately knew what it was doing, and what the result would be. Reading your code, my eyes had to jump up and down the page, I had to figure out where and how each fn was being. Heck, I even had to figure out what was a fn vs a letted name, because there were some random letted evaluations I didnā€™t expect. Emotionally speaking, the former was rather straightforward and not at all frustrating to read. The latter was extremely frustrating given the minuscule scope of the task.

pithyless 2020-09-02T12:40:18.032Z

Fair enough, my point was showing how I would explore the task. I believe @admin055 had not explored enough via the REPL or that code wouldn't have ended up all in -main. I could be wrong. Would it be more readable if there were just three functions? parse-product, generate-csv, and -main? The dom-* could easily be inlined, the interop is fairly straightforward. But to each is own, I guess. ĀÆ\(惄)/ĀÆ

2020-09-02T12:42:02.032200Z

why generate-csv? Isnā€™t that just what -main does?

2020-09-02T12:42:26.032400Z

Arg parsing? I could buy that.

2020-09-02T12:43:11.032600Z

My rule is: Every fn, every name, must prove its worth. It must have some purpose.

2020-09-02T12:44:00.032800Z

So for this, yeah a few fns make some sense: arg parsing, input, transform, output

pithyless 2020-09-02T12:46:24.033Z

That's just personal preference; I always prefer to wrap my logic in a better-named function and treat -main as just a side-effect of how the JVM works. (i.e. I would not expect to eval (-main ...) in the repl, just to not have to deal with arg parsing, etc.). I realize that's not an objective objection. šŸ˜„

2šŸ‘
pithyless 2020-09-02T12:58:40.033200Z

> My rule is: Every fn, every name, must prove its worth. It must haveĀ someĀ purpose. I think one would be hard pressed to find someone on Clojurians who would object to this truism. If I had chosen better names, you should not have had to jump up and down the page. šŸ™‚

2020-09-02T13:44:54.033400Z

Itā€™s not a ā€œtruism,ā€ and itā€™s not ā€œjust personal preference.ā€ There can be no solution to this problem that excludes those steps. Whether you decide to name them or not is, of course, preference. Another example of this is needing a concept in multiple places. Whether you name this shared concept or not, it exists. Pretty much every other name is manufactured entirely by the developer and should be added only after significant consideration. As you allude, sometimes there are indeed reasons to manufacture names: personal preference, documentation, readability, scanability. However, when you choose one of these reasons the upsides are small, and the downsides are significant. The problem with the code wasnā€™t the particular names you chose. It was the number of completely arbitrary, non-essential names you chose.

2020-09-02T14:00:47.033900Z

@pithyless @potetm Wow thank you both for your reviews and advices ! it exceeds what I expected!

2šŸ¤˜
pithyless 2020-09-02T14:32:23.034200Z

I feel I'm being misquoted. My truism comment was related to this: > potetm: My rule is: Every fn, every name, must prove its worth. It must haveĀ someĀ purpose. > pithyless: I think one would be hard pressed to find someone on Clojurians who would object to this truism. I stand by my statement, that this is a truism. Everyone on Clojurians will agree with this statement. Every function name we choose, must have some purpose. Otherwise all our function names could just be random-uuid. We can only debate whether a specific function is well-named or if it's purpose is worth polluting our global and/or lexical scope with more contextual overhead. My personal preference comment was related to this: > potetm: whyĀ `generate-csv`? Isnā€™t that just whatĀ `-main`Ā does? > pithyless: That's just personal preference; I always prefer to wrap my logic in a better-named function and treatĀ `-main`Ā as just a side-effect of how the JVM works. Here I feel that there is benefit to naming the purpose of the action, not just the context from which it is executed. Mainly, because it often turns out there is more than one way you may want to initiate this action (e.g. not just from the CLI). But if someone were to block this PR, because -main just calls out to generate-csv ? Well, I would choose not to die on this hill. Hence, "personal preference". šŸ™‚ > The problem with the code wasnā€™t the particular names you chose. It was the number ofĀ completely arbitrary, non-essentialĀ names you chose. In my view, this contradicts your previous statement: > There can beĀ noĀ solution to this problem that excludes those steps. Whether you decide to name them or not is, of course, preference. If I felt the names were - completely arbitrary, non-essential - I would not have defined them. They obviously helped me think through the problem and I must have thought they would help the future reader (perhaps me), or I would have removed them. We can debate whether they are well-named and helpful (e.g. I did agree that those interop fns did not have particularly high noise-to-signal ratios and could just as well be inlined), but it's really hard to debate the finer points when the counter stance is: completely arbitrary, non-essential names. I honestly never expected such a heated discussion over such a small piece of functionality. It really brings back the nostalgia of communities bike-shedding over the most inane details. I would have probably scrutinized my code a lot more before posting; or not even bothered. My real feeling on the subject is to just let it be, because no matter what we come up with - when it comes to elegantly parsing some XML to CSV, Perl programmers have us beat. šŸ˜‰ :troll: Thanks @admin055 for the thought-provoking code and @potetm for the thought-provoking discussion!

1šŸ‘
2020-09-02T14:36:25.034400Z

I like your 2 approaches and I guess a mix between the 2 will suit me very well :) Before posting the link on this channel, I was already thinking about my code that I had simply translated from Python to Clojure and that I will probably have to apply some functional notions like pure functions, etc.

2020-09-02T14:36:54.034600Z

> I feel Iā€™m being misquoted. Well. Yes. You feel this way, because, after your clarification, that appears to be what happened šŸ˜„ That makes sense now.

2020-09-02T14:37:29.034900Z

I wasnā€™t trying to be offensive or rude by saying ā€œcompletely arbitrary, non-essential names.ā€ My apologies that I was unclear.

pithyless 2020-09-02T14:38:48.035100Z

šŸ»

1šŸŗ1
2020-09-02T14:39:07.035300Z

I was trying to put things into 2 buckets: Things the solution space demands, and things the programmer makes up. Being in the second bucket isnā€™t bad ā€” I do it all the time. But we can acknowledge when itā€™s the case, what weā€™re hoping to gain from it, and the mental overhead it brings.

2020-09-02T14:42:36.035800Z

> That's just personal preference; I always prefer to wrap my logic in a better-named function and treat -main as just a side-effect of how the JVM works. (i.e. I would not expect to eval (-main ...) in the repl, just to not have to deal with arg parsing, etc.). I realize that's not an objective objection. @pithyless Yes it's a preference that suits me, I really see the advantage. It makes me think eg. to the function (init) with Reagent / Re-frame which calls the function (mount). One initializes the application while the other mounts the virtual dom and that is quite clear.

2020-09-02T15:31:15.036200Z

@pithyless I take this opportunity to say that I enjoyed your video on datalog that I watched last week. Crystal clear! Thx

3šŸ¤˜
2020-09-02T16:18:48.036700Z

@admin055 the same

1šŸ‘
2020-09-02T16:39:21.037300Z

@vincent.cantin Merci !