4
\$\begingroup\$

I'm using the following function to index ebook data with Lucene. I'm looking to improve the structure and organization of this function.

public string BuildIndex(int pageNum, int pageSize)
{
    var watch = new System.Diagnostics.Stopwatch();
    watch.Start();
    var version = Lucene.Net.Util.Version.LUCENE_30;

    Analyzer analyzer = new StandardAnalyzer(version);
    DirectoryInfo path = new DirectoryInfo("C:\\LuceneIndex");
    Lucene.Net.Store.Directory directory = new MMapDirectory(path);
    using (IndexWriter iwriter = new IndexWriter(directory, analyzer, IndexWriter.MaxFieldLength.UNLIMITED))
    {
        //Initialize the StringBuilder with 4 MB to accommodate our data
        StringBuilder bodyBuilder = new StringBuilder(4*1024*1024);
        var files = ReaderDAL.FetchBooklistSearchData(pageNum, pageSize);
        foreach (var file in files) {
            bodyBuilder.Clear();
            DirectoryInfo filePath = new DirectoryInfo(file.IndexFilePath);

            if (filePath.Exists)
            {
                foreach (var pageFile in filePath.EnumerateFiles())
                {
                    using (StreamReader reader = pageFile.OpenText()) {
                        String line = reader.ReadToEnd();
                        bodyBuilder.AppendLine(line);
                    }
                }
            }
            //file.IndexFilePath
            Document doc = new Document();
            doc.Add(new Field("id", file.BookId.ToString(), Field.Store.YES, Field.Index.NO));
            doc.Add(new Field("author", file.Author, Field.Store.YES, Field.Index.ANALYZED));
            doc.Add(new Field("annotation", file.Annotation, Field.Store.YES, Field.Index.ANALYZED));
            doc.Add(new Field("publisher", file.Publisher, Field.Store.YES, Field.Index.ANALYZED));
            doc.Add(new Field("title", file.Title, Field.Store.YES, Field.Index.ANALYZED));
            doc.Add(new Field("body", bodyBuilder.ToString(), Field.Store.YES, Field.Index.ANALYZED));
            iwriter.AddDocument(doc);
        }

        iwriter.Optimize();
        iwriter.Flush(true, true, true);
    }

    watch.Stop();

    return watch.Elapsed.TotalSeconds.ToString();
}
\$\endgroup\$
8
\$\begingroup\$

The function is called BuildIndex, but it could be seen as doing the following:

  • Measuring its execution time.
  • Creating an Analyzer.
  • Creating a Lucene.Net.Store.Directory.
  • Using the analyzer and the directory to create an IndexWriter.
  • Iterating files
  • Loading document body from physical file if it exists at specified location.
  • Creating a new Document for each file, adding 6 Fields.
  • Returning execution time.

The first thing that strikes me is that there seems to have a performance concern that shadows the code's intent: is the goal of the method to build an index, or to calculate how long it takes to build an index? The first thing I'd do is return void and remove the first thing in that list - focused code does one thing, it has only a single responsibility in mind.

The next 3 points are closely related and could easily fit inside a method whose job is to return a new IndexWriter:

private IndexWriter CreateWriter(Analyzer analyzer, string path)
{
    var directory = new DirectoryInfo(path);
    var mMapDirectory = new MMapDirectory(directory);

    return new IndexWriter(mMapDirectory, analyzer, IndexWriter.MaxFieldLength.UNLIMITED)
}

The reason this method is taking an Analyzer parameter is because it leaves you the liberty of calling it and passing in a StandardAnalyzer today, and a CustomAnalyzer tomorrow, without changing anything in the method, only the way the analyzer gets instantiated.

In your original code, the version, analyzer path and directory locals all only exist to create the IndexWriter. Extracting the instantiation of this writer into its own method further reduces noise and increases signal. Consider this:

public void BuildIndex(Analyzer analyzer, int pageNum, int pageSize)
{
    using(var writer = CreateWriter(analyzer, @"C:\LuceneIndex"))
    {
        //...
    }
}

Now BuildIndex doesn't care anymore about what Lucene version the analyzer is configured with; not anymore does it care about the actual type of that analyzer either.


The body of the using block declares a bodyBuilder variable that I would have simply called builder. In fact, the StreamReader, is part of implementation details of another method, just like the creation of a new document:

