Proposed Solution #3

safafa · 2022-01-18T15:04:57Z

In this PR I tried solving the given problem by making these changes :

Basket_controller.rb :

Implemented index, new, show, create actions
Implemented Build_goods private method. It's used in the create action and allows us to build the goods that belong to the newly created basket.

goods.rb :

Implemented calulate_tax_update_price a callback method that allows us to calculate the taxes and update the good's price.

basket.rb

Implemented update_taxes_update_price a callback method that allows us to calculate the Sales taxes and update the basket's total price.

The final output :

nachoabad · 2022-01-19T09:01:27Z

Congratulations on your first PR Safa! 🎉

I will probably write many comments but don't feel discouraged 😄 We all went through learning these things by someone else pointing them out to us.

nachoabad · 2022-01-19T09:06:21Z

As a first tip, I suggest you always assign yourself your own PRs:

And request someone (or multiple team members) you think could be interested in reviewing it:

nachoabad · 2022-01-19T12:13:08Z

You used camel case to name your branch workBranch, but the most common style is lower cases with hyphens: short-descriptive-name.

Also, there is no need to use the word branch in a git branch.

nachoabad · 2022-01-19T12:35:03Z

spec/features/basket_2.txt

@@ -0,0 +1,2 @@
+1 imported box of chocolates at 10.00
+1 imported bottle of perfume at 47.50


You have repeated this file here and in the root folder. Normally these kinds of files should be all under a descriptive directory name (input_baskets?... naming is hard 😅)

nachoabad · 2022-01-19T12:51:58Z

README.md


-### EXPECTED OUTPUT
+yarn


It's great that you wrote a README detailing the versions and dependencies 👏🏽.

However, this plain line is confusing: are we supposed to install yarn? Run the yarn command? Think of the end-user and try to write instructions as simple and concrete as possible.

nachoabad · 2022-01-19T15:12:43Z

config/routes.rb

@@ -0,0 +1,5 @@
+Rails.application.routes.draw do
+  root 'baskets#index'
+  resources :baskets, except: [:destroy] 


Great use of Rails conventions here 👍🏽

nachoabad · 2022-01-19T15:20:24Z

Gemfile

+
+# Reduces boot times through caching; required in config/boot.rb
+gem 'bootsnap', '>= 1.4.2', require: false
+gem 'rubocop', '>= 1.0', '< 2.0'


Great that you use Rubocop and that there are no offenses 👏🏽 . But follow documentation best practices and add require: false since it is a standalone tool.

nachoabad · 2022-01-19T15:32:20Z

app/controllers/baskets_controller.rb

+
+  private
+
+  def build_goods(basket)


It's good that you used a private to not expose it as a public method. But this logic might not belong to a controller.

A controller is a type of class that's responsible for making sense out of a request made in an application and producing appropriate output. So this logic to split and parse a file can go in another class. That way, you can easily unit test that class and the controller just has one line to call that class.

nachoabad · 2022-01-19T15:46:59Z

app/models/basket.rb

@@ -1,3 +1,7 @@
 class Basket < ApplicationRecord
  has_many :goods, dependent: :destroy
+
+  def handle_file(file)


In my opinion, this method does not belong to the basket class, because all it does is it splits any file by line. Is unrelated to the domain of a Basket. It just so happens that you need it to get data related to a Basket, but that is all.

In this case, you could extract a class: https://refactoring.guru/extract-class

nachoabad · 2022-01-20T09:00:00Z

app/models/good.rb

+  private
+
+  def calulate_tax_update_price
+    exampt = %w[chocolate book pills]


In this case, because those values are going to be constant and might be needed elsewhere in the application, a good Ruby practice is to extract it as a constant and place it at the top of the class:

EXEMPT_FROM_TAX = %w[chocolate book pills]

nachoabad · 2022-01-20T09:02:57Z

app/models/good.rb

+                                                                 end
+    self.import_tax = (price.to_f.round(2) * 0.05).round(2) if name.include?('imported')
+    fprice = quantity.to_f * (price.to_f.round(2) + basic_tax.to_f.round(2) + import_tax.to_f.round(2))
+    self.final_price = fprice.round(2)


Here, in just one method, you are setting basic_tax, import_tax, and final_price. You could extract 3 individual methods for each so it's easier and clearer to unit test each one.

nachoabad · 2022-01-20T09:04:55Z

app/views/baskets/_goods.html.erb

@@ -0,0 +1,5 @@
+<ul>


Nice use of partials 👏🏽

nachoabad · 2022-01-20T09:15:04Z

app/models/basket.rb

+    self.total = sum_total
+  end
+
+  def sum_tax


This is just a matter of style, but you can TDD this by first writing the spec with the different values you expect, and then write the method and run the specs as you write it. Again, this is not wrong, just a preference that you could try and decide what works best for you 😄

nachoabad · 2022-01-20T09:17:50Z

Gemfile

@@ -46,7 +46,9 @@ end
 group :test do
  # Adds support for Capybara system testing and selenium driver
  gem 'capybara', '>= 2.15'
+  gem 'rspec-rails', '~> 5.0.0'


Just a tip:

If you plan to use Rspec you can create your application like this: rails new my_app -T so you don't have the standard minitest folder

nachoabad · 2022-01-20T09:24:43Z

app/models/good.rb

    self.final_price = fprice.round(2)
  end
+
+  def round_up(value)
+    (value * 20).ceil / 20.0


Here you can extract those magic numbers into a constant and give them a descriptive name. More on that here: https://refactoring.guru/replace-magic-number-with-symbolic-constant

nachoabad · 2022-01-20T09:27:41Z

spec/models/good_spec.rb

