2
\$\begingroup\$

I have a class that is testing to see which enzyme-filled blood a person in need of blood should get. My code works, but it is so big for the small thing that it's doing, and it looks very bad and in need of condensing.

import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Random;
import java.util.Scanner;

public class Tester {

    private static ArrayList<Type> allTypes = new ArrayList<>();

    private static Scanner scanner = new Scanner(System.in);

    private static void makeTypes() throws IOException {

        ArrayList<String> allLines = new ArrayList<>();

        System.out.println("What is the filepath holding the information?");
        String filePath = scanner.next();
        File f = new File(filePath);
        Scanner input = new Scanner (f);
        while(input.hasNextLine()) {
            String toAdd = input.nextLine().replaceAll("\\s+", "");
            allLines.add(toAdd);
            System.out.println(toAdd);
        }


        // Rh

        // D
        char[] DArray = allLines.get(0).toCharArray();

        for(int i=0; i<11; i++) { // Ds
            int amount = 0;
            if(DArray[i] == '+') {
                amount = 2;
            }
            Type D = new Type("D" + (i+1), i+1, amount);
            allTypes.add(D);
        }

        for(int i=0; i<11; i++) { // Cs
            int amount = 0;
            if(i+1==2 || i+1==3) {
                amount = 2;
            } else if(i+1==1||i+1==11) {
                amount = 1;
            }
            Type C = new Type("C" + (i+1), i+1, amount);
            allTypes.add(C);
        }

        for(int i=0; i<11; i++) { // Es
            int amount = 0;
            if(i+1==4) {
                amount = 2;
            } else if(i+1==5) {
                amount = 1;
            }
            Type E = new Type("E" + (i+1), i+1, amount);
            allTypes.add(E);
        }

        for(int i=0; i<11; i++) { // cs
            int amount = 0;
            if(i+1>=4 && i+1<=10) {
                amount = 2;
            } else if(i+1==1 || i+1==11) {
                amount = 1;
            }
            Type c = new Type("c" + (i+1), i+1, amount);
            allTypes.add(c);
        }

        for(int i=0; i<11; i++) { // es
            int amount = 0;
            if(i+1<=3 || i+1>=6) {
                amount = 2;
            } else if(i+1==5) {
                amount = 1;
            }
            Type e = new Type("e" + (i+1), i+1, amount);
            allTypes.add(e);
        }

        // MNSs

        for(int i=0; i<11; i++) { // Ms
            int amount = 0;
            if(i+1<=3 || i+1==6 || i+1==9) {
                amount = 2;
            } else if(i+1==1 || i+1==4 || i+1==5 || i+1==7 || i+1==8 || i+1==10) {
                amount = 1;
            }
            Type M = new Type("M" + (i+1), i+1, amount);
            allTypes.add(M);
        }

        for(int i=0; i<11; i++) { // Ns
            int amount = 0;
            if(i+1==2 || i+1==11) {
                amount = 2;
            } else if(i+1==1 || i+1==4 || i+1==5 || i+1==7 || i+1==8 || i+1==10) {
                amount = 1;
            }
            Type N = new Type("N" + (i+1), i+1, amount);
            allTypes.add(N);
        }

        for(int i=0; i<11; i++) { // Ss
            int amount = 0;
            if(i+1<=1 || i+1==9) {
                amount = 2;
            } else if(i+1==3 || i+1==7 || i+1==11) {
                amount = 1;
            }
            Type S = new Type("S" + (i+1), i+1, amount);
            allTypes.add(S);
        }

        for(int i=0; i<11; i++) { // ss
            int amount = 0;
            if(i+1==2 || i+1==4 || i+1==5 || i+1==6 || i+1==8 || i+1==10) {
                amount = 2;
            } else if(i+1==3 || i+1==7 || i+1==11) {
                amount = 1;
            }
            Type s = new Type("s" + (i+1), i+1, amount);
            allTypes.add(s);
        }

        // P

        for(int i=0; i<11; i++) { // P1s
            int amount = 0;
            if(i+1==1 || i+1==2 || i+1==11 || i+1>=4 && i+1<=9) {
                amount = 2;
            }
            Type P1 = new Type("P1-" + (i+1), i+1, amount);
            allTypes.add(P1);
        }

        // Lewis

        for(int i=0; i<11; i++) { // Leas
            int amount = 0;
            if(i+1==6 || i+1==7 || i+1==10) {
                amount = 2;
            }
            Type Lea = new Type("Lea" + (i+1), i+1, amount);
            allTypes.add(Lea);
        }

        for(int i=0; i<11; i++) { // Lebs
            int amount = 0;
            if(i+1==1 || i+1==2 || i+1==3 || i+1==4 || i+1==8 || i+1==9) {
                amount = 2;
            }
            Type Leb = new Type("Leb" + (i+1), i+1, amount);
            allTypes.add(Leb);
        }

        // Kell

        for(int i=0; i<11; i++) { // Ks
            int amount = 0;
            if(i+1==4 || i+1==7) {
                amount = 1;
            }
            Type K = new Type("K" + (i+1), i+1, amount);
            allTypes.add(K);
        }

        for(int i=0; i<11; i++) { // ks
            int amount = 0;
            if(i+1!=4 || i+1!=7) {
                amount = 2;
            } else if(i+1==4 || i+1==7) {
                amount = 1;
            }
            Type K = new Type("K" + (i+1), i+1, amount);
            allTypes.add(K);
        }

        // Duffy

        for(int i=0; i<11; i++) { // Fyas
            int amount = 0;
            if(i+1==3 || i+1==7 || i+1==11) {
                amount = 2;
            } else if(i+1==1 || i+1==5 || i+1==8 || i+1==9) {
                amount = 1;
            }
            Type Fya = new Type("Fya" + (i+1), i+1, amount);
            allTypes.add(Fya);
        }

        for(int i=0; i<11; i++) { // Fybs
            int amount = 0;
            if(i+1==2 || i+1==6 || i+1==10) {
                amount = 2;
            } else if(i+1==1 || i+1==5 || i+1==8 || i+1==9) {
                amount = 1;
            }
            Type Fyb = new Type("Fyb" + (i+1), i+1, amount);
            allTypes.add(Fyb);
        }

        // Kidd

        for(int i=0; i<11; i++) { // Jkas
            int amount = 0;
            if(i+1==3 || i+1==8 || i+1==11) {
                amount = 2;
            } else if(i+1==1 || i+1==2 || i+1==4 || i+1==5 || i+1==6 || i+1==10) {
                amount = 1;
            }
            Type Jka = new Type("Jka" + (i+1), i+1, amount);
            allTypes.add(Jka);
        }

        for(int i=0; i<11; i++) { // Jkbs
            int amount = 0;
            if(i+1==7 || i+1==9) {
                amount = 2;
            } else if(i+1==1 || i+1==2 || i+1==4 || i+1==5 || i+1==6 || i+1==10) {
                amount = 1;
            }
            Type Fyb = new Type("Fyb" + (i+1), i+1, amount);
            allTypes.add(Fyb);
        }
    }

    public static void main(String[] args) throws IOException {         
        Random random = new Random();
        makeTypes();

        for(int i=0; i<10; i++) {
            int cellToWant = random.nextInt(allTypes.size());
            System.out.println(allTypes.get(cellToWant).getString() + " has " + allTypes.get(cellToWant).pluses());
        }

        ArrayList<String> toCheckFor = new ArrayList<>();

        System.out.println();


        // D
        int DmarkOffs = 0;
        for(int i=(11*(1-1)); i<11+(11*(1-1)); i++) { // marking off Ds
            if(allTypes.get(i).pluses()==2) {
                DmarkOffs++;
            }
        }
        // checking if D is good
        if(DmarkOffs < 3) {
            toCheckFor.add("D");
        }


        // C
        int CmarkOffs = 0;
        for(int i=(11*(2-1)); i<11+(11*(2-1)); i++) { // marking off Cs
            if(allTypes.get(i).pluses()==2) {
                CmarkOffs++;
            }
        }
        // checking if C is good
        if(CmarkOffs < 3) {
            toCheckFor.add("C");
        }


        // E
        int EmarkOffs = 0;
        for(int i=(11*(3-1)); i<11+(11*(3-1)); i++) { // marking off Es
            if(allTypes.get(i).pluses()==2) {
                EmarkOffs++;
            }
        }
        // checking if E is good
        if(EmarkOffs < 3) {
            toCheckFor.add("E");
        }


        // c
        int cmarkOffs = 0;
        for(int i=(11*(4-1)); i<11+(11*(4-1)); i++) { // marking off cs
            if(allTypes.get(i).pluses()==2) {
                cmarkOffs++;
            }
        }
        // checking if E is good
        if(cmarkOffs < 3) {
            toCheckFor.add("E");
        }


        // e
        int emarkOffs = 0;
        for(int i=(11*(5-1)); i<11+(11*(5-1)); i++) { // marking off es
            if(allTypes.get(i).pluses()==2) {
                emarkOffs++;
            }
        }
        // checking if E is good
        if(emarkOffs < 3) {
            toCheckFor.add("e");
        }


        // MNSs

        // M
        int MmarkOffs = 0;
        for(int i=(11*(6-1)); i<11+(11*(6-1)); i++) { // marking off Ms
            if(allTypes.get(i).pluses()==2) {
                MmarkOffs++;
            }
        }
        // checking if M is good
        if(MmarkOffs < 3) {
            toCheckFor.add("M");
        }

        // N
        int NmarkOffs = 0;
        for(int i=(11*(7-1)); i<11+(11*(7-1)); i++) { // marking off Ns
            if(allTypes.get(i).pluses()==2) {
                NmarkOffs++;
            }
        }
        // checking if N is good
        if(NmarkOffs < 3) {
            toCheckFor.add("N");
        }

        // S
        int SmarkOffs = 0;
        for(int i=(11*(8-1)); i<11+(11*(8-1)); i++) { // marking off Ss
            if(allTypes.get(i).pluses()==2) {
                SmarkOffs++;
            }
        }
        // checking if S is good
        if(SmarkOffs < 3) {
            toCheckFor.add("S");
        }

        // s
        int smarkOffs = 0;
        for(int i=(11*(9-1)); i<11+(11*(9-1)); i++) { // marking off ss
            if(allTypes.get(i).pluses()==2) {
                smarkOffs++;
            }
        }
        // checking if s is good
        if(smarkOffs < 3) {
            toCheckFor.add("s");
        }

        // P

        // P1
        int P1markOffs = 0;
        for(int i=(11*(10-1)); i<11+(11*(10-1)); i++) { // marking off P1s
            if(allTypes.get(i).pluses()==2) {
                P1markOffs++;
            }
        }
        // checking if P1 is good
        if(P1markOffs < 3) {
            toCheckFor.add("P1");
        }

        // Lewis

        // Lea
        int LeamarkOffs = 0;
        for(int i=(11*(11-1)); i<11+(11*(11-1)); i++) { // marking off Leas
            if(allTypes.get(i).pluses()==2) {
                LeamarkOffs++;
            }
        }
        // checking if Lea is good
        if(LeamarkOffs < 3) {
            toCheckFor.add("Lea");
        }

        // Leb
        int LebmarkOffs = 0;
        for(int i=(11*(12-1)); i<11+(11*(12-1)); i++) { // marking off Lebs
            if(allTypes.get(i).pluses()==2) {
                LebmarkOffs++;
            }
        }
        // checking if Leb is good
        if(LebmarkOffs < 3) {
            toCheckFor.add("Leb");
        }   

        // Kell

        // K
        int KmarkOffs = 0;
        for(int i=(11*(13-1)); i<11+(11*(13-1)); i++) { // marking off Ks
            if(allTypes.get(i).pluses()==2) {
                KmarkOffs++;
            }
        }
        // checking if K is good
        if(KmarkOffs < 3) {
            toCheckFor.add("K");
        }

        // k
        int kmarkOffs = 0;
        for(int i=(11*(14-1)); i<11+(11*(14-1)); i++) { // marking off ks
            if(allTypes.get(i).pluses()==2) {
                kmarkOffs++;
            }
        }
        // checking if k is good
        if(kmarkOffs < 3) {
            toCheckFor.add("k");
        }

        // Duffy

        // Fya
        int FyamarkOffs = 0;
        for(int i=(11*(15-1)); i<11+(11*(15-1)); i++) { // marking off Fyas
            if(allTypes.get(i).pluses()==2) {
                FyamarkOffs++;
            }
        }
        // checking if Fya is good
        if(FyamarkOffs < 3) {
            toCheckFor.add("Fya");
        }

        // Fyb
        int FybmarkOffs = 0;
        for(int i=(11*(16-1)); i<11+(11*(16-1)); i++) { // marking off Fybs
            if(allTypes.get(i).pluses()==2) {
                FybmarkOffs++;
            }
        }
        // checking if Fyb is good
        if(FybmarkOffs < 3) {
            toCheckFor.add("Fyb");
        }

        // Kidd

        // Jka
        int JkamarkOffs = 0;
        for(int i=(11*(17-1)); i<11+(11*(17-1)); i++) { // marking off Jkas
            if(allTypes.get(i).pluses()==2) {
                JkamarkOffs++;
            }
        }
        // checking if Jka is good
        if(JkamarkOffs < 3) {
            toCheckFor.add("Jka");
        }

        // Jkb
        int JkbmarkOffs = 0;
        for(int i=(11*(18-1)); i<11+(11*(18-1)); i++) { // marking off Jkbs
            if(allTypes.get(i).pluses()==2) {
                JkbmarkOffs++;
            }
        }
        // checking if Jkb is good
        if(JkbmarkOffs < 3) {
            toCheckFor.add("Jkb");
        }

        System.out.println(toCheckFor);
    }

}

All 486 lines of it. Anything here that can be condensed, I would like condensed just to cut off some of those lines.

public class Type {

    String name;

    private int cellNum;

    private int pluses;

    public Type(String name, int cellNum, int pluses) {
        this.name = name;
        this.cellNum = cellNum;
        this.pluses = pluses;
    }

    public String getString() {
        return name;
    }

    public int cellNumber() {
        return cellNum;
    }

    public int pluses() {
        return pluses;
    }

}

And what is returned to the console:

What lines are the D's +s on?
P1-11 has 2
S8 has 0
e3 has 2
Lea6 has 2
Fyb6 has 1
c1 has 1
Lea2 has 0
P1-9 has 2
Fyb2 has 1
Fya7 has 2

[C, E, N, S, K, Jkb]
\$\endgroup\$
1
  • \$\begingroup\$ It would help if you explain the problem you are trying to solve in text? What does the input data look like (example?) \$\endgroup\$ Feb 14 '18 at 10:49
1
\$\begingroup\$

Few things where you can start, and after that, it should be easier to make next improvement. There is a lot of the same looking code, like:

for(int i=0; i<11; i++) { // Cs
        int amount = 0;
        if(i+1==2 || i+1==3) {
            amount = 2;
        } else if(i+1==1||i+1==11) {
            amount = 1;
        }
        Type C = new Type("C" + (i+1), i+1, amount);
        allTypes.add(C);
    }