using(var writer = CreateWriter(analyzer, @"C:\LuceneIndex"))
{
    var files = ReaderDAL.FetchBooklistSearchData(pageNum, pageSize);
    foreach (var file in files)
    {
        //Initialize the StringBuilder with 4 MB to accommodate our data:
        var builder = new StringBuilder(4*1024*1024);

        var directory = new DirectoryInfo(file.IndexFilePath);
        if (directory.Exists)
        {
            ReadAllPageFilesInFolder(directory, builder);
        }

        writer.AddDocument(CreateDocument(file, builder.ToString()));
    }

    writer.Optimize();
    writer.Flush(true, true, true);
}

The advantage of a StreamReader is that it allows you to load a large file's content in small chunks.. but using ReadToEnd() is essentially the same as calling File.ReadAllText(), so if the files are easily digestible it would be more easily readable to go that way:

private void ReadAllPageFilesInFolder(DirectoryInfo directory, StringBuilder builder)
{
    foreach (var file in directory.EnumerateFiles())
    {
        // is this file really a PageFile?

        builder.AppendLine(File.ReadAllText(file.FullName));
    }        
}

Moving the document creation into its own method makes it easier to later change how a document gets created (new field(s), new settings, etc.):

private Document CreateDocument(WhateverThatTypeIs file, string body)
{
    var result = new Document();

    result.Add(new Field("id", file.BookId.ToString(), Field.Store.YES, Field.Index.NO));
    result.Add(new Field("author", file.Author, Field.Store.YES, Field.Index.ANALYZED));
    result.Add(new Field("annotation", file.Annotation, Field.Store.YES, Field.Index.ANALYZED));
    result.Add(new Field("publisher", file.Publisher, Field.Store.YES, Field.Index.ANALYZED));
    result.Add(new Field("title", file.Title, Field.Store.YES, Field.Index.ANALYZED));
    result.Add(new Field("body", body, Field.Store.YES, Field.Index.ANALYZED));

    return result;
}

That's all nice, but now you want BuildIndex to measure its execution time and return it as a string, right? Wrong.

Let's say the BuildIndex method is located inside an IndexBuilder class; I'd make the method virtual...

public class IndexBuilder
{
    public virtual void BuildIndex(Analyzer analyzer, int pageNum, int pageSize)
    {
        // the above code
    }
}

... and then I'd create a class whose role is to decorate the existing functionality with another functionality, that I can substitute for an IndexBuilder wherever I need one:

public IndexBuilderStopWatchDecorator : IndexBuilder
{
    private readonly IndexBuilder _builder;

    public IndexBuilderStopWatchDecorator(IndexBuilder builder)
    {
        _builder = builder;
    }

    public override void BuildIndex(Analyzer analyzer, int pageNum, int pageSize)
    {
        var watch = StopWatch.StartNew();
        _builder.BuildIndex(analyzer, pageNum, pageSize);
        watch.Stop();
        // log watch.ElapsedMilliseconds,
        // write watch.Elapsed.TotalSeconds to debug output,
        // and/or do whatever the original code is doing with that value.
    }
}
\$\endgroup\$
7
\$\begingroup\$

I'm glad you are using using for your iwriter.

I also like the one comment you have included. It states why the next line of code does what it does, not what it is doing.

The return value should be a long rather than a string. I think this portrays you are expecting a number rather than an alphanumeric string.

I would standardize your use of var. You start off using it very well, then you stop and start using the type in the declaration.

For the stopwatch start, I would just use var watch = Stopwatch.StartNew(); static method. This allows you to get rid of the line to actually start the stopwatch.

The Lucene version should be a const within the class. This will allow the compiler to optimize it better.

I would rename iwriter to indexWriter. This will eliminate the confusion of what i actually is. (In Hungarian Notation, which I am not saying to use, i usually indicates the variable is an int, and it might confuse people).

If you have control of a document, I would make the add fluent:

public Document Add(Field field)
{
    // Add field as required

    return this;
}

Doing this will allow you to string the adds together:

Document doc = new Document();
doc.Add(new Field("id", file.BookId.ToString(), Field.Store.YES, Field.Index.NO))
   .Add(new Field("author", file.Author, Field.Store.YES, Field.Index.ANALYZED))
   .Add(new Field("annotation", file.Annotation, Field.Store.YES, Field.Index.ANALYZED))
   .Add(new Field("publisher", file.Publisher, Field.Store.YES, Field.Index.ANALYZED))
   .Add(new Field("title", file.Title, Field.Store.YES, Field.Index.ANALYZED))
   .Add(new Field("body", bodyBuilder.ToString(), Field.Store.YES, Field.Index.ANALYZED));

Your return could be simply watch.Elapsed, I don't think there is any reason to have to stop it first.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for posting. I don't have control of the Document class, but the strung-together Add calls look nice. \$\endgroup\$
    – Brian
    May 30 '14 at 20:20

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.