Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can error handling be improved here? #7118

Open
Anton-4 opened this issue Sep 26, 2024 · 1 comment
Open

Can error handling be improved here? #7118

Anton-4 opened this issue Sep 26, 2024 · 1 comment
Labels
enhancement New feature or request

Comments

@Anton-4
Copy link
Collaborator

Anton-4 commented Sep 26, 2024

From exercism/roc#118 :

My solution is really verbose. Sometimes I don't really know how to solve a simple problem without a full page of code in Roc, with a lot of error handling. I mean here's the equivalent code in Python, it took me about 2 minutes to write. I'm not quite there yet in Roc:

matrix = [[1,5,3],[1,4,3],[1, 2, 2]]
result = [{"row": y + 1, "column": x + 1} for (y, row) in enumerate(matrix) for (x, val) in enumerate(row)
if val == max(row) and val == min((0 if len(r) < x else r[x]) for r in matrix)]
@Anton-4 Anton-4 added the enhancement New feature or request label Sep 26, 2024
@ageron
Copy link
Contributor

ageron commented Sep 26, 2024

Thanks for filing this issue @Anton-4 . Here's the corresponding Roc code. It's about 30 lines of code, but it's probably possible to reduce that a bit.

module [saddlePoints]

Forest : List (List U8)
Position : { row : U64, column : U64 }

saddlePoints : Forest -> Set Position
saddlePoints = \treeHeights ->
    tallestTreesEastWest =
        treeHeights
        |> List.mapWithIndex \row, rowIndex ->
            maxInRow = row |> List.max |> Result.withDefault 0
            row
            |> List.mapWithIndex \height, columnIndex ->
                if height == maxInRow then [{ row: rowIndex + 1, column: columnIndex + 1 }] else []
            |> List.joinMap \id -> id
        |> List.joinMap \id -> id
        |> Set.fromList

    numColumns = treeHeights |> List.map List.len |> List.max |> Result.withDefault 0
    smallestTreeNorthSouth =
        List.range { start: At 0, end: Before numColumns }
            |> List.map \columnIndex ->
                column =
                    treeHeights
                        |> List.mapWithIndex \row, rowIndex ->
                            row
                                |> List.get? columnIndex
                                |> \height -> Ok { height, rowIndex }
                        |> List.keepOks \id -> id

                minInColumn = column |> List.map .height |> List.min |> Result.withDefault 0
                column
                |> List.keepIf \{ height } -> height == minInColumn
                |> List.map \{ rowIndex } -> { row: rowIndex + 1, column: columnIndex + 1 }
            |> List.joinMap \id -> id
            |> Set.fromList

    tallestTreesEastWest |> Set.intersection smallestTreeNorthSouth

Of course, conciseness must not hinder clarity and readability. My one-liner above is hard to read, but if you space it out just a bit (about 6 to 8 lines), it becomes very readable and intuitive, imho:

def saddle_points(tree_heights):
    row_maxs = [max(row) for row in tree_heights]
    col_mins = [min(col) for col in zip(*tree_heights)]
    return [
        {"row": row_index + 1, "column": col_index + 1}
        for (row_index, row) in enumerate(tree_heights)
        for (col_index, val) in enumerate(row)
        if val == row_maxs[row_index] and val == col_mins[col_index]
    ]

I looked at the solutions of other languages, and Ruby is about as concise. Rust is around 15 lines. Several other languages are around 15-20 lines long. My Roc solution is 30 lines long, but perhaps someone can do better? The solution for the Java track is over 80 lines long, split across two files. And the Go solution is over 150 lines long (huh?!).

I really struggled to write concise code on this particular exercise, so think it's a good case study to see what can be done improve conciseness. Perhaps it's the error handling. Perhaps it's the lack of a List.mapN function. Perhaps it's something else entirely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants