0
\$\begingroup\$

I just want to get some advice about code optimization. How can I make this more OOP? How many class should I have for this kind of task? My code already works but I was told to use OOP in a way that it would make sense using it.

import java.util.Scanner;
import java.util.regex.Matcher; 
import java.util.regex.Pattern; 
import java.io.File;
import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.FileReader;
import java.io.FileWriter;
import java.util.LinkedHashMap; 
import java.util.List;
import java.util.ArrayList;
import java.util.Map;
import java.util.Iterator;
import java.util.List;

public class JavaKeyValue {

    private static Scanner sc;
    private static Matrix matrix;
    private static JavaKeyValue jvKV;
    private String pathToFile = "/Desktop/";
    private String fileName = "javatext.txt";
    private static LinkedHashMap<String, String> lHM;

    public static void main(String[]args) {
        sc=  new Scanner(System.in);
        jvKV = new JavaKeyValue();
        lHM = new LinkedHashMap<String,String>();
        String action;

        jvKV.createNewTable("Start");

        System.out.println("Matrix has been created");

        do {
            System.out.println("Enter PRINT for Print");
            System.out.println("Enter EDIT for Edit");
            System.out.println("Enter SEARCH for Search");
            System.out.println("Enter RESET for Reset");
            System.out.println("Enter ADD for Add Row");
            System.out.println("Enter SORT for Sort");
            System.out.println("Enter EXIT for Exit");

            action=sc.next();
            sc.nextLine();
            jvKV.doAction(action);
        } while(!action.equalsIgnoreCase("Exit"));
    }

    public void createNewTable(String status) {
        int inputCounter = 0, row = 0, col = 0;
        String kvTextPairs,table, action;

        while(inputCounter <= 3) {
            try{
                System.out.println("Enter the number of rows:");
                row=sc.nextInt();
                System.out.println("Enter the number of columns:");
                col=sc.nextInt();
                break;
            }catch(Exception e) {
                sc.nextLine();
                inputCounter++;
                System.out.println("Please input a number");
                System.out.println("Attempt #"+inputCounter);
                if (inputCounter == 3) {
                    System.out.println("\n Program Terminating");
                    System.exit(0);
                }
            }
        }

        kvTextPairs = jvKV.getKVTextPairs(status, row, col);

        matrix = new Matrix(lHM, kvTextPairs, row, col);

        lHM = matrix.kvTextPairTolHM(lHM);
        table = matrix.createTable(lHM);
    }

    public String getKVTextPairs(String status, int row, int col) {
        String textFile = "";
        String st;
        if(status.equalsIgnoreCase("Start")) {
            try {
                File file = new File(pathToFile+fileName);
                BufferedReader br = new BufferedReader(new FileReader(file));

                while((st = br.readLine()) != null){
                    textFile += st;
                }
            } catch(Exception e) {
                textFile = jvKV.keyValueGenerator(row,col);

                System.out.println("Text File cannot  be found, Creating new txt file named "+fileName+" with random generated Key,Value");

                jvKV.createFile(textFile);
            }
        } else {
            System.out.println("Table has been resetted!");
            textFile = jvKV.keyValueGenerator(row,col);
            jvKV.createFile(textFile);      
        }
    return textFile;
    }

    public void createFile(String textFile) {
        try {
            File newFile = new File(pathToFile+fileName);
            if (!newFile.exists()) {
                newFile.createNewFile();
            }

            FileWriter fw = new FileWriter(newFile.getAbsoluteFile());
            BufferedWriter bw = new BufferedWriter(fw);

            bw.write(textFile);
            bw.close();
        } catch(Exception e) {
            System.out.println(e.getMessage());
        }
    }

    public String keyValueGenerator(int row, int col) {
        char charVal;
        String textFile = "";
        int numbersofAscii = 93, value;
        for(int i=0; i<row; i++) {
            for(int a=0; a<col; a++) {
                String text = "";
                textFile += "( ";

                for(int b=0; b<3; b++) {
                    value = 32+(int)(Math.random()*numbersofAscii);
                    charVal = (char)value;
                    textFile += charVal;
                }
                textFile+=",";

                for(int c=0; c<3; c++) {
                    value = 32+(int)(Math.random()*numbersofAscii);
                    charVal = (char)value;
                    textFile += charVal;
                }
                textFile += " )";
            }
            textFile += "\n";
        }
        return textFile;
    }

