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 started Java programming yesterday (so don't expect too much), and I've written some code. My code works the way I want it to work, but there are obviously things wrong with it.

I was wondering how it could be improved. I think I've added unnecessary things. I'm practicing using classes and other stuff.

the testing.java files contains:

import java.util.Scanner;

class testing {
    private static Scanner input_sn;
    private static Scanner input_fn;
    private static Scanner input_mem;

    public static void main(String[] args){

        String First_Name;
        String Second_Name;
        int members;
        int count;


        System.out.println("Members: ");
        input_mem = new Scanner(System.in);
        members = input_mem.nextInt();

        funcs funcsObj = new funcs();

        for (count = 0; count < members; count++)
        {
            System.out.println("What is the first name? ");
            input_fn = new Scanner(System.in);
            First_Name = input_fn.nextLine();

            System.out.println("What is the second name? ");
            input_sn = new Scanner(System.in);
            Second_Name = input_sn.nextLine();

            funcsObj.names( funcsObj.setFn(First_Name), funcsObj.setSn(Second_Name));
        }   
    }
}

and my funcs.java file contains:

public class funcs
{
    private String firstName;
    private String secondName;
    private static int members = 0;

    public String setFn(String fn)
    {
        firstName = fn;
        return fn;
    }

    public String setSn(String sn)
    {
        secondName = sn;
        return sn;
    }


    public void names(String fn, String sn)
    {
        firstName = fn;
        secondName = sn;
        members++;
        System.out.printf("%d\t%s\t%s\n", members, fn, sn);
    }
}

I think most of the problems can be found in the funcs.java file.

Thanks

share|improve this question
    
You've added a "Performance" tag. Is the program too slow for you? Just how fast do you need it to be? –  luiscubal Feb 5 '13 at 20:54
add comment

2 Answers 2

Styling:

You have to fix the case of your identifiers to be more idiomatic. It means:

  • Class names should be Capitalized ("Testing", "Funcs", ...)
  • Class members and variables should use camel case ("firstName", "inputSn", ...)

Also:

  • The method names of class funcs behaves like a "setter", so it should have a name like setNames. In general, try to use action verbs in your method names.
  • Edit: the setXX() methods of funcs should not return anything, as they are "setters", they are there to "set".

Code quality:

  • Variables like "String First_Name" should not all be declared at the beginning of the method, but instead next to the place where they are first used. This way you limit their scope (which makes programming errors more clear) and the code is easier to read.
  • Similarily, if you don't have a compelling reason to have all the Scanner instances as class members, declare them in your main method where they are used.
  • Why do you use different Scanner objects for every input? You can simply use one Scanner and use its nextLine() method 3 times. That's the only "performance" tips that you can get, I think (not that you would have any performance problem with such a short program anyway).
  • Edit: In the line funcsObj.names( funcsObj.setFn(First_Name), funcsObj.setSn(Second_Name)); you are actually setting the members of your funcs objects twice. First in the setFn and setSn methods and then in the names method. You don't need to. Just call your setXX methods. If you want to have the members thing incremented and the data displayed, you can rename names into incrementMembers and remove all setting code from there.
share|improve this answer
2  
the setXX() methods of funcs should not return anything, as they are "setters" well, unless they return this, allowing for method-chaining. but that's probably outside the scope of this issue. –  Michael Paulukonis Feb 5 '13 at 21:23
add comment

Given that Cyrille has already given some good advice, here's a more 'learned' solution, showing some patterns and possibilities to watch out for:

public class App {
    public static void main(String[] args) {

        new Thread(new Runnable() {
                       public void run() {
                           Testing.collectAndReport(System.out, System.in);
                       }
                   }).start();
    }
}

The first thing that you should know is that it's always best to isolate pretty much everything from your main(...) method. Among other things, this means that it can be easy to include your actual application in some other framework, simply by saying "instantiate this class". Parsing args as needed should be fine.
For the same reason, it's best for the main method to not 'run' your actual application, either. This is more for when dealing with Swing, where you're supposed to use the 'event-dispatch' thread, but here results in (directly) creating a thread to start.
Thread should not be sub-classed, so an anonymous inner sub-class of Runnable (an interface) is used. The one required method, run(), contains the actual 'startup' code of the application.
It's always best to 'inject' dependencies, such as input/output streams (System.in and System.out). For one thing, this allows other implementations to be swapped out as necessary; in our 'application framework' example, this means I could use a fancy text editor, instead of requiring use of the console.

public final class Member {

private final String firstName;
private final String lastName;

public Member(String firstName, String lastName) {
    this.firstName = firstName;
    this.lastName = lastName;
}

public String getFirstName() {
    return firstName;
}

public String getLastName() {
    return lastName;
}

@Override
public String toString() {
    return String.format("%s\t%s", firstName, lastName);
}

}

Learn how to make good use of 'value' types - small classes, comprising no more than a few primitives. Unless there are very good reasons, these should be 'Immutable' (although you're allowed to 'cheat', sortof) - among other things, doing so makes multi-threaded code much simpler. Use things like an overrideable toString() method to be able to get an 'understandable' output.

import java.io.InputStream;
import java.io.PrintStream;
import java.util.Iterator;
import java.util.Scanner;

public class Testing {

    public static void collectAndReport(PrintStream out, InputStream in) {
        final Scanner input = new Scanner(in);

        // This count is because you want to print out the index.
        int count = 1;
        for (Member member : getMembers(out, input)) {
            out.printf("%d\t%s\t\n", count++, member);
        }

    }

    private static Iterable<Member> getMembers(final PrintStream out, final Scanner input) {
        return new Iterable<Member>() {

            public Iterator<Member> iterator() {

                out.println("Number of Members:");
                final int numberOfMembers = input.nextInt();

                return new Iterator<Member>() {

                    int member = 0;

                    public void remove() {
                        throw new UnsupportedOperationException("Not Implemented");
                    }

                    public Member next() {
                        out.println("What is the first name?");
                        final String firstName = input.next();

                        out.println("What is the second name?");
                        final String lastName = input.next();

                        return new Member(firstName, lastName);
                    }

                    public boolean hasNext() {
                        return member++ < numberOfMembers;
                    }
                };
            }
        };
    }
}

... Learn how to cheat. Computer programming is largely about being able to think in abstractions. Learn what abstractions your language supports, and how to use them. Also learn the standard libraries (and the more commonly used public ones), and what they enable.
For instance, by implementing Iterable<Member> like this, I can work with the results as a collections much easier. By implementing Iterator<Member>, I can trivially modify this code to accept 'early exit' from the member creation, or even allow 'infinite' entries (by changing how hasNext() works).

All code compiles and runs on my local JVM/eclipse instance.

share|improve this answer
add comment

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.