CODEWITHSUNDEEP
matlabrefactoring
user1725619
user1725619

Reputation:

How can I re-write this MATLAB code in a more idiomatic and efficient way?

Here is the requirement: the input is a number which will divide an image into several equal part. For example, if the input is 4, it will return 3 parts: the values are the imgSize/4 imgSize/4 * 2 and imgSize/4 * 3.

If the input is n, then it will return n - 1 elements, as in the following implementation:

if (colorLevel == 8)
    divide_thres = [ round(imgSize/8) round(imgSize/8)*2 round(imgSize/8)*3 round(imgSize/8)*4 
        round(imgSize/8)*5 round(imgSize/8)*6 round(imgSize/8)*7 ];
elseif (colorLevel == 4)
    divide_thres = [ round(imgSize/4) round(imgSize/4)*2 round(imgSize/4)*3 ];
elseif (colorLevel == 3)
    divide_thres = [ round(imgSize/3) round(imgSize/3)*2 ];
end

I want to allow the user to input a value between 2 and 255, and then automatically generate the divide_thres corresponding to that input. How can I re-write this code to be more efficient?

Upvotes: 1

Views: 127

Answers (2)

jub0bs
jub0bs

Reputation: 66244

There are several problems with your code:

  • In each case, you unnecessarily divide imgSize by the same factor multiple times (instead of just once and for all).
  • You perform many "manual" scalar multiplications, but you could simply multiply the scalar value by a vector generated by the range 1 : colorLevel - 1.

  • Because the length of the resulting divide_thres vector can be easily computed from the value of colorLevel, there is no need to treat each case separately in an if statement.

    Moreover, even if you had to compute the length in a different manner for the cases imgSize = 3, 4, and 8, you would be better off using a switch statement instead of an if statement, because the former would save you the trouble of writing imgSize == ... each time, which is prone to errors and a form of code duplication.

Here's a much simplified approach:

if 2 <= colorLevel && colorLevel <= 255
    divide_thres = round(imgSize / colorLevel) * (1 : colorLevel - 1);
else
    error('invalid colorLevel value') % (or some other informative message)
end

Upvotes: 7

Divakar
Divakar

Reputation: 221574

This should do it -

divide_thres = [1:colorLevel-1]*round(imgSize/colorLevel)

Upvotes: 1

Related Questions