Skip to content

Refactor JSON serialization for distributions#1276

Closed
ayushk687 wants to merge 2 commits intoNOAA-FIMS:devfrom
ayushk687:patch-1
Closed

Refactor JSON serialization for distributions#1276
ayushk687 wants to merge 2 commits intoNOAA-FIMS:devfrom
ayushk687:patch-1

Conversation

@ayushk687
Copy link
Copy Markdown

@ayushk687 ayushk687 commented Mar 7, 2026

🚀 refactor: remove duplication in distribution JSON serialization

closes #1260

📌 Summary

Refactors duplicated JSON serialization logic for normal and log_normal distributions into a single reusable helper function.

This reduces maintenance overhead and improves consistency while keeping behavior unchanged.


🧩 Problem

Both distributions implemented nearly identical JSON serialization logic, differing only in "module_type".
This duplication:

  • Increased maintenance cost
  • Made extensions error-prone
  • Violated DRY principle

✅ Solution

  • Introduced helper function: build_density_json(...)
  • Parameterized "module_type"
  • Replaced duplicated blocks with helper calls

🔧 Implementation

inline std::string build_density_json(
    const std::string& module_type,
    const std::vector<double>& params
) {
    rapidjson::Document doc;
    doc.SetObject();
    auto& alloc = doc.GetAllocator();

    doc.AddMember("module_type", rapidjson::Value(module_type.c_str(), alloc), alloc);

    rapidjson::Value param_array(rapidjson::kArrayType);
    for (double p : params) {
        param_array.PushBack(p, alloc);
    }

    doc.AddMember("parameters", param_array, alloc);

    rapidjson::StringBuffer buffer;
    rapidjson::Writer<rapidjson::StringBuffer> writer(buffer);
    doc.Accept(writer);

    return buffer.GetString();
}
Usage
```return build_density_json("normal", params);```

Files Changed

`FIMS/inst/include/interface/rcpp/rcpp_objects/rcpp_distribution.hpp`


Testing

Verified JSON output remains unchanged

No API or behavior changes

🔍 Improvements

Removes duplication (DRY)

Improves readability and modularity

Simplifies adding new distributions

Reduces maintenance overhead

⚠️ Compatibility

No breaking changes

Output format unchanged

Refactored duplicated JSON serialization logic for normal and log_normal distributions into a reusable helper function to reduce maintenance burden.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 7, 2026

Thank you for contributing to FIMS and opening your first PR here! We are happy to have your contributions. Please ensure that the PR is made to the dev branch and let us know if you need any help! Also, we encourage you to introduce yourself to the community on the introduction thread in our Discussions.

Refactored the JSON serialization logic to eliminate duplication for normal and log_normal distributions.
@kellijohnson-NOAA
Copy link
Copy Markdown
Contributor

@ayushk687 thank you for opening this PR. Unfortunately, the PR needs to be made to dev and not to main. Can you please rebase your branch to the dev branch and I can change the PR to main on my end. Then I can review it properly. Thank you and please let me know if you need any guidance rebasing.

@kellijohnson-NOAA kellijohnson-NOAA self-requested a review March 10, 2026 04:17
@kellijohnson-NOAA kellijohnson-NOAA changed the base branch from main to dev March 10, 2026 04:17
@kellijohnson-NOAA
Copy link
Copy Markdown
Contributor

I am planning on reviewing this tomorrow but I do not see the changes? There is one file that was added with information about the PR but I cannot see any changes to the code in the files.

@ayushk687
Copy link
Copy Markdown
Author

@kellijohnson-NOAA yaa,sure will soon change the pr with nessary changes
thanks for the guidance

@kellijohnson-NOAA
Copy link
Copy Markdown
Contributor

@ayushk687 do you plan on revamping this PR?

@ayushk687
Copy link
Copy Markdown
Author

@kellijohnson-NOAA mam,have made changes in pr ,plese allow to draft propasal for projects

@kellijohnson-NOAA
Copy link
Copy Markdown
Contributor

I do not see any changes for this PR in the code, instead I see the following which makes me confused
image

@kellijohnson-NOAA
Copy link
Copy Markdown
Contributor

@ayushk687 thank you for your interest in FIMS. I am going to close this Pull Request because it has been open for two weeks and I still haven't seen any progress on it. If you would like to continue to help with the FIMS project, which we welcome, I suggest that you work through a much smaller issue to show that you understand how git and GitHub work. We are happy to help with learning this but both this Pull Request and #1308 indicate to me that working through some GitHub tutorials before opening additional Pull Requests in open source repositories could be helpful. Thank you, Kelli

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.

[Developer Issue]: Remedy duplicated code for writing json of distributions

2 participants