Skip to content

Replace nested ifelse blocks#280

Open
Moohan wants to merge 3 commits intomainfrom
lint/replace_nested_ifelse
Open

Replace nested ifelse blocks#280
Moohan wants to merge 3 commits intomainfrom
lint/replace_nested_ifelse

Conversation

@Moohan
Copy link
Copy Markdown
Member

@Moohan Moohan commented Jun 16, 2025

Suggested by {lintr}

Calling ifelse() in nested calls is problematic for two main reasons:

  1. It can be hard to read – mapping the code to the expected output for such code can be a messy task/require a lot of mental bandwidth, especially for code that nests more than once
  2. It is inefficient – ifelse() can evaluate all of its arguments at both yes and no (see https://stackoverflow.com/q/16275149); this issue is exacerbated for nested calls

I think the code is significantly more readable now, however, ensuring it's functionally the same is a bit tricky, especially for the more complicated block in Demographics!

@Moohan Moohan changed the title Lint/replace nested ifelse Replace nested ifelse blocks Jun 16, 2025
@Moohan Moohan marked this pull request as ready for review June 16, 2025 15:44
@Moohan Moohan requested review from a team and cfraser2020 as code owners June 16, 2025 15:44
@Moohan Moohan requested review from CliveWG and removed request for a team June 16, 2025 15:44
@Moohan Moohan force-pushed the lint/replace_nested_ifelse branch from c056849 to 6514c7e Compare August 1, 2025 11:29
@Moohan Moohan requested a review from Copilot September 2, 2025 16:12
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR replaces nested ifelse() statements with case_when() statements to improve code readability and performance. The changes eliminate deeply nested conditional logic that was difficult to follow and potentially inefficient due to ifelse() evaluating all arguments.

  • Refactored nested ifelse() blocks in two files to use case_when() for cleaner conditional logic
  • Improved variable naming and code structure in the population projection calculations
  • Enhanced readability while maintaining the same functional behavior

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
Master RMarkdown Document & Render Code/Global Script.R Simplified number formatting function using case_when() instead of nested ifelse()
Demographics/1. Demographics - Population.R Replaced multiple nested ifelse() blocks with case_when() and refactored population projection logic

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@Moohan Moohan force-pushed the lint/replace_nested_ifelse branch from fc1a28d to 2ccad16 Compare September 2, 2025 16:23
@Moohan Moohan force-pushed the lint/replace_nested_ifelse branch from 9c52e9d to 8a1275a Compare October 2, 2025 14:57
@Moohan Moohan force-pushed the lint/replace_nested_ifelse branch from 8a1275a to 2127cf4 Compare October 14, 2025 11:18
@Moohan Moohan force-pushed the lint/replace_nested_ifelse branch 2 times, most recently from 34d8563 to a92efbf Compare November 20, 2025 10:35
@Moohan Moohan force-pushed the lint/replace_nested_ifelse branch from f83b651 to cd70731 Compare January 12, 2026 14:44
@Moohan Moohan requested a review from RyanDuffy01 as a code owner March 26, 2026 16:17
@Moohan Moohan force-pushed the lint/replace_nested_ifelse branch from 3ff02cd to affca5b Compare April 6, 2026 12:17
Moohan and others added 2 commits April 6, 2026 13:19
Also used glue to make it more readable.

This change also ensures the text is referring to the latest years available, whereas it was previously hardcoded to the 6th year (`pop_proj_dat[6, 2]`)
Style code
Update Demographics/1. Demographics - Population.R

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Update Master RMarkdown Document & Render Code/Global Script.R

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Moohan Moohan force-pushed the lint/replace_nested_ifelse branch from affca5b to 39551a8 Compare April 6, 2026 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants