4
\$\begingroup\$

I'm currently making a social media app that contains profile details such as interests in a text file. The following code works by using markers in the text file to upload these details to map data structures. I'm wondering if there is a more efficient way of doing this.

textList = ReadFile(FileName);
System.out.println("File Length" + textList.size());
for (int i = 0; i < textList.size(); i++){
    String[] split = splitTxt(textList.get(i), "\\s+");
    List<String> friends = new ArrayList<>();
    List<String> interests = new ArrayList<>();
    if (split.length >= 2){
        String[] interestArray = splitTxt(split[2], ";");
        for(int j = 0; j < interestArray.length; j++){
            interests.add(interestArray[j]);
        }
        Accounts.put(split[0],split[1]);
        intrestDic.put(split[0],interests);
        for(int j= 3; j < split.length; j++){
            friends.add(split[j]);

        }
        FriendsDic.put(split[0],friends);
        System.out.println(FriendsDic.get(split[0]));
    }

}

I've created a split function that looks for marker to split it on

public static String[] splitTxt(String text, String type){
    String[] parsedText = text.split(type);
    return parsedText;
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. To make this a good question, you should include a small excerpt from the text file. \$\endgroup\$ – 200_success Apr 29 '17 at 5:40
4
\$\begingroup\$

Range-based for loop

for (int i = 0; i < textList.size(); i++){
    String[] split = splitTxt(textList.get(i), "\\s+");

You can simplify this with a range-based for loop.

for (String text : textList) {
    String[] split = text.split("\\s+");

No need to manage i manually.

I also eliminated the call to splitTxt, which makes the code smaller and easier to read.

Bug

    List<String> friends = new ArrayList<>();
    List<String> interests = new ArrayList<>();
    if (split.length >= 2){
        String[] interestArray = splitTxt(split[2], ";");

This will cause an array index out of bounds when the split.length is 2.

    if (split.length < 3) {
        continue;
    }

    String[] interestArray = splitTxt(split[2], ";");
    List<String> friends = new ArrayList<>();
    List<String> interests = new ArrayList<>();

When split is of length 2, only 0 and 1 are valid indexes. So split[2] would be out of bounds. Making it a 3 instead of a 2 would fix that.

Switching to an early exit changes from >= to < and allows us to reduce the level of indent. It works here because the entire code for an iteration after that point is inside the if.

There is no point to declaring and initializing friends and interests if we're not going to use them. So do those tasks after the check, not prior.

asList

        String[] interestArray = splitTxt(split[2], ";");
        for(int j = 0; j < interestArray.length; j++){
            interests.add(interestArray[j]);
        }
        Accounts.put(split[0],split[1]);
        intrestDic.put(split[0],interests);

You don't need to manually copy one by one.

        Accounts.put(split[0],split[1]);
        intrestDic.put(split[0], new ArrayList<String>(Arrays.asList(split[2].split(";"))));

Using a copy constructor with Arrays.asList will allow the whole thing to be copied at once.

Summary

private static final int HEADER_SIZE = 3;

and later

for (String text : ReadFile(FileName)) {
    String[] split = text.split("\\s+");
    if (split.length < HEADER_SIZE) {
        continue;
    }

    Accounts.put(split[0], split[1]);
    intrestDic.put(split[0], new ArrayList<String>(Arrays.asList(split[2].split(";"))));
    List<String> friends = Arrays.asList(split).subList(HEADER_SIZE, split.length));
    FriendsDic.put(split[0], new ArrayList<String>(friends);
}

This isn't necessarily more efficient, although it could be (depending on how Java optimizes it). The primary advantage of this version is that it leaves maintenance to the Java engine.

Hibernate

Hibernate (and Java Persistence Annotations in general) is for exactly this task. It maps classes to storage and vice versa. Now, that might be a heavyweight solution, but it is one that exactly addresses the problem.

You can write persistence code manually, without annotations. But I would generally still use a database if you intend the code to scale at all. You don't want to read all the records every time you want one record. Rather than reinventing that wheel, just use the existing solution: a database.

Hibernate makes it comparatively easy to switch from one database to another.

\$\endgroup\$
3
\$\begingroup\$

"A note on defining methods."

This method has no added value whatsoever. All it does is calling another method with same method signature (with the first arg shifted as invokee). This could have been a good choice if either (1) you needed some validation (2) argument/result type parsing was involved (3) calling this method would have been a lot less code to type. But none of these are true. Try avoiding an anti-pattern like this.

public static String[] splitTxt(String text, String type){
    String[] parsedText = text.split(type);
    return parsedText;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ From where I stand this method is splittier than the original one :-P because this one can split text and the other one just splits. \$\endgroup\$ – t3chb0t Aug 31 at 6:26

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.