why not to change it into:

for(int i=1; i<=11; i++) { // Cs
        int amount = 0;
        if(i==2 || i==3) {
            allTypes.add(new Type("C" + (i), i, 2));
        } else if(i==1||i==11) {
            allTypes.add(new Type("C" + (i), i, 1));
        }
    }

Also, you are always using i+1 why not start counting from 1 and you can delete a lot of +1.

If you changed you for's as I did it, you can connect them E.g

for(int i=1; i<=11; i++) { // Cs
        int amount = 0;
        if(i==2 || i==3) {
            allTypes.add(new Type("C" + (i), i, 2));
        } 
        if(i==4) {
            allTypes.add(new Type("E" + (i), i, 2));
        }
        if(i==5) {
            allTypes.add(new Type("E" + (i), i, 1));
        }
        if(i==1||i==11) {
            allTypes.add(new Type("C" + (i), i, 1));
        }
    }

Do you get the idea? I delete else statement, single ifs will do their job.

Next step you can create a separate method to adding New Type where parameters to pass will be enzyme name and amount of it.

If it is still not enough you can clean your ladder of ifs (similar construction to switch statement) using proper pattern design like Command pattern.

\$\endgroup\$
3
  • \$\begingroup\$ there is a performance penalty when deleting the else clause \$\endgroup\$ Feb 12 '18 at 8:16
  • \$\begingroup\$ True Sharon. And I too would have written them as } else if( ) { instead. But I doubt the couple of boolean checks really outweigh any object creation in terms of performance. \$\endgroup\$
    – Imus
    Feb 12 '18 at 10:37
  • \$\begingroup\$ Sharon thanks for answer :) sure it increase performance cost but penalthy is too small to think about it. Readability of code is more important thing than performance. Of course there is a line you should never break. Thanks to deleting those else we can go step further and implement proper patter design. And else clause is increasing cyclomatic complexity. I am not saying using else is always wrong aproach, but in this case we can erase them. What do you think about it? More about this aproach you can find in Jeff Bay's chapter "object Calisthenics" from book The ThoughtWorks Anthology. \$\endgroup\$
    – Rafalsky
    Feb 12 '18 at 11:46
