Ways to improve foll. Java code

Question

Any ways to improve the foll. code block :

public class MyUnits {
    public static String MILLSECONDS = "milliseconds";
    public static String SECONDS = "seconds";
    public static String MINUTES = "minutes";
    public static String HOURS = "hours";

    public int quantity;
    public String units;

    public MyUnits(int quantity, String units) {
        this.quantity = quantity;
        this.units = units;
    }

    public String toString() {
        return (quantity + " " + units);
    }

    // Test code
    public static void main(String[] args) {
        System.out.println(new MyUnits(1, MyUnits.MILLSECONDS));
        System.out.println(new MyUnits(2, MyUnits.SECONDS));
        System.out.println(new MyUnits(3, MyUnits.MINUTES));
        System.out.println(new MyUnits(4, MyUnits.HOURS));
    }
}

Any help will be appreciated.

In which way? What is your goal? Performance? Readability? Obfuscation? — Daniel, Jan 26 '11 at 05:58
Please note that Java already has its own TimeUnit class in java.util.concurrent. — Daniel, Jan 26 '11 at 05:59
@Daniel : Not in any particular way. Just, in general, ways to improve code. — Pritish, Jan 26 '11 at 06:03

score 3 · Answer 1 · answered Jan 26 '11 at 06:03

The static ints should be final. The usual modifier for a "constant" in Java is

public static final <Type> <name> = <value>;

A bigger enhancement: exchange the static int with enum:

public enum Unit {MILLISECOND, SECOND, MINUTE, HOUR}

The new constructor signature would be:

public MyUnits(int quantity, Unit unit) { ... }

The non-static fields in MyUnit should be made private. Add getter/setter methods for access.

And finally (and for serious code only), I'd separate the test code from the class. Have a look at testing frameworks, like junit and implement separate test classes.

score 1 · Answer 2 · answered Jan 26 '11 at 06:07

Mark your public static variables as final; better yet, use enums.
Don't let instance variables (quantity, units) be public. Provide "getter" methods to read their values. Consider not providing "setter" methods to change their values. This makes your class immutable, which can make it easier to use (the state of an immutable object is much more predictable than that of a mutable object!)
Be more specific about what the code is intended to do; then make it do that. (This also allows you to ask more specific questions, which would get you better answers.)
Add comments, especially javadoc comments.

score 0 · Answer 3 · 2011-01-26T06:09:15.370

0

use enum for the unit within your class.

public enum MyUnit {
MILLSECONDS(1, "milliseconds"), SECOND(2, "seconds"),
MINUTES(3,"minutes"), HOURS(4, "hours");



private MyUnit(int quantity, String units) {
    this.quantity = quantity;
    this.units = units;
}

private int quantity;
private  String units;

public String toString() {
    return (quantity + " " + units);
}
    /*getters setters*/
}

and just call like that

MyUnit.HOURS.getUnits();
MyUnit.HOURS.getCuantity();

edited Jan 26 '11 at 06:09

answered Jan 26 '11 at 06:00

Removed +1, because you don't know what enums are, and the quantity seems to be arbitrary in his implementation. – Daniel Jan 26 '11 at 06:07
@Daniel - disagree, hilal knows pretty well how to use enums. OTH, agree, because the OP's concept of "MyUnit" was different - he wanted to create objects to describe "a quantity of something", like "4 milliseconds" or "25 milliseconds". – Andreas Dolk Jan 26 '11 at 09:14
hilal: Sorry, you know what enum are. Simple the kind how you used them would not allow other quantities. Sorry. – Daniel Jan 26 '11 at 09:27

Daniel · Answer 4 · 2011-01-26T06:10:50.433

0

Add clone(), hashcode() and equals(), and implement Comparable, Serializable and Cloneable.

Remove the test code and move it to a test class. Use JUnit for testing.

Write a function public Date addToDate(Date date).

edited Jan 26 '11 at 06:10

answered Jan 26 '11 at 06:00

Daniel

27,718
20
89
133

I'd upvote if you take out Serializable and Cloneable -- definitely take out Cloneable!! – Amir Afghani Jan 26 '11 at 06:04
read this ... http://www.xenoveritas.org/blog/xeno/java_copy_constructors_and_clone ... for example. – Stephen C Jan 26 '11 at 06:26
This article shows, what you can do wrong, and makes clone() only a bad idea if there are final fields, or the subclasses miss to implement clone(), too. His "Solution" is to use copy constructors, but thse could also be missed by subclasses. I really can't agree with the author, so Cloneable() stays in my list (even if I like copy constructors more), and no +1. – Daniel Jan 26 '11 at 06:51

Ways to improve foll. Java code

4 Answers4

Linked