Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

RCurtain 1.0 - Refactor code and update version #11

Open
wants to merge 52 commits into
base: master
Choose a base branch
from
Open

RCurtain 1.0 - Refactor code and update version #11

wants to merge 52 commits into from

Conversation

lucasqueiroz
Copy link

@lucasqueiroz lucasqueiroz commented Jul 11, 2018

RCurtain 1.0 - Refactor code and update version

  • I have followed the guidelines at the Contribution document
  • I have created unit tests

Description

⚠️This is a major release, because the name of the RCurtain class was changed ⚠️

⚠️Before the Pull Request is merged, Codacy should be configured, and TravisCI removed. ⚠️

def allowed_percentage(feature_name)
allowed_percentage = RCurtain.feature.percentage(feature_name)
default_percentage = RCurtain.configuration.default_percentage
allowed_percentage.nil? ? default_percentage : allowed_percentage
Copy link

Choose a reason for hiding this comment

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

Aqui você poderia ter feito:

def allowed_percentage(feature_name)
  RCurtain.feature.percentage(feature_name) || RCurtain.configuration.default_percentage
end

def percentage_allowed?(feature_name)
allowed_percentage = allowed_percentage(feature_name).to_i
rand_percentage = rand(0..100)
rand_percentage <= allowed_percentage
Copy link

Choose a reason for hiding this comment

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

Essa variável rand_percentage é desnecessária. Crie um método pro rand(0..100) no lugar de criar uma variável.

users.nil? || users.empty?
end
def percentage_allowed?(feature_name)
allowed_percentage = allowed_percentage(feature_name).to_i
Copy link

Choose a reason for hiding this comment

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

Outra variável desnecessária

@redis = Redis.new(url: RCurtain.configuration.url)
end

def add_users(feature_name, users)
Copy link

Choose a reason for hiding this comment

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

Deveria criar classes para Users e Percentage

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants