More about coding style

In a previous post, coding convention, which is more important than design patterns, I pointed out some bad styles.

Recently , I saw very interesting style. Before explaining about it, let’s take a look at it.

for(i=0; i<4; i++)
{
	char s&#91;4&#93;={'I','P','B',':'};

	double frame_size,mquant_avg,MB_info,MB_data,X,d;

	X = favg(v_enc->v_vars.frame_stat_info[i].X, v_enc->v_vars.frame_stat_info[i].X_d_num);
	d = 100.*favg(v_enc->v_vars.frame_stat_info[i].d, v_enc->v_vars.frame_stat_info[i].X_d_num);
	frame_size =  favg(v_enc->v_vars.frame_stat_info[i].frame_size_sum, v_enc->v_vars.frame_stat_info[i].frame_size_num);
	mquant_avg =  favg(v_enc->v_vars.frame_stat_info[i].mquant_avg_sum, v_enc->v_vars.frame_stat_info[i].mquant_avg_num);
	if(v_enc->v_vars.frame_stat_info[i].mbiBits_sum == 0)
	{
	 	v_enc->v_ctrl.inf_printf( "   %c:mquant=%6.2f, X=%6.2f, d=%7.3f, size=%6.0f, frames: %d", s[i], mquant_avg,  X, d, frame_size/8., (long_d)v_enc->v_vars.frame_stat_info[i].mquant_avg_num);
	}

I’m sorry that it is pretty hard to find where the s[] is used. It is in the braces where inf_printf() is.
Why it is weird is because of the usage of iteration on s[i].
Usually, people declares s[i] before for( i =… ). Although the code above works, you can easily see that the original writer thought differently than others.
Also, the code is too dense. So, it is bad enough for your eyes.

Let’s take a look at other style.

ConvertXXXX_XXXX(
                       planes, luma_stride, SrcWidth, SrcHeight,
                       pbSrc, pbSrc2, SrcLineSize, SrcWidth, SrcHeight,
                       ColorSpaceFourcc, optFlags, color_sampling,
                       v_enc->v_vars.prog_frame, v_enc->v_setting.deinterlacing_mode);

Codes like above is very typical C/C++ code. There are too many arguments and it is pretty hard to figure out actual meaning of each parameter. What does “pb” stand for in pbSrc?
What does the “planes” mean? Is it color space plane? Or a plane for images with different resolution in pyramidal search method? It is not clear to figure out what they mean.

Here are other examples.

FRM_BUF_FILLED : What does FRM mean? It seems to be “frame”. But some will interpret it as “from”.

FEATURE_BOT_FIELD_FIRST : What does the “BOT” mean? Is it for a field named “bot”? What is the code author’s original intention. If you read this, the meaning of the “BOT” becomes clear.

FEATURE_TOP_FIELD_FIRST

How about this?

[theLockXmlDocument lock]

Wouldn’t xmlDocLock or LockForXMLDoc be better?

It is always better to set a function’s name starting with verb, and variable’s name as noun.

5 responses to this post.

  1. 요즘 언급하신 내용들이 눈에 들어옵니다.
    참 깔끔하고 명확하게 코딩을 한다는 게 자기 의지와는 상관없이 언어의 특성이나 API 특성때문에 말끔하게 되지는 않지만 위처럼 생각을 좀 정리해보면서 해보는 습관이 굉장히 도움이 되리라 생각됩니다.

    Reply

  2. Posted by jongampark on January 29, 2009 at 9:06 AM

    안녕하세요?
    맞아요. 더군다나 코딩을 할때는, 이렇게 하면 될까 저렇게 하면 될까 생각하면서 하고, 디버깅용 코드도 들어가서 깔끔하게 하기가 곤란하게 될때가 많죠. 특히 테스팅용 코딩을 할때 그렇더라구요.
    하지만 그래도 변수 이름을 최대한 알기 쉽게 정하고, operator 앞뒤에 공백을 넣는다거나, 관련있는 일을 하는 라인들끼리는 붙이고, 아닌 것과는 한 줄 띄고 하는게, 코딩을 하는데 도움이 되고 특히나 다음번에 볼때 이해하기가 쉽더군요.
    근데 단순히 한 화면에서 볼 수있는 양이 많다는 이유로 다닥 다닥 붙이고, 몇글자 타이핑하기 귀찮다고 자기만 알게 변수를 짭게 하는 사람들이 있는데.. 어차피 요샌 auto completion도 에지간한 환경에서 다 되는데 굳이 그렇게 귀찮아 할 필요가 있나 싶어요. 제가 봤던 최악의 것은 모든 변수를 p, pp, i, ii 뭐 이런 식으로 한 loop 안에서 쓰는 것을 봤습니다. 도대체 뭐하는 코드인지 알아 볼 수가 없더군요.

    사람들이 일부러 그러는 경우도 많습니다만, 적당히 해야할 거 같습니다.
    관리자자 채용자도, 코딩을 알아보기 힘들게 해서 자기 위치를 지키려는 사람은 과감히 자르고, 어려워도 제대로 알아보기 쉽게 하는 사람을 끝까지 데리고 가주는 게 있어야 이게 고쳐질것도 같습니다.

    Reply

  3. 개발자들이 자기 습관을 잘 고칠려고 하지는 않는 것 같습니다. 아무래도 머리가 굵어져서 그런 것 같기도 하고 회사 전반적인 시스템 차원에서는 고쳐야 할 것 같은데 적당히 아는게 더 무서운 것인지 안고칠려고 하는 노력이 더 많아 보입니다.
    개인적으로는 잡아다가 혼을 내주고 싶지만 이제 나이도 있고 참아볼려고 노력중입니다….ㅎㅎㅎ

    Reply

  4. Posted by jongampark on January 30, 2009 at 8:07 PM

    WebKit이나 Mozilla같은 프로젝트를 보면 code reviewer란 사람들이 있잖아요?
    아마 이 사람들이 제출된 코드가 제대로 된건지, 전체와 잘 어울리는지를 파악하는 거 같더군요.
    항상 궁금했던게, 어떻게 Apple은 그 방대한 API를 만들면서 그렇게 깔끔하게 만드는가였는데요.. 아마 이런 전담 code reviewer가 있어서, 이미 동작하는 코드를 정리해서 특정 가이드라인에 맞는 코드로 만들어 놓는게 아닐까해요.
    이런 식으로 개발하는 회사는 지금껏 본 적은 없지만.. 뭔가 개발을 활발하게 하는 오픈 소스 프로젝트에는 꼭 이런 code reviewer가 있는 거 같더군요.

    Reply

  5. 굉장히 좋은 지적이십니다.
    문제는 그걸 알고도 하지 않는 경우가 대부분이긴 합니다.
    최근에 회사에서 좀 정신적으로 충격(?)을 받는 일이 있었습니다.
    예전부터 함께 일을 했던 동료였다가 최근들어 다시 호흡을 맞추기 시작했죠.
    과거에는 객체지향 언어나 코드 리뷰..이런 것들을 굉장히 많이 생각하고 했던 친구가 용역일을 너무 오래하다보니 그런 감이 모두 사라져서 인지…웬만한 동작 코드를 노티피케이션을 사용해서 코드를 작성했드라구요?
    처음에 보면서 이 노티가 무슨 노티인지 몰라가지고 한참 헤매다가 간신히 파악하고 웬만한 노티는 다 없애고 싱글톤 또는 일반적인 객체 메시지 타입으로 분리를 하고 최대한 모델과 뷰를 분리를 해뒀더니….엄청 귀찮다고 투덜투덜 거리더군요.
    노티는 한방에 되는데….이렇게 해뒀다고….ㅋㅋ…..
    이해를 시켜볼려고 했으나 나이도 있고 그러니 그냥 나뒀습니다. 아마 나중에라도 알기를 바랄 뿐입니다.
    결론은 되기만 하면 된다는 생각이 코드의 아름다움 추구나 재활용성 측면에서의 여러가지 고찰들을 피하는 근본적인 이유같기도 합니다.
    음…전 이제 이 나이먹어서야 어떻게 하면 좀 아름답게 짜서 나중에 좀 더 편리하게 진화를 시켜볼까 하는 생각이 많이 드는데…. 제 생각이 틀린 것 같지도 않은데…아무튼 좀 혼란스럽군요..

    Reply

Leave a Reply

Please log in using one of these methods to post your comment:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: