patterncppMinor
Image processing algorithm
Viewed 0 times
algorithmprocessingimage
Problem
This is code I need to maintain. I'm trying to make this as an example for code gone bad, for C programmers going to C++.
Please provide any comment you can. The original code is about 40 pages long, so I just gave this "small example".
```
void Algo::compute( Image pInImg, Image pImg, Image* deBugImg)
{
int ind_r, ind_c, ind_app_r, ind_app_c , ind_g_r, ind_g_c;
int Gv_approx[7][7], Gh_approx[7][7], Bv_approx[5][5], Bh_approx[5][5], Rv_approx[5][5], Rh_approx[5][5];
int Gv_clip_approx[7][7], Gh_clip_approx[7][7];
int HH_h, HH_v;
int Yv_approx[5][3], Yh_approx[3][5], Uv_approx[5][3], Uh_approx[3][5], Vv_approx[5][3], Vh_approx[3][5] ;
int R_ahd[5][5], G_ahd[5][5], B_ahd[5][5];
int Y_ahd[5][5], U_ahd[5][5], V_ahd[5][5];
int w_v_q, w_h_q;
int avg_gr_q=0, avg_gb_q=0, avg_G_q, avg_G3_q, G_std_q, G3_std_q, Gr_std_q, Gb_std_q, AvgG_std_q;
int avg_G3_q_rm1, avg_G3_q_rp1, avg_G3_q_cm1, avg_G3_q_cp1, G3_std_q_rm1, G3_std_q_rp1, G3_std_q_cm1, G3_std_q_cp1;// Research checkers FCC
int std_prec = (1 x_vect = {2310};
std::array y_vect = {1723};
FILE *fp_log=fopen(m_OutputFileName.c_str(),"wt");
int w_clp_array[7], w_clp_array_even[7], w_clp_array_odd[7];
// define config units
Prepare_Config_Unit_DIAG1();
Prepare_config_Unit_DIAG2();
Prepare_config_Unit_HF();
Prepare_config_Unit_DIAGSAT();
// produce corinng threshold calculation outside main loop
int Corin_TH_array[4096];
for(int ii = 0; ii1)
{
MSB = (int)(log((float)ii)/log(2.0));
valinlog= ii - (10)
{
MSB2 =(int)(log((float)valinlog)/log(2.0));
Corin_TH_array[ii] = MSB + (MSB2 == (MSB-1));
}
else
{
Corin_TH_array[ii] = MSB;
}
}
else
Please provide any comment you can. The original code is about 40 pages long, so I just gave this "small example".
```
void Algo::compute( Image pInImg, Image pImg, Image* deBugImg)
{
int ind_r, ind_c, ind_app_r, ind_app_c , ind_g_r, ind_g_c;
int Gv_approx[7][7], Gh_approx[7][7], Bv_approx[5][5], Bh_approx[5][5], Rv_approx[5][5], Rh_approx[5][5];
int Gv_clip_approx[7][7], Gh_clip_approx[7][7];
int HH_h, HH_v;
int Yv_approx[5][3], Yh_approx[3][5], Uv_approx[5][3], Uh_approx[3][5], Vv_approx[5][3], Vh_approx[3][5] ;
int R_ahd[5][5], G_ahd[5][5], B_ahd[5][5];
int Y_ahd[5][5], U_ahd[5][5], V_ahd[5][5];
int w_v_q, w_h_q;
int avg_gr_q=0, avg_gb_q=0, avg_G_q, avg_G3_q, G_std_q, G3_std_q, Gr_std_q, Gb_std_q, AvgG_std_q;
int avg_G3_q_rm1, avg_G3_q_rp1, avg_G3_q_cm1, avg_G3_q_cp1, G3_std_q_rm1, G3_std_q_rp1, G3_std_q_cm1, G3_std_q_cp1;// Research checkers FCC
int std_prec = (1 x_vect = {2310};
std::array y_vect = {1723};
FILE *fp_log=fopen(m_OutputFileName.c_str(),"wt");
int w_clp_array[7], w_clp_array_even[7], w_clp_array_odd[7];
// define config units
Prepare_Config_Unit_DIAG1();
Prepare_config_Unit_DIAG2();
Prepare_config_Unit_HF();
Prepare_config_Unit_DIAGSAT();
// produce corinng threshold calculation outside main loop
int Corin_TH_array[4096];
for(int ii = 0; ii1)
{
MSB = (int)(log((float)ii)/log(2.0));
valinlog= ii - (10)
{
MSB2 =(int)(log((float)valinlog)/log(2.0));
Corin_TH_array[ii] = MSB + (MSB2 == (MSB-1));
}
else
{
Corin_TH_array[ii] = MSB;
}
}
else
Solution
Given that a good rule of thumb is to keep functions small enough to fit on a page, I want the kind of screen these coders have...
Seriously, function calls are cheap on modern hardware, and compilers can do the inlining for you today. I'd bet the code can gain some performance simply by chopping it into smaller functions.
You might get away with giving a variable a name like
Speaking of comments, if you can't be bothered to put stuff in functions, at least write a comment explaining what each loop is doing.
Finally, I know C isn't an object-oriented language at heart, but it does support structs. Use them to group related values together.
Seriously, function calls are cheap on modern hardware, and compilers can do the inlining for you today. I'd bet the code can gain some performance simply by chopping it into smaller functions.
You might get away with giving a variable a name like
HH_h if there were <10 variables in the function, but for this monstrosity you'd need to have a more descriptive name than that, and also a comment for every variable, describing exactly what it represents.Speaking of comments, if you can't be bothered to put stuff in functions, at least write a comment explaining what each loop is doing.
Finally, I know C isn't an object-oriented language at heart, but it does support structs. Use them to group related values together.
Context
StackExchange Code Review Q#44900, answer score: 6
Revisions (0)
No revisions yet.