9
\$\begingroup\$

I am new to Java and wrote one small program. It works fine, but it looks like it doesn't meet OOP concepts. Could someone look and give me advice so that I can fine tune?

public class App {    public static void main(String[] arg) {        String str = new String(                "]d010036195815011121123456789]d17YYMMDD1012345678901");        String matchItemAI = new String("01"); //GTIN-14        String matchSNoAI  = new String("21"); //Serial Number        String matchExpDtAI = new String("]d17");// Expiry Date        String matchLotNoAI = new String("10"); //Lot Number        //Company Prefix        String matchCompPrefixUS = new String("03"); //US Company Prefix        String matchCompPrefixCork = new String("03"); //US Company Cork        String matchCompPrefixSKorea = new String("03"); //US Company South Korea               String value = str;        String value1 =  new String();        String value2 =  new String();        String value3 =  new String();        char ch;        int pos;        // 1. Need to print ]d0100361958150111 like that 61958-1501-1        // 2. Need to print 21123456789 like that 123456789        // 3. Need to print ]d17YYMMDD like that YYMMDD        // 4. Need to print 1012345678901 like that 12345678901        // GS1 Start with this String....It's a GS1 2d Bar Code, Confirmed Then        // Process the record        if (str.startsWith("]d")) {            System.out.println("GS1 2D Input String :" + str);            ch = str.charAt(2);            switch (ch) {            case '0':                System.out                        .println("Calculating Unit of Sale (0) Packaging Indicator digits for GTIN 14's   : "                                + str.charAt(2));                   pos = str.indexOf(matchItemAI);                System.out.println("GS1 pos:" + value.substring(pos)+" POS Value  " +pos);                value = value.substring(pos + 5, value.length());                value1 = value.substring(0, 5);                value2 = value.substring(value1.length(), value1.length() + 4);                value3 = value.substring(value1.length() + value2.length(),                        value1.length() + value2.length() + 1);                value3 = value1 + "-" + value2 + "-" + value3;                value = value.trim();                System.out.println("Found string Item :  " + value3);                // /GET Serial Number                pos = str.indexOf(matchSNoAI); // AI 21                // System.out.println("Found string SNo :  " + pos);                value = str.substring(pos + 2, str.lastIndexOf(matchExpDtAI)); //pos + 2 + 9);                value = value.trim();                System.out.println("Found string SNo :    " + value);                pos = str.lastIndexOf(matchExpDtAI);                // System.out.println("Found string Expiry Date " + pos);                value = str.substring(pos + 4, pos + 4 + 6);                value = value.trim();                System.out.println("Found string Expiry Date :   " + value);                pos = str.lastIndexOf(matchLotNoAI);                // System.out.println("Found string Lot Number " + pos);                value = str.substring(pos + 2, pos + 2 + 11);                value = value.trim();                System.out.println("Found string Lot Number  : " + value);                break;            case '3':                System.out                        .println("Calculating Bundle/Multipack 3 Packaging Indicator digits for GTIN 14's  : "                                + str.charAt(2));                break;            case '5':                System.out                        .println("Calculating Shipper 5 Packaging Indicator digits for GTIN 14's  : "                                + str.charAt(2));                break;            default:                System.out                        .println("Error - invalid selection entered! for Multipacking ");                break;            }        }    }}

Here I made change as per your suggestion, can you please review and let me know any other suggestion ?

public class App {public static void main(String[] arg) {    String str = new String(            "]d010036195815011121123456789]d17YYMMDD1012345678901");    String matchItemAI = "01"; //GTIN-14    String matchSNoAI  = "21"; //Serial Number    String matchExpDtAI = "]d17";// ExpiryDate    String matchLotNoAI = "10"; //Lot Number    String WitoutFNC1 = str;    String NDCCode    = "";    String itemRef    =  "";    String itemNo     =  "";    int pos;    // GS1 Start with this String....It's a GS1 2d Bar Code, Confirmed     if (str.startsWith("]d")) {        System.out.println("GS1 2D Input String :" + str);        switch (str.charAt(2))  {        case '0':            pos = str.indexOf(matchItemAI);            System.out.println("GS1 pos:" + WitoutFNC1.substring(pos)+" POS Value  " +pos);            WitoutFNC1 = WitoutFNC1.substring(pos+5, WitoutFNC1.length());            NDCCode = WitoutFNC1.substring(0, 5);            itemRef = WitoutFNC1.substring(NDCCode.length(), NDCCode.length() + 4);            itemNo = WitoutFNC1.substring(NDCCode.length() + itemRef.length(),NDCCode.length() + itemRef.length() + 1);            itemNo = String.format("%s-%s-%s",NDCCode, itemRef ,itemNo);            WitoutFNC1 = WitoutFNC1.trim();            System.out.println("Found string Item :  " + itemNo);            // /GET Serial Number            pos = str.indexOf(matchSNoAI); // AI 21            // System.out.println("Found string SNo :  " + pos);            WitoutFNC1 = str.substring(pos + 2, str.lastIndexOf(matchExpDtAI)); //pos + 2 + 9);            WitoutFNC1 = WitoutFNC1.trim();            System.out.println("Found string SNo :    " + WitoutFNC1);            pos = str.lastIndexOf(matchExpDtAI);            // System.out.println("Found string Expiry Date " + pos);            WitoutFNC1 = str.substring(pos + 4, pos + 4 + 6);            WitoutFNC1 = WitoutFNC1.trim();            System.out.println("Found string Expiry Date :   " + WitoutFNC1);            pos = str.lastIndexOf(matchLotNoAI);            // System.out.println("Found string Lot Number " + pos);            WitoutFNC1 = str.substring(pos + 2, pos + 2 + 11);            WitoutFNC1 = WitoutFNC1.trim();            System.out.println("Found string Lot Number  : " + WitoutFNC1);            break;        case '3':            System.out                    .println("Calculating Bundle/Multipack 3 Packaging Indicator digits for GTIN 14's  : "                            + str.charAt(2));            break;        case '5':            System.out                    .println("Calculating Shipper 5 Packaging Indicator digits for GTIN 14's  : "                            + str.charAt(2));            break;        default:            System.out                    .println("Error - invalid selection entered! for Multipacking ");            break;        }    }    }}
ChrisW's user avatar
ChrisW
13.1k1 gold badge35 silver badges76 bronze badges
askedJan 28, 2014 at 22:17
user3246453's user avatar
\$\endgroup\$
7
  • \$\begingroup\$"but it's not like it should be" - does this mean that it does not work as intended?\$\endgroup\$CommentedJan 28, 2014 at 22:20
  • 1
    \$\begingroup\$it' working as it is but looks like it's not meeting OOP concept\$\endgroup\$CommentedJan 28, 2014 at 22:25
  • \$\begingroup\$I see. Would you mind re-wording that phrase in your question? It could confuse some people to believe that your code does not work as intended, and therefore is off-topic.\$\endgroup\$CommentedJan 28, 2014 at 22:32
  • 3
    \$\begingroup\$Please don't edit the original code, because that would invalidate the existing answer.\$\endgroup\$CommentedJan 29, 2014 at 0:23
  • 1
    \$\begingroup\$@user3246453 I edited your question to add your new codeafter (not instead of) the original code. Or, instead of adding to an existing question, it's also possible to ask a new question: seeHow to deal with follow-up questions?\$\endgroup\$CommentedJan 29, 2014 at 1:23

2 Answers2

6
\$\begingroup\$
  1. When you look at the color highlighting of your code, you can see thatWithoutFNC1 is colored blue like other Java types. This also suggests that in the Java world, it is not recommended for non-static variable names to start with an upper case.

