current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite

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;
        }
    }

    }
}
share|improve this question
    
"but it's not like it should be" - does this mean that it does not work as intended? –  syb0rg Jan 28 '14 at 22:20
1  
it' working as it is but looks like it's not meeting OOP concept –  user3246453 Jan 28 '14 at 22:25
    
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. –  syb0rg Jan 28 '14 at 22:32
3  
Please don't edit the original code, because that would invalidate the existing answer. –  ChrisW Jan 29 '14 at 0:23
1  
@user3246453 I edited your question to add your new code after (not instead of) the original code. Or, instead of adding to an existing question, it's also possible to ask a new question: see How to deal with follow-up questions? –  ChrisW Jan 29 '14 at 1:23

2 Answers 2

up vote 5 down vote accepted
  1. When you look at the color highlighting of your code, you can see that WithoutFNC1 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 printing Found string Item : is not going to be helpful...
  4. Just a simple note on most methods for String: 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 the String.
share|improve this answer
up vote 11 down vote

  • You consistently do String foo = new String("some string"). This is useless, as "some string" already is a string. Simplify your code to String 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 do char ch = str.charAt(2). The same holds true for pos, value, value1, value2, value3.

  • Actually, you don't even need the ch variable as you could do switch (str.charAt(2)) { ... } directly.

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

  • You have certain magic numbers that could be replaced. E.g. in str.substring(pos + 4, pos + 4 + 6), the 4 is actually matchExpDtAI.length(). I have no idea where the 6 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 the value1, value2, and value3 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 use String.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 be sensible (it isn't), you have other parts of your code to clean up first.

share|improve this answer
2  
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 –  user3246453 Jan 28 '14 at 23:00

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.