-1
\$\begingroup\$

The project uses functions to part by part create an ElasticSearch query. In this case, the final query would look like this:

{
    "_source": true,
    "from": 0,
    "size": 0,
    "sort": [],
    "aggs": {
        "body": {
            "nested": {
                "path": "preparers"
            },
            "aggs": {
                "body": {
                    "terms": {
                        "field": "preparers.employee.id",
                        "size": 1000,
                        "order": {
                            "_count": "desc"
                        },
                        "include": "-1097",
                        "min_doc_count": 0
                    }
                }
            }
        }
    }
}

Here is the function currently used to add the terms part of the query:

        public void AddTerms(string field, int size, int minDocCount = 1, string orderBy = "_count", string order = "desc", string missing = "")
        {
            var str = new StringBuilder();
            if (missing.Length > 0)
                str.Append($"\"terms\": {{ \"field\": \"{field}\", \"size\": {size}, \"order\": {{ \"{orderBy}\": \"{order}\" }}, \"missing\": \"{missing}\", \"min_doc_count\": {minDocCount} }}");
            else
                str.Append($"\"terms\": {{ \"field\": \"{field}\", \"size\": {size}, \"order\": {{ \"{orderBy}\": \"{order}\" }},  \"min_doc_count\": {minDocCount} }}");

            _aggregation_body = str.ToString();
        }

Currently this function does not add the include property to the terms object, so I need to add it. So my first idea was to add else if like so:

        public void AddTerms(string field, int size, int minDocCount = 1, string orderBy = "_count", string order = "desc", string missing = "", string include = "")
        {
            var str = new StringBuilder();
            if (missing.Length > 0)
                str.Append($"\"terms\": {{ \"field\": \"{field}\", \"size\": {size}, \"order\": {{ \"{orderBy}\": \"{order}\" }}, \"missing\": \"{missing}\", \"min_doc_count\": {minDocCount} }}");
            else if (include.Length > 0)
                str.Append($"\"terms\": {{ \"field\": \"{field}\", \"size\": {size}, \"order\": {{ \"{orderBy}\": \"{order}\" }}, \"include\": \"{include}\",  \"min_doc_count\": {minDocCount} }}");
            else
                str.Append($"\"terms\": {{ \"field\": \"{field}\", \"size\": {size}, \"order\": {{ \"{orderBy}\": \"{order}\" }},  \"min_doc_count\": {minDocCount} }}");

            _aggregation_body = str.ToString();
        }

What worries me is that include will be necessary very rarely. It will be contained in maybe 1 out of each 100,000 searches (and each search will have many terms properties). So that else if will almost always evaluate to false.

So is there perhaps a better way how to add the include to the query? Or is this not a big deal?

I thought of maybe calling the existing AddTerms(), then trimming off the last bracket and adding include, but that can cause issues if the returned string is somehow changed (although this seems unlikely).

\$\endgroup\$
6
  • \$\begingroup\$ The title should describe, what your code is doing. Hope I understood it right. \$\endgroup\$
    – convert
    Feb 26 at 17:39
  • 3
    \$\begingroup\$ Welcome to the Code Review Community. We attempt to help you improve your code by making insightful observations about the code. We can't really tell which version of the code is better without seeing more of the code. In this case it would be very helpful if we could see the entire class including the using statements at the top. Our guidelines are different from Stack Overflow, please read A guide to Code Review for Stack Overflow users \$\endgroup\$
    – pacmaninbw
    Feb 26 at 17:56
  • \$\begingroup\$ @convert There is a lot more wrong with this question than just the title. I would not have approved the edit. \$\endgroup\$
    – pacmaninbw
    Feb 26 at 17:57
  • 1
    \$\begingroup\$ using Json.Net or System.Text.Json would safe you the hassle. \$\endgroup\$
    – iSR5
    Feb 27 at 6:34
  • 1
    \$\begingroup\$ Is there any particular constraint why do you want to solve this problem via string manipulation? \$\endgroup\$ Feb 27 at 8:04

0

Browse other questions tagged or ask your own question.