Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I just want to know if this looks good.

 require 'sqlite3'

  # Code to open the SQlite database.
  @db = SQLite3::Database.open "data.db"


  # Method used to start the information gathering.
  def start
    puts "Enter a Bots name:"
    @name = gets.chomp
    puts "Enter the age:"
    @age = gets.chomp
    puts "Enter a job:"
    @job = gets.chomp
  end


  # Method used to insert the data into SQlite.
  def insert
    {
      "name:" => @name,
      "age:" => @age,
      "job:" => @job
    }.each do |pair|
      @db.execute "insert into bots values ( ?, ?,? )", pair
    end
  end


  # Method used to put info on the created bot and asks to show all bots.
  def view
    puts "Your bot has been added..."
    puts "The name is #{@name} and the age is #{@age} and the job is #{@job}"
    sleep 3
    puts "Would you like to list all the bots?"
    answer = gets.chomp.downcase

    if answer == "yes"
      @db.execute( "select * from bots" ) do |row|
      puts  row 
    end
  else
    puts "Goodbye!"
  end


  # Close the SQlite data connection.
  @db.close()
end


# Start the sections of the program.
start
insert
view
share|improve this question

closed as off-topic by rolfl Jun 26 at 14:49

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. Such questions may be suitable for Stack Overflow or Programmers. After the question has been edited to contain working code, we will consider reopening it." – rolfl
If this question can be reworded to fit the rules in the help center, please edit the question.

    
I don't think your end are all paired up. Please fix the code in the question? –  200_success Jun 25 at 22:10
    
It also doesn't make sense that you expect a pair to fill three ? placeholders. –  200_success Jun 25 at 22:14
    
The code works fine last I tested it.. –  Mark Jun 26 at 4:48
    
This code is not broken, it works fine. I was just asking if the style is ok. and if there were better methods..aka code review? –  Mark Jun 26 at 4:49
    
Re-closed. Desplite your assurances that the code works, I cannot see what the end immediately after the @db.close() matches up with. There is no begin for that, and no other control flow structure I can see either. Care to point me to where the matching start is? –  rolfl Jun 26 at 15:19
add comment

1 Answer

up vote 4 down vote accepted

I can only comment on SQL, a few things I would improve on this line:

@db.execute "insert into bots values ( ?, ?,? )", pair

  1. I would recommend to always explicitly declare which columns you are using on an INSERT clause. The main reason, aside from clarity, is that if a column was added or dropped or just moved around somehow, then your data would get inserted in the wrong place.

  2. Nitpicking a bit, but I think SQL is easier to read if you capitalize reserved words to make them easier to tell from columns/tables/aliases/etc. Not so big a deal in such a small script but in a large script it would make a big difference. Also best to try to keep the spacing consistent. So how about this:

@db.execute "INSERT INTO bots (name,age,job) VALUES (?,?,?)", pair

Hopefully someone is better equipped than I to comment on the meat of your script.

share|improve this answer
1  
I refactored the code to look better.. And thanks for the nitpicking i need it github.com/iheartkode/Bots/blob/master/Bot.rb –  Mark Jun 25 at 21:53
add comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.