Skip to content

Review#1

Open
ivanovaolga wants to merge 1 commit intomasterfrom
new
Open

Review#1
ivanovaolga wants to merge 1 commit intomasterfrom
new

Conversation

@ivanovaolga
Copy link
Collaborator

No description provided.

module Services::Sms
class Base
SMS_PROVIDERS = [
KCELL = "Kcell"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Лучше убрать лишние пробелы


SETTINGS = [
FALLBACK_PROVIDER_SETTING = "sms_service.fallback_provider",
DEFAULT_PROVIDER_SETTING = "sms_service.default_provider",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Лучше выровнять пробелы
Убрать запятую в последней строке

provider_klass_for(provider_to_use).new(phone, message)
end

attr_accessor :phone, :message, :error_code
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Лучше перенести на самый верх, чтобы не искать какие аксессоры есть в классе

end

# Services::Sms::Base.prepare("+371 20128732", "hello world", "gsm")
def self.prepare(phone, message, custom_provider=nil)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Именованные аргументы, особенно когда nil, чтобы при добавлении нового аргументы не приходилось переделывать все использования метода

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Я бы убрала комментарий, потому что непонятно зачем он нужен, использование метода хорошо понятно из объявления

# Services::Sms::Base.prepare("+371 20128732", "hello world", "gsm")
def self.prepare(phone, message, custom_provider=nil)
provider_to_use = custom_provider.presence ||
PREFERRED_SENDER_FOR[PhoneNumber::CarrierDetective.call(phone: phone)].call
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PREFERRED_SENDER_FOR не объявлен нигде

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

formatted_phone = PhoneNumber::CarrierDetective.call(phone: phone)
provider_to_use = custom_provider.presence || PREFERRED_SENDER_FOR[formatted_phone].call

end

def parse_response(response)
return if !response.body
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

return unless response.body

Copy link
Collaborator Author

Choose a reason for hiding this comment

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


def prepare_params
{
"client_message_id": current_uid,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Я бы выровняла хеш

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Prefer symbols instead of strings as hash keys.
https://github.com/rubocop-hq/ruby-style-guide#symbols-as-keys

end

private
def current_uid
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Я бы отделила пустой строкой и убрала отступ вправо в дальнейших объявлениях методов

http.request(request)
end


Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Одна пустая строка, видимо, случайно здесь)

request = Net::HTTP::Post.new(uri.request_uri, headers)
request.basic_auth(config["username"], config["password"])
request.body = params.to_json
http.request(request)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Я бы поиграла выравниваниями и пустыми строками

  uri  = URI(url)
  http = Net::HTTP.new(uri.host)
  
  request      = Net::HTTP::Post.new(uri.request_uri, headers)
  request.body = params.to_json
  request.basic_auth(config["username"], config["password"])

  http.request(request) 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant