I don't notice it when I'm writing it, and even if I look at it after I finish writing it, it's difficult to disassemble and reconstruct the code I wrote. That's because you need a different approach to writing code when rebuilding. The place where I once thought that this was the case and rewrote the code with a blank head is the mountain. This time, I will write out some patterns that I noticed in such a rewritten part. It is a memorandum based on my own experience.
Actually, the process you want to do is the same, but there are two types of param contents that are passed as arguments, on_total and on_this_month, and if you detect either of those two, you wanted to include a process to sort. The first one I wrote is below.
bad_1.rb
def self.ascend_checkins(param, hash)
if param == "on_total"
hash.sort_by { |k,v| -v["total"] }.to_h
elsif param == "on_this_month"
hash.sort_by { |k,v| -v["month"] }.to_h
end
end
For the reason mentioned above, the same sort process has been included, so modify it as follows.
mod_1.rb
def self.ascend_checkins(hash, param)
key_of = {"on_total" => "total", "on_this_month" => "month"}
value = key_of[param]
raise "unknown param" if !value
hash.sort_by { |k,v| -v[value] }.to_h
end
Since there are only two params, prepare a hash, receive the param, judge the value corresponding to it by the boolean value, and if it is true, delete the same process by executing the process.
The following is the process of extracting the user ID as a character string, counting the number of it, and converting it into a hash. It has become a process with many temporary variables when preparing an empty array.
bad_2.rb
def self.get_monthly_count(ids, users)
uids = []
ids.each do |id|
uids.push id.uid.to_s
end
uid = {}
count = 0
users.each do |user|
uid[user] = uids[count]
count += 1
end
uid.to_h
end
It's tidy up in one line, in the case of Ruby.
mod_2.rb
def self.get_monthly_count(ids, users)
users.zip(ids.map{ |id| id.uid.to_s }).to_h
end
First, if you use the map method, you only need one line to extract it as a character string and recreate it as an array. If you create an empty array and write processing using indexes, it seems that you can write clean code from the beginning if you think of using this kind of method first. After that, I use the zip method to create a pair array and finally hash it.
It is a method that can combine elements of an array like a chuck.
zip.rb
array_keys = ["Tom", "Bob", "Joge"]
array_values = [167, 156, 181]
zipped = array_keys.zip(array_values)
p zipped # [["Tom", 167], ["Bob", 156], ["Joge", 181]]
hash = zipped.to_h
p hash # {"Tom"=>167, "Bob"=>156, "Joge"=>181}
There are three, user_names, uid, and users, but you don't need to create them like this.
bad_3.rb
def self.get(check_ins)
user_names = []
check_ins.each do |check_in|
uid = check_in.uid
users = community_number_collection.document(uid).get
user_names.push users.data[:nickname]
end
user_names
end
If you have an empty array, use map to process it, and put it on one line without creating extra variables.
mod_3.rb
def self.get(check_ins)
check_ins.map do |check_in|
community_number_collection.document(check_in.uid).get.data[:nickname]
end
end
Combine the ones mentioned above to shorten the process.
bad_4.rb
def self.get_total(merged_data, users)
sum = []
merged_data.keys.each do |value|
uid = merged_data[value]["uid"]
sum.push self.where(uid: uid).count
end
sum
total = {}
count = 0
users.each do |user|
total[user] = sum[count]
count += 1
end
total
end
Map and zip will make it much shorter. It was also a discovery that the entire block processing was pushed into a variable called sum. The block is an expression, and it will be possible to assign it.
mod_4.rb
def self.get_total(merged_data, users)
sums = merged_data.keys.map do |key|
self.where(uid: merged_data[key]["uid"]).count
end
users.zip(sums).to_h
end
Hashes count duplicates and are useful for aggregating. But the shorter it is, the better.
bad_5.rb
def self.get_this_month(check_ins)
check = {}
check_ins.each do |check_in|
uid = check_in.uid.to_s
uid_count = check[uid] || 0
check[uid] = uid_count + 1
end
check.values.each do |value|
value
end
end
I haven't used an empty array this time, but if you put the process of extracting the elements of the original array into each, it's better to recreate the desired array with map and turn that array with each. .. Also, if you use the default method, you can set all values to 0 once.
mod_5.rb
def self.get_this_month(check_ins)
check = {}
check.default = 0
check_ins.map{ |c| c.uid.to_s }.each do |uid|
check[uid] += 1
end
check.values
end
Before the code below, there were two functions that did the same thing, except that the keys of the hash passed were different. I put it together in one abstract function. After preparing an empty array, declare an empty array with key specified (nested = {}), and in each, a hash with no value is created for key (nested [key] = {} ).
sample_1.rb
def self.label_hash(hash_dictionary, keys)
nested = {}
keys.each do |key|
nested[key] = {}
hash_dictionary.each do |name, hash|
nested[key][name] = hash[key]
end
end
nested
end
As a calling method, the method of grouping the arguments into a hash is used. In the upper function, the words used are close to abstract, and in the lower call, words with strong concreteness are used. You can see that the roles are clearly separated by naming.
sample_2.rb
labeled_totally_count = CheckIn.label_hash({
"month" => zipped_monthly_count,
"total" => zipped_totally_count
}, zipped_monthly_count.keys)
--When there are many temporary variables, imagine how to write them erased.
--Be aware of the moment you use the method provided by Ruby depending on the situation (especially if you are writing an empty array and index counting process)
Another problem with writing code that is difficult to read is that the name you are already using is rooted in the programming world as a different concept, so a skilled engineer can see the code. Be aware that it can often be recalled at the end, leading to misreading, and as a result, the code you write is hard to read.
Recommended Posts