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.

Please review this code.

import java.util.Scanner;
import java.util.* ;
public class LMComplex {
Scanner name  = new Scanner(System.in);
Scanner age  = new Scanner(System.in);
Scanner gender  = new Scanner(System.in);
Scanner height  = new Scanner(System.in);
String usrname;
String usrage;
String usrgender;
Double usrheight;
public void main(){
System.out.println("Enter your name: ");
usrname = name.nextLine();
System.out.println("Enter your age(Numbers): ");
usrage = age.nextLine();
System.out.println("Enter your gender(male/female): ");
usrgender = gender.nextLine();
System.out.println("Enter your height(feet.inches . 5 feet 5 inches = 5.5): ");
usrheight = height.nextDouble();
Double rusrheight = Math.ceil(usrheight);
if(rusrheight > usrheight)
{
rusrheight = rusrheight - 1;
}
else 
{

}
Double rusrheightininches = usrheight - rusrheight;
if(rusrheightininches >= 0.10 && rusrheightininches < 0.12)
{
rusrheightininches = rusrheightininches * 100;
} else {
rusrheightininches = rusrheightininches * 10;
}
Double rusrheightinincheschecker = rusrheightininches;
rusrheightininches = Math.ceil(rusrheightininches);
if(rusrheightininches > rusrheightinincheschecker)
{
rusrheightininches = rusrheightininches - 1;
}
else 
{

}
System.out.println("Hii " + usrname + ". I am Cartic, a basic textual virtual personal assistant. I do know that you are " + usrage +" years old. You are a " + usrgender + " and are " + rusrheight + " feet " + rusrheightininches + " inch(es) tall");
}
}
share|improve this question
6  
Format your code (indent blocks) for easy reading. –  MrSmith42 Apr 24 at 11:35
1  
@MrSmith42 that's a review... If rolfl hadn't already posted an answer almost solely about formatting, I'd have suggested you do, but rolfl was the fastest reviewer in the west here ;) –  Vogel612 Apr 24 at 11:37
2  
"just 14 years old and have been learning java" That's great! Stick with it, Java is going to be around a long time. The world needs good programmers, and the best ones are those who start young. (I started programming at 12, and started Java at 14. At the time, I didn't have the benefit of StackOverflow & StackExchange.) :) –  musicwithoutpaper Apr 24 at 15:00

6 Answers 6

Indenting is the first part of common practice that you should consider when sharing your code with anyone. Here is your code, which I have put though the 'standard' indenting function of Eclipse:

import java.util.Scanner;

public class LMComplex {
    Scanner name = new Scanner(System.in);
    Scanner age = new Scanner(System.in);
    Scanner gender = new Scanner(System.in);
    Scanner height = new Scanner(System.in);
    String usrname;
    String usrage;
    String usrgender;
    Double usrheight;

    public void main() {
        System.out.println("Enter your name: ");
        usrname = name.nextLine();
        System.out.println("Enter your age(Numbers): ");
        usrage = age.nextLine();
        System.out.println("Enter your gender(male/female): ");
        usrgender = gender.nextLine();
        System.out.println("Enter your height(feet.inches . 5 feet 5 inches = 5.5): ");
        usrheight = height.nextDouble();
        Double rusrheight = Math.ceil(usrheight);
        if (rusrheight > usrheight)
        {
            rusrheight = rusrheight - 1;
        }
        else
        {

        }
        Double rusrheightininches = usrheight - rusrheight;
        if (rusrheightininches >= 0.10 && rusrheightininches < 0.12)
        {
            rusrheightininches = rusrheightininches * 100;
        } else {
            rusrheightininches = rusrheightininches * 10;
        }
        Double rusrheightinincheschecker = rusrheightininches;
        rusrheightininches = Math.ceil(rusrheightininches);
        if (rusrheightininches > rusrheightinincheschecker)
        {
            rusrheightininches = rusrheightininches - 1;
        }
        else
        {

        }
        System.out.println("Hii " + usrname + ". I am Cartic, a basic textual virtual personal assistant. I do know that you are " + usrage
                + " years old. You are a " + usrgender + " and are " + rusrheight + " feet " + rusrheightininches + " inch(es) tall");
    }
}

Now, it is much easier to see the structure.

