Day 8 - Part 1 & 2 #4
Conversation
| @@ -9,6 +9,7 @@ import ( | |||
| day04 "github.com/doniacld/adventofcode/puzzles/2020/04" | |||
| day05 "github.com/doniacld/adventofcode/puzzles/2020/05" | |||
| day06 "github.com/doniacld/adventofcode/puzzles/2020/06" | |||
| day08 "github.com/doniacld/adventofcode/puzzles/2020/08" | |||
There was a problem hiding this comment.
What happened to day 7 ?
There was a problem hiding this comment.
I forgot to assign you the code review...
| if err != nil { | ||
| return "", err | ||
| } | ||
| // store the initial ops before any modification by the Part one |
There was a problem hiding this comment.
| // store the initial ops before any modification by the Part one | |
| // store the initial ops before any modification by Part one |
puzzles/2020/08/day08.go
Outdated
| return "", err | ||
| } | ||
| // store the initial ops before any modification by the Part one | ||
| tmp := ops.ResetOps() |
There was a problem hiding this comment.
Maybe name it part2 rather than tmp
puzzles/2020/08/game/corrupted.go
Outdated
| switch o.name { | ||
| case nopOp: | ||
| idx = ops.nop(idx) | ||
| break |
There was a problem hiding this comment.
Oh you silly Java girl... You don't need this in go
puzzles/2020/08/game/corrupted.go
Outdated
| func (ops Operations) jump(idx int) int { | ||
| ops[idx].seen = true | ||
| if ops[idx].value < 0 { | ||
| return idx - (abs(ops[idx].value) % len(ops)) |
There was a problem hiding this comment.
if value is negative, you are computing the absolute of it, which is always - value and substracting it, which means you are adding it. I therefore conslude that this line is exactly identical to line 50 and the entire if should be removed (with fire).
puzzles/2020/08/game/fixed.go
Outdated
| return 0 | ||
| } | ||
|
|
||
| func (ops Operations) ResetOps() Operations { |
There was a problem hiding this comment.
reset or duplicate ?
puzzles/2020/08/game/fixed.go
Outdated
|
|
||
| func (ops Operations) ResetOps() Operations { | ||
| // store the tmp ops for modification | ||
| tmpOps := make(Operations, len(ops)) |
There was a problem hiding this comment.
I think it's not temporary if you are returning it. The use of newOps or something would be clearer.
| switch o.name { | ||
| case nopOp: | ||
| idx = ops.nops(i) | ||
| break |
puzzles/2020/08/game/fixed.go
Outdated
| return false, accCounter | ||
| } | ||
|
|
||
| //idx not in the range anymore |
There was a problem hiding this comment.
| //idx not in the range anymore | |
| // idx not in the range anymore |
puzzles/2020/08/game/fixed.go
Outdated
| } | ||
|
|
||
| // computeToTheLastOp calculates the accumulator counter depending on operations | ||
| // exit false if the the operation is already seen or the index is not anymore in the range |
There was a problem hiding this comment.
| // exit false if the the operation is already seen or the index is not anymore in the range | |
| // returns false if the operation is already seen or the index is not in the range anymore |
puzzles/2020/08/game/corrupted.go
Outdated
|
|
||
| // ComputeCorruptedAcc computes the accumulator in the corrupted game | ||
| // ends when an Operation is seen twice | ||
| func (ops Operations) ComputeCorruptedAcc() (int, int) { |
There was a problem hiding this comment.
name output fields when they have the same type
puzzles/2020/08/game/corrupted.go
Outdated
| idx, accCounter := 0, 0 | ||
| for true { | ||
| o := ops[idx] | ||
| // op already seen, game over |
There was a problem hiding this comment.
I'd rather have this as the exit criterium of the infinite loop
for ; ! o.seen ; o = ops[idx] {|
|
||
| // nop for no Operation does nothing, go to the next instruction | ||
| func (ops Operations) nop(idx int) int { | ||
| ops[idx].seen = true |
There was a problem hiding this comment.
there's a dangerous side effect that you're applying here that's based on how the language copies items.
Use
func (ops *Operations) nop(idx int) int {to be more explicit about this func. Same goes for all the funcs that affect the contents of ops.
| // nop for no Operation does nothing, go to the next instruction | ||
| func (ops Operations) nop(idx int) int { | ||
| ops[idx].seen = true | ||
| return (idx + 1) % len(ops) |
There was a problem hiding this comment.
do you ever check that idx is > 0 ?
There was a problem hiding this comment.
that's kind of done in fixed.go:81
There was a problem hiding this comment.
@floppyzedolfin Should I merge the duplicated operations ?
There was a problem hiding this comment.
If you can avoid having duplicated code, you should do what it takes
puzzles/2020/08/game/fixed.go
Outdated
| func (ops Operations) computeToTheLastOp() (bool, int) { | ||
| i, accCounter := 0, 0 | ||
|
|
||
| for true { |
There was a problem hiding this comment.
infinite loops deserve a comment on why we should expect them not to run forever
puzzles/2020/08/game/fixed.go
Outdated
|
|
||
| for true { | ||
| o := ops[i] | ||
| idx := 0 |
puzzles/2020/08/game/fixed.go
Outdated
| // computeToTheLastOp calculates the accumulator counter depending on operations | ||
| // exit false if the the operation is already seen or the index is not anymore in the range | ||
| func (ops Operations) computeToTheLastOp() (bool, int) { | ||
| i, accCounter := 0, 0 |
| // game ends at the last instruction | ||
| // a jmp is replaced by a nop or the other way around | ||
| func (ops Operations) ComputeFixedAcc() int { | ||
| for i := 0; i < len(ops)-1; i++ { |
There was a problem hiding this comment.
or for _, op := range ops {
| return accCounter | ||
| } | ||
| } | ||
| // should not happened |
There was a problem hiding this comment.
| // should not happened | |
| // should not happen |
| currentIdx, nextIdx, accCounter := 0, 0, 0 | ||
|
|
||
| o := ops[currentIdx] | ||
| for ; nextIdx != len(ops)-1; o = ops[currentIdx] { |
There was a problem hiding this comment.
givin line 55, I think it makes a lot more sense to check currentIndex
No description provided.