1
\$\begingroup\$

I wrote a script in Ruby for a better family budget overview. The expenses are hold under a CSV file. Any feedback is welcomed!

require "csv"

FILE_DB = Dir.home << "/expenses2017.csv"
CATHEGORIES = %w[ Baby Food Car Travel Books Poker Programming Rent OtherBills Beauty ]
MONTHS = %w[ Jan Feb Mar Apr May Jun Jul Aug Sep Oct Nov Dec ]

class Expense

    @@Description_min_length = 3
    @@Min_amount = 1
    @@Abort_msg = "Run the program again."

    attr_accessor :cathegory, :description, :amount, :month

    private def capitalize_first_letter_and_downcase_the_remaining_ones(string)
        string[0].upcase << string[1, string.length].downcase
    end



def cathegory=(value)
            value = capitalize_first_letter_and_downcase_the_remaining_ones(value)
            if not CATHEGORIES.include?(value)
                puts "Invalid cathegory, valid ones are #{CATHEGORIES.join(",")}"
                abort @@Abort_msg
            end
            @cathegory = value
        end

        def description=(value)
            if value.length < @@Description_min_length
                puts "Description can't be less than #{@@Description_min_length}"
                abort @@Abort_msg
            end
            @description = value
        end

        def amount=(value)
            if value < @@Min_amount
                puts "Amount can't be less than #{@@Min_amount}"
                abort @@Abort_msg
            end
            @amount = value
        end

        def month=(value)
            value = capitalize_first_letter_and_downcase_the_remaining_ones(value)
            if not MONTHS.include?(value)
                puts "Invalid month, valid ones are #{MONTHS.join(",")}"
                abort @@Abort_msg
            end
            @month = value
        end

        def initialize(cathegory, description, amount, month)
            self.cathegory = cathegory
            self.description = description
            self.amount = amount
            self.month = month
        end

        def to_s
            "#{@cathegory} #{@description} #{@amount.to_s} #{@time}"
        end

        def to_array
            [cathegory, description, amount.to_s, month]
        end

end

    class InfoGainer
        attr_reader :item

        def initialize(column, column_value)
            @column = column
            @column_value = column_value
        end

        def get_info
            if @column[0] == "m"
                get_money_spend(@column_value, true, false)
            elsif @column[0] == "c"
                get_money_spend(@column_value, false, true)
            else

            end
        end

    def get_money_spend(spend_criteria, month=True, cathegory=False)
         sum = 0

         CSV.foreach(FILE_DB) do |row|
             row_content = row.inspect.split(",")
             month_column_value = row_content[3][2,3]
             cathegory_column_value = row_content[0][2..row_content[0].length - 2]

             if month && spend_criteria == month_column_value
                sum += row_content[2][2, 1].to_i
             elsif cathegory && spend_criteria == cathegory_column_value
                sum += row_content[2][2, 1].to_i
             end
         end

         sum
    end

end

def print_friendly_sequence(sequence)
    result = ""
    sequence.each_with_index do |val, index|
        result << "#{index + 1}.#{val}\n"
    end
    puts result
end

def take_input
    puts "Enter your expense in a line format 'Cathegory Description Amount Month'"
    puts "Example => 'Food bread 1 Jan'"
    $stdin.gets.chomp.split(" ")
end

def write_expense_to_file(expense)
    if File.exists?(FILE_DB)
        CSV.open(FILE_DB, "a+") do |csv_file|
            csv_file << expense.to_array
        end
    else
        CSV.open(FILE_DB, "w") do |csv_file|
            csv_file << %w[ cathegory description amount month]
            csv_file << expense.to_array
        end
    end
    puts "Record has been added."
end

def welcome
    puts "Expense diary/calculator. Press: "
    puts "1 for adding a new expense."
    puts "2 for information about the money spend for specific month."
    puts "3 for information about the money for specific cathegory."
    $stdin.gets.chomp.to_i
end

def get_proper_info_gainer(choice)
    if choice == 2
        puts "Enter a month: "
        month = $stdin.gets.chomp
        ig = InfoGainer.new("month", month)
        return ig
    elsif choice == 3
        puts "Enter a cathegory: "
        cath = $stdin.gets.chomp
        ig = InfoGainer.new("cathegory", cath)
        return ig
    else
        puts "Enter a price: "
        price = $stin.gets.chomp.to_i
        InfoGainer.new("price", price)
    end
