Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I have this code to generate some tables in my PDF document for my android app. It works fine, I just think I need to refactor it, as basically is the same structure repeated 3 times.

//tables!
  public static PdfPTable createTableA() {

      //empty space
        Paragraph paragraph = new Paragraph();
        addEmptyLine(paragraph, 3);

        int numberOfCells = 0;
        int numberOfIdealCells = 0;

        //int
        numberOfCells = Checks.aResults.size();
        numberOfIdealCells = numberOfCells;  


          Log.d("mensa", "numero de preguntas : "+numberOfCells);


          //TODO REFACTOR THIS LOGIC TO KNOW IF NUMBER OF CELLS OK!

          if (numberOfCells %3 != 0) {
              Log.d("mensa", "no lo es");
              numberOfIdealCells +=1;

              if (numberOfIdealCells %3 != 0) {
                  numberOfIdealCells +=1;

            }

        }

          Log.d("mensa", "sikas");
          Log.d("mensa", "numero de preguntas afters: "+numberOfIdealCells);

            // a table with three columns
            PdfPTable table = new PdfPTable(3);

            Checks.aResults.keySet().iterator();

            Iterator<String> iteratorSectionC = Checks.aResults.keySet().iterator();
            while(iteratorSectionC.hasNext()) {

                String key=(String)iteratorSectionC.next();

                int value=(int)Checks.aResults.get(key);

               // Log.d("mensa", "mi Pregunta iteratorSectionC::\n"+key+" valor::"+value);

                //Paragraph pN = new Paragraph(key+" val::"+value);
                //document.add(pN);
                table.addCell(key+" val::"+value);

            }

            //add empty cells

            int difference = numberOfIdealCells - numberOfCells;

            if (difference > 0) {

                for(int i = 0; i<difference ; i++){
                    table.addCell("");

                }
            }



            return table;
        }

  public static PdfPTable createTableB() {

      //empty space
        Paragraph paragraph = new Paragraph();
        addEmptyLine(paragraph, 3);

        int numberOfCells = 0;
        int numberOfIdealCells = 0;

        //int
        numberOfCells = Checks.bResults.size();
        numberOfIdealCells = numberOfCells;  


          Log.d("mensa", "numero de preguntas : "+numberOfCells);


          //TODO REFACTOR THIS LOGIC TO KNOW IF NUMBER OF CELLS OK!

          if (numberOfCells %3 != 0) {
              Log.d("mensa", "no lo es");
              numberOfIdealCells +=1;

              if (numberOfIdealCells %3 != 0) {
                  numberOfIdealCells +=1;

            }

        }

          Log.d("mensa", "sikas");
          Log.d("mensa", "numero de preguntas afters: "+numberOfIdealCells);

            // a table with three columns
            PdfPTable table = new PdfPTable(3);

            Checks.bResults.keySet().iterator();

            Iterator<String> iteratorSectionC = Checks.bResults.keySet().iterator();
            while(iteratorSectionC.hasNext()) {

                String key=(String)iteratorSectionC.next();

                int value=(int)Checks.bResults.get(key);

               // Log.d("mensa", "mi Pregunta iteratorSectionC::\n"+key+" valor::"+value);

                //Paragraph pN = new Paragraph(key+" val::"+value);
                //document.add(pN);
                table.addCell(key+" val::"+value);

            }

            //add empty cells

            int difference = numberOfIdealCells - numberOfCells;

            if (difference > 0) {

                for(int i = 0; i<difference ; i++){
                    table.addCell("");

                }
            }



            return table;
        }

  public static PdfPTable createTableC() {

      //empty space
        Paragraph paragraph = new Paragraph();
        addEmptyLine(paragraph, 3);  

    int numberOfCells = 0;
    int numberOfIdealCells = 0;

    //int
    numberOfCells = Checks.cResults.size();
    numberOfIdealCells = numberOfCells;  


      Log.d("mensa", "numero de preguntas : "+numberOfCells);


      //TODO REFACTOR THIS LOGIC TO KNOW IF NUMBER OF CELLS OK!

      if (numberOfCells %3 != 0) {
          Log.d("mensa", "no lo es");
          numberOfIdealCells +=1;

          if (numberOfIdealCells %3 != 0) {
              numberOfIdealCells +=1;

        }

    }

      Log.d("mensa", "sikas");
      Log.d("mensa", "numero de preguntas afters: "+numberOfIdealCells);

        // a table with three columns
        PdfPTable table = new PdfPTable(3);

        Checks.cResults.keySet().iterator();

        Iterator<String> iteratorSectionC = Checks.cResults.keySet().iterator();
        while(iteratorSectionC.hasNext()) {

            String key=(String)iteratorSectionC.next();

            int value=(int)Checks.cResults.get(key);

           // Log.d("mensa", "mi Pregunta iteratorSectionC::\n"+key+" valor::"+value);

            //Paragraph pN = new Paragraph(key+" val::"+value);
            //document.add(pN);
            table.addCell(key+" val::"+value);

        }

        //add empty cells

        int difference = numberOfIdealCells - numberOfCells;

        if (difference > 0) {

            for(int i = 0; i<difference ; i++){
                table.addCell("");

            }
        }



        return table;
    }

