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

tree-building: port #835

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions config.json
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,18 @@
"maybe"
]
},
{
"slug": "tree-building",
"uuid": "508868ec-bd5f-4826-a4c0-ab2a545e7038",
"core": false,
"unlocked_by": "collatz-conjecture",
"difficulty": 5,
"topics": [
"maybe",
"recursion",
"trees"
]
},
{
"slug": "atbash-cipher",
"uuid": "4b6d04f4-8cfc-4d8a-ac6e-77f3889f27c3",
Expand Down
83 changes: 83 additions & 0 deletions exercises/tree-building/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
# Tree Building

Refactor a tree building algorithm.

Some web-forums have a tree layout, so posts are presented as a tree. However
the posts are typically stored in a database as an unsorted set of records. Thus
when presenting the posts to the user the tree structure has to be
reconstructed.

Your job will be to refactor a working but slow and ugly piece of code that
implements the tree building logic for highly abstracted records. The records
only contain an ID number and a parent ID number. The ID number is always
between 0 (inclusive) and the length of the record list (exclusive). All records
have a parent ID lower than their own ID, except for the root record, which has
a parent ID that's equal to its own ID.

An example tree:

```text
root (ID: 0, parent ID: 0)
|-- child1 (ID: 1, parent ID: 0)
| |-- grandchild1 (ID: 2, parent ID: 1)
| +-- grandchild2 (ID: 4, parent ID: 1)
+-- child2 (ID: 3, parent ID: 0)
| +-- grandchild3 (ID: 6, parent ID: 3)
+-- child3 (ID: 5, parent ID: 0)
```


## Getting Started

For installation and learning resources, refer to the
[exercism help page](http://exercism.io/languages/haskell).

## Running the tests

To run the test suite, execute the following command:

```bash
stack test
```

#### If you get an error message like this...

```
No .cabal file found in directory
```

You are probably running an old stack version and need
to upgrade it.

#### Otherwise, if you get an error message like this...

```
No compiler found, expected minor version match with...
Try running "stack setup" to install the correct GHC...
```

Just do as it says and it will download and install
the correct compiler version:

```bash
stack setup
```

## Running *GHCi*

If you want to play with your solution in GHCi, just run the command:

```bash
stack ghci
```

## Feedback, Issues, Pull Requests

The [exercism/haskell](https://github.com/exercism/haskell) repository on
GitHub is the home for all of the Haskell exercises.

If you have feedback about an exercise, or want to help implementing a new
one, head over there and create an issue. We'll do our best to help you!

## Submitting Incomplete Solutions
It's possible to submit an incomplete solution so you can see how others have completed the exercise.
21 changes: 21 additions & 0 deletions exercises/tree-building/examples/success-standard/package.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
name: tree-building
version: 1.0.0.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since there is no canonical-data.json, by what's written in https://github.com/exercism/haskell/blob/master/README.md#exercise-versioning please use 0.1.0.s instead of 1.0.0.s version numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, cool. I wasn't sure of the protocol.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

upon further review of other example package.yaml, I find that we just don't have version in example package.yaml, likely so we don't need to update them in multiple places. So this one should be removed

sshine marked this conversation as resolved.
Show resolved Hide resolved

dependencies:
- base

library:
exposed-modules: TreeBuilding
source-dirs: src
ghc-options: -Wall
# dependencies:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these lines can (and should) be removed from example's package.yaml

sshine marked this conversation as resolved.
Show resolved Hide resolved
# - foo # List here the packages you
sshine marked this conversation as resolved.
Show resolved Hide resolved
# - bar # want to use in your solution.
sshine marked this conversation as resolved.
Show resolved Hide resolved

tests:
test:
main: Tests.hs
source-dirs: test
dependencies:
- tree-building
- hspec
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
module TreeBuilding (newTree, Record(..), Tree(..)) where

import Data.List
import Data.Maybe (isNothing)

type Id = Int
type Children = [Tree]
type ParentGrouping = (Maybe Id, [Id])

data Record = Record Id (Maybe Id) deriving (Eq, Show)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you created these two as helper functions, you might as well create them as record fields instead.

Suggested change
data Record = Record Id (Maybe Id) deriving (Eq, Show)
data Record = Record
{ recordId :: Id
, recordParent :: Maybe Id
} deriving (Eq, Show)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea


data Tree = Leaf Id | Branch Id Children deriving (Eq, Show)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go with Data.Tree from containers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I'll look into that.


newTree :: [Record] -> Maybe Tree
newTree records
| cycles records = Nothing
| succIdCheck records = build . groupByParent $ records
sshine marked this conversation as resolved.
Show resolved Hide resolved
| otherwise = Nothing

-- Checks for cycling in newTree
cycles :: [Record] -> Bool
cycles = any checkRecord
where checkRecord r =
case r of
(Record i Nothing) -> if' (i /= 0) True False
Copy link
Contributor

@sshine sshine Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(Record i Nothing) -> if' (i /= 0) True False
Record i Nothing -> i /= 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, that should be pretty obvious 😂

(Record i (Just p)) -> if' (p >= i) True False
Copy link
Contributor

@sshine sshine Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
(Record i (Just p)) -> if' (p >= i) True False
Record i (Just p) -> p >= i


build :: [ParentGrouping] -> Maybe Tree
build [] = Nothing
build (x:xs)
| not . validRoot $ x = Nothing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| not . validRoot $ x = Nothing
| not (validRoot x) = Nothing

| null xs = Just (Leaf 0)
| otherwise = Just (Branch 0 (build' xs))

-- Internal build
build' :: [ParentGrouping] -> [Tree]
build' xs
| length xs == 1 = map Leaf (snd (head xs))
| otherwise = buildWithChildren xs
where
buildWithChildren (y:ys) =
map (\yid ->
let children = filter ((== Just yid) . fst) ys
in if not (null children)
then Branch yid (build' children)
else Leaf yid
) (snd y)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is not exhaustive.

I might write it like:

build' :: [ParentGrouping] -> [Tree]
build' [(_, yids)] = map Leaf yids
build' ((_, yids):pgs) = map buildWithChildren yids
  where
    buildWithChildren yid =
      let children = [ child | child <- pgs, fst child == Just yid ]
      in if null children
         then Leaf yid
         else Branch yid (build' children)

And I might address that it's not exhaustive by concluding that it shouldn't be empty.


-- Validate the Root node
validRoot :: ParentGrouping -> Bool
validRoot (p, ids) = isNothing p && length ids == 1 && sum ids == 0

groupByParent :: [Record] -> [ParentGrouping]
groupByParent = sortOn fst
. map (\xs -> (recordParent (head xs), map recordId xs))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import Control.Arrow ((&&&))

(\x -> (f x, g x)) = f &&& g
Suggested change
. map (\xs -> (recordParent (head xs), map recordId xs))
. map (recordParent . head &&& map recordId))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I haven't used the Control.Arrow library before. It is exciting to see it in action here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only really use &&&, ***, first and second from Control.Arrow, and the only arrow I ever think about is (,).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may like to know that first and second actually live in two places nowadays: There is a better version in Data.Bifunctor. A bifunctor is considerably less scary than an arrow, I think.

. groupBy parentsEq
. sortOn recordParent
. sortOn recordId
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will have the possibly unintuitive consequence that it'll primarily sort on recordParent and secondarily on recordId. An alternative is to sort with a combined function using the Monoid instance of Ordering described here:

sortBy $ \x y -> comparing recordParent x y `mappend` comparing recordId x y

or something similar...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool


parentsEq :: Record -> Record -> Bool
parentsEq rx ry = recordParent rx == recordParent ry

recordParent :: Record -> Maybe Int
recordParent (Record _ p) = p

recordId :: Record -> Id
recordId (Record i _) = i

-- verification
succIdCheck :: [Record] -> Bool
succIdCheck = all (\(x,y) -> succ x == y) . pairwise . sort . map recordId
where pairwise = zip <*> tail
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this check is necessary.

Can't a database skip IDs as long as they're monotonically increasing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. Taking all of the tests from the golang version of this exercise has the concequence of those tests incorporated simply by virtue of their being there.

I have not had to deal with the premis of this exercise in the real world. I suppose if users delete comments, then we could definitely end up with trees that have gaps, although a lit of forums seem to keep a marker that it is deleted.

I suppose that is what we have to think about here.
is this tree building for a system where users have the right to completely delete their comments? or is it creepy like facebook.?


-- General Tools

if' :: Bool -> a -> a -> a
if' p a b = if p then a else b
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer necessary.

21 changes: 21 additions & 0 deletions exercises/tree-building/package.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
name: tree-building
version: 1.0.0.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since there is no canonical-data.json, by what's written in https://github.com/exercism/haskell/blob/master/README.md#exercise-versioning please use 0.1.0.s instead of 1.0.0.s version numbers


dependencies:
- base

library:
exposed-modules: TreeBuilding
source-dirs: src
ghc-options: -Wall
# dependencies:
# - foo # List here the packages you
# - bar # want to use in your solution.

tests:
test:
main: Tests.hs
source-dirs: test
dependencies:
- tree-building
- hspec
49 changes: 49 additions & 0 deletions exercises/tree-building/src/TreeBuilding.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
module TreeBuilding (newTree, Record(..), Tree(..)) where

import Data.List
import Data.Ord (comparing)
--import Data.Maybe (fromMaybe)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure an unused import is necessary in the stub.

Suggested change
--import Data.Maybe (fromMaybe)


type Id = Int
type Children = [Tree]

data Record = Record Id (Maybe Id) deriving (Show)

data Tree = Leaf Id | Branch Id Children deriving (Eq, Show)

newTree :: [Record] -> Maybe Tree
newTree records
| cycleCheck records = Nothing
| succIdCheck records == True = build . groupByParent $ records
| otherwise = Nothing
where
recordParent = \(Record _ p) -> p
recordId = \(Record i _) -> i
succIdCheck = all (\(x,y) -> succ x == y) . pairwise . sort . map recordId
where pairwise = zip <*> tail
cycleCheck = any id . map (\(r) -> case r of
(Record i Nothing) -> if i == 0
then False
else True
(Record i (Just p)) -> if p >= i
then True
else False)
groupByParent = sortOn fst . map (\xs -> (recordParent (head xs), map recordId xs)) . groupBy parentsEq . sortBy (comparing recordParent) . sortBy (comparing recordId)
parentsEq rx ry = recordParent rx == recordParent ry
build [] = Nothing
build (x:xs)
| not . rootCheck $ x = Nothing
| xs == [] = Just (Leaf 0)
| otherwise = Just (Branch 0 (recursiveBuilding xs))
where
rootCheck (p,ids)
| p /= Nothing || (length ids /= 1 && (head ids) /= 0) = False
| otherwise = True
recursiveBuilding ys
| length ys == 1 = map (\yid -> Leaf yid) (snd (head ys))
| otherwise = (\(z:zs) -> map (\xid ->
let children = filter ((== (Just xid)) . fst) zs
in if length children > 0
then Branch xid (recursiveBuilding children)
else Leaf xid
) (snd z)) ys
1 change: 1 addition & 0 deletions exercises/tree-building/stack.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
resolver: lts-12.4
Loading