Usertool For Website Admins -- Follow up

Question

A while ago, I posted about creating a user tool for forum admins that allows them to add, edit, and delete users from their desktops. This still is not complete, posting for review on tool so far

I've finished with the delete_user method and also added a couple new features to it, what this code does, is take your input, and save it to a YAML file ( will be converting YAML to JSON ). It's advantages are that you don't have to go directly to your database until you actually need to uplaod the file, so you can create an entire list of users from your desktop without sending one validation e-mail, or having to validate one user.

#!/usr/bin/env ruby
require 'yaml'
def menu
 print "Welcome to User Menu. Would you like to [A]dd users, [D]elete users, [E]dit users, [V]iew all users, or [Q]uit the system?\n"
 input = gets.chomp!
 case input
 when /a/i
 add_user
 when /d/i
 delete_user
 when /e/i
 edit_user
 when /v/i
 view_all
 else
 puts "Exiting system.."
 exit
 end
end
def load_hash
 File.exist?('users.yml') ? YAML.load_file('users.yml') : {users: {}}
end
def save_hash(hash)
 File.open('users.yml', 'w') { |s| s.write(hash.to_yaml) }
end
def add_user
 hash = load_hash
 hash[:users][:"#{prompt('Enter username:')}"] =
 {email_address: prompt("Enter email:"),
 member_status: prompt("Enter member status:")}
 save_hash(hash)
 add_user if restart
end
def delete_user
 hash = load_hash
 to_delete = prompt("Enter username:").to_sym
 if hash[:users].has_key?(to_delete)
 hash[:users].delete(to_delete)
 else
 puts 'User not found'
 end
 save_hash(hash)
 delete_user if restart
end
def edit_user
 edit_user if restart
end
def view_all
 puts load_hash
end
def restart
 puts "Would you like to edit another user?"
 input = gets.chomp
 if input =~ /yes/i
 return true
 else
 puts "Exiting program.."
 exit
 end
end
def prompt ( message )
 puts message
 gets.chomp
end
menu

Overview of the YAML file:

---
:username: TEST
:email_address: TEST
:member_status: TEST

Is there anything here that I can do better? Better syntax, combine methods, edit something? I'm looking for every possible example.

Answer 1

Bug

Adding a user with a username that is already taken overwrites the existing user.

Adding a user with a blank username is allowed. (Maybe you should allow alphanumeric characters only, and set a minimum required length)

Unbounded recursion

def add_user
 # ...
 add_user if restart
end

The recursion you're using for repeating this function can grow arbitrarily deep, causing a stack overflow error. Use a loop instead:

def add_user
 loop do
 # ...
 break unless restart
 end
end

Then, restart doesn't need to harshly exit the whole program. It can simply return a boolean:

def restart
 puts "Would you like to edit another user?"
 input = gets.chomp
 return input =~ /yes/i
end

(You can print "Exiting program" at the end of the program, if you want)

Also, don't exit in menu. It's not needed.

Nitpicking

print "...?\n"

Use puts instead of using a newline in the string.

if hash[:users].has_key?(to_delete)
 hash[:users].delete(to_delete)
else
 puts 'User not found'
end

Hash#delete returns the deleted value, or nil if the key is not found. So you can use its return value for the condition:

user = hash[:users].delete(to_delete)
unless user
 puts 'User not found'
end

Also, rename to_delete to username.

puts "Would you like to edit another user?"

This message appears for every action, not just editing. Pass a string argument to specify the action (e.g. "add" or "edit") and use it when printing the message. Also, rename restart to again? to be more accurate.

Creating a class

Most methods operate on the same data structure: the users hash. So it makes sense to put them all inside a class which stores the hash as a field. As a bonus, you won't need to reread the users file every time you perform an action.

class UsersList
 def add(username, email_address, member_status)
 file[:users][username.to_sym] = {
 email_address: email_address,
 member_status: member_status
 }
 end
 def remove(username)
 file[:users].delete(username)
 end
 def save
 save_hash
 end
 def to_s # for viewing
 file.to_s
 end
 private
 def file
 # Return @hash, which stores the hash
 # If it's not loaded yet (@hash is nil), initialize it with load_hash first
 @hash ||= load_hash
 end
 def load_hash
 File.exist?('users.yml') ? YAML.load_file('users.yml') : {users: {}}
 end
 def save_hash
 File.open('users.yml', 'w') { |s| s.write(@hash.to_yaml) }
 end
end

Notice all methods for accessing the hash use the private method file. (I named it file because hash is an existing method of all objects). file returns the hash, but only loads it once from the file, when it's first needed.

Note the methods only deal with the logic of the operations, not with interacting with the user, in order to seperate these two concerns.

To use this class, you'll need to make an instance of it:

$users_list = UsersList.new # global variable for handling the users list

Then change add_user, delete_user and view_all to use it:

def add_user
 loop do
 username = prompt('Enter username: ')
 email_address = prompt('Enter email:')
 member_status = prompt('Enter member status:')
 $users_list.add(username, email_address, member_status)
 break unless again?('add')
 end
end
def delete_user
 loop do
 username = prompt("Enter username:").to_sym
 unless $users_list.remove(username)
 puts 'User not found'
 end
 break unless again?('delete')
 end
end
def view_all
 puts $users_list.to_s
end

Finally, at the end of your program, save the users list:

$users_list.save

• 13aal: In order to make it so the username can only contain certain elements and a required length would I have to run it against another method?
• Spike: @LostBam In UsersList#add you can check the username against a regex (e.g. [/\A[a-zA-Z0-9]{6,}\z/] rubular.com/r/mHcUaXbjMA)), and return false if it doesn't match. Then in add_user check the return value (like delete_user does with $users_list.remove)