Java code-style guidelines put the { opening brace on the same line as the control statement, so, for example, the code:

        if (rusrheightininches >= 0.10 && rusrheightininches < 0.12)
        {
            rusrheightininches = rusrheightininches * 100;
        } else {
            rusrheightininches = rusrheightininches * 10;
        }

should be:

        if (rusrheightininches >= 0.10 && rusrheightininches < 0.12) {
            rusrheightininches = rusrheightininches * 100;
        } else {
            rusrheightininches = rusrheightininches * 10;
        }

Also, in the previous if statement you have an empty else block.... you can just remove it:

        if (rusrheight > usrheight)
        {
            rusrheight = rusrheight - 1;
        }
        else
        {

        }

becomes:

        if (rusrheight > usrheight) {
            rusrheight = rusrheight - 1;
        }

As for your variables, they are nice descriptive names, but, in Java, it is common to use what is called 'camelCase', where the first letter is lower-case, and the subsequent first-word-letters are Capitalized.

Also, while it is often convenient to shorten long parts of variables, the abbreviation of 'user' to 'usr' is not saving you much.....

So, for example, your variables :

String usrname;
String usrage;
String usrgender;
Double usrheight;

would be

String userName;
String userAge;
String userGender;
Double userHeight;

I would recommend that you use an IDE (Eclipse/IntelliJ/Netbeans) to help you get these things right. There is some debate about whether new programmers should use an IDE or not, because they make some things really easy and you may miss understanding some of the basic requirements of the language.... but, I believe that the IDE's allow you to focus on the stuff that is more important, like the code content.

share|improve this answer
    
admittedly i fall into the group that recommends against new developers using advanced IDEs. Eclipse is a great IDE, but features like intellisense and the numerous editor plugins, allow for bad practices. Its important for any programmer to learn how to read and understand a JavaDoc, instead of just letting intellisense tell you the methods. It also requires you to pay more attention to the Models you're building. my recommendation would be jgrasp.org/schools.html it's no frills it has the JVM, but it was developed by Auburn University and is an excellent platform to learn on. –  Madullah Apr 24 at 15:14
    
@Madullah I understand both perspectives, and agree with a lot of the arguments on both sides... ;-) In my opinion, the 'balance' falls in favour of using an IDE (for me the tipping-factor is the ease-of-debugging running programs). –  rolfl Apr 24 at 15:17
    
I agree, my software recommendation, has an excellent visual debugger. jgrasp.org/debugger.html and is used by many HS and Universities becuase of what it does and doesn't provide. –  Madullah Apr 24 at 15:21
Scanner name  = new Scanner(System.in);
Scanner age  = new Scanner(System.in);
Scanner gender  = new Scanner(System.in);
Scanner height  = new Scanner(System.in);

There is absolutely no reason to have four scanner variables. It does not matter which one you use, it would be better to just have one: Scanner scanner = new Scanner(System.in);

Also, you should close the Scanner once your done with it by calling scanner.close(); (or use try-with-resources statement)


Double usrheight;

You don't need to use Double, use double instead. The difference is that double is a primitive type, which means that among other things it cannot be set to null.


else 
{

}

There's no reason in including an else if you don't do anything in it. Remove that part.


A variable name such as rusrheightininches is easier to read by using "camelCase", I would name it realUserHeightInches

Instead of using rusrheightininches = rusrheightininches * 100; you can do rusrheightininches *= 100; which is just a shorter way of writing it.


Variable Scope. Your variables are currently declared as fields in the class. They only need to be accessible inside one method, so you can declare them within that method as local variables.


Taking it further

Java is an Object Oriented language. Write your own classes, use objects, add more methods. Everything does not need to be done inside one method. I suggest you read Oracle's tutorial at http://docs.oracle.com/javase/tutorial/java/javaOO/ . You could use a Person class for example with the properties name, age, gender height.

share|improve this answer

This code does not work.

When you try to run your program, Java will complain because it can't find the main method. Isn't there a main method right there? Yes, but it has the wrong signature. Java looks for a public static void main (String[] argv) – both the static and the String[] argument are important.

The simplest way to add that is

public static void main(String[] argv) {
    new LMComplex().main();
}

The next big problem is that you are creating multiple Scanners around the same input stream System.in. Due to buffering, lines will get lost. Instead, create only one Scanner, and request multiple nextLines from it:

Scanner in = new Scanner(System.in);
System.out.println("Enter your name: ");
usrname = in.nextLine();
System.out.println("Enter your age(Numbers): ");
usrage = in.nextLine();
...

