Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLEANUP: Fix rest_ntokens calculation on bop mget/smget #724

Merged
merged 1 commit into from
Dec 15, 2023

Conversation

namsic
Copy link
Collaborator

@namsic namsic commented Dec 14, 2023

bop mget/smget 명령의 ..._ntokens 관련 변수를 의미에 맞게 정리합니다.

  • read_ntokens: 앞부분 처리된 token 개수 (== 다음에 처리해야 할 token idx)
  • post_ntokens: 뒷부분 처리된 token 개수
  • rest_ntokens: 아직 처리되지 않은 token 개수

Copy link
Collaborator

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 완료

} else {
print_invalid_command(c, tokens, ntokens);
out_string(c, "CLIENT_ERROR bad command line format");
return;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

직관적으로는 아래 코드가 읽기가 좋습니다.

if (rest_ntokens == 1) {
    if (! safe_strtoul(tokens[read_ntokens].value, &count)) {
        . . .
    }
} else if (rest_ntokens == 2) {
    if ((! safe_strtoul(tokens[read_ntokens].value, &offset)) ||
        (! safe_strtoul(tokens[read_ntokens+1].value, &count))) {
        . . .
    }
} else {
    . . .
}

중복 코드를 제거한다면 아래와 같이 구현할 수 있습니다.
코드 읽기에 문제가 없다면, 아래 코드도 괜찮다고 봅니다.

if ((rest_ntokens > 2) ||
    (rest_ntokens > 1 && !safe_strtoul(tokens[read_ntokens++].value, &offset) ||
    (rest_ntokens > 0 && !safe_strtoul(tokens[read_ntokens++].value, &count)) {
    print_invalid_command(c, tokens, ntokens);
    out_string(c, "CLIENT_ERROR bad command line format");
    return;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 아래 코드가 간결해서 좋은 것 같습니다.

중복 코드를 제거한다면 아래와 같이 구현할 수 있습니다. 코드 읽기에 문제가 없다면, 아래 코드도 괜찮다고 봅니다.

if ((rest_ntokens > 2) ||
    (rest_ntokens > 1 && !safe_strtoul(tokens[read_ntokens++].value, &offset)) ||
    (rest_ntokens > 0 && !safe_strtoul(tokens[read_ntokens++].value, &count))) {
    print_invalid_command(c, tokens, ntokens);
    out_string(c, "CLIENT_ERROR bad command line format");
    return;
}

그런데 언뜻 보기에는 rest_ntokens == 0일 때 에러 처리가 없는 것처럼 보일 수 있을 것 같은데,
별도로 코멘트나 assert문을 넣지 않아도 이해하기에 충분할까요?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그러면 if 문 조건에서 (rest_ntokens > 2) || (rest_ntokens <= 0) || 와 같이 조건을 추가하는 것이 좋겠습니다.

@jhpark816 jhpark816 merged commit 7b9e56b into naver:develop Dec 15, 2023
1 check passed
@namsic namsic deleted the bop_mget branch December 15, 2023 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants