Refactor a huge Switch Statement (1400 lines of Code)

Hi everyone,

so i have a programm and part of that programm is a specific image processing routine. I didn't write this myself but i have to refactor it and i want to know whether there is some kind of standard method to refactor switch statements.

I give you an example of the code:

case 'Use ROI'
          if(2==exist(fs{1},'file'))
              try
                  load(fs{1},'-mat');
                  BW = roipoly(tmpImg,xy(:,1),xy(:,2));
                  if(isnumeric(fs{2})&&fs{2}) % inverted
                      strImg.pro = ~BW.*tmpImg;
                  else
                      strImg.pro = BW.*tmpImg;
                  end
              catch
                  warning('Could not load ROI');
                  strImg.pro = tmpImg;
              end
          else
              strImg.pro = tmpImg;
              warning(['Could not load ROI file ''',fs{1},'''']);
          end
case 'Blurring'
          h = fspecial('average',fs{1});
          if(fs{1}>200)
              pad = 0;%fs{1};
              sub = .5*pad;
              F = fft2(tmpImg, size(tmpImg,1)+pad, size(tmpImg,2)+pad);
              H = fft2(double(h), size(tmpImg,1)+pad, size(tmpImg,2)+pad);
              %Multiply the transform by the filter:
              G=H.*F;
              %Obtain the real part of the inverse FFT of G:
              g=abs(ifft2(G));
              %Crop the top, left rectangle to the original size:
              strImg.pro=g(sub+1:size(g,1)-sub,sub+1:size(g,2)-sub);

            else
                warning off;
                strImg.pro = im2double(imfilter(im2uint8(tmpImg),h,'symmetric','conv'));
                warning on;
            end

So i basically have about 80 of these case statements and i refuse to believe that this is the best way to do this. But since i don't have a lot of experience i'd rather ask you guys before i do anything stupid. Because if it really is the best option than i will simply leave it alone. But it looks pretty messy and gets kind of confusing especially if more if statements are added. Thanks a lot for your input and ideas!

Answer 1

[Walter Roberson]

ismember() against a cell array of character vectors, take the second output, to find which of the vectors it is. Use that value to index a cell array of function handles, pull out that one function handle, and invoke it with appropriate arguments.

This requires that you define an interface of which variables each case is permitted to set.

[Julian]

Hello Walter,

thank you for your answer.

I think i got the first part, but i'm struggling with the second one ('defining an interface of which variables each case is permitted to set') So far i came up with this code :

 option = 'Use ROI';                               % example option
processingOptions = {'Use ROI';'Blurring'};       % cell array with all of the available options
A = ismember(processingOptions,option);           % check if an option was selected
index = find(A == 1);                             % get index of the option
functions = {@useROI; @Blurring};                 % cell array with the functions
ipFunction = functions{index};                    % extract the wanted function
strImg = ipFunction(fs,strImg,tmpImg);            % execute the function

Basically what i have to do is make a function out of every case statement right? But how do i define this interface that you were talking about? Could you go a little bit more into detail? Because up to now the code would only work for one specific function, the useROI function. Thanks a lot!

[Walter Roberson]

processingOptions = {'Use ROI';'Blurring'};
functions = {@useROI; @Blurring};                 % cell array with the functions
[isvalid, index] = ismember(option, processingOptions);
if isvalid
  ipFunction = functions{index};
  strImg = ipFunction(fs, strImg, tmpImg);
else
   fprintf(2, 'Say what?!\n');
end

[Stephen23]

@Julian: Note that all of the functions will require exactly the same inputs (simplest, as you do now), or you will need some kind of cell array with indexing to select the inputs, or some alternative (e.g. supplying the inputs as fields of a structure), or get rid of them altogether and use nested functions.

Note that find is superfluous, get rid of it.

[Julian]

Thank you both, the functions will definitely need different kind of inputs. Would a struct with one field with all the functions and one field with all the inputs be a good solution?

It seems like this is a lot of work, but do you think it is worth it? Its still better than this huge switch statement right?

[Stephen23]

@Julian: handling many different inputs sounds like a lot of hassle to me. But nested function would possibly mean you could avoid all of that, by simply defining the required variables in the main function workspace:

function stringImg = main()
... define any common values.
strImg = struct(); % define output
...
C = {@fun1,fun2,...}
idx = ismember(...)
C{idx}() % call the function
...
   function fun1()
   ...
   strImg.pro = ...
   end
   function fun2()
   ...
   strImg.pro = ...
   end
   ...
end

[Julian]

All right, this sounds really good. So up to now i have the following code:

 function strImg = main(strImg, sMethod,strOrgImInf) 
tmpImg = strImg.tmp;
[option,fs] = getfiltersize(sMethod);
processingOptions = {'Subimage';'Subregion';'Use ROI'};
functions = {@subimage;  @subregion; @useROI;}
[~, index] = ismember(option,processingOptions);
strImg = functions{index};
    function strImg = useROI(fs, tmpImg)

        if(2 == exist(fs{1},'file'))
            try
                load(fs{1},'-mat');
                BW = roipoly(tmpImg,xy(:,1),xy(:,2));
                if(isnumeric(fs{2})&&fs{2}) % inverted
                    strImg.pro = ~BW.*tmpImg;
                else
                    strImg.pro = BW.*tmpImg;
                end
            catch
                warning('Could not load ROI');
                strImg.pro = tmpImg;
            end
        else
            strImg.pro = tmpImg;
            warning(['Could not load ROI file ''',fs{1},'''']);
        end
    end
  end

But my problem is now that when i run it. 'processingOptions' contains @main/useROI and then functions{index} won't find the right nested function. But i can't figure out where the problem is.

[Stephen23]

" processingOptions doesn't contain @main/subimage and then functions{index} won't find the right nested function. But i can't figure out where the problem is."

Of course ismember could not find 'subimage', because processingOptions does not contain that character vector! ismember is case sensitive, so 'Subimage' (like you wrote in your code) is not the same as 'subimage' (like you used in your comment). If you want ismember to perform a case-insensitive comparison, then apply lower to its inputs.

Also you did not define a function named subimage.

[Julian]

Sorry i mixed up some things. i only pasted in the first lines of the code, this is why you can't see the subimage function... but what i actually meant was:

'processingOptions' now contains @main/useROI (and @mainsubimage, @main/subregion etc) so when i call functions{index} it will not jump to the right nested functions. Sorry for the confusion

[Stephen23]

@Julian: you don't need main in the handle definition.

[Julian]

The thing is i have it exactly like that;

functions = {@subimage; @subregion; @useROI;}

So no use of main at all. But still all of the functions that are defined later on in the part with all of the nested functions are stored with @main/... if i add a new function but won't yet define it in the part with the other nested functions and simply run it up to functions{@subimage; @subregion; @useROI; @newFunc} than only this handle won't get the @main/ .

Answer 2

[Steven Lord]

Instead of using ismember or a struct filled with function handles, I would move the code inside each of your case statements into their own function (a local function in that same file, a private in a directory named private, a function in a package directory, or a regular old function if it is sufficiently general that this code and others could use it.) Then you code would look like:

switch  taskToPerform
  case 'Use ROI'
    strImg = performROI(fs, strImg);
  case 'Blurring'
    strImg = performBlurring(fs, strImg);
  case ...

Organizing each task into their own separate functions lets you insulate those tasks from new variables that you may introduce in the main function later on, control how much and which information they can access from the main function, and (if defined as a package function or a regular old function) lets you test those functions in isolation so that you can be more confident that they perform correctly when used as part of your larger application.

As part of that refactoring, you may find that certain of your task functions share code. In that case, consider extracting those common operations into their own helper functions.

[Adam]

This is what I would do also. Removing the switch statement itself seems like it would obfuscate the meaning and create code that is a lot less intuitive, whereas moving out all the code inside, leaving just one-line function calls for each case, with a well-named function, seems to me a much more readable way to tidy up the code.