I have a resource file that I load, when needed, to check if a certain word is among the ones in the file.

I want to get rid of the method-wide synchronized keyword. There's no point in all threads waiting while wordsInFile.contains(word.toLowerCase()); runs.

Original:

private Set<String> wordsInFile = new HashSet<String>;
private static synchronized boolean IsFound(String word)
{
  if (word == null)
  {
    return false;
  }

  if (wordsInFile.isEmpty())
  {
    wordsInFile = new HashSet<String>();

    InputStream inputStream = WebUtils.class.getClassLoader().getResourceAsStream("words.txt");
    InputStreamReader inputStreamReader = new InputStreamReader(inputStream);

    try (BufferedReader bufferedReader = new BufferedReader(inputStreamReader))
    {
      String line = bufferedReader.readLine();
      while (line != null)
      {
        wordsInFile.add(line);
        line = bufferedReader.readLine(); // they're all lowercase
      }
    }
    catch (Exception aEx)
    {
      ;
    }
  }

  return wordsInFile.contains(word.toLowerCase());
}

Is this OK?

private Set<String> wordsInFile;
private static boolean IsFound(String word)
{
  if (word == null)
  {
    return false;
  }

  synchronized (wordsInFile)
  {
    if (wordsInFile.isEmpty())
    {
      wordsInFile = new HashSet<String>();
    
      InputStream inputStream = WebUtils.class.getClassLoader().getResourceAsStream("words.txt");
      InputStreamReader inputStreamReader = new InputStreamReader(inputStream);
    
      try (BufferedReader bufferedReader = new BufferedReader(inputStreamReader))
      {
        String line = bufferedReader.readLine();
        while (line != null)
        {
          wordsInFile.add(line);
          line = bufferedReader.readLine(); // they're all lowercase
        }
      }
      catch (Exception aEx)
      {
        ;
      }
    }
  }

  return wordsInFile.contains(word.toLowerCase());
}

CodePudding user response：

I would solve this with an inner "holder" class, where the Set is immutable once loaded. Do the loading in the constructor of that class, and create the class lazily, synchronizing on the holder.

Immutable objects don't need to be synchronized across threads.

import org.apache.commons.io.IOUtils;

import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;

public class WordFinder {

    private static class DictionaryHolder {
        private final Set<String> words;
        private DictionaryHolder() {
            System.out.println( "Loading words..." );
            Set<String> loaded = new HashSet<>();
            try( InputStream in = WordFinder.class.getResourceAsStream( "words.txt" ) ) {
                loaded.addAll( IOUtils.readLines( in, StandardCharsets.UTF_8 ) );
            } catch( IOException ioe ) {
                throw new RuntimeException( ioe );
            }
            this.words = Collections.unmodifiableSet(loaded);
            System.out.println( "Words loaded..." );
        }
    }

    private static DictionaryHolder dictionary = null;

    private static void justDoSomething() {
        System.out.println( "Doing something..." );
    }

    private static boolean IsFound(String word) {
        synchronized( DictionaryHolder.class ) {
            if(dictionary == null) {
                dictionary = new DictionaryHolder();
            }
        }
        System.out.println( "Checking word "   word   "..." );
        return dictionary.words.contains( word );
    }

    public static void main(String[] args) {
        WordFinder.justDoSomething();
        WordFinder.IsFound( "foo" );
        WordFinder.IsFound( "bar" );
    }
}

CodePudding user response：

This is a variant of @Stewart answer. You can handle lazy load without synchronize by making use of class loader for DictionaryHolder. This works because JVM won't execute the static initialiser of a class multiple times if two threads called methods of that class at same time:

public class WordFinder {
    private static class DictionaryHolder {
        private static final Set<String> words;
        static {
            System.out.println( "Loading words..." );
            words = Set.of("hello","world");
            //words = yourLoadCode();
            System.out.println( "Words loaded..." );
        }

        public static Set<String> dictionary() {
            return words;
        }
    }

    private static void justDoSomething() {
        System.out.println( "Doing something..." );
    }

    private static boolean IsFound(String word) {
        System.out.println( "Checking word "   word   "..." );
        return DictionaryHolder.dictionary().contains( word );
    }

    public static void main(String[] args) {
        WordFinder.justDoSomething();
        WordFinder.IsFound( "foo" );
        WordFinder.IsFound( "hello" );
    }
}

Note that the lazy loading class DictionaryHolder does not have to be an inner class, it can be declared in any package outside. The above prints lines below confirming the lazy load on first call to IsFound("foo"):

Doing something...
Checking word foo...
Loading words...
Words loaded...
Checking word hello...

It's never a good idea to suppress exceptions. If you think throws IOException is inappropriate, catch and re-throw as new RuntimeException(previousException) so that callers are alerted to issues.