Skip to main content
Code Review

Return to Answer

added 153 characters in body
Source Link
Perry
Perry
  • 186
  • 2

I won't give it all away, since that would take away some of the fun of Advent of Code, but I'll point out:

First, yes, List.mem takes time proportional to the length of your list, which means if you're calling it over and over again it can be quite slow indeed. There might be faster structures you could use if you want to track if a previous value has been seen before, like Sets. Also, you should not be using == for equality here but rather =; I recommend learning the difference between = and ==, you will want to know it.

Second, your code is not very idiomatic OCaml; in general, if you're doing things like nesting lots of ifs and using List.hd instead of pattern matching, you may not be doing things quite right. This won't impact your performance but might impact readability and correctness. (For example, the match l with | [] -> ... pattern will warn you if a pattern match isn't exhaustive, which avoids nasty bugs.)

I won't give it all away, since that would take away some of the fun of Advent of Code, but I'll point out:

First, yes, List.mem takes time proportional to the length of your list, which means if you're calling it over and over again it can be quite slow indeed. There might be faster structures you could use if you want to track if a previous value has been seen before, like Sets.

Second, your code is not very idiomatic OCaml; in general, if you're doing things like nesting lots of ifs and using List.hd instead of pattern matching, you may not be doing things quite right. This won't impact your performance but might impact readability and correctness. (For example, the match l with | [] -> ... pattern will warn you if a pattern match isn't exhaustive, which avoids nasty bugs.)

I won't give it all away, since that would take away some of the fun of Advent of Code, but I'll point out:

First, yes, List.mem takes time proportional to the length of your list, which means if you're calling it over and over again it can be quite slow indeed. There might be faster structures you could use if you want to track if a previous value has been seen before, like Sets. Also, you should not be using == for equality here but rather =; I recommend learning the difference between = and ==, you will want to know it.

Second, your code is not very idiomatic OCaml; in general, if you're doing things like nesting lots of ifs and using List.hd instead of pattern matching, you may not be doing things quite right. This won't impact your performance but might impact readability and correctness. (For example, the match l with | [] -> ... pattern will warn you if a pattern match isn't exhaustive, which avoids nasty bugs.)

Source Link
Perry
Perry
  • 186
  • 2

I won't give it all away, since that would take away some of the fun of Advent of Code, but I'll point out:

First, yes, List.mem takes time proportional to the length of your list, which means if you're calling it over and over again it can be quite slow indeed. There might be faster structures you could use if you want to track if a previous value has been seen before, like Sets.

Second, your code is not very idiomatic OCaml; in general, if you're doing things like nesting lots of ifs and using List.hd instead of pattern matching, you may not be doing things quite right. This won't impact your performance but might impact readability and correctness. (For example, the match l with | [] -> ... pattern will warn you if a pattern match isn't exhaustive, which avoids nasty bugs.)