Return to Answer

added 2142 characters in body

Source Link

edited Jan 23, 2021 at 22:37

def jolt_differences
  @jolt_differences ||= fetch_jolt_differences
end

def fetch_jolt_differences
  Hash.new(0).tap do |result|
    @adapters.each_with_index do |adapter, i|
        next if is_charger?(adapter)

        prev_adapter = @adapters[i-1]
        result[adapter - prev_adapter] += 1
    end
  end
end

# to_index method Your to_index function can be simplified with a group_by. Also the name is not really reflecting what it does, I willwould name it something like adapters_grouped_by_index.

def adapters_grouped_by_index
      @adapters_grouped_by_index ||= adapters_with_charger_and_device.group_by.with_index { |_, i| i }
end

adapters_grouped_by_index[adapter]

tap method

I often see you using a pattern where you have a lookresult object and then return it at part 2 later onthe end of the method.

def some_method
  connections = [] # e.g. connection

  # do something with connection
  # connection << 1

  connections
end

You could replace this pattern with the tap method.

[].tap |connections|
  connections << 1
end

https://medium.com/aviabird/ruby-tap-that-method-90c8a801fd6a

or equals

Instead of a conditional assignment you can use a ||= (or equals)

@memorized_ways = { 0 => 1 } unless @memorized_ways

# vs

@memorized_ways ||= { 0 => 1 }

https://stackoverflow.com/questions/995593/what-does-or-equals-mean-in-ruby

Convention memoization value

It is a convention that the variable holding a memoized value has the same name as the method, sometimes prefixed with an underscore.

def jolt_differences
  return @count if @count

  @count = Hash.new(0)
  @adapters.each_with_index do |adapter, i|
      next if is_charger?(adapter)
      prev_adapter = @adapters[i-1]
      @count[adapter - prev_adapter] += 1
  end
  @count
end

This should really be @jolt_differences (or @_jolt_differences) instead of @count.

def jolt_differences
  return @jolt_differences if @count

  @jolt_differences = Hash.new(0)
  @adapters.each_with_index do |adapter, i|
      next if is_charger?(adapter)
      prev_adapter = @adapters[i-1]
      @jolt_differences[adapter - prev_adapter] += 1
  end
  @jolt_differences
end

private

Make all methods except part_1_answer and part_2_answer private as they should only accessed from inside the class.

By the way, I would also think about better names for these methods.

def jolt_differences
  @jolt_differences ||= fetch_jolt_differences
end

def fetch_jolt_differences
  Hash.new(0).tap do |result|
    @adapters.each_with_index do |adapter, i|
        next if is_charger?(adapter)

        prev_adapter = @adapters[i-1]
        result[adapter - prev_adapter] += 1
    end
  end
end

I will have a look at part 2 later on.

def jolt_differences
  @jolt_differences ||= fetch_jolt_differences
end

def fetch_jolt_differences
  Hash.new(0).tap do |result|
    @adapters.each_with_index do |adapter, i|
      next if is_charger?(adapter)

      prev_adapter = @adapters[i-1]
      result[adapter - prev_adapter] += 1
    end
  end
end

# to_index method Your to_index function can be simplified with a group_by. Also the name is not really reflecting what it does, I would name it something like adapters_grouped_by_index.

def adapters_grouped_by_index
      @adapters_grouped_by_index ||= adapters_with_charger_and_device.group_by.with_index { |_, i| i }
end

adapters_grouped_by_index[adapter]

tap method

I often see you using a pattern where you have a result object and then return it at the end of the method.

def some_method
  connections = [] # e.g. connection

  # do something with connection
  # connection << 1

  connections
end

You could replace this pattern with the tap method.

[].tap |connections|
  connections << 1
end

https://medium.com/aviabird/ruby-tap-that-method-90c8a801fd6a

or equals

Instead of a conditional assignment you can use a ||= (or equals)

@memorized_ways = { 0 => 1 } unless @memorized_ways

# vs

@memorized_ways ||= { 0 => 1 }

https://stackoverflow.com/questions/995593/what-does-or-equals-mean-in-ruby

Convention memoization value

It is a convention that the variable holding a memoized value has the same name as the method, sometimes prefixed with an underscore.

def jolt_differences
  return @count if @count

  @count = Hash.new(0)
  @adapters.each_with_index do |adapter, i|
      next if is_charger?(adapter)
      prev_adapter = @adapters[i-1]
      @count[adapter - prev_adapter] += 1
  end
  @count
end

This should really be @jolt_differences (or @_jolt_differences) instead of @count.

def jolt_differences
  return @jolt_differences if @count

  @jolt_differences = Hash.new(0)
  @adapters.each_with_index do |adapter, i|
      next if is_charger?(adapter)
      prev_adapter = @adapters[i-1]
      @jolt_differences[adapter - prev_adapter] += 1
  end
  @jolt_differences
end

private

Make all methods except part_1_answer and part_2_answer private as they should only accessed from inside the class.

By the way, I would also think about better names for these methods.

Source Link

answered Jan 23, 2021 at 9:29

Christian Bruckmayer

Advent of Code is a good idea to improve your coding skills. Good luck with switching careers. Here are some improvements.

Snake_Case vs CamelCase

In Ruby, modules and classes use CamelCase. You class name should be AdapterArray. Speaking of that, the class name is also not very speaking, maybe you can come up with something more descriptive?

Loading file in class

I personally would not load the file in the class but outside and just pass in the array. The advantage of this is that you can write tests easier.

class AdapterArray
  attr_reader :filepath

  def initialize(adapter_list)
    @adapter_list = adapter_list.sort
  end
end

adapter_list = File.readlines(filepath, chomp: true).map(&:to_i)
AdapterArray.new(adapter_list)

# in your tests you can do now
AdapterArray.new([10, 5, 1])

Magic value

A magic value are unique values with unexplained meaning or multiple occurrences which could (preferably) be replaced with named constants. In your code, e.g. the 1 and 3 are magic value you should give a name with extracting to a constant / method.

CHARGER_VALUE = 0

def adapter_list_with_charger_and_device
  adapters.unshift(CHARGER_VALUE).push(device_value)
end

def device_value
  adapter_list.last + 3
end

https://en.wikipedia.org/wiki/Magic_number_(programming)

JoltDifferences

Good idea to memoize this! I would maybe think about splitting it into two methods and use ||= instead of the guard clause.

def jolt_differences
  @jolt_differences ||= fetch_jolt_differences
end

def fetch_jolt_differences
  Hash.new(0).tap do |result|
    @adapters.each_with_index do |adapter, i|
        next if is_charger?(adapter)

        prev_adapter = @adapters[i-1]
        result[adapter - prev_adapter] += 1
    end
  end
end

Use attr_readers instead of instance variable

Instead of accessing the instance variables directly you should use attribute readers.

class AdapterArray
  attr_reader :adapter_list

  def my_method
    adapter_list # same as @adapter_list
  end
end

I will have a look at part 2 later on.