end


puts "Cathegories"
print_friendly_sequence(CATHEGORIES)
puts "\n"

puts "Months"
print_friendly_sequence(MONTHS)
puts "\n"
choice = welcome


if choice == 1
    input_parts = take_input
    expense = Expense.new(input_parts[0], input_parts[1], input_parts[2].to_i, input_parts[3])
    write_expense_to_file(expense)
else
    info_gainer = get_proper_info_gainer(choice)
    answer = info_gainer.get_info
    puts "You have spend #{answer}"
end
\$\endgroup\$
  • \$\begingroup\$ Not sure you need the method named capitalize_first_letter_and_downcase_the_remaining_ones when you can simply .downcase.capitalize any string. \$\endgroup\$ – Mark Thomas Jan 10 '17 at 16:17
3
\$\begingroup\$

Small feedback:

  1. cathegory => category (check grammar)
  2. capitalize_first_letter_and_downcase_the_remaining_ones - you don't need this method, you can just do value.capitalize 2) if not => unless
  3. class variables should be in snake_case (Min_amount => min_amount and so on) It's also not recommended to use class variables at all (https://github.com/bbatsov/ruby-style-guide#no-class-vars)
  4. Expense#initialize takes a lot of arguments, consider using arguments hash or Struct
  5. get_money_spend(spend_criteria, month=True, cathegory=False) should be get_money_spend(spend_criteria, month=true, cathegory=false) (maybe this method should be named get_money_spent or smth like that)

I also think code needs more improvements besides these.

| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

General Ruby Things

attr_accessor vs. attr_reader

In Expense, don't use attr_accessor if you are defining a writer method, use attr_reader instead. It doesn't cause any harm as you are simply redefining the writer methods down below, but it might confuse people expecting certain behaviors later.

Symbols as identifiers

Symbols are a common way to create identifiers instead of using full String objects. Some benefits are memory efficiency, but also communicating what you expect their role to be in the code.

# An Array of Symbols   
CATEGORIES = [:baby, :food, :car, :travel, :books, ... ]

# In Ruby 2.0+
CATEGORIES = %i(baby food car travel books)

Class variables are generally scary

While there are some use cases for class variables (@@ident), I've found they cause more harm then good. In the Expense class you seem to be using them in place of constants, which you do have in the global scope. You can put constants in class scope as well. If you're trying to avoid making the available outside the class, use a private method or inject an instance of another class that contains the configuration.

Don't abort the program from within a class

This is something that you'll find more troublesome as you work on larger and larger projects. Consider the responsibility of each class. Is the Expense class responsible for deciding whether to continue execution in an error state? Generally the top level "main" method can handle exceptional cases and decide how to proceed. This also becomes a requirement when you start writing tests for your code.

class Expense
  def category=(value)
    # Raise an error condition here.
    raise ArgumentError, "Invalid Category!" unless CATEGORIES.include?(value.to_sym)
    @category = value
  end
end

def main
  # ...
  # handle user input, create Expense objects, etc.
rescue => e
  warn "#{e.class}: #{e}"
  exit 1
end

Code Layout

What's an InfoGainer?

Maintainable code should be self-documenting. It's not clear to me what an InfoGainer does, exactly, by reading the code. After a few passes I was able to surmise that it finds Expenses based on criteria. It's a bit advanced, but take a look at the Repository Pattern.

Separate concerns: database parsing/updates vs. program logic.

You program does two main things: find Expenses by some logic, and add new Expenses. Ideally, each set of concerns is mapped in a layered architecture, such as:

class Database
  def initialize(filename)
    @filename = filename
    # load filename, convert data to mapped set of Expenses
    @all_expenses = parse_file(@filename)
  end

  def get_expenses_by_month(month)
  end

  def get_expenses_by_category(category)
  end

  def add_expense(expense)
  end

  def save!
    write_expenses(@all_expenses, @filename)
  end
end

In this way, you could even swap out your CSV file for a SQLite database or remote MySQL database, for example, since your "business logic" (making new expenses, computing reports) is completely separate from your persistence logic. This approach also allows you to test one core aspect of your program separate from the others.

Separate concerns: User interaction.

Likewise, if you wrapped your user interaction logic you'd be able to build a GUI or web-service version of the application without touching the business logic on persistence layers.


I hope this helps and keep at it!

| improve this answer | |
\$\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.