3
\$\begingroup\$

I'm new with Ruby and am trying to improve my coding skills. This program gets personal information for three people, and then prints them.

It works, but I'm sure it's not the best and certainly not the fanciest way to do it.

I was hoping to get some advice here on how to do it faster/shorter/better/smarter...It is really hard to improve without knowing your mistakes.

The code:

# Define a person with name, age, origin
class Person
  def input(count) 
    puts "Hi, let\'s create three new Person. Start with number #{count}"
    puts "What\'s your name?"
    @name = gets
    @name = @name.delete("\n")
    puts "How old are you?"
    @age = gets
    @age = @age.delete("\n")
    puts "Where are you from?"
    @origin = gets
    @origin = @origin.delete("\n")
    puts "I summarize! You are #{@name} and #{@age} years old from #{@origin}"
    puts "Is that correct? \nEnter \"0=Yeah\" or \"Nah\""
    @antwort = gets
    if @antwort.to_i == 0
      puts "Sweet #{@name}, let us get to the next one"
      person_return = [@name, @age,@origin]
      return person_return
    else 
      puts "Get off. Let us try again"  # How to go retry?
    end
  end
end

person_one = Person.new
var_person_one = person_one.input(1)
puts "Press Enter for next Person\n"
gets

person_two = Person.new.input(2)
var_person_two = person_two
puts "Press Enter for next Person\n"
gets

person_three = Person.new.input(3)
var_person_three = person_three

p var_person_one
p var_person_two
p var_person_three

Output:

Hi, let's create three new Person. Start with number 1
What's your name?
Kevin
How old are you?
29
Where are you from?
Hamburg
I summarize! You are Kevin and 29 years old from Hamburg
Is that correct? 
Enter "0=Yeah" or "Nah"
0
Sweet Kevin, let us get to the next one
Press Enter for next Person

Hi, let's create three new Person. Start with number 2
What's your name?
Steven
How old are you?
22
Where are you from?
London
I summarize! You are Steven and 22 years old from London
Is that correct? 
Enter "0=Yeah" or "Nah"
0
Sweet Steven, let us get to the next one
Press Enter for next Person

Hi, let's create three new Person. Start with number 3
What's your name?
Mike
How old are you?
33
Where are you from?
Tampa,FL
I summarize! You are Mike and 33 years old from Tampa,FL
Is that correct? 
Enter "0=Yeah" or "Nah"
1
Get off. Let us try again
["Kevin", "29", "Hamburg"]
["Steven", "22", "London"]
nil
\$\endgroup\$
  • 2
    \$\begingroup\$ Functionality missing is something that should be handled via StackOverflow because you don't know how to implement it and the implementation bits of that are not on topic for Code Review. General code review also requires that the code behave the way you expect it to, which it sounds like it doesn't, so it may be skirting the line of 'on-topicness' because you indicate you are missing functionality that you desire. (I'll leave that up to the community to determine however) \$\endgroup\$ – Thomas Ward Oct 29 at 19:21
  • \$\begingroup\$ Also, it sounds like you should be looking into how loops in Ruby work. (Literally, that's what you need, a loop around the inquiry until you get a valid message, before continuing or exiting) \$\endgroup\$ – Thomas Ward Oct 29 at 19:25
  • 2
    \$\begingroup\$ Hi Thomas, thanks for the quick answer! I might put it in wrong words. The code snippet works fine, it does what it should do! I'm just looking for a way to improve and write better code. I deleted the question for goto-function/loop to make it more clear. (You already solved that question by mentioning a loop around the inquiry) \$\endgroup\$ – Kevin Oo Oct 29 at 19:26
1
\$\begingroup\$

I have a few suggestions. Some high-level and then I'll go through your code.

My first suggestion would be to focus on readability. That means:

  • good use of spacing
  • descriptive names for modules, classes, methods, variables, everything
  • think about the different concepts involved
  • extract into simple modules, classes and methods
  • start small
  • read the documentation (here), particularly string, hash, array

For example, look at a subsection of your code with a little spacing:

class Person
  def input(count) 
    puts "Hi, let\'s create three new Person. Start with number #{count}"

    puts "What\'s your name?"
    @name = gets
    @name = @name.delete("\n")

    puts "How old are you?"
    @age = gets
    @age = @age.delete("\n")

    puts "Where are you from?"
    @origin = gets
    @origin = @origin.delete("\n")
  end
end

I would argue you can clearly see the structure of the 3 questions, and all it took was 3 blank lines in the right place.

Next up is proper naming. You look good for the most part. You have @name, @age, @origin.. you have @antwort. From what I understand that is German for answer. Up to you if you want to mix German and English, but be aware of the native language of other people who will be reading your code. I would do the safe thing and use @answer instead.

Avoid redundancy in naming. For example, you have return person_return. Instead, just use return person. Instead of var_person_one just use person_one (note: this is still not descriptive enough, but better).

Now let's have a look at the different concepts involved, turn them into classes.

You have a Person.. but the question and answer format doesn't really suit the person itself. Think about what you could do if you introduced a Question class and an Answer class.

You have name, age and origin as answers to your questions about person, then you build up an array of those instance variables and call that array a person, but those are attributes of the Person class, so add them to it:

class Person
  attr_accessor :age, :name, :origin
end

You also have this concept of a QA session being interacted with through the console. So, you could create a QuestionAnswerSession.

To not complicate things too much you could get quite far by using just a Person and QuestionAnswerSession and forget the separate Question and Answer classes.

The qa session is responsible for outputting questions to the console and listening for answers, then using those answers to create Person objects.

My next suggestion would be to start small. Just introduce a couple of attributes and questions at first and forget about the multiple persons. Try to figure out the overall structure. It will be fewer lines and will be easier to take in and allow to focus on the design.

Let's say we have a Person with a name and age attribute, and we have corresponding questions. From that we want to assign name and age to a Person and, finally, output the Person to the console.

Let's create the person:

class Person
  attr_accessor :name, :age
end

# usage
# Person.new(name: 'Dave', age: 20)
# or
# person = Person.new
# person.name = 'Dave'
# person.age = 20

Now let's build the QA session.

class QuestionAnswerSession
  def start
    puts "Hi, let's create a new Person"

    person = Person.new

    puts "What's your name?"
    person.name = gets.strip

    puts "What's your age?"
    person.age = gets.strip

    puts person
  end
end

qa = QuestionAnswerSession.new
qa.start

Few minor things first. Notice I've removed What\'s which is now simply What's. This is an escape character and is only required if the enclosing quote is the same. Since you are using double quotes you do not need to escape it.

Also note I have replaced @age.delete("\n") with String#strip (docs here). This will remove the newline character for you.

The qa session itself is fairly simple. You instantiate it first, then you call the start method which outputs a question and starts listening for input. It then builds up attributes for a person object and outputs the final result.

I would add one small aspect to this to give you an idea on how to expand the behaviour. Let us say we wanted the person to be able to speak for itself. When we do puts person this is what we get:

#<Person:0x0000000002572518 @name="Dave", @age="20">

Not very nice. If you wanted to output something cleaner, you could add it to the qa session class (puts "Hi, my name is #{person.name}"), but you could also add a simple method to the Person class such as:

class Person
  def introduce_self
    "Hi, my name is #{name} and I'm #{age}"
  end
end

Then in the qa session, instead of puts person you could do puts person.introduce_self.

Extracting different responsibilities into simple, descriptive methods is a great way to keep things manageable, rather than dumping everything into the same method.

Though there are other ways you could improve your code and it could be designed with numerous approaches, I'd urge you to focus on these important aspects first as outlined above, then tackle multiple Person objects later.

Hope that helps! If you have any questions leave a comment.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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