HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Overlapping table fragments

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fragmentsoverlappingtable

Problem

One of my side projects is a table driven scanner/parser generator, in an effort to reduce the size of the transition table I represent the generated table A[x, y] as A'[x + I[y]] instead of A'[x + y * c]. This allows me to overlap identical parts of the table.

The following is the code responsible for actually arranging the rows. I'm not very happy with it, particularly the number of arrays that need to be constructed. Any improvements to readability/performance/effectiveness would be much appreciated.

```
///
/// A two dimentional array, or table, can be represented by a single dimentional table
/// where the data for position (x, y) is stored at index x + table_width * y. If the
/// table is read-only then the multiplication can be replaced with an index lookup
/// allowing identical portions of the table to be overlapped thus (depending on the
/// data) reducing the memory footprint of the table.
///
/// The purpose of this class is to take a bunch of rows and try to find an arrangement
/// with a good approximation of the minimal space required. Finding the optimal solution
/// reduces to the "Traveling Sailsman Problem", so a good approximation is sufficient.
///
/// This particular algorithm is intended to work with the characteristics of the sparse
/// state transition tables of the lexer/parser and will probably perform very poorly
/// for other cases.
///
/// Algorithm Overview:
///
/// * If two fragments with the same content are encountered, they are combined with
/// identical offsets.
///
/// * Each fragment gets two tags, one for each end. all start tags are put into one list
/// and all end tags in another.
///
/// * Sort both lists first by "end value" (the very first value for a start tag and the
/// very last value for an end tag) and second by "end length" (the number of
/// consecutive times "end value" appears) in descending order.
///
/// * Make a single pass over both lists at the same time, combining distinct fragments
//

Solution

Did some reading during the past few days about clean code and I can spot a bit of complexity in yours. Just some things that I think would improve the readability:

Multiple similar blocks like :

states[write] = states1[read1];
        offsets[write] = offsets1[read1];
        write++;
        read1++;


if this could be refactored into a function it would mean a + for readability.

Because of the else if I would also replace

if (diff  0)
        {
            states[write] = states2[read2];
            ---
        }
        else
        {
            throw new InvalidOperationException ....
        }


with:

if (diff == 0)
   throw new ....

if (diff > 0)
{
   ...
}
else
{
    ...
}


This also looks like duplicate code:

while (read1 < states1.Length)
    {
        states[write] = states1[read1];
        offsets[write] = offsets1[read1];
        write++;
        read1++;
    }

    while (read2 < states2.Length)
    {
        states[write] = states2[read2];
        offsets[write] = offsets2[read2] + offset;
        write++;
        read2++;
    }

Code Snippets

states[write] = states1[read1];
        offsets[write] = offsets1[read1];
        write++;
        read1++;
if (diff < 0)
        {
            states[write] = states1[read1];
            ---
        }
        else if (diff > 0)
        {
            states[write] = states2[read2];
            ---
        }
        else
        {
            throw new InvalidOperationException ....
        }
if (diff == 0)
   throw new ....

if (diff > 0)
{
   ...
}
else
{
    ...
}
while (read1 < states1.Length)
    {
        states[write] = states1[read1];
        offsets[write] = offsets1[read1];
        write++;
        read1++;
    }

    while (read2 < states2.Length)
    {
        states[write] = states2[read2];
        offsets[write] = offsets2[read2] + offset;
        write++;
        read2++;
    }

Context

StackExchange Code Review Q#1308, answer score: 5

Revisions (0)

No revisions yet.