3
\$\begingroup\$

I made this math script, where you enter an equation and it solves it for you. I was wondering what you thought of the code, and what I could have done better or what doesn't follow Ruby best practices. The code is also on Github here if you want to see the readme and how to use define_equation()

#!/usr/bin/env ruby

require "readline"
require "cmath"

# PREFIXES = { -15 => "f", -12 => "p", -9 => "n",
#            -6 => "µ", -3 => "m", 0 => "",
#             3 => "k", 6 => "M", 9 => "G",
#             12 => "T", 15 => "P", 18 => "E" }

def define_equation(file)
    File.foreach(file) { |line|
        name, var, equation = line.split(":")
        Equations.class_eval %[
            def #{name}(#{var})
                #{equation}
            end
        ]
    }
end

def all_formulas
    (Equations.instance_methods(false) - [:method_missing])
    .map { |i| i.to_s.gsub(/_/, "-") }
end

class Equations

    C = 3e8

    def m_with_hz(frequency)
        C / frequency
    end

    alias_method :hz_with_m, :m_with_hz

    def qudrtc_frml_with_a_b_c(a, b, c)
        positive = (-b + CMath.sqrt(b ** 2 - 4 * a * c)) / 2 * a
        negative = (-b - CMath.sqrt(b ** 2 - 4 * a * c)) / 2 * a

        # possibly add positive and negative .to_r as well as the float
        [positive, negative]
    end

    def pythgrn_thrm_with_a_b(a, b)
        a = Math.sqrt(a ** 2 + b ** 2)
        a = "sqrt(#{(a ** 2).round})" unless a.to_i == a
        a
    end

    def pythgrn_thrm_with_b_c(b, c)
        a = Math.sqrt(c ** 2 - b ** 2)
        a = "sqrt(#{(a ** 2).round})" unless a.to_i == a
        a
    end

    def method_missing(m, *args, &block)
        "That equation doesn't exist here. (yet)"
    end
end

class Cli

    attr_reader :equations

    def initialize
        @equations = Equations.new
    end

    def help
        ["To use an equation, just type something like this: hz-with-m 100",
        "Thats 100 meters, converted to hertz",
        "Or something like this: pythgrn-thrm-with-b-c 4 5",
        "That's a^2 = 5^2 - 4^2, or more familiarly, a^2 + 4^2 = 5^2",
        "You also can use e for scientific notation. Eg. 3e5 = 300000",
        "Type quit to quit",
        "",
        "Available Equations:",
        all_formulas]
    end

    def method_missing(m, *args, &block)
        # checks if it only contains numbers, math symbols, and e
        if args.all? { |arg| arg.match(/\A[\d+\-*\/.e ]+\z/) }
            args.map! { |arg| eval(arg) }
            @equations.send(m, *args)
        else
            "Invalid number"
        end
    end

    def quit
        exit
    end

end

cli = Cli.new

case ARGV[0]
when "-l", "--load"
    ARGV.shift
    define_equation(ARGV.shift)
    # I will add more stuff here
end

if __FILE__ == $0
    puts "Type help for help."
    Readline.completion_append_character = " "
    Readline.completion_proc = proc { |s|
        all_formulas.grep(/^#{Regexp.escape(s)}/)
    }

    loop do
        command, *variables = Readline::readline("> ", true)
        .downcase.strip.squeeze(" ").split(" ")
        unless command.nil?
            command.gsub!(/-/, "_")
            puts begin
                cli.send(command, *variables)
            rescue ArgumentError
                argc = cli.equations.method(command).arity
                arg_list = (1..argc).map { |i| "<number_#{i}>" }.join(" ")
                "Usage: #{command} #{arg_list}"
            end
        end
    end
end

Here are the tests:

#!/usr/bin/env ruby

require "rubygems"
gem "minitest"
require "minitest/autorun"
require "minitest/pride"
load "solve"

describe Equations do

    before do
        @equations = Equations.new
    end

    describe "m_with_hz" do
        it "must return correct value" do
            @equations.m_with_hz(300e12).must_equal 1.0e-06
            @equations.m_with_hz(3e6).must_equal 100
        end
    end

    describe "hz_with_m" do
        it "must return correct value" do
            @equations.hz_with_m(100e3).must_equal 3e3
            @equations.hz_with_m(100.0e-12).must_equal 3e18
        end
    end

    describe "qudrtc_frml_with_a_b_c" do
        it "must return correct value" do
            @equations.qudrtc_frml_with_a_b_c(1, 2, -3).must_equal [1.0, -3.0]
        end
    end

    describe "pythgrn_thrm_with_a_b" do
        it "must return correct value" do
            @equations.pythgrn_thrm_with_a_b(3, 4).must_equal 5
            @equations.pythgrn_thrm_with_a_b(4, 5).must_equal "sqrt(41)"
        end
    end

    describe "pythgrn_thrm_with_b_c" do
        it "must return correct value" do
            @equations.pythgrn_thrm_with_b_c(4, 5).must_equal 3
            @equations.pythgrn_thrm_with_b_c(3, 4).must_equal "sqrt(7)"
        end
    end

    describe "equation does not exist" do
        it "must warn user" do
            @equations.doesnt_exist(500).must_equal "That equation doesn't exist here. (yet)"
        end
    end

end

describe Cli do

    before do
        @cli = Cli.new
        @error_msg = "Invalid number"
    end

    describe "an equation is ran" do
        it "must warn user if an invalid number is used" do
            @cli.doesnt_matter("hello").must_equal @error_msg
            @cli.doesnt_matter("34five").must_equal @error_msg
            @cli.doesnt_matter("q12").must_equal @error_msg
        end

        it "must work if a valid equation is ran" do
            @cli.doesnt_matter("123.3").wont_equal @error_msg
            @cli.doesnt_matter("12e3").wont_equal @error_msg
            @cli.doesnt_matter("1e-4").wont_equal @error_msg
        end
    end

end

describe "flags" do

    describe "--load" do

        before do
            @cli = Cli.new
        end

        it "must load equations" do
            # in progress
            define_equation("sample_equations")
        end
    end

end

# write tests for importing equations
\$\endgroup\$
2
\$\begingroup\$

There is a bug in your quadratic formula. Watch your operator associativity!

I'm not a fan of the method names qudrtc_frml_with_a_b_c and pythgrn_thrm_with_a_b, and I think that the poor naming is a symptom of a poor class design.

First of all, drpng vwls makes your interface hard for others to use. Learn from Ken Thompson's greatest regret.

Next, I would remove "formula" and "theorem" from the method names. The caller generally doesn't care how you solve the quadratic equation — whether you do it by using the quadratic formula, completing the square, etc. Similarly, the fact that there is a Pythagorean Theorem is unimportant; the focus should be on the subject of the Pythagorean Theorem, which is the right triangle.

Third, I question the benefit of adding all of these unrelated equations as methods of one class.

Finally, the suffixes …_with_a_b and …_with_b_c for your Pythagorean solver suggest that your solution is too rigid. It would be nice if the user could enter all but one of the variables as constraints, and ask the program to solve for the remaining unknown.

I suggest the following design. It takes advantage of one of Ruby's strengths, which is the ability to craft a human-friendly domain-specific language.

require 'cmath'

class QuadraticEquation
  attr_writer :a, :b, :c, :x

  def x
    positive = (-@b + CMath.sqrt(@b ** 2 - 4 * @a * @c)) / (2 * @a)
    negative = (-@b - CMath.sqrt(@b ** 2 - 4 * @a * @c)) / (2 * @a)
    [positive, negative]
  end

  def a
    (-@b * @x - @c) / (@x ** 2)
  end

  def b
    (-@a * @x ** 2 - @c) / @x
  end

  def c
    -@a * @x ** 2 - @b * @x
  end
end

class RightTriangle
  attr_writer :a, :b, :c

  def a
    Math.sqrt(@c ** 2 - @b ** 2)
  end

  def b
    Math.sqrt(@c ** 2 - @a ** 2)
  end

  def c
    Math.sqrt(@a ** 2 + @b ** 2)
  end

  alias :hypotenuse  :c
  alias :hypotenuse= :c=
end

Here it is in use:

irb(main):001:0> require 'formula'
=> true
irb(main):002:0> triangle = RightTriangle.new
=> #<RightTriangle:0x007fe14301f500>
irb(main):003:0> triangle.hypotenuse = 5
=> 5
irb(main):004:0> triangle.a = 3
=> 3
irb(main):005:0> triangle.b
=> 4.0
\$\endgroup\$
  • 1
    \$\begingroup\$ +1 - nice answer! I like your design, but I would have not initialized the parameters as 0 - instead I would leave them as nil and define the getters as @a || (-@b * @x - @c) / (@x ** 2). This way all getters will give valid results, and not just the ones which were not initialized \$\endgroup\$ – Uri Agassi Apr 21 '14 at 16:49
  • \$\begingroup\$ @UriAgassi I've incorporated your suggestion to leave the constraints uninitialized. I'm unsure about your second suggestion, though. Continuing with the triangle example, if the caller then sets triangle.b = 7 and asks to solve for triangle.a, what would happen? I would be necessary to set triangle.a = nil before calling triangle.a. \$\endgroup\$ – 200_success Apr 21 '14 at 17:41
  • \$\begingroup\$ @UriAgassi What do you think of abusing the ? suffix? So, a= to set a constraint, a to retrieve a constraint, a? to solve for an unknown. It violates the naming convention that ? marks a predicate, but works well for this domain-specific language. \$\endgroup\$ – 200_success Apr 21 '14 at 17:44
  • \$\begingroup\$ I think it is very unconventional, but I kinda like it :-) it gives a consistent language, and once the ground rules are set, is self-explanatory. My problem is what if the caller does set all the variables? For example - a=5, b=12, c=15? now c? == 13 and c == 15 inconsistent sets should be validated with or without the new convention... \$\endgroup\$ – Uri Agassi Apr 21 '14 at 18:56
  • \$\begingroup\$ @UriAgassi I've posted a second answer. In an over-constrained situation, just solve for the unknown as if it were never set. \$\endgroup\$ – 200_success Apr 21 '14 at 19:25
