How to refactor this switch case

Question

I'm trying to read the data from excel and extracting the data from each column and storing it in another variable. Each case represents the column. Using fbDataField I'm adding the row data into the List name fbList. Can this code be refactored to make it look a clean code?

for(int i=0;i< workbook.getNumberOfSheets(); i++){

    Sheet sheet = workbook.getSheetAt(i);

    Iterator<Row> rowIterator = sheet.iterator();

    while(rowIterator.hasNext()){

        FacebookFields fbDataField= new FacebookFields();
        Row row = rowIterator.next();
        if(row.getRowNum()== 0 )
        {
            continue;
        }
        Iterator<Cell> cellIterator = row.cellIterator();

        while(cellIterator.hasNext()){

            Cell cell= cellIterator.next();

            switch(cell.getColumnIndex()){
            case 0: fbDataField.setName(cell.getStringCellValue());
            break;
            case 1: fbDataField.setId(cell.getStringCellValue());
            break;
            case 2: fbDataField.setDate(cell.getStringCellValue());
            break;
            case 3: fbDataField.setMessage(cell.getStringCellValue());
            break;
            case 4: fbDataField.setType(cell.getStringCellValue());
            break;
            case 5: fbDataField.setPage(cell.getStringCellValue());
            break;
            case 6: fbDataField.setLikeCount(String.valueOf(cell.getNumericCellValue()));
            break;
            case 7: fbDataField.setCommentCount(String.valueOf(cell.getNumericCellValue()));
            break;
            case 8: fbDataField.setShareCount(String.valueOf(cell.getNumericCellValue()));
            break;

            }

        } 
        fbList.add(fbDataField);
    }
}

You may get a better response if you add the appropriate language tag to the question. I can't decide whether it is C++ or Java (with C# another possibility). The code layout in the case is grotty, but there's probably a better way to organize the code altogether. — Jonathan Leffler, Oct 05 '15 at 07:19
Just make a new class for handling the `switch` statemement and that is your refactoring done. Refactoring is not about reinventing the wheel, it is about segmenting your code to smaller logical pieces. Basically any piece of code that does something and can be described with verb should be class/method. — The Law, Oct 05 '15 at 08:00
@TheLaw Why make a new class? The class already exists, and it's called `List>`. — , Oct 05 '15 at 08:25
@elyse That is about preference in code design, I'd say. I like to keep my classes at screen length at most and separate them by functionality, for readability purposes. — The Law, Oct 05 '15 at 08:39

score 3 · Answer 1 · edited May 23 '17 at 12:03

The documentation for POI¹ states that it has iterator() aliases to enable foreach loops²:

for (Sheet sheet : workbook) {
    for (Row row : sheet) {

        if (row.getRowNum() == 0) {
            continue;
        }

        FacebookFields record = new FacebookFields();

        for (Cell cell : row) {
            switch (cell.getColumnIndex()) {
                case 0: record.setName        (cell.getStringCellValue()); break; 
                case 1: record.setId          (cell.getStringCellValue()); break; 
                case 2: record.setDate        (cell.getStringCellValue()); break; 
                case 3: record.setMessage     (cell.getStringCellValue()); break; 
                case 4: record.setType        (cell.getStringCellValue()); break; 
                case 5: record.setPage        (cell.getStringCellValue()); break; 
                case 6: record.setLikeCount   (String.valueOf(cell.getNumericCellValue())); break; 
                case 7: record.setCommentCount(String.valueOf(cell.getNumericCellValue())); break; 
                case 8: record.setShareCount  (String.valueOf(cell.getNumericCellValue())); break; 
            }
        }

        fbList.add(record);
    }
}

Note:

I'd consider changing the types of the numerical fields to not be String
I'd consider extracting methods to read a row and to read a worksheet:

for (Sheet sheet : workbook) {
    readSheet(sheet, fbList);
}

private static void readSheet(final Sheet sheet, final List<FacebookFields> fbList) {
    for (Row row : sheet)
        if (row.getRowNum() > 0)
            fbList.add(readRow(row));
}

