User input happy birthday program
$begingroup$
For self-study homework I made a Java program that takes user input, creates people as objects, and says "Happy Birthday" to each person.
I am just wondering what mistakes I made here or how it could have been better.
PERSON CLASS
public class PeopleObjects{
// For practice, make the people objects rather than just a list of names.
private String name;
private String birthDay;
public void setName(String name){
this.name = name;
}
public String getName(){
return name;
}
public void setBirthDay(String myBirth){
birthDay = myBirth;
}
public String getBirthDay(){
return birthDay;
}
}
MAIN
import java.util.Scanner;
import java.util.ArrayList;
public class PeopleObjectsTestDrive{
public static void main(String args){
ArrayList<PeopleObjects> listOfPeopleObjects = new ArrayList<PeopleObjects>();
Scanner input = new Scanner(System.in);
int userChoice = 0;
System.out.println("Enter any number other than 1 to start program."+"n");
while ((userChoice = input.nextInt()) != 1){
PeopleObjects people = new PeopleObjects();
System.out.println("n###What is the name of the person?###n");
input.nextLine();//for some reason we have to use this twice to avoid bug
people.setName(input.nextLine());
System.out.println("n###What is their birthday?###n");
people.setBirthDay(input.nextLine());
listOfPeopleObjects.add(people);
System.out.println("nDo you want to quit and see list? Enter 1 for yes and 0 for non");
}
System.out.println("nnnn");
for(int i=0; i < listOfPeopleObjects.size(); i++){
if(listOfPeopleObjects.get(i).getName().equals("Joe")){
System.out.println("Happy Birthday Joe! You are special! Your birthday is on "+listOfPeopleObjects.get(i).getBirthDay()+" n");
}else if(i%2 == 0){
System.out.println("Happy Birthday to "+listOfPeopleObjects.get(i).getName()+".n");
}else{
System.out.println("Happy Birthday to "+listOfPeopleObjects.get(i).getName()+" too!!!n");
}//end if else
}//end for
}
}
java beginner array homework
New contributor
$endgroup$
add a comment |
$begingroup$
For self-study homework I made a Java program that takes user input, creates people as objects, and says "Happy Birthday" to each person.
I am just wondering what mistakes I made here or how it could have been better.
PERSON CLASS
public class PeopleObjects{
// For practice, make the people objects rather than just a list of names.
private String name;
private String birthDay;
public void setName(String name){
this.name = name;
}
public String getName(){
return name;
}
public void setBirthDay(String myBirth){
birthDay = myBirth;
}
public String getBirthDay(){
return birthDay;
}
}
MAIN
import java.util.Scanner;
import java.util.ArrayList;
public class PeopleObjectsTestDrive{
public static void main(String args){
ArrayList<PeopleObjects> listOfPeopleObjects = new ArrayList<PeopleObjects>();
Scanner input = new Scanner(System.in);
int userChoice = 0;
System.out.println("Enter any number other than 1 to start program."+"n");
while ((userChoice = input.nextInt()) != 1){
PeopleObjects people = new PeopleObjects();
System.out.println("n###What is the name of the person?###n");
input.nextLine();//for some reason we have to use this twice to avoid bug
people.setName(input.nextLine());
System.out.println("n###What is their birthday?###n");
people.setBirthDay(input.nextLine());
listOfPeopleObjects.add(people);
System.out.println("nDo you want to quit and see list? Enter 1 for yes and 0 for non");
}
System.out.println("nnnn");
for(int i=0; i < listOfPeopleObjects.size(); i++){
if(listOfPeopleObjects.get(i).getName().equals("Joe")){
System.out.println("Happy Birthday Joe! You are special! Your birthday is on "+listOfPeopleObjects.get(i).getBirthDay()+" n");
}else if(i%2 == 0){
System.out.println("Happy Birthday to "+listOfPeopleObjects.get(i).getName()+".n");
}else{
System.out.println("Happy Birthday to "+listOfPeopleObjects.get(i).getName()+" too!!!n");
}//end if else
}//end for
}
}
java beginner array homework
New contributor
$endgroup$
$begingroup$
Welcome to code review. This looks interesting to me. Hope you learn a lot here.
$endgroup$
– 422_unprocessable_entity
7 hours ago
add a comment |
$begingroup$
For self-study homework I made a Java program that takes user input, creates people as objects, and says "Happy Birthday" to each person.
I am just wondering what mistakes I made here or how it could have been better.
PERSON CLASS
public class PeopleObjects{
// For practice, make the people objects rather than just a list of names.
private String name;
private String birthDay;
public void setName(String name){
this.name = name;
}
public String getName(){
return name;
}
public void setBirthDay(String myBirth){
birthDay = myBirth;
}
public String getBirthDay(){
return birthDay;
}
}
MAIN
import java.util.Scanner;
import java.util.ArrayList;
public class PeopleObjectsTestDrive{
public static void main(String args){
ArrayList<PeopleObjects> listOfPeopleObjects = new ArrayList<PeopleObjects>();
Scanner input = new Scanner(System.in);
int userChoice = 0;
System.out.println("Enter any number other than 1 to start program."+"n");
while ((userChoice = input.nextInt()) != 1){
PeopleObjects people = new PeopleObjects();
System.out.println("n###What is the name of the person?###n");
input.nextLine();//for some reason we have to use this twice to avoid bug
people.setName(input.nextLine());
System.out.println("n###What is their birthday?###n");
people.setBirthDay(input.nextLine());
listOfPeopleObjects.add(people);
System.out.println("nDo you want to quit and see list? Enter 1 for yes and 0 for non");
}
System.out.println("nnnn");
for(int i=0; i < listOfPeopleObjects.size(); i++){
if(listOfPeopleObjects.get(i).getName().equals("Joe")){
System.out.println("Happy Birthday Joe! You are special! Your birthday is on "+listOfPeopleObjects.get(i).getBirthDay()+" n");
}else if(i%2 == 0){
System.out.println("Happy Birthday to "+listOfPeopleObjects.get(i).getName()+".n");
}else{
System.out.println("Happy Birthday to "+listOfPeopleObjects.get(i).getName()+" too!!!n");
}//end if else
}//end for
}
}
java beginner array homework
New contributor
$endgroup$
For self-study homework I made a Java program that takes user input, creates people as objects, and says "Happy Birthday" to each person.
I am just wondering what mistakes I made here or how it could have been better.
PERSON CLASS
public class PeopleObjects{
// For practice, make the people objects rather than just a list of names.
private String name;
private String birthDay;
public void setName(String name){
this.name = name;
}
public String getName(){
return name;
}
public void setBirthDay(String myBirth){
birthDay = myBirth;
}
public String getBirthDay(){
return birthDay;
}
}
MAIN
import java.util.Scanner;
import java.util.ArrayList;
public class PeopleObjectsTestDrive{
public static void main(String args){
ArrayList<PeopleObjects> listOfPeopleObjects = new ArrayList<PeopleObjects>();
Scanner input = new Scanner(System.in);
int userChoice = 0;
System.out.println("Enter any number other than 1 to start program."+"n");
while ((userChoice = input.nextInt()) != 1){
PeopleObjects people = new PeopleObjects();
System.out.println("n###What is the name of the person?###n");
input.nextLine();//for some reason we have to use this twice to avoid bug
people.setName(input.nextLine());
System.out.println("n###What is their birthday?###n");
people.setBirthDay(input.nextLine());
listOfPeopleObjects.add(people);
System.out.println("nDo you want to quit and see list? Enter 1 for yes and 0 for non");
}
System.out.println("nnnn");
for(int i=0; i < listOfPeopleObjects.size(); i++){
if(listOfPeopleObjects.get(i).getName().equals("Joe")){
System.out.println("Happy Birthday Joe! You are special! Your birthday is on "+listOfPeopleObjects.get(i).getBirthDay()+" n");
}else if(i%2 == 0){
System.out.println("Happy Birthday to "+listOfPeopleObjects.get(i).getName()+".n");
}else{
System.out.println("Happy Birthday to "+listOfPeopleObjects.get(i).getName()+" too!!!n");
}//end if else
}//end for
}
}
java beginner array homework
java beginner array homework
New contributor
New contributor
New contributor
asked 8 hours ago
Steven HalversonSteven Halverson
262
262
New contributor
New contributor
$begingroup$
Welcome to code review. This looks interesting to me. Hope you learn a lot here.
$endgroup$
– 422_unprocessable_entity
7 hours ago
add a comment |
$begingroup$
Welcome to code review. This looks interesting to me. Hope you learn a lot here.
$endgroup$
– 422_unprocessable_entity
7 hours ago
$begingroup$
Welcome to code review. This looks interesting to me. Hope you learn a lot here.
$endgroup$
– 422_unprocessable_entity
7 hours ago
$begingroup$
Welcome to code review. This looks interesting to me. Hope you learn a lot here.
$endgroup$
– 422_unprocessable_entity
7 hours ago
add a comment |
2 Answers
2
active
oldest
votes
$begingroup$
Why nextLine
after nextInt
?
while ((userChoice = input.nextInt()) != 1){
PeopleObjects people = new PeopleObjects();
System.out.println("n###What is the name of the person?###n");
input.nextLine();//for some reason we have to use this twice to avoid bug
While an explanatory comment is helpful, you may want to go ahead and search out the explanation: essentially, the nextInt
only reads as far as the end of the number; it does not read the succeeding end-of-line. So if you want to read the next line, you have to first read that end-of-line so that you can start reading after it. Once you do that, you might move that line higher, right after the nextInt
.
I prefer comments to be on separate lines, although others disagree.
Favor interfaces over implementations
ArrayList<PeopleObjects> listOfPeopleObjects = new ArrayList<PeopleObjects>();
When defining a type, the general preference is to use the interface rather than the implementation. So
List<Person> people = new ArrayList<>();
This encourages you to code to the interface, only using methods that are available in the interface. That in turn makes it easier to switch implementations.
In the more recent versions of Java, you don't need to specify the type parameter twice. The second time you can just say <>
and trust the compiler to figure it out.
PersonObjects
is a plural name for a singular thing. So we can just make that singular. And there's not much point in saying that it is an object (which it actually isn't, it's a class). Every instantiation of a class in Java is an object. It could be just Person
.
Similarly, listOfPeopleObjects
could be just people
. There's some controversy over the list
. There is a notation called Hungarian notation where the type is included in the name. I personally prefer to just use plural names for collections of objects. But others disagree.
Clear prompts
System.out.println("nDo you want to quit and see list? Enter 1 for yes and 0 for non");
Why do you want to indicate yes? Or no? You want to indicate yes to quit and no to continue. So why say yes and no? Consider
System.out.println("nDo you want to quit and see list? Enter 1 to quit or 0 to continuen");
Now we don't have to figure out what yes and no mean in this context (e.g. you could have been asking "Do you want to continue?" with everything else the same.
do
/while
You may want to read up on the do
/while
syntax. That would allow you to avoid asking for an initial response. I.e. you wouldn't have to enter anything to start the processing. Only to continue afterwards.
It would disrupt your current nextInt
/nextLine
pattern though. You might consider how you could make that work. Hint: a custom method could do both commands in one unit of work.
$endgroup$
add a comment |
$begingroup$
PeopleObjects
is an odd name for a couple reasons:
- Both words are plural, even though each instance only represents a single person.
- Having "object" in the name is a little redundant. It's implied that the class is representing an object.
I'd just call the class Person
.
And you didn't give PeopleObjects
a constructor. You're constructing an instance of the class ahead of time, then asking for input and calling the set
ters. What if you forgot to set the birthday or name? They would be left as null
and potentially cause problems later. If properties are required (which both name and birthday would presumably be), have them supplied by a constructor:
public Person(String name, String birthDay) {
this.name = name;
this.birthDay = birthDay;
}
. . .
List<Person> people = new ArrayList<Person>();
Scanner input = new Scanner(System.in);
System.out.println("Enter any number other than 1 to start program."+"n");
while (input.nextInt() != 1){
// This is needed since nextInt doesn't consume the newline
input.nextLine();
System.out.println("n###What is the name of the person?###n");
String name = input.nextLine();
System.out.println("n###What is their birthday?###n");
String birthday = input.nextLine();
Person person = new Person(name, birthday);
people.add(person);
System.out.println("nDo you want to quit and see list? Enter 1 for yes and 0 for non");
}
I changed a few things about that loop:
As mentioned above, the person isn't constructed until the necessary data is already obtained, and the data is passed directly to the constructor.
userChoice
was never used. It was just storing the users entry, but that choice was only ever used in the condition. If you really wanted it for debugging purposes, it may be appropriate, but I didn't think that it was necessary here.I renamed the list of people to simply
people
. This was replacing yourPeopleObjects people = new PeopleObjects();
line. Again, calling a single personpeople
is confusing. I also changed the type ofpeople
toList
, fromArrayList
. Always use the broadest type possible. In this case, you don't need any operations specific toArrayList
; you're only usingList
methods. To see why this is important, give this a read over. Note the example used in the question.
Your other loop at the bottom is sub-optimal as well. First, you're constantly writing people.get(i)
. For an ArrayList
, get
is very fast, but those calls are causing unneeded bloat. Create an intermediate variable person
in the loop to neaten things up:
for(int i = 0; i < people.size(); i++) {
Person person = people.get(i);
String name = person.getName();
if(name.equals("Joe")) {
System.out.println("Happy Birthday Joe! You are special! Your birthday is on "
+ person.getBirthDay() + " n");
} else if(i % 2 == 0) {
System.out.println("Happy Birthday to " + name + ".n");
} else {
System.out.println("Happy Birthday to " + name + " too!!!n");
}
}
I ended up creating a name
variable as well since the name of the person was needed in multiple places.
You also had inconsistent spacing that I corrected. Some places had everything all smooshed together, and then other places (like with ==
) had spacing. I added spaces around all the operators, then broke up one of the longer lines. I also added empty lines after each println
call so the cases were a little more distinct.
Unless you need the i % 2 == 0
check to give a special message to every arbitrary second person, this loop could be slightly simplified by using an enhanced for-loop that doesn't rely on indices:
// Read as "for each person in people"
for(Person person : people) {
String name = person.getName();
if(name.equals("Joe")) {
System.out.println("Happy Birthday Joe! You are special! Your birthday is on "
+ person.getBirthDay() + " n");
} else {
System.out.println("Happy Birthday to " + name + " too!!!n");
}
}
$endgroup$
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
Steven Halverson is a new contributor. Be nice, and check out our Code of Conduct.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f214120%2fuser-input-happy-birthday-program%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
Why nextLine
after nextInt
?
while ((userChoice = input.nextInt()) != 1){
PeopleObjects people = new PeopleObjects();
System.out.println("n###What is the name of the person?###n");
input.nextLine();//for some reason we have to use this twice to avoid bug
While an explanatory comment is helpful, you may want to go ahead and search out the explanation: essentially, the nextInt
only reads as far as the end of the number; it does not read the succeeding end-of-line. So if you want to read the next line, you have to first read that end-of-line so that you can start reading after it. Once you do that, you might move that line higher, right after the nextInt
.
I prefer comments to be on separate lines, although others disagree.
Favor interfaces over implementations
ArrayList<PeopleObjects> listOfPeopleObjects = new ArrayList<PeopleObjects>();
When defining a type, the general preference is to use the interface rather than the implementation. So
List<Person> people = new ArrayList<>();
This encourages you to code to the interface, only using methods that are available in the interface. That in turn makes it easier to switch implementations.
In the more recent versions of Java, you don't need to specify the type parameter twice. The second time you can just say <>
and trust the compiler to figure it out.
PersonObjects
is a plural name for a singular thing. So we can just make that singular. And there's not much point in saying that it is an object (which it actually isn't, it's a class). Every instantiation of a class in Java is an object. It could be just Person
.
Similarly, listOfPeopleObjects
could be just people
. There's some controversy over the list
. There is a notation called Hungarian notation where the type is included in the name. I personally prefer to just use plural names for collections of objects. But others disagree.
Clear prompts
System.out.println("nDo you want to quit and see list? Enter 1 for yes and 0 for non");
Why do you want to indicate yes? Or no? You want to indicate yes to quit and no to continue. So why say yes and no? Consider
System.out.println("nDo you want to quit and see list? Enter 1 to quit or 0 to continuen");
Now we don't have to figure out what yes and no mean in this context (e.g. you could have been asking "Do you want to continue?" with everything else the same.
do
/while
You may want to read up on the do
/while
syntax. That would allow you to avoid asking for an initial response. I.e. you wouldn't have to enter anything to start the processing. Only to continue afterwards.
It would disrupt your current nextInt
/nextLine
pattern though. You might consider how you could make that work. Hint: a custom method could do both commands in one unit of work.
$endgroup$
add a comment |
$begingroup$
Why nextLine
after nextInt
?
while ((userChoice = input.nextInt()) != 1){
PeopleObjects people = new PeopleObjects();
System.out.println("n###What is the name of the person?###n");
input.nextLine();//for some reason we have to use this twice to avoid bug
While an explanatory comment is helpful, you may want to go ahead and search out the explanation: essentially, the nextInt
only reads as far as the end of the number; it does not read the succeeding end-of-line. So if you want to read the next line, you have to first read that end-of-line so that you can start reading after it. Once you do that, you might move that line higher, right after the nextInt
.
I prefer comments to be on separate lines, although others disagree.
Favor interfaces over implementations
ArrayList<PeopleObjects> listOfPeopleObjects = new ArrayList<PeopleObjects>();
When defining a type, the general preference is to use the interface rather than the implementation. So
List<Person> people = new ArrayList<>();
This encourages you to code to the interface, only using methods that are available in the interface. That in turn makes it easier to switch implementations.
In the more recent versions of Java, you don't need to specify the type parameter twice. The second time you can just say <>
and trust the compiler to figure it out.
PersonObjects
is a plural name for a singular thing. So we can just make that singular. And there's not much point in saying that it is an object (which it actually isn't, it's a class). Every instantiation of a class in Java is an object. It could be just Person
.
Similarly, listOfPeopleObjects
could be just people
. There's some controversy over the list
. There is a notation called Hungarian notation where the type is included in the name. I personally prefer to just use plural names for collections of objects. But others disagree.
Clear prompts
System.out.println("nDo you want to quit and see list? Enter 1 for yes and 0 for non");
Why do you want to indicate yes? Or no? You want to indicate yes to quit and no to continue. So why say yes and no? Consider
System.out.println("nDo you want to quit and see list? Enter 1 to quit or 0 to continuen");
Now we don't have to figure out what yes and no mean in this context (e.g. you could have been asking "Do you want to continue?" with everything else the same.
do
/while
You may want to read up on the do
/while
syntax. That would allow you to avoid asking for an initial response. I.e. you wouldn't have to enter anything to start the processing. Only to continue afterwards.
It would disrupt your current nextInt
/nextLine
pattern though. You might consider how you could make that work. Hint: a custom method could do both commands in one unit of work.
$endgroup$
add a comment |
$begingroup$
Why nextLine
after nextInt
?
while ((userChoice = input.nextInt()) != 1){
PeopleObjects people = new PeopleObjects();
System.out.println("n###What is the name of the person?###n");
input.nextLine();//for some reason we have to use this twice to avoid bug
While an explanatory comment is helpful, you may want to go ahead and search out the explanation: essentially, the nextInt
only reads as far as the end of the number; it does not read the succeeding end-of-line. So if you want to read the next line, you have to first read that end-of-line so that you can start reading after it. Once you do that, you might move that line higher, right after the nextInt
.
I prefer comments to be on separate lines, although others disagree.
Favor interfaces over implementations
ArrayList<PeopleObjects> listOfPeopleObjects = new ArrayList<PeopleObjects>();
When defining a type, the general preference is to use the interface rather than the implementation. So
List<Person> people = new ArrayList<>();
This encourages you to code to the interface, only using methods that are available in the interface. That in turn makes it easier to switch implementations.
In the more recent versions of Java, you don't need to specify the type parameter twice. The second time you can just say <>
and trust the compiler to figure it out.
PersonObjects
is a plural name for a singular thing. So we can just make that singular. And there's not much point in saying that it is an object (which it actually isn't, it's a class). Every instantiation of a class in Java is an object. It could be just Person
.
Similarly, listOfPeopleObjects
could be just people
. There's some controversy over the list
. There is a notation called Hungarian notation where the type is included in the name. I personally prefer to just use plural names for collections of objects. But others disagree.
Clear prompts
System.out.println("nDo you want to quit and see list? Enter 1 for yes and 0 for non");
Why do you want to indicate yes? Or no? You want to indicate yes to quit and no to continue. So why say yes and no? Consider
System.out.println("nDo you want to quit and see list? Enter 1 to quit or 0 to continuen");
Now we don't have to figure out what yes and no mean in this context (e.g. you could have been asking "Do you want to continue?" with everything else the same.
do
/while
You may want to read up on the do
/while
syntax. That would allow you to avoid asking for an initial response. I.e. you wouldn't have to enter anything to start the processing. Only to continue afterwards.
It would disrupt your current nextInt
/nextLine
pattern though. You might consider how you could make that work. Hint: a custom method could do both commands in one unit of work.
$endgroup$
Why nextLine
after nextInt
?
while ((userChoice = input.nextInt()) != 1){
PeopleObjects people = new PeopleObjects();
System.out.println("n###What is the name of the person?###n");
input.nextLine();//for some reason we have to use this twice to avoid bug
While an explanatory comment is helpful, you may want to go ahead and search out the explanation: essentially, the nextInt
only reads as far as the end of the number; it does not read the succeeding end-of-line. So if you want to read the next line, you have to first read that end-of-line so that you can start reading after it. Once you do that, you might move that line higher, right after the nextInt
.
I prefer comments to be on separate lines, although others disagree.
Favor interfaces over implementations
ArrayList<PeopleObjects> listOfPeopleObjects = new ArrayList<PeopleObjects>();
When defining a type, the general preference is to use the interface rather than the implementation. So
List<Person> people = new ArrayList<>();
This encourages you to code to the interface, only using methods that are available in the interface. That in turn makes it easier to switch implementations.
In the more recent versions of Java, you don't need to specify the type parameter twice. The second time you can just say <>
and trust the compiler to figure it out.
PersonObjects
is a plural name for a singular thing. So we can just make that singular. And there's not much point in saying that it is an object (which it actually isn't, it's a class). Every instantiation of a class in Java is an object. It could be just Person
.
Similarly, listOfPeopleObjects
could be just people
. There's some controversy over the list
. There is a notation called Hungarian notation where the type is included in the name. I personally prefer to just use plural names for collections of objects. But others disagree.
Clear prompts
System.out.println("nDo you want to quit and see list? Enter 1 for yes and 0 for non");
Why do you want to indicate yes? Or no? You want to indicate yes to quit and no to continue. So why say yes and no? Consider
System.out.println("nDo you want to quit and see list? Enter 1 to quit or 0 to continuen");
Now we don't have to figure out what yes and no mean in this context (e.g. you could have been asking "Do you want to continue?" with everything else the same.
do
/while
You may want to read up on the do
/while
syntax. That would allow you to avoid asking for an initial response. I.e. you wouldn't have to enter anything to start the processing. Only to continue afterwards.
It would disrupt your current nextInt
/nextLine
pattern though. You might consider how you could make that work. Hint: a custom method could do both commands in one unit of work.
answered 7 hours ago
mdfst13mdfst13
17.8k62157
17.8k62157
add a comment |
add a comment |
$begingroup$
PeopleObjects
is an odd name for a couple reasons:
- Both words are plural, even though each instance only represents a single person.
- Having "object" in the name is a little redundant. It's implied that the class is representing an object.
I'd just call the class Person
.
And you didn't give PeopleObjects
a constructor. You're constructing an instance of the class ahead of time, then asking for input and calling the set
ters. What if you forgot to set the birthday or name? They would be left as null
and potentially cause problems later. If properties are required (which both name and birthday would presumably be), have them supplied by a constructor:
public Person(String name, String birthDay) {
this.name = name;
this.birthDay = birthDay;
}
. . .
List<Person> people = new ArrayList<Person>();
Scanner input = new Scanner(System.in);
System.out.println("Enter any number other than 1 to start program."+"n");
while (input.nextInt() != 1){
// This is needed since nextInt doesn't consume the newline
input.nextLine();
System.out.println("n###What is the name of the person?###n");
String name = input.nextLine();
System.out.println("n###What is their birthday?###n");
String birthday = input.nextLine();
Person person = new Person(name, birthday);
people.add(person);
System.out.println("nDo you want to quit and see list? Enter 1 for yes and 0 for non");
}
I changed a few things about that loop:
As mentioned above, the person isn't constructed until the necessary data is already obtained, and the data is passed directly to the constructor.
userChoice
was never used. It was just storing the users entry, but that choice was only ever used in the condition. If you really wanted it for debugging purposes, it may be appropriate, but I didn't think that it was necessary here.I renamed the list of people to simply
people
. This was replacing yourPeopleObjects people = new PeopleObjects();
line. Again, calling a single personpeople
is confusing. I also changed the type ofpeople
toList
, fromArrayList
. Always use the broadest type possible. In this case, you don't need any operations specific toArrayList
; you're only usingList
methods. To see why this is important, give this a read over. Note the example used in the question.
Your other loop at the bottom is sub-optimal as well. First, you're constantly writing people.get(i)
. For an ArrayList
, get
is very fast, but those calls are causing unneeded bloat. Create an intermediate variable person
in the loop to neaten things up:
for(int i = 0; i < people.size(); i++) {
Person person = people.get(i);
String name = person.getName();
if(name.equals("Joe")) {
System.out.println("Happy Birthday Joe! You are special! Your birthday is on "
+ person.getBirthDay() + " n");
} else if(i % 2 == 0) {
System.out.println("Happy Birthday to " + name + ".n");
} else {
System.out.println("Happy Birthday to " + name + " too!!!n");
}
}
I ended up creating a name
variable as well since the name of the person was needed in multiple places.
You also had inconsistent spacing that I corrected. Some places had everything all smooshed together, and then other places (like with ==
) had spacing. I added spaces around all the operators, then broke up one of the longer lines. I also added empty lines after each println
call so the cases were a little more distinct.
Unless you need the i % 2 == 0
check to give a special message to every arbitrary second person, this loop could be slightly simplified by using an enhanced for-loop that doesn't rely on indices:
// Read as "for each person in people"
for(Person person : people) {
String name = person.getName();
if(name.equals("Joe")) {
System.out.println("Happy Birthday Joe! You are special! Your birthday is on "
+ person.getBirthDay() + " n");
} else {
System.out.println("Happy Birthday to " + name + " too!!!n");
}
}
$endgroup$
add a comment |
$begingroup$
PeopleObjects
is an odd name for a couple reasons:
- Both words are plural, even though each instance only represents a single person.
- Having "object" in the name is a little redundant. It's implied that the class is representing an object.
I'd just call the class Person
.
And you didn't give PeopleObjects
a constructor. You're constructing an instance of the class ahead of time, then asking for input and calling the set
ters. What if you forgot to set the birthday or name? They would be left as null
and potentially cause problems later. If properties are required (which both name and birthday would presumably be), have them supplied by a constructor:
public Person(String name, String birthDay) {
this.name = name;
this.birthDay = birthDay;
}
. . .
List<Person> people = new ArrayList<Person>();
Scanner input = new Scanner(System.in);
System.out.println("Enter any number other than 1 to start program."+"n");
while (input.nextInt() != 1){
// This is needed since nextInt doesn't consume the newline
input.nextLine();
System.out.println("n###What is the name of the person?###n");
String name = input.nextLine();
System.out.println("n###What is their birthday?###n");
String birthday = input.nextLine();
Person person = new Person(name, birthday);
people.add(person);
System.out.println("nDo you want to quit and see list? Enter 1 for yes and 0 for non");
}
I changed a few things about that loop:
As mentioned above, the person isn't constructed until the necessary data is already obtained, and the data is passed directly to the constructor.
userChoice
was never used. It was just storing the users entry, but that choice was only ever used in the condition. If you really wanted it for debugging purposes, it may be appropriate, but I didn't think that it was necessary here.I renamed the list of people to simply
people
. This was replacing yourPeopleObjects people = new PeopleObjects();
line. Again, calling a single personpeople
is confusing. I also changed the type ofpeople
toList
, fromArrayList
. Always use the broadest type possible. In this case, you don't need any operations specific toArrayList
; you're only usingList
methods. To see why this is important, give this a read over. Note the example used in the question.
Your other loop at the bottom is sub-optimal as well. First, you're constantly writing people.get(i)
. For an ArrayList
, get
is very fast, but those calls are causing unneeded bloat. Create an intermediate variable person
in the loop to neaten things up:
for(int i = 0; i < people.size(); i++) {
Person person = people.get(i);
String name = person.getName();
if(name.equals("Joe")) {
System.out.println("Happy Birthday Joe! You are special! Your birthday is on "
+ person.getBirthDay() + " n");
} else if(i % 2 == 0) {
System.out.println("Happy Birthday to " + name + ".n");
} else {
System.out.println("Happy Birthday to " + name + " too!!!n");
}
}
I ended up creating a name
variable as well since the name of the person was needed in multiple places.
You also had inconsistent spacing that I corrected. Some places had everything all smooshed together, and then other places (like with ==
) had spacing. I added spaces around all the operators, then broke up one of the longer lines. I also added empty lines after each println
call so the cases were a little more distinct.
Unless you need the i % 2 == 0
check to give a special message to every arbitrary second person, this loop could be slightly simplified by using an enhanced for-loop that doesn't rely on indices:
// Read as "for each person in people"
for(Person person : people) {
String name = person.getName();
if(name.equals("Joe")) {
System.out.println("Happy Birthday Joe! You are special! Your birthday is on "
+ person.getBirthDay() + " n");
} else {
System.out.println("Happy Birthday to " + name + " too!!!n");
}
}
$endgroup$
add a comment |
$begingroup$
PeopleObjects
is an odd name for a couple reasons:
- Both words are plural, even though each instance only represents a single person.
- Having "object" in the name is a little redundant. It's implied that the class is representing an object.
I'd just call the class Person
.
And you didn't give PeopleObjects
a constructor. You're constructing an instance of the class ahead of time, then asking for input and calling the set
ters. What if you forgot to set the birthday or name? They would be left as null
and potentially cause problems later. If properties are required (which both name and birthday would presumably be), have them supplied by a constructor:
public Person(String name, String birthDay) {
this.name = name;
this.birthDay = birthDay;
}
. . .
List<Person> people = new ArrayList<Person>();
Scanner input = new Scanner(System.in);
System.out.println("Enter any number other than 1 to start program."+"n");
while (input.nextInt() != 1){
// This is needed since nextInt doesn't consume the newline
input.nextLine();
System.out.println("n###What is the name of the person?###n");
String name = input.nextLine();
System.out.println("n###What is their birthday?###n");
String birthday = input.nextLine();
Person person = new Person(name, birthday);
people.add(person);
System.out.println("nDo you want to quit and see list? Enter 1 for yes and 0 for non");
}
I changed a few things about that loop:
As mentioned above, the person isn't constructed until the necessary data is already obtained, and the data is passed directly to the constructor.
userChoice
was never used. It was just storing the users entry, but that choice was only ever used in the condition. If you really wanted it for debugging purposes, it may be appropriate, but I didn't think that it was necessary here.I renamed the list of people to simply
people
. This was replacing yourPeopleObjects people = new PeopleObjects();
line. Again, calling a single personpeople
is confusing. I also changed the type ofpeople
toList
, fromArrayList
. Always use the broadest type possible. In this case, you don't need any operations specific toArrayList
; you're only usingList
methods. To see why this is important, give this a read over. Note the example used in the question.
Your other loop at the bottom is sub-optimal as well. First, you're constantly writing people.get(i)
. For an ArrayList
, get
is very fast, but those calls are causing unneeded bloat. Create an intermediate variable person
in the loop to neaten things up:
for(int i = 0; i < people.size(); i++) {
Person person = people.get(i);
String name = person.getName();
if(name.equals("Joe")) {
System.out.println("Happy Birthday Joe! You are special! Your birthday is on "
+ person.getBirthDay() + " n");
} else if(i % 2 == 0) {
System.out.println("Happy Birthday to " + name + ".n");
} else {
System.out.println("Happy Birthday to " + name + " too!!!n");
}
}
I ended up creating a name
variable as well since the name of the person was needed in multiple places.
You also had inconsistent spacing that I corrected. Some places had everything all smooshed together, and then other places (like with ==
) had spacing. I added spaces around all the operators, then broke up one of the longer lines. I also added empty lines after each println
call so the cases were a little more distinct.
Unless you need the i % 2 == 0
check to give a special message to every arbitrary second person, this loop could be slightly simplified by using an enhanced for-loop that doesn't rely on indices:
// Read as "for each person in people"
for(Person person : people) {
String name = person.getName();
if(name.equals("Joe")) {
System.out.println("Happy Birthday Joe! You are special! Your birthday is on "
+ person.getBirthDay() + " n");
} else {
System.out.println("Happy Birthday to " + name + " too!!!n");
}
}
$endgroup$
PeopleObjects
is an odd name for a couple reasons:
- Both words are plural, even though each instance only represents a single person.
- Having "object" in the name is a little redundant. It's implied that the class is representing an object.
I'd just call the class Person
.
And you didn't give PeopleObjects
a constructor. You're constructing an instance of the class ahead of time, then asking for input and calling the set
ters. What if you forgot to set the birthday or name? They would be left as null
and potentially cause problems later. If properties are required (which both name and birthday would presumably be), have them supplied by a constructor:
public Person(String name, String birthDay) {
this.name = name;
this.birthDay = birthDay;
}
. . .
List<Person> people = new ArrayList<Person>();
Scanner input = new Scanner(System.in);
System.out.println("Enter any number other than 1 to start program."+"n");
while (input.nextInt() != 1){
// This is needed since nextInt doesn't consume the newline
input.nextLine();
System.out.println("n###What is the name of the person?###n");
String name = input.nextLine();
System.out.println("n###What is their birthday?###n");
String birthday = input.nextLine();
Person person = new Person(name, birthday);
people.add(person);
System.out.println("nDo you want to quit and see list? Enter 1 for yes and 0 for non");
}
I changed a few things about that loop:
As mentioned above, the person isn't constructed until the necessary data is already obtained, and the data is passed directly to the constructor.
userChoice
was never used. It was just storing the users entry, but that choice was only ever used in the condition. If you really wanted it for debugging purposes, it may be appropriate, but I didn't think that it was necessary here.I renamed the list of people to simply
people
. This was replacing yourPeopleObjects people = new PeopleObjects();
line. Again, calling a single personpeople
is confusing. I also changed the type ofpeople
toList
, fromArrayList
. Always use the broadest type possible. In this case, you don't need any operations specific toArrayList
; you're only usingList
methods. To see why this is important, give this a read over. Note the example used in the question.
Your other loop at the bottom is sub-optimal as well. First, you're constantly writing people.get(i)
. For an ArrayList
, get
is very fast, but those calls are causing unneeded bloat. Create an intermediate variable person
in the loop to neaten things up:
for(int i = 0; i < people.size(); i++) {
Person person = people.get(i);
String name = person.getName();
if(name.equals("Joe")) {
System.out.println("Happy Birthday Joe! You are special! Your birthday is on "
+ person.getBirthDay() + " n");
} else if(i % 2 == 0) {
System.out.println("Happy Birthday to " + name + ".n");
} else {
System.out.println("Happy Birthday to " + name + " too!!!n");
}
}
I ended up creating a name
variable as well since the name of the person was needed in multiple places.
You also had inconsistent spacing that I corrected. Some places had everything all smooshed together, and then other places (like with ==
) had spacing. I added spaces around all the operators, then broke up one of the longer lines. I also added empty lines after each println
call so the cases were a little more distinct.
Unless you need the i % 2 == 0
check to give a special message to every arbitrary second person, this loop could be slightly simplified by using an enhanced for-loop that doesn't rely on indices:
// Read as "for each person in people"
for(Person person : people) {
String name = person.getName();
if(name.equals("Joe")) {
System.out.println("Happy Birthday Joe! You are special! Your birthday is on "
+ person.getBirthDay() + " n");
} else {
System.out.println("Happy Birthday to " + name + " too!!!n");
}
}
edited 6 hours ago
answered 7 hours ago
CarcigenicateCarcigenicate
3,44711631
3,44711631
add a comment |
add a comment |
Steven Halverson is a new contributor. Be nice, and check out our Code of Conduct.
Steven Halverson is a new contributor. Be nice, and check out our Code of Conduct.
Steven Halverson is a new contributor. Be nice, and check out our Code of Conduct.
Steven Halverson is a new contributor. Be nice, and check out our Code of Conduct.
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f214120%2fuser-input-happy-birthday-program%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
$begingroup$
Welcome to code review. This looks interesting to me. Hope you learn a lot here.
$endgroup$
– 422_unprocessable_entity
7 hours ago