  2. These two blocks are very similar and you can already create a static method for it, for starters:

        pos = str.lastIndexOf(matchExpDtAI);    // System.out.println("Found string Expiry Date " + pos);    WitoutFNC1 = str.substring(pos + 4, pos + 4 + 6);    WitoutFNC1 = WitoutFNC1.trim();    System.out.println("Found string Expiry Date :   " + WitoutFNC1);    pos = str.lastIndexOf(matchLotNoAI);    // System.out.println("Found string Lot Number " + pos);    WitoutFNC1 = str.substring(pos + 2, pos + 2 + 11);    WitoutFNC1 = WitoutFNC1.trim();    System.out.println("Found string Lot Number  : " + WitoutFNC1);
  3. Also, when you say a substring is "found", do you really mean that the string is not empty? Or perhaps you can apply other validation to ensure that the desired substring is really found? You can create a static method to test your "found" substrings. Otherwise, I suppose deriving an empty substring and printingFound string Item : is not going to be helpful...
  4. Just a simple note on most methods forString: If you find yourself repeating variable assignment, you can string one method after another, e.g." this is some text ".substring(5).trim(), because they simply return copies of theString.
answeredJan 29, 2014 at 3:51
h.j.k.'s user avatar
\$\endgroup\$
12
\$\begingroup\$

