Home » Tools » Ploter refactoring

Ploter refactoring

Some days ago I mentioned on twitter that I feel trapped inside infinite refactoring loop. I see various places that I’m not satisfied with in my code, and it keeps me from adding new features. Yesterday I finally added one, simple algorithm to check how refactoring simplified code structure… well… it’s not that bad. But I decided to work some more on ploter class. It have some nasty magic numbers and very messy draw function (you can see it here).

To make things little easier I use techniques from Fowler’s Refactoring. Complete code for ploter class can be found here, althrough I’ll focus only on drawSolutionPlot methood.

First step was Replacing Magic Number with Symbolic Constants.

  • constant int scale, that replaced 20 which basically was used to draw proportional rectalgnes and lines,
  • second (after colorTab) array int LineStartingY that contains y values for each machine line,
  • const int LineStartingX that contain x value after which jobs should be drawed (before that X is part reserved for labels),
  • constant labelOffset QPointF instance to store x and y offsets for setting job labels offsets (so instead of using 45 for 2nd machine labels I’ll now use lineStartingY + labelOffset.y),
  • const int jobHeight to replace another 20 value,

Before:

TimeLabels[j]->setPos(24+offsetFor1stJob*20,25);
plot.addRect(20+offsetFor1stJob*20,20,20*jobWidth,20,pen,brush);

After:

TimeLabels[j]->setPos(labelOffset.x() + offsetFor1stJob * scale,
 lineStartingY[0] + labelOffset.y());

plot.addRect(lineStartingX + offsetFor1stJob * scale, lineStartingY[0],
 jobWidth * scale, jobHeight , pen, brush);

It’s probably some more readable. I simplify it furthermore with introduction of explaining variable to use instead of lineStartingX + offsetFor1stJob * scale.

Second step is inlining Temp’s. If you look at main drawing loop you’ll see that I use the same instructions triple to draw plot part for each machine. I’d be great to finally change it to loop. First I use three variables:

int jobWidth = 0;
int job2Width = 0;
int job3Width = 0;

Well… it’s useless, and I don’t know what I was thinking when I first writed this part of code. We use one job(x)Width at time, so there’s nothing wrong with using just jobWidth. Not to mention that all this variables always stored times for one job (but on different machine) so name was quite misleading… then I renamed variable currentMachineJobWidth is soo much better name for this variable.

Also I changed QString tmp name to currentLabelContent. It’s soo much better now.

Another step in order to introduce machine loop inside main drawing loop was replacing parameters with… Well… something… drawSolutionPlot declaration is terrible… have a look at declaration:

QGraphicsScene* Ploter::drawSolutionPlot(Machine* first, Machine* second,
   Machine* third, int JobCount, int MachineCount , QString result)

See?
I use all machines as parameters, and another int that contains amout of machines… it’d be better to use machine array and it’s length as parameters, but… let’s have a look at GUI.

machine_spin

That item inside red circle is machine spinbox, user can change it’s value to change amount of machines in problem. Now we can use dynamic array, list or simple, fixed array with lenght equalled to max of spin box.
Dynamic array is impractical – modifing array lenght after every value change requires too much memory swapping.
List may looks ok, but if we work on finite, small number of machines (like max 3 for Johnson) it’s disputable solution. Keeping all machines seems better than creating new and deleting unused after every spinbox value change… soo… I’ll settle (for now) with fixed array of machines.

New method declaration:

QGraphicsScene* Ploter::drawSolutionPlot(Machine** machines, 
   int MachineCount, int JobCount, QString result)

Basically drawing single job from machine contains of few steps applied for each job on list:
1) Calculate job duration, that’ll be used as job rectangle width.
2) For 2nd and 3rd machine check offset points from which method’ll start drawing job rectangle.
3) Prepare label for current job (set text and position based on job offset)
4) Set actual job rectangle based on job offset, scale and y offset.
5) Go to 1) on another machine

So… crucial was creating collection of machines (to iterate thro it), and little array to replace offsetFor(X)job – I couldn’t use one tmp variable like in job(x)width because every machine have own offset rised by job duration on previous machines (you cannot start job on 3rd machine if 2nd is still proccesing it), and current job duration.

I won’t show a ‘before’ method. It’s too long, too ugly and you probably saw it already if you followed first link in that text.
An ‘after’ method loop looks like that:

// Declarations
int labelIdxj=0;
for(int i=1; i < JobCount+1; ++i){
 for(int j=0; j < MachineCount ; ++j){
  jobLabelContent.setNum(machines[j]->getJobId(i));
  jobLabelContent.insert(0,"Z");
   
  currentMachineJobWidth = machines[j]->getJobDuration(i);

  if( i > 0 && offsetForJob[j-1] >= offsetForJob[j])
   offsetForJob[j] = offsetForJob[j-1];

  TimeLabels[labelIdxj]->setText(jobLabelContent);
  TimeLabels[labelIdxj]->
     setPos(labelOffset.x() + offsetForJob[j] * scale,
            lineStartingY[j] + labelOffset.y());

  plot.addRect(lineStartingX + offsetForJob[j] * scale, 
    lineStartingY[j], 
    currentMachineJobWidth * scale,
    jobHeight ,
    pen, brush);
  plot.addItem(TimeLabels[labelIdxj++]);

  offsetForJob[j] += currentMachineJobWidth;
 }

 pen.setColor(colorTab[i]);
 brush.setColor(colorTab[i]);
}

And well… this concludes (for now) my refactoring. It’s still quite ugly, but I believe it’s more readable now. See for yourself!

Posted in Tools and tagged as ,

3 comments on “Ploter refactoring