2
\$\begingroup\$

I believe you are using eval much too liberally.

The feature for adding new formulas is debatable (personally it is more of a security issue than a value feature, since you make no validations on the code you are evaluating), but for parsing arguments, the use of eval is completely redundant. Why don't you simply use built-in parsers for numbers, like BigDecimal?

require 'bigdecimal'

BigDecimal.new("123.3").to_f
# => 123.3
BigDecimal.new("12e3").to_f
# => 12000.0
BigDecimal.new("1e-4").to_f
# => 0.0001

On a different note, I was not familiar with Readline.completion_proc, and it looks really cool! I'm going to look for where I might make use of that!

\$\endgroup\$
1
\$\begingroup\$

In response to a comment from @UriAgassi on my previous answer, I've devised this variant that distinguishes between retrieving constraints and solving for unknowns.

The naming convention is:

  • a= to set a constraint
  • a to retrieve a constraint
  • a? to solve for an unknown

It violates the naming convention that ? marks a predicate, but works well for this domain-specific language.

require 'cmath'

class QuadraticEquation
  attr_accessor :a, :b, :c, :x

  def x?
    positive = (-b + CMath.sqrt(b ** 2 - 4 * a * c)) / (2 * a)
    negative = (-b - CMath.sqrt(b ** 2 - 4 * a * c)) / (2 * a)
    [positive, negative]
  end

  def a?
    (-b * x - c) / (x ** 2)
  end

  def b?
    (-a * x ** 2 - c) / x
  end

  def c?
    -a * x ** 2 - b * x
  end
end

class RightTriangle
  attr_accessor :a, :b, :c

  def a?
    Math.sqrt(c ** 2 - b ** 2)
  end

  def b?
    Math.sqrt(c ** 2 - a ** 2)
  end

  def c?
    Math.sqrt(a ** 2 + b ** 2)
  end

  alias :hypotenuse  :c
  alias :hypotenuse= :c=
  alias :hypotenuse? :c?
end

One improvement is that the code above isn't littered with @ sigils. However, the main benefit of disambiguating between retrieving constraints and solving for unknowns can be observed in an over-constrained situation:

irb(main):001:0> require 'formula'
=> true
irb(main):002:0> triangle = RightTriangle.new
=> #<RightTriangle:0x007f90b401f800>
irb(main):003:0> triangle.a, triangle.b, triangle.c = 5, 12, 37
=> [5, 12, 37]
irb(main):004:0> triangle.a?
=> 35.0
irb(main):005:0> triangle.c?
=> 13.0
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.