+      @imported = Good.new(name: 'imported', quantity: '1', price: '47.50')
+      @exampt = Good.new(name: 'book', quantity: '1', price: '12.49')
+      @basic = Good.new(name: 'music', quantity: '1', price: '14.99')
+    end


Here you can create factories or fixtures, and reuse them throughout your code. More on that here: https://semaphoreci.com/community/tutorials/working-effectively-with-data-factories-using-factorygirl

nachoabad · 2022-01-20T09:32:17Z

spec/models/good_spec.rb

+      @imported.destroy
+      @exampt.destroy
+      @basic.destroy
+    end


Same here: factories will take care to create themselves and clean up after the spec, making your tests more readable, and you are free from doing this house cleaning.

nachoabad · 2022-01-20T09:35:01Z

spec/models/good_spec.rb

+      @exampt.save
+      expect(@exampt.final_price).to be == '12.49'
+    end
+  end


Is this spec you need to use context so the reader knows what each spec is doing. More on that here: https://www.betterspecs.org/#contexts.

Otherwise it can be confusing to read because in line 33 it calculates basic tax, but it line 38 it does not 😄 If you write a descriptive context line before the text, it then makes sense.

nachoabad · 2022-01-20T09:42:14Z

spec/features/baskets_spec.rb

+      click_button 'submit'
+      expect(page).to have_content '1 imported bottle of perfume : 54.65'
+    end
+end


Nice that you feature spec this 👏🏽. To improve this test, maybe you can extract the common visit + attach + submit into a reusable before methods. Or better yet tests the 4 expected outputs in just one go, making your tests faster.

nachoabad · 2022-01-20T11:02:15Z

app/models/good.rb

+  def calulate_tax_update_price
+    exampt = %w[chocolate book pills]
+    self.basic_tax = round_up(price.to_f.round(2) * 0.1) unless exampt.any? do |exeption|
+                                                                  name.include?(exeption)


Tiny orthography errors: I think it should be exempt and exception

nachoabad · 2022-01-20T12:36:45Z

spec/models/good_spec.rb

@@ -4,4 +4,40 @@
  describe 'associations' do
    it { should belong_to(:basket) }
  end
+
+  describe 'tax calculation' do


When unit testing a model, a good practice is to describe the methods. More on that here: https://www.betterspecs.org/#describe

Safa ABALLAGH and others added 11 commits January 17, 2022 18:08

Set up linters

9767a33

Add Basket & Good models Setup associantion

01d8747

Setup routes Create actions & index view

c2023d7

Add method calculate tax for goods

60f06db

Update show view

97148dd

Update taxes and total price for basket

32b9f30

Setup rspec and shoulda

d4e2bcb

Implement round up

b4cfe2c

Add tests Fix good name

5ccbc5e

Fix linter err

e538d27

Update README.md

d663534

nachoabad self-requested a review January 19, 2022 08:53

safafa self-assigned this Jan 19, 2022

nachoabad reviewed Jan 19, 2022

View reviewed changes

nachoabad reviewed Jan 20, 2022

View reviewed changes

app/views/baskets/_goods.html.erb

@@ -0,0 +1,5 @@

<ul>

Copy link

nachoabad Jan 20, 2022

Choose a reason for hiding this comment

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

Nice use of partials 👏🏽

safafa reacted with thumbs up emoji

nachoabad reviewed Jan 20, 2022

View reviewed changes

Safa ABALLAGH added 9 commits January 20, 2022 17:28

Refactor baskets_spec.rb add before method

ab1c73d

Refactor good model & add magic numbers

0983ba9

Move build_goods to baskets_helper

f64af76

Remove handle_file from basket

b399fab

Setup factorybot test callback method

3cdb270

Correct good's factory

eae6920

Refactor build_goods_from_file

f02be56

Test basket model

077fa2d

Refactor tests

85e8ff2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposed Solution #3

Proposed Solution #3

safafa commented Jan 18, 2022 •

edited

Loading

nachoabad commented Jan 19, 2022 •

edited

Loading

nachoabad commented Jan 19, 2022

nachoabad commented Jan 19, 2022 •

edited

Loading

nachoabad Jan 19, 2022

nachoabad Jan 19, 2022

nachoabad Jan 19, 2022

nachoabad Jan 19, 2022 •

edited

Loading

nachoabad Jan 19, 2022

nachoabad Jan 19, 2022 •

edited

Loading

nachoabad Jan 20, 2022

nachoabad Jan 20, 2022

nachoabad Jan 20, 2022

nachoabad Jan 20, 2022

nachoabad Jan 20, 2022

nachoabad Jan 20, 2022

nachoabad Jan 20, 2022

nachoabad Jan 20, 2022

nachoabad Jan 20, 2022 •

edited

Loading

nachoabad Jan 20, 2022

nachoabad Jan 20, 2022

nachoabad Jan 20, 2022 •

edited

Loading

		@@ -0,0 +1,2 @@
		1 imported box of chocolates at 10.00
		1 imported bottle of perfume at 47.50

Proposed Solution #3

Are you sure you want to change the base?

Proposed Solution #3

Conversation

safafa commented Jan 18, 2022 • edited Loading

nachoabad commented Jan 19, 2022 • edited Loading

nachoabad commented Jan 19, 2022

nachoabad commented Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nachoabad Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nachoabad Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nachoabad Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nachoabad Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

safafa commented Jan 18, 2022 •

edited

Loading

nachoabad commented Jan 19, 2022 •

edited

Loading

nachoabad commented Jan 19, 2022 •

edited

Loading

nachoabad Jan 19, 2022 •

edited

Loading

nachoabad Jan 19, 2022 •

edited

Loading

nachoabad Jan 20, 2022 •

edited

Loading

nachoabad Jan 20, 2022 •

edited

Loading