Add guidelines on judicious use of recursion #6

nickelization · 2017-01-13T22:51:02Z

No description provided.

paulhenrich

README looks good aside from minor formatting issue. See my notes re. examples.

paulhenrich · 2017-01-13T23:00:22Z

src/recursion.erl

@@ -0,0 +1,31 @@
+-module(recursion).
+


Went to go play with this and noticed that the functions aren't exported.

Yeah, good call. I semi-deliberately left out the exports since I was mainly intending for this code to be a concise, illustrative example, rather than something anyone might ever actually want to run. I suppose it'd be easy enough to just add the one export line though and have a working module, so I'll go ahead and do that.

paulhenrich · 2017-01-13T23:04:09Z

src/recursion.erl

+recurse(S) ->
+    lists:reverse(recurse(S, [])).
+
+recurse([], Acc) ->


Not sure if this is meant as an illustration of your point (if so 👍 😄 ), but this doesn't work.

Hahaha I wish I could claim credit for intentionally writing a bug in this function, but admittedly I did not actually test the code since I was just trying to convey the gist of the idea, and didn't expect anyone to actually run it 😆 . I will fix it along with addressing your other comment above, and then do a rebase and push.

paulhenrich · 2017-01-13T23:06:47Z

README.md

+Additionally, to an experienced Erlang developer, folds and list comprehensions are much easier to understand than complex recursive functions. Such contstructs behave predictably: they always perform an action for each element in a list. A recursive function may work similarly, but it often requires careful scrutiny to verify what path the control flow will actually take through the code in practice.
+
+***
+***


minor formatting (one too many breaks)

Whoops, nice catch. Thanks! Will also fix this in a minute.

paulhenrich · 2017-01-13T23:18:35Z

👍

tburghart · 2017-01-15T14:21:14Z

Isn't this subject already covered somewhere?
I recall something like "don't write your own recursion" in either the inaka version or something from @lehoff

The title's also feels inaccurate - you're not suggesting avoiding recursion, necessarily, just advising against rolling your own.

nickelization · 2017-01-17T15:14:19Z

@tburghart I also thought we had something about this already in the guidelines, but it appears we don't. I also don't see anything in the original Inaka version. I know this has come up at meetups before, so maybe we're both just recalling having discussed it.

I'm not sure if I understand the difference between you're trying to point out between avoiding recursion vs. advising against rolling your own. By titling the section "avoid recursion when possible", I meant to imply "avoid writing code using recursion when possible"; I wasn't that explicit, but I'm not sure how else someone would interpret it, or whether adding such verbosity would address your concerns. Could you explain a little more about what you found inaccurate there, and perhaps suggest a better title?

tburghart · 2017-01-18T15:21:27Z

Yeah, there was a separate "architecture guidelines" doc on the wiki somewhere ... maybe I'll see if I can find it.

As for the title, I might be being too pedantic, but Erlang is all about recursion - many of the lists functions are, after all, explicitly recursive. My point was only that we have no desire to avoid recursion, what we want to discourage is people writing their own when stdlib contains a function that will do it for them.

This also conflicts to an extent with our recommendation to use named functions instead of closures when possible, as it's pretty common to have to wrap a function in a fun to fit it into the stdlib recursive functions, and given the choice I'd opt to write my own over using a closure when I don't need the encompassing state.

nickelization · 2017-01-18T15:40:14Z

Ahh got it, that makes sense - I hadn't considered the angle of "lists:foldl is recursive" and I certainly wouldn't want to discourage anyone from using that ;). I'll try to think of a way to rewrite the title for extra specificity.

That's an interesting point about named functions vs. closures, I hadn't thought about that particular tradeoff. I mean the ideal scenario is you can pass a named function directly into a library function, e.g. lists:map(fun my_map_fun/1, List), but of course that's not always going to suffice....

My personal feeling is that most of the time I'd still rather we pass a closure into a higher-order function than roll our own recursion, but I can see the argument either way. Perhaps we should defer to the architecture group and see if they can offer any additional thoughts on this.

This should improve clarity, since technically functions like foldl and map use recursion; we want people to avoid writing their own recursive functions, not avoid using any function that does recursion under the hood.

raghavkarol · 2017-01-18T22:58:45Z

src/recursion.erl

+
+%% GOOD: uses a fold instead to achieve the same result,
+%% but this time more safely, and with fewer lines of code
+fold(S) ->


Lists map lists:map(fun string:to_upper/1, S) would make the good example better (and almost the BEST ;-)

Valid point! I had really wanted to demonstrate using a fold in my example, since almost any recursive list iteration function can be converted to a fold, so perhaps I will add a map example alongside fold.

JeetKunDoug · 2017-01-19T18:25:43Z

I think what @tburghart was talking about is mentioned in https://github.com/basho/internal_wiki/wiki/Code-Review-Guidelines#code-clarityquality - we had a long conversation about the wording of it over email, so may be worth taking the wording from there?

nickelization · 2017-01-19T18:50:04Z

@JeetKunDoug 👍 I will update the wording to match.

This better conveys when and why you should avoid manual recursion.

paulhenrich · 2017-01-26T20:43:22Z

👍

bashopatricia · 2017-02-02T01:02:35Z

create jira issue

paulhenrich reviewed Jan 13, 2017

View reviewed changes

Add guidelines on judicious use of recursion

4abfb96

nickelization force-pushed the avoid-recursion branch from 86e6ef8 to 4abfb96 Compare January 13, 2017 23:14

Update title of recursion notes

f5516f5

This should improve clarity, since technically functions like foldl and map use recursion; we want people to avoid writing their own recursive functions, not avoid using any function that does recursion under the hood.

raghavkarol reviewed Jan 18, 2017

View reviewed changes

Add map example to recursion.erl

dadf059

Reword recursion guidelines again

be5c529

This better conveys when and why you should avoid manual recursion.

Add guidelines on judicious use of recursion #6

Are you sure you want to change the base?

Add guidelines on judicious use of recursion #6

Uh oh!

Conversation

nickelization commented Jan 13, 2017

Uh oh!

paulhenrich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulhenrich Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulhenrich commented Jan 13, 2017

Uh oh!

tburghart commented Jan 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickelization commented Jan 17, 2017

Uh oh!

tburghart commented Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickelization commented Jan 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeetKunDoug commented Jan 19, 2017

Uh oh!

nickelization commented Jan 19, 2017

Uh oh!

paulhenrich commented Jan 26, 2017

Uh oh!

bashopatricia commented Feb 2, 2017

Uh oh!

Uh oh!

paulhenrich Jan 13, 2017 •

edited

Loading

tburghart commented Jan 15, 2017 •

edited

Loading

tburghart commented Jan 18, 2017 •

edited

Loading