• Create Account

### #ActualYrjö P.

Posted 28 April 2013 - 08:17 AM

Your second (working) solution gets messy after it gets to insert-everywhere/in-one-word.
The approach is fine, you just haven't written it neatly.

- Your argument names are cryptic and sometimes in an illogical order.
- You write extra functions that obscure rather than clarify. For instance, I have no idea what "move-letters" is supposed to do and what arguments it's supposed to take unless I read the code. And when I read the code, I find it's just (append x (list y)). Stuff like this is better inlined.
- You should always nest definitions that are only intended to be used inside another function. It makes the structure of the code far easier to comprehend, plus it often further simplifies code by allowing you to drop any function arguments that are already defined in the outer scope.

I wrote an exact Scala equivalent to your insert-everywhere/in-one-word function but fixed all of the above. I think it looks fine with these changes. Note how the recursive helper has one less argument than yours, and one function even turned into a local definition with no arguments.
def insertEverywhere(c: Char, string: List[Char]) = {
def recInsert(stringBefore: List[Char], stringAfter: List[Char]): List[List[Char]] = {
val newWord = stringBefore ::: (c :: stringAfter)
if (stringAfter.isEmpty) List(newWord)
else newWord :: recInsert(stringBefore ::: List(stringAfter.head), stringAfter.tail)
}
recInsert(Nil, string)
}
Direct Racket translation: :: is cons, ::: is append, head is first, tail is rest, Nil is empty, val is define.

### #2Yrjö P.

Posted 28 April 2013 - 08:15 AM

Your second (working) solution gets messy after it gets to insert-everywhere/in-one-word.
The approach is fine, you just haven't written it neatly.

- Your argument names are cryptic and sometimes in an illogical order.
- You write extra functions that obscure rather than clarify. For instance, I have no idea what "move-letters" is supposed to do and what arguments it's supposed to take unless I read the code. And when I read the code, I find it's just (append x (list y)). Stuff like this is better inlined.
- You should always nest definitions that are only intended to be used inside another function. It makes the structure of the code far easier to comprehend, plus it often further simplifies code by allowing you to drop any function arguments that are already defined in the outer scope.

I wrote an exact Scala equivalent to your insert-everywhere/in-one-word function but fixed all of the above. I think it looks fine with these changes. Note how the recursive helper has one less argument than yours, and one function even turned into a local definition with no arguments.
def insertEverywhere(c: Char, string: List[Char]): List[List[Char]] = {
def recInsert(stringBefore: List[Char], stringAfter: List[Char]): List[List[Char]] = {
val newWord = stringBefore ::: (c :: stringAfter)
if (stringAfter.isEmpty) List(newWord)
else newWord :: recInsert(stringBefore ::: List(stringAfter.head), stringAfter.tail)
}
recInsert(Nil, string)
}
Direct Racket translation: :: is cons, ::: is append, head is first, tail is rest, Nil is empty, val is define.

### #1Yrjö P.

Posted 28 April 2013 - 08:13 AM

Your second (working) solution gets messy after it gets to insert-everywhere/in-one-word. The actual code that gets executed is fine, the issue is how you have laid it out.

- Your argument names are too cryptic and sometimes in an illogical order.
- You write extra functions that obscure rather than clarify. For instance, I have no idea what "move-letters" is supposed to do and what arguments it's supposed to take unless I read the code. And when I read the code, I find it's just (append x (list y)). Stuff like this is better inlined.
- You should always nest definitions that are only intended to be used inside another function. It makes the structure of the code far easier to comprehend, plus it often increases readability by allowing you to drop any function arguments that are already defined in the outer scope.

I wrote an exact Scala equivalent to your insert-everywhere/in-one-word function but fixed all of the above. I think it looks fine with these changes. Note how the recursive helper has one less argument than yours, and one function even turned into a local definition with no arguments.
def insertEverywhere(c: Char, string: List[Char]): List[List[Char]] = {
def recInsert(stringBefore: List[Char], stringAfter: List[Char]): List[List[Char]] = {
val newWord = stringBefore ::: (c :: stringAfter)
if (stringAfter.isEmpty) List(newWord)
else newWord :: recInsert(stringBefore ::: List(stringAfter.head), stringAfter.tail)
}
recInsert(Nil, string)
}
Direct Racket translation: :: is cons, ::: is append, head is first, tail is rest, Nil is empty, val is define.

PARTNERS