Once these issues are fixed, the program will actually start to work correctly, and there are many style issues to improve (see rolfl's answer for a good summary).

Another bug: The nextDouble method of a Scanner is locale dependent. When I enter 5.5 as the height, I will get an exception because the decimal separator in my German locale is the comma – 5,5 works perfectly. To avoid this, we need to set a locale that works independently of the system locale:

Scanner in = new Scanner(System.in);
in.useLocale(new Locale("en", "US"));
share|improve this answer
    
I think saying "This code does not work." just because it's lacking a main method is a bit overkill. Besides lacking a main method, the code itself runs perfectly fine. I did not encounter any problem with lines getting lost, any way to reproduce that? –  Simon André Forsberg Apr 24 at 11:38
    
@SimonAndréForsberg I thought the same thing, but then I ran the code. Apparently creating multiple scanners somehow exhausts the lines (?) –  amon Apr 24 at 11:41
    
I also ran the code (in Eclipse), and I did not encounter that problem. Strange. Ideone bug? –  Simon André Forsberg Apr 24 at 11:45
    
@SimonAndréForsberg I can reproduce the failure on my machine, by piping the input through STDIN. My java -version output: java version "1.7.0_51" OpenJDK Runtime Environment (IcedTea 2.4.4) (7u51-2.4.4-0ubuntu0.12.04.2) OpenJDK Server VM (build 24.45-b08, mixed mode, sharing) –  amon Apr 24 at 11:52

Naming:

You should use readable names for your variables. Furthermore the convention for Java-naming is camelCase. Your fieldnames should start with a lower-case letter and each new "word" inside of these names, should start with a single upper-case letter.

readable here means, you shouldn't have to do some crazy tounge-exercise when reading variable names out loud. usr and similar is not good, there is no need to save characters in the wrong place:

usrname --> userName
usrage  --> userAge
//repeat with all other variables
share|improve this answer

Most has been said already but I'll add that you can contract these statements

rusrheightininches = rusrheightininches * 100; 
rusrheightininches = rusrheightininches - 1;

to

rusrheightininches *= 100;
rusrheightininches -= 1;

I'll restate the importance of naming conventions: this is unreadable

rusrheightinincheschecker 

but this isn't:

rUserHeightInInchesChecker
share|improve this answer

Your height input routine is broken, because you have used a floating-point number to represent two numbers that just happen to be separated by a dot. The fundamental flaw is that 5 ft. 1 in. is indistinguishable from 5 ft. 10 in.

System.out.println("Enter your height(feet.inches . 5 feet 5 inches = 5.5): ");
usrheight = height.nextDouble();
Double rusrheight = Math.ceil(usrheight);
if(rusrheight > usrheight)
{
rusrheight = rusrheight - 1;
}
else 
{

}
Double rusrheightininches = usrheight - rusrheight;
if(rusrheightininches >= 0.10 && rusrheightininches < 0.12)
{
rusrheightininches = rusrheightininches * 100;
} else {
rusrheightininches = rusrheightininches * 10;
}
Double rusrheightinincheschecker = rusrheightininches;
rusrheightininches = Math.ceil(rusrheightininches);
if(rusrheightininches > rusrheightinincheschecker)
{
rusrheightininches = rusrheightininches - 1;
}
else 
{

}

There are a lot of puzzling moments as I read through that code.

  • Why are there empty else clauses? Just leave them out.
  • Why do you declare rusrheight, rusrheightininches, and rusrheightinincheschecker as Double objects? That will cause much unnecessary autoboxing and unboxing.
  • What does rusrheight mean? Isn't it just Math.floor(usrheight)?
  • What is the point of rusrheightinincheschecker? It just helps to compute Math.floor(rusrheightininches) the hard way.

In the end, though, those concerns are irrelevant since the parsing strategy is fundamentally flawed. You really ought to write a custom parsing routine. For example:

/**
 * Reads a height in feet and inches, in the format fff.ii.
 * The input must be at the end of a line.
 *
 * @return The height as inches (i.e., "4.3" is interpreted as 4 * 12 + 3 = 51).
 */
public static double readHeightAsInches(Scanner scan)
        throws InputMismatchException {
    scan.next(Pattern.compile("([+-]?\\d+)(?:\\.(\\d*\\.?\\d*))?$"));
    MatchResult match = scan.match();
    int feet = Integer.parseInt(match.group(1));
    double inches = (match.group(2) != null && !match.group(2).isEmpty()) ?
        Double.parseDouble(match.group(2)) : 0.0;
    return 12 * feet + (feet >= 0 ? inches : -inches);
}

It's easier to return a single number representing the number of inches rather than a pair of numbers representing feet and inches.

That would be paired with a formatting routine:

public static String formatHeight(double inches) {
    StringBuffer sb = new StringBuffer(20);
    if (inches < 0) {
        sb.append('-');
        inches = -inches;
    }
    if (inches >= 24) {
        sb.append((int)inches / 12).append(" feet ");
    } else if (inches >= 12) {
        sb.append((int)inches / 12).append(" foot ");
    }
    sb.append(inches %= 12);
    sb.append(inches >= 1 && inches < 2 ? " inch" : " inches");
    return sb.toString();
}
share|improve this answer
    
Agreed on all points, but the replacement code looks rather daunting, especially the use of a regex. To read in feet and inches, it'd be simpler just to read them in using two separate questions, or alternatively to read in one line and split it, parsing each element of the result. For formatting, I'd use int feet = (int)inches / 12; int extraInches = inches % 12;, and then the pluralization checks become clearer. Support for fractional inches and negative height seems unnecessary. –  deltab Apr 25 at 12:46
    
@deltab Agreed, that the code could be much simpler if you don't need to implement the originally intended behaviour. –  200_success Apr 25 at 16:34

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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