open-source-parsers / open-source-parsers/jsoncpp

Why are the branches of the if statement not implemented consistently?

Open
#1,520 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug question
Dominant language
C++
Stars
8.9k
Forks
2.7k
Avg merge
31m
Merged PRs (30d)
1

Description

Dear JSONCPP developers,

I was looking at the following if statement starting on
https://github.com/open-source-parsers/jsoncpp/blob/69098a18b9af0c47549d9a271c054d13ca92b006/src/lib_json/json_writer.cpp#L734

It took me some time to realize that the rather complex code in the then branch:
https://github.com/open-source-parsers/jsoncpp/blob/69098a18b9af0c47549d9a271c054d13ca92b006/src/lib_json/json_writer.cpp#L738-L757
is equal to joining the elements in the range [0,size) where size != 0 with a separator,
and a functional equivalent, yet simpler, implementation is

      for (unsigned index = 0; index < size ; ++index) {
        const Value& childValue = value[index];
        writeCommentBeforeValue(childValue);
        if (hasChildValue)
          writeWithIndent(childValues_[index]);
        else {
          if (!indented_)
            writeIndent();
          indented_ = true;
          writeValue(childValue);
          indented_ = false;
        }
        if (index < size -1) {
           *document_ << ",";
        }
        writeCommentAfterValueOnSameLine(childValue);
      }

I assume performance (saving a comparison) is the reason for the extra complexity.

Yet, when looking at the else branch
https://github.com/open-source-parsers/jsoncpp/blob/69098a18b9af0c47549d9a271c054d13ca92b006/src/lib_json/json_writer.cpp#L764-L768
that is not optimized, like the then branch, to

      unsigned index = 0;
      for (;;) {
        *document_ << childValues_[index];
        if (++index == size)
		break;
        *document_ << ", ";
      }

So I wonder

  1. what is more important for jsoncpp code: readability or performance?
  2. Shouldn't these two branches not be implemented consistently?

Thanks in advance for your answers (and possibly code improvements)!

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the linked branches in src/lib_json/json_writer.cpp around lines 734-768 and compare their handling of child values and separators. Review the issue's readability-versus-performance questions, then establish which implementation convention the project wants; the work is done when the branches are made consistent with that decision.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.