Java : is there any way that this code can be improved

Question

I have this coding inside my java file . This code basically receives an int value and sets a char value for an Object

Student st = new Student();

if (attributeName.equals("Grade")) {
    int sidevalue = Integer.parseInt(attribute.getValue());  // This returns an int value , and this value needs to be converted into char later as shown 
    if(sidevalue==1)
        st.grade=1;
    else if(sidevalue==2)
        st.grade=2;
    else if(sidevalue==5)
        st.grade=3;
}


class Student
{
    char grade ;
}

Start with formatting it correctly. – Thomas Jungblut Mar 01 '12 at 10:28 — Thomas Jungblut, Mar 01 '12 at 10:28
Please post the full source.. you may get better feedbacks. – Jayan Mar 01 '12 at 10:35 — Jayan, Mar 01 '12 at 10:35

Andreas Dolk · Answer 1 · 2012-03-01T10:49:00.313

Integer.parseInt throws an exception, if the attribute name is not a number. Catch it and handle the error.
The Student class should be made public and go to a separate java file
The grade field should be an int (your passing numbers)
Alternatively - if you need to store chars, you may have to say st.grade = '1'; (to pass a '1' instead of a 0x01)
The grade field should be private, use getGrade and setGrade methods to read an write the property
I don't understand the meaning of "sidevalue" - if it does not have a well known meaning in the domains context, then consider renaming it.
The local variable st should be renamed to student
the if-else-if chain could be replaced by a switch-case statement.

score 3 · Answer 2 · answered Mar 01 '12 at 10:36

You could use a map

Map<Integer, Integer> gradeMappings = new HashMap<Integer, Integer>();
gradeMappings.put(1,1);
gradeMappings.put(2,2);
gradeMappings.put(3,5);

if (attributeName.equals("Grade")) {
    int sidevalue = Integer.parseInt(attribute.getValue());
    st.grade = gradeMappings.get(sidevalue);
}

In real life, you'd want to add some exception checking for when the attribute value is not a key in the map. You could also make the default behaviour to use the parsed attribute value as the grade, and only override the value if an appropriate entry is present in the map.

score 2 · Accepted Answer · edited May 23 '17 at 12:03

A switch case check here statement could avoid multiple if else indentation!

switch(sidevalue){

   case 1: st.grade = 1;
            break;
   case 2: st.grade = 2;
            break;
   case 5: st.grade = 3;
            break;
   default: break;
}

There is no significant execution difference in running between if-else and switch. Observed differences may be due to the sample space of the particular code you are running.

In the little code snippet you provide there is not a best choice, except that switch statement provides improved readability.

Check these links for further details:

Thank you very much , could anybody please let me know is using Switch over if else does improve the performance ?? — Pawan, Mar 01 '12 at 11:16

score 1 · Answer 4 · answered Mar 01 '12 at 10:28

1

Look into the control keyword switch. This will somewhat alleviate this style of coding.

answered Mar 01 '12 at 10:28

Nim

33,299
2
62
101

score 1 · Answer 5 · answered Mar 01 '12 at 10:31

The best way to improve this code is to write a test for it.

Problems at this stage: 'grade' should be private. Grade is probably a first class object.
How do you plan to support other grades? Say 4 or 6? What happens if sidevalue comes back with invalid value?

score 0 · Answer 6 · answered Mar 01 '12 at 11:05

Others have suggested minor syntax changes (which may be helpful), but I think you are best thinking about this in an object-oriented sense, encapsulating this logic into the student class itself, that way you will only need to write it once. If you need different logic for different scenarios you can always use inheritance and override setGrade.

Student st = new Student();
st.setGrade(Integer.parseInt(attribute.getValue()))

class Student{
    private char grade ;

    public setGrade(int sidevalue){
        if(sidevalue==1)
            grade=1;
        else if(sidevalue==2)
            grade=2;
        else if(sidevalue==5)
            grade=3;
        else
            throw new IllegalArgumentException();
    }

    public char getGrade(){ 
        return grade; 
    }
}

Siddharth Garg · Answer 7 · 2012-03-01T12:06:07.320

0

If you have canonical String values stored in attribute.getValue() then you can remove integer parsing and simply compare string values

String sidevalue=attribute.getValue()

if(sidevalue=="1"){

}else if(sidevalue=="2"){

}...

Refer to String.intern method for more details. This will help improve performance of your program.

edited Mar 01 '12 at 12:06

answered Mar 01 '12 at 11:50

Siddharth Garg

1
1

score -1 · Answer 8 · answered Mar 01 '12 at 10:38

-1

1) Start using coding formats. Its advisable to use objStudent rather than some 'st'.

2) There is no use of creating int sidevalue. Directly use a switch case as give below:

switch(Integer.parseInt(attribute.getValue()))
{
  case 1:
    ----
.
.
.

and so on...

answered Mar 01 '12 at 10:38

Anuj Balan

7,629
23
58
92

1

I don't think "objStudent" is advisable at all. That kind of Hungarian notation should be discouraged. – duffymo Mar 01 '12 at 10:55
I've learnt it from sites which have encouraged such a notation so that there wont be any ambiguity referring objects of the classes. – Anuj Balan Mar 01 '12 at 11:03
Can you show me a site which says the above. I'd rather like to rectify myself if I was wrong all this time about naming class objects. Hope you'd come out with a reason for your comments or else why do you post it? Support your statement with some sites which says so. – Anuj Balan Mar 01 '12 at 11:07
@Ajj: `Can you show me a site which says the above` Ok, how about stack overflow? http://stackoverflow.com/questions/111933/why-shouldnt-i-use-hungarian-notation – Sean Reilly Mar 01 '12 at 12:21
You need to look at other sites. Hungarian notation is not recommended. – duffymo Mar 01 '12 at 13:08

Java : is there any way that this code can be improved

8 Answers8