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