  • You consistently doString foo = new String("some string"). This is useless, as"some string" already is a string. Simplify your code toString foo = "...".

  • You declare some variables far before you use them. Try to declare your variables as close to their point of use as possible, e.g. instead of

    char ch;...ch = str.charAt(2);

    you could just dochar ch = str.charAt(2). The same holds true forpos,value,value1,value2,value3.

  • Actually, you don't even need thech variable as you could doswitch (str.charAt(2)) { ... } directly.

  • Some of your variables have very bad named:value1 does not communicate anyintent or meaning. Other variables use unecessary abbreviations.

  • You have certain magic numbers that could be replaced. E.g. instr.substring(pos + 4, pos + 4 + 6), the4 is actuallymatchExpDtAI.length(). I have no idea where the6 comes from.

  • This code is pure obfuscation:

    value = value.substring(pos + 5, value.length());value1 = value.substring(0, 5);value2 = value.substring(value1.length(), value1.length() + 4);value3 = value.substring(value1.length() + value2.length(),                value1.length() + value2.length() + 1);value3 = value1 + "-" + value2 + "-" + value3;value = value.trim();System.out.println("Found string Item :  " + value3);

    Note that you don't use thevalue1,value2, andvalue3 variables outside of this snippet, so they are unecessary. If we count the lengths of those substrings, we can see that this code should be equivalent:

    value = value.substring(pos + 5, value.length());System.out.println("Found string Item : " +    value.substring(0,     5        ) + "-" +    value.substring(5,     5 + 4    ) + "-" +    value.substring(5 + 4, 5 + 4 + 1));value = value.trim();

    Those constant offsets can be folded, which makes it easier to see that these substrings are actually consecutive:

    value = value.substring(pos + 5), value.length());System.out.println("Found string Item : " +    value.substring(0,  5) + "-" +    value.substring(5,  9) + "-" +    value.substring(9, 10));value = value.trim();

  • Actually, let's useString.format for that:

    value = value.substring(pos + 5, value.length());System.out.println(String.format("Found string Item: %s-%s-%s",    value.substring(0,  5),    value.substring(5,  9),    value.substring(9, 10)));value = value.trim();

Before thinking about whether an object-oriented approach would besensible (it isn't), you have other parts of your code to clean up first.

answeredJan 28, 2014 at 22:55
amon's user avatar
\$\endgroup\$
1
  • 2
    \$\begingroup\$Amon, Thanks for reviwing the code and your valuable suggestion and feedback, I will make change as you suggested, it's really helpful for me, please keep doing good work\$\endgroup\$CommentedJan 28, 2014 at 23:00

You mustlog in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.