0

I did the sepia filter for the cs50 week 4 problem "filter) and although it works, I've been wondering, is there is not a more "proper" way to write the code, that is with out repeating that many if sentences? If you can give an idea or if you think is not worthy to change the code please tell me and I'll appreciate your answer.

void sepia(int height, int width, RGBTRIPLE image[height][width])
{
    for (int i=0 ; i<height ; i++)
    {
        for (int j=0 ; j<width ; j++)
        {
            int originalRed=image[i][j].rgbtRed;
            int originalBlue=image[i][j].rgbtBlue;
            int originalGreen=image[i][j].rgbtGreen;
            
            int SepiaRed=round(.393 * originalRed + .769 * originalGreen + .189 * originalBlue); 
            int SepiaBlue=round(.272 * originalRed + .534 * originalGreen + .131 * originalBlue);
            int SepiaGreen=round(.349 * originalRed + .686 * originalGreen + .168 * originalBlue);
            
            if (SepiaRed>255)
            {
                image[i][j].rgbtRed=255;    
            }
            else
            {
                image[i][j].rgbtRed=SepiaRed;
            }
            
            if (SepiaBlue>255)
            {
                image[i][j].rgbtBlue=255;
            }
            else
            {
                image[i][j].rgbtBlue=SepiaBlue;
            }
            
            if (SepiaGreen>255)
            {
                image[i][j].rgbtGreen=255;   
            }
            else 
            {
                image[i][j].rgbtGreen=SepiaGreen;
            }
            
            
        }
    }
    
    return;
}
1

It's very readable as it is, with little inefficiency. The only things that I can think of doing would be to delete the temp vars original(color) and use the array vars directly. That would eliminate 3 assignments. The improvement in efficiency would be trivial though.

Next, each if/else sequence could be replaced by using the ternary operator. For example:

image[i][j].rgbtRed = SepiaRed>255 ? 255 : SepiaRed;  

I'll let you google how the ternary operator works. But, it would reduce 8 lines of code to 1. (I find that most programmers don't use this operator, preferring if/else.)

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .