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

Day 7 - Part 1 & 2 #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Day 7 - Part 1 & 2 #3

wants to merge 1 commit into from

Conversation

doniacld
Copy link
Owner

@doniacld doniacld commented Jul 6, 2021

I definitely struggled a lot.
Hope to read a better cruising speed.

package bags

// Bags defines a map with the bag color as key and its family as value
type Bags map[string]bagFamily
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a (minor) problem in having an exposed type containing non-exposed types.
In this case, for a bigger project, either

  • expose BagFamily
  • make an interface for Bag that something implements

s := make(seen, 0)

// loop until the visit of all paths ends
for true {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you really need a loop here ? An infinite one, on top of that ?

if !s.isSeen(target) {
s.insertSeen(target)
counter++
counter = r.browseParents(r[target].parents, s, counter)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to me you can start the for loop with counter++

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rewrite these two (three) ifs thusly :

if !isSeen(target) {
  // include this bag as seen
  s.insertSeen(target)
  counter++

  // check for parents, and include them if any
  if r.hasParent(target) {
    counter = r.browseParents(r[target].parents, s, counter)
  }
}

if _, ok := s[target]; ok {
return true
}
return false
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about return ok ?


var (
regSourceBag = regexp.MustCompile("^([a-z]+ [a-z]+) bags contain (.*).")
regContentBags = regexp.MustCompile("([0-9]+) ([a-z]+ [a-z]+) bags?")
Copy link
Collaborator

@floppyzedolfin floppyzedolfin Jul 17, 2021

Choose a reason for hiding this comment

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

Do you really need to split the words apart ?

worldsapart

counter := 1
v := r[target]
for _, c := range v.children {
counter += c.nb * r.countBags(c.name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can also perform some memoisation and store the number of bags inside [nameX] in bag.

This would avoid having to compute twice the contents of "shiny brown bag" (and of its children)

// CountPossiblePaths returns the number of possible bags paths to get the target bag
func (r Bags) CountPossiblePaths(target string) int {
var count int
// maintain a list of the seen bags to avoid to count duplicates
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// maintain a list of the seen bags to avoid to count duplicates
// maintain a list of the bags seen to avoid to count duplicates

// and counts the number of ways to access this bag
func (r Bags) browseParents(v []bag, s seen, counter int) int {
// browse the list of parents of the target bag
for i := 0; i < len(v); i++ {
Copy link
Collaborator

@floppyzedolfin floppyzedolfin Jul 18, 2021

Choose a reason for hiding this comment

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

for _, parent := range v {

log.Fatal(err)
}
// extract the bag name
leave := strings.Join(l[2:], "")
Copy link
Collaborator

@floppyzedolfin floppyzedolfin Jul 18, 2021

Choose a reason for hiding this comment

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

(this is the reason for the gif - you could be using l[2] instead of l[2:])

}

// insertParentBag insert a parent bag into the map
func (r Bags) insertParentBag(name string, parent bag) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

use func (r *Bags) ...

func (r Bags) insertParentBag(name string, parent bag) {
v, ok := r[name]
if !ok {
r[name] = newBagFamily()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand - this value seems (to me) to be overridden 3 lines below.

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

Successfully merging this pull request may close these issues.

2 participants