private static FacebookFields readRow(final Row row) {
    FacebookFields record = new FacebookFields();

    for (Cell cell : row) {
        switch (cell.getColumnIndex()) {
            case 0: record.setName        (cell.getStringCellValue()); break; 
            case 1: record.setId          (cell.getStringCellValue()); break; 
            case 2: record.setDate        (cell.getStringCellValue()); break; 
            case 3: record.setMessage     (cell.getStringCellValue()); break; 
            case 4: record.setType        (cell.getStringCellValue()); break; 
            case 5: record.setPage        (cell.getStringCellValue()); break; 
            case 6: record.setLikeCount   (cell.getNumericCellValue()); break; 
            case 7: record.setCommentCount(cell.getNumericCellValue()); break; 
            case 8: record.setShareCount  (cell.getNumericCellValue()); break; 
        }
    }

    return record;
}

¹ e.g. https://poi.apache.org/apidocs/org/apache/poi/hssf/usermodel/HSSFWorkbook.html

² How does the Java 'for each' loop work?

Why would you? You don't, unless you make a far more complicated design. I wouldn't unless you keep reading different table layouts all the time — sehe, Oct 05 '15 at 08:18
@OldCurmudgeon just posted a nifty trick that I would recommend for such cases :) — sehe, Oct 05 '15 at 08:27

OldCurmudgeon · Answer 2 · 2015-10-05T09:27:58.090

I'd use an enum as a copy factory.

enum CellToFb {

    Name {

                @Override
                void copy(FacebookFields fb, Cell cell) {
                    fb.setName(cell.getStringCellValue());
                }
            },
    Id {

                @Override
                void copy(FacebookFields fb, Cell cell) {
                    fb.setId(cell.getStringCellValue());
                }
            };

    // Every enum must be able to copy one cell to the correct FB field.
    public abstract void copy(FacebookFields fb, Cell cell);
}

public void test() {
    // Get all copiers - one for each column.
    CellToFb[] copiers = CellToFb.values();
    while (cellIterator.hasNext()) {
        Cell cell = cellIterator.next();
        // Call the correct copier.
        copiers[cell.getColumnIndex()].copy(fbDataField, cell);
    }
}

Each enum knows how to copy a specific column. Just make sure the enums match up with the columns. Your copy code becomes very simple.

This technique ports nicely to Java 8 lambdas.

// Couple of simple demo classes.
class From {

    String id;
    String name;

    @Override
    public String toString() {
        return "{" + id + "," + name + "}";
    }
}

class To {

    String identifier;
    String value;

    @Override
    public String toString() {
        return "{" + identifier + "," + value + "}";
    }
}

enum FromToTo implements BiConsumer<To, From> {

    // Each field defines it's own copying lambda.
    Id((to, from) -> to.identifier = from.id),
    Value((to, from) -> to.value = from.name);
    // The copying lambda.
    private final BiConsumer<To, From> copy;

    private FromToTo(BiConsumer<To, From> copy) {
        this.copy = copy;
    }

    @Override
    public void accept(To to, From from) {
        // Delegate to the copying lambda.
        copy.accept(to, from);
    }

    // Take s static copy to avoid duplicating.
    private static final FromToTo[] copiers = FromToTo.values();

    public static void copy(To to, From from) {
        // Useful function to copy fields from -> to
        Arrays.stream(copiers)
                .forEach(ftt -> ftt.accept(to, from));
    }

}

public void test() {
    From f = new From();
    f.id = "id";
    f.name = "name";
    To t = new To();
    FromToTo.copy(t, f);
    System.out.println(f + " -> " + t);
}

@sehe - It is resilient to change too! Add a new column -> add a new `enum`. It's still really a glorified `switch` statement but it moves stuff out of loops which is always a good thing for me. — OldCurmudgeon, Oct 05 '15 at 08:25

How to refactor this switch case

2 Answers2