1
\$\begingroup\$

uhm i'm not sure why don't you use methods to organise your code?

you are always iteration over these eleven lines so why don't you use a method and remove duplicated code?

public void addType(Predicate<Integer> p1, Predicate<Integer> p2, String typeName )
for(int i=1; i<=11; i++) { // thanks @ Rafalsky
    int amount = 0;
    if(p1.test(i)) {
        amount = 2;
    } else if(p2.test(i)) {
        amount = 1;
    }
    allTypes.add(new Type(typeName, i, amount)); //don't add the cellNum into the name - better write a derivant toString-method
}

this would make your code waaaaaay easier to read

//instead of this block:
/*
for(int i=0; i<11; i++) { // Cs
    int amount = 0;
    if(i+1==2 || i+1==3) {
        amount = 2;
    } else if(i+1==1||i+1==11) {
        amount = 1;
    }
    Type C = new Type("C" + (i+1), i+1, amount);
    allTypes.add(C);
}
*/
//simply write THIS ONE-LINE code:
addType(e->(e==2||e==3), e->(e==1||e==11), "C"); //Cs

//and all the other blocks
addType(e->(e==4), e->(e==5), "E"); //Es
addType(e->(e>=4&&e<=10), e->(e==1||e==11), "c"); //cs
addType(e->(e<=3||e>=6), e->(e==5), "e"); //es
addType(e->(e<=3||e==6||e==9), e->(e==5||e==4||e==5|e==7||e==8||e==10), "M"); //MNSs
...

another issue would handle the 'filtering' of the types...

int EmarkOffs = 0;
for(int i=(11*(3-1)); i<11+(11*(3-1)); i++) { // marking off Es
    if(allTypes.get(i).pluses()==2) {
        EmarkOffs++;
    }
}

it could be simplified if you just really filter the types...

int EmarkOffs = allTypes.stream().filter(type -> type.getname().equals("E") && type.pluses()==2).count();

again it's a simple ONE-LINER that does all the work, thanks java8

you could also think of using better variable names or better getter/setter names. Java convention is Type.getPluses() (not Type.pluses()) and it would eMarkOffs (not EmarkOffs)

NOTE: this code has not been tested/compiled - it may contain syntax errors.

\$\endgroup\$
0
0
\$\begingroup\$

The majority of the code deals with creating (many) instances of Type with the correct data. As you observed, this creates code that is is both long, obscure and error prone. Data is best represented as text in a in text file and then all that is needed is to load the file into memory and parse the text into the list of instances.

You do not need to invent the wheel: There are several established text formats that can represent the data, and there are libraries that will do the reading, parsing and even create the instances for you. In software engineering, this is called Object deserialization .

The two most popular text formats today are XML and JSON. JSON is more compact and supports data types other than text. So we can design the JSON representation of a Type object like this:

{
  "name": "D1",
  "cellNum": 1,
  "pluses": 0
}

This representation is both clear, concise and is portable (can be passed to another program, web service, stored in DB etc) So welcome to the wonderful world of Object serialization/deserialization. There are many libraries and documentation/tutorials on the subject. I will point you to Jackson Json parser that is considered one of the most popular.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.