    public void doAction(String action) {
        switch(action.toUpperCase()){
            case "PRINT":
                System.out.println(matrix.getTable());
                break;

            case "EDIT":
                System.out.println("Do you want to edit Key? Value? Both?");
                String editChoice=sc.next();
                sc.nextLine();

                String curKey, newKey, newValue, exitKey="EXIT";

                switch(editChoice.toUpperCase()) {
                    case "KEY":

                        System.out.println("Edit new key, Enter the KEY that you want to replace:");
                        curKey = sc.nextLine();

                        System.out.println("Enter the KEY you want as replacement:");
                        newKey = sc.nextLine();

                        lHM = new LinkedHashMap<String, String>(matrix.editKey(curKey, newKey, lHM));

                        break;

                    case "VALUE":
                        System.out.println("Edit new VALUE, Enter the KEY of the VALUE you want to replace:");
                        curKey = sc.nextLine();

                        System.out.println("Enter the VALUE you want as replacement:");
                        newValue = sc.nextLine();
                        newValue = newValue.substring(0,3);

                        lHM.put(curKey, newValue);

                        break;

                    case "BOTH":

                        System.out.println("Edit BOTH, Enter the KEY of the ITEM you want to replace");
                        curKey = sc.nextLine();

                        System.out.println("Enter the KEY you want as replacement:");
                        newKey = sc.nextLine();

                        System.out.println("Enter the VALUE  you want as replacement:");
                        newValue = sc.nextLine();

                        newValue = newValue.substring(0,3);

                        lHM.put(curKey, newValue);
                        lHM = new LinkedHashMap<String, String>(matrix.editKey(curKey, newKey, lHM));

                        break;
                }
                break;

            case "SEARCH":
                System.out.println("Enter the text that you want to search:");
                String searchText = sc.nextLine();

                matrix.searchTable(lHM, searchText);
                break;

            case "RESET":
                jvKV.createNewTable("Reset");
                break;

            case "ADD":
                int addRow;

                System.out.println("Enter the number of rows you want to add:");
                addRow=sc.nextInt();

                matrix.addRow(addRow, lHM);

                break;

            case "SORT":

                break;

            case "EXIT":

                break;      

        }

        matrix.createTable(lHM);
    }

}

Second Class

import java.util.Scanner;
import java.util.regex.Matcher; 
import java.util.regex.Pattern; 
import java.io.File;
import java.io.BufferedReader;
import java.io.FileReader;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.ArrayList;
import java.util.Map;
import java.util.Iterator;
import java.util.List;

public class Matrix{

    private Matrix matrix;
    private JavaKeyValue jvKV;
    private LinkedHashMap lHM;
    private String kvTextPair;
    private int row,col;
    private String table;

    public Matrix(){
        matrix = this;
        jvKV = new JavaKeyValue();
    }

    public Matrix(LinkedHashMap lHM, String kvTextPair, int row, int col) {
        this();
        this.lHM = lHM;
        this.kvTextPair = kvTextPair;
        this.row = row;
        this.col = col;
    }

    public LinkedHashMap kvTextPairTolHM(LinkedHashMap lHM) {
        lHM = new LinkedHashMap<String, String>();
        Pattern pattern = Pattern.compile("\\( ((.{3})\\,(.{3})) \\)");
        Matcher matcher = pattern.matcher(this.kvTextPair);
        String item, key, value;

        while(matcher.find()) {
            item=matcher.group(1);
            key=matcher.group(2);
            value=matcher.group(3);
            lHM.put(key, value);
        }
        return lHM;
    }

    public String createTable(LinkedHashMap lHM) {
        String table = "";
        int colCounter = 0, rowCounter = 0, row = this.row, col = this.col;
        Iterator<Map.Entry<String, String>> lHMIterator = lHM.entrySet().iterator();

        while(lHMIterator.hasNext()){
            Map.Entry<String, String> entry = lHMIterator.next();
            table += "{"+ entry.getKey()+ ","+ entry.getValue()+ "} ";
            colCounter++;
            if(colCounter == col) {
                table += "\n";
                colCounter = 0;
                rowCounter++;

                if(rowCounter == row) {
                    break;
                }
            }
        }
        this.table = table;
        return table;
    }


    public String getTable() {
        return this.table;
    }

    public LinkedHashMap editKey(String oldKey, String newKey, LinkedHashMap lHM) {
        LinkedHashMap<String, String> newlHM = new LinkedHashMap<String, String>();
        Iterator<Map.Entry<String, String>> lHMIterator = lHM.entrySet().iterator();

        if(!lHM.containsKey(oldKey)) {
            System.out.println("This key doesn not exist");
            return lHM;
        }

        while(lHMIterator.hasNext()) {
            Map.Entry<String, String> entry = lHMIterator.next();
            String key = entry.getKey();
            String value = entry.getValue();
            if(key.equals(oldKey)) {
                key = newKey;
                if(key.length()>3){
                    key=key.substring(0,3); 
                }
            }
            newlHM.put(key, value);
        }
        return newlHM;
    }

    public void searchTable(LinkedHashMap lHM, String searchText) {
        String result = "";
        int col = this.col, row = this.row;
        List<ArrayList<String>> list = new ArrayList<>();
        list.add(new ArrayList<>());
        int rowCounter = 0, colCounter = 0;
        Iterator<Map.Entry<String,String>> lHMIterator =    lHM.entrySet().iterator();
        while(lHMIterator.hasNext()) {
            Map.Entry<String, String> entry=lHMIterator.next();
            String key = entry.getKey();
            String value = entry.getValue();
            String keyval = key+","+value;
            result += this.searchOccur(searchText, key, "KEY", rowCounter, colCounter);
            result += this.searchOccur(searchText, value, "VALUE", rowCounter, colCounter);
            list.get(rowCounter).add(keyval);
            colCounter++;
            if(colCounter == col) {
                list.add(new ArrayList<>());
                colCounter=0;
                rowCounter++;
            }
        }
        System.out.println(result);
    }

    public String searchOccur(String searchText,String text, String keyval, int rowCounter, int colCounter) {
        String res = "", comparison;
        int occurences=0;

        for(int i=0; i<=text.length()-searchText.length(); i++) {
            comparison="";
            for(int a=0; a<searchText.length(); a++) {
                comparison += text.charAt(i+a);
            }
            if(comparison.equals(searchText)) {
                occurences += 1;
            }
        }
        if(occurences != 0) {
            res += "Occurences for "+keyval+" at ("+rowCounter+","+colCounter+") is "+occurences+"\n";
        }
        return res;
    } 

    public void addRow(int row, LinkedHashMap lHM) {
        this.row += row;
        char charVal;
        int numbersofAscii=93, value;

        for(int i=0; i<row; i++) {
            for(int a=0; a<this.col; a++) {
                String text = "", key = "", val = "";

                for(int b=0; b<3; b++) {
                    value = 32+(int)(Math.random()*numbersofAscii);
                    charVal = (char)value;
                    key += charVal;
                }
                for(int c=0; c<3; c++) {
                    value = 32+(int)(Math.random()*numbersofAscii);
                    charVal = (char)value;
                    val += charVal;
                }
                lHM.put(key, val);
            }
        }
    }
}
New contributor
leks123 is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$

put on hold as unclear what you're asking by Toby Speight, VisualMelon, Mast, pacmaninbw, RubberDuck 20 hours ago

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 3
    \$\begingroup\$ it would greatly help if you elaborate (beyond the title) about the function of the classes. maybe add sample input and output. reading a not-that-short source code with one line description is needlessly difficult \$\endgroup\$ – Sharon Ben Asher Aug 13 at 13:53
  • \$\begingroup\$ basically what the first class does it that it looks for a textfile if it exist get the key-value pairs that satisfies the pattern. If not it makes a file creates random key-value pairs. It also ask for the userinput about the matrix' row and column, and what functions it should do (print, prints out a matrix with k-v pair, edit ask to edit key,value,both, search search for a set of chars inside each k-v, reset just recreate a new table)q \$\endgroup\$ – leks123 Aug 13 at 14:37
  • \$\begingroup\$ now second class has the functions that relates to the table, like creating it, searching through it, displaying it. Now the code is working perfectly fine and as intended. I'm just wondering does it really look like "OOP" enough? We were told to use OOP Concepts in a case that it would make sense. I just thought that maybe I got it too simple? or something. I dont know. \$\endgroup\$ – leks123 Aug 13 at 14:40
  • 4
    \$\begingroup\$ @leks123 please add example input and output to the question. \$\endgroup\$ – Roland Illig Aug 15 at 7:11
  • 2
    \$\begingroup\$ @leks123, please edit your question to add the explanation (comments are transient, so the information needs to be part of the question). \$\endgroup\$ – Toby Speight 2 days ago
2
\$\begingroup\$

This will be a rather general review since there is a decent chunk of code, and a lot of it contains rather non-helpful variable names. I'm also not a Java expert so I won't be able to suggest things such as built-in features that already solve your problem as I don't know very many of them.


Variable Naming

The variables do not have names that are helpful. Short names like sc are ok if they are localized to one part of the code where the type and use give enough context to know what they are. But jvKV, lHM, and st are rather short and are scattered everywhere.

I think the issue stems from the problem that the names seem to be based on the implementation rather than on an idea. Does the name jvKV need to include the detail that it is a Java KeyValue? Why not just a KeyValue? If you decided that a linked hashmap was no longer the best choice of data structure, but that you instead need to use a tree hashmap, you are left with two not so great choices

  1. Rename lHM to tHM and hope you don't miss any references to it. Did you remember to update the docs? And the comments? And the debugging print statements?
  2. Keep the now out of date name.

Neither option is desirable so try and avoid them by leaving implementation details out of the name. This can be very hard in practice.


main

I'm not confident in this advice, so take it with a grain of salt. The first class looks like this

public class JavaKeyValue {
    ...
    private static JavaKeyValue jvKV;
    ...
    public static void main(String[]args) {
    ...
        jvKV.doAction(action);
    ...
    public void doAction(String action) {
    ...

This smells a bit funny to me, mixing static things like main with non-static instance calls like doAction. Should they be in the same class? I would reach for a driver class that essentially manages the main method, and leave JavaKeyValue alone to do the business logic.

public class Driver {
    public static void main(String[] args) {
        JavaKeyValue xxx = new...
        ...
        String action = xxx.readUserAction();
        xxx.doAction(action);
    }
}

User Input

The Scanner sc is hard at work running around the place. I think you should give it a break by separating reading input from creating a table. The new signature could be something like

public void createNewTable(String status, int rows, int cols)

and reading the number of rows and columns is done in a method to itself.


for(int b=0; b<3; b++) {
    value = 32+(int)(Math.random()*numbersofAscii);
    charVal = (char)value;
    textFile += charVal;
}
textFile+=",";
for(int c=0; c<3; c++) {
    value = 32+(int)(Math.random()*numbersofAscii);
    charVal = (char)value;
    textFile += charVal;
}

This is repeated code, and a good candidate for a method

public String threeRandomCharacters() {
    // Generate 3 ASCII characters from code the code range [32, 125]
    // SIDENOTE: Why is this limited to 125 instead of 127?
    String result = "";

    int numberOfPrintableASCIICharacters = 93;
    for(int i=0; i<3; i++) {
        value = 32 + (int)(Math.random()*numberOfPrintableASCIICharacters);
        result += (char)value;
    }
    return result;
}

When isolated to a method, it looks like there might be a couple of problems with the code. Does it uniformly generate ASCII characters? Is the loop necessary with only 3 iterations? Luckily this is far easier to test now that it is a function by itself.


doAction(String action) {
    switch(action.toUpperCase()){

What happens if the user makes a mistake when inputting the action. PRITN will just fall through the cracks and might do something unexpected.


newValue = sc.nextLine();
newValue = newValue.substring(0,3);

This sort of truncation of the user input is rather unexpected. Is it necessary? If so, is there some way of telling the user, like repeating back the input you received, or including the truncation as a message before entering the value.


public LinkedHashMap kvTextPairTolHM(LinkedHashMap lHM) {
    lHM = new LinkedHashMap<String, String>();
    Pattern pattern = Pattern.compile("\\( ((.{3})\\,(.{3})) \\)");
    ...

This seems very overkill for such a simple pattern. The regex pattern is exaggerated due to matching special symbols, but still. Here is some example psuedocode that hopefully captures all the assumptions made in the regex implementation.

Map keyValueTextToMap() {
    String text = this.text;
    // The text pair is of the form "( XXX,XXX )"
    // where X is an ASCII character in the range [32, 125]
    if text.length() != 11 ERROR
    if text.substring(0, 3) != "( " ERROR
    if text.substring(9)    != " )" ERROR
    if text.charAt(5)       !=  '," ERROR
    String item, key, value = text.substring(...
    Map map = new Map<>()
    map.put(key, value)
    return map
}

LinkedHashMap editKey(String oldKey, String newKey, LinkedHashMap lHM)

I think the return value here should be a result; good if the key edit was successful and bad if it fails for any reason. Returning the map doesn't make much sense as you can't do anything with the returned map that you couldn't have done with the original. Bad might also indicate why the edit failed which you may want to log, but the editKey method should not be printing to stdout.


for(int i=0; i<=text.length()-searchText.length(); i++) {
    comparison="";
    for(int a=0; a<searchText.length(); a++) {
        comparison += text.charAt(i+a);
    }
    if(comparison.equals(searchText)) {
        occurences += 1;
    }
}

This looks like it could be a lot nicer to read if you use some built-in methods. Unfortunately I don't know which ones. Maybe replacing the loop building comparasion with text.substring(i, i + searchText.length()) will work?

\$\endgroup\$
1
\$\begingroup\$

Import

In both classes you import dependencies that are unused.

In JavaKeyValue you are importing

import java.util.Scanner;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.io.File;
import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.FileReader;
import java.io.FileWriter;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.ArrayList;
import java.util.Map;
import java.util.Iterator;
import java.util.List;

but using only

java.io.File;
java.io.BufferedReader;
java.io.BufferedWriter;
java.io.FileReader;
java.io.FileWriter;
java.util.Scanner;
java.util.LinkedHashMap;

The benefits of importing only what you need are:

  • avoid namespace collisions
  • better readability, because you know the dependencies at a glance
  • faster compilation

Variables

One Declaration per Line

From Oracle's Code Conventions:

6.1 Number Per Line
One declaration per line is recommended since it encourages commenting

In general, the fewer things that happen on a line, the better.

int inputCounter = 0, row = 0, col = 0;
String kvTextPairs,table, action;
String curKey, newKey, newValue, exitKey="EXIT";

Useless Variables

public void createNewTable(String status) {
   int inputCounter = 0, row = 0, col = 0;
   String kvTextPairs,table, action;

   /* ... */

   table = matrix.createTable(lHM);
}

The variables table and action are useless.

The return value of matrix.createTable(lHM) gets assigned to table but because table is a local variable of createNewTable the statement has no effect and action gets declared but is never used.

An other unused variable is exitKey in doActions(String).

String curKey, newKey, newValue, exitKey="EXIT";

Method Naming

textFile = jvKV.keyValueGenerator(row,col);

I wouldn't expect that a keyValueGenerator returns a textFile..

To your Questions

How can I make this more OOP?

Detect objects that are in you code base. For example inside doAction(String):

public void doAction(String action) {
   switch(action.toUpperCase()){
       case "PRINT":
           System.out.println(matrix.getTable());
           break;
       case "EDIT":
           /* ... */
       case "SEARCH":
           /* ... */
       case "RESET":
           /* ... */
       case "SORT":
           /* ... */
       case "EXIT":
           /* ... */
   }
}

Looks like you could have an abstraction Action with concreted types Print, Edit, ..., Exit.

After that you could create a factory class:

class ActionFactory {
    public Action createBy(String name) {
        switch(name.toUpperCase()){
            case "PRINT":
                return new PrintAction();
            case "EDIT":
                return new EditAction();
            case "SEARCH":
                /* ... */
            case "RESET":
                /* ... */
            case "SORT":
                /* ... */
            case "EXIT":
                /* ... */
        }
    }
}

Now let some polymorphism do the magic:

public static void main(String[]args) {
    /* ... */


    jvKV.createNewTable("Start");

    System.out.println("Matrix has been created");

    do {
        /* ... */

        String actionName = sc.next();

        Action action = actionFactory.buildBy(actionName);
        action.execute();

    } while(!action.equalsIgnoreCase("Exit"));
}

The method name createTable let assume that there should be a Table.

How many class should I have for this kind of task?

As many as you need.. there is no magic number

\$\endgroup\$

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