HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Matching program in Java

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
programmatchingjava

Problem

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 = val

Solution

-
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.

Code Snippets

char ch;
...
ch = str.charAt(2);
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);
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();
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();
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();

Context

StackExchange Code Review Q#40304, answer score: 11

Revisions (0)

No revisions yet.