My problem is that i need to pass my iterator for each different block, but I can't seem to find a proper way to pass it to my function, Shall I make it a static variable and according to case A,B,C use the corresponding iterator?

        Iterator<String> iteratorSectionC = Checks.cResults.keySet().iterator();

or how to pass it in a function parameter?

share|improve this question

2 Answers

up vote 3 down vote accepted

Getting rid of the duplication is fairly simple, since the three methods were actual duplicates, aside from the access to the Map.

To refactor : in the createTableA() method, introduce local variable from the expression that gets the map, then extract method, that takes the local variable as a parameter. Then reimplement createTableB() and createTableC() to also call the new method. In createTableA() you can then inline the local variable again.

I've also done some other things. I've replaced the calculation of the ideal number of cells (as comments in the code already suggested). And I've extracted a few methods that help readability. Actually this made me wonder wether the Paragraph part (see my addSpace() method) really belongs in this method, since it has nothing to do with creating a table.

I've also trimmed some unnecessary code (casts, and creating an unused iterator), and made use of the foreach loop structure rather than your original while loop. I've introduced an explaining constant for the number of columns, and trimmed the logging and comments that added noise (they were in Spanish, and didn't help me, so too bad :) )

The methods like createTableA() should probably be inlined in the client code.

private static final int COLUMNS = 3;

public static PdfPTable createTableA() {
    return createTable(Checks.aResults);
}

public static PdfPTable createTableB() {
    return createTable(Checks.bResults);
}

public static PdfPTable createTableC() {
    return createTable(Checks.cResults);
}

private static PdfPTable createTable(Map map) {
    addSpace();

    PdfPTable table = new PdfPTable(COLUMNS);
    for (Object key : map.keySet()) {
        table.addCell(key + " val::" + map.get(key));
    }
    addPaddingCells(table, paddingCells(map.size()));
    return table;
}

private static int paddingCells(int totalCells) {
    return (totalCells + COLUMNS - 1)/ COLUMNS * COLUMNS - totalCells;
}

private static void addSpace() {
    addEmptyLine(new Paragraph(), 3);
}

private static void addPaddingCells(PdfPTable table, int number) {
    if (number > 0) {
        for(int i = 0; i< number; i++){
            table.addCell("");
        }
    }
}
share|improve this answer
thanks! excellent answer! – MaKo May 6 at 18:25
On a second inspection I also noticed that the if check in addPAddingCells() is superfluous. – bowmore May 7 at 5:23
up vote 1 down vote

It is not clear what you are realy trying to do. But if I understand you right, you want to save a lot of duplicate code while creating different tables. I added two variables to the table creation method, which could now create the different tables without the code duplication.

Iterators are meant to be traversed only once. Maybe your problem arose from another point of view what iterators can be used for?

Please correct me if I misunderstod you and clearify your question to get more and better answers.

public static void HXLGRMP(){
    tableOne = Class.createTable(Checks.aResults, 1)
    tableOne = Class.createTable(Checks.bResults, 2)
    tableOne = Class.createTable(Checks.cResults, 3)
}


public static PdfPTable createTable(type_of<Checks.Results> results, int tableNumber) {
    //empty space
    Paragraph paragraph = new Paragraph();
    addEmptyLine(paragraph, 3);

    int numberOfCells = 0;
    int numberOfIdealCells = 0;

    //int
    numberOfCells = results.size();
    numberOfIdealCells = numberOfCells;  

    Log.d("mensa", "numero de preguntas : "+numberOfCells);

    //TODO REFACTOR THIS LOGIC TO KNOW IF NUMBER OF CELLS OK!
    if (numberOfCells %3 != 0) {
        Log.d("mensa", "no lo es");
        numberOfIdealCells +=1;
        numberOfIdealCells +=1;
    }

    Log.d("mensa", "sikas");
    Log.d("mensa", "numero de preguntas afters: "+numberOfIdealCells);

    // a table with three columns
    PdfPTable table = new PdfPTable(tableNumber);

    Iterator<String> iteratorSection = results.keySet().iterator();
    while(iteratorSection.hasNext()) {

        String key=(String)iteratorSection.next();

        int value=(int)results.get(key);

        // Log.d("mensa", "mi Pregunta iteratorSection::\n"+key+" valor::"+value);
        //Paragraph pN = new Paragraph(key+" val::"+value);
        //document.add(pN);
        table.addCell(key+" val::"+value);
    }

    //add empty cells
    int difference = numberOfIdealCells - numberOfCells;

    if (difference > 0) {
        for(int i = 0; i<difference ; i++){
            table.addCell("");
        }
    }

    return table;
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.