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

refactor(app): Making impossible states impossible #265

Merged
merged 10 commits into from
Jun 8, 2024
Merged

refactor(app): Making impossible states impossible #265

merged 10 commits into from
Jun 8, 2024

Conversation

kyu08
Copy link
Owner

@kyu08 kyu08 commented May 21, 2024

Motivation

What's changed

Made impossible states impossible.

At first, I show main change point.

Before

pub struct Model<'a> {
    pub app_state: AppState,
    pub current_pane: CurrentPane,
    pub makefile: Makefile,
    pub search_text_area: TextArea_<'a>,
    pub targets_list_state: ListState,
    pub histories: Option<Histories>, // When home dir could not be found, this is None.
    pub histories_list_state: ListState,
}

pub enum AppState {
    SelectingTarget,
    ExecuteTarget(Option<String>),
    ShouldQuit,
}

After

pub struct Model<'a> {
    pub app_state: AppState<'a>,
}

pub enum AppState<'a> {
    SelectTarget(SelectTargetState<'a>),
    ExecuteTarget(Option<String>),
    ShouldQuit,
}

pub struct SelectTargetState<'a> {
    pub current_pane: CurrentPane,
    pub makefile: Makefile,
    pub search_text_area: TextArea_<'a>,
    pub targets_list_state: ListState,
    pub histories: Option<Histories>,
    pub histories_list_state: ListState,
}

By this change, we can no longer express impossible states like below.

Model {
    app_state: AppState::ShouldQuit,
    current_pane: CurrentPane::Main,
    makefile: Makefile::new_for_test(),
    targets_list_state: ListState::with_selected(ListState::default(), Some(0)),
    search_text_area: TextArea_(TextArea::default()),
    histories: init_histories(vec!["history0".to_string(), "history1".to_string()]),
    histories_list_state: ListState::with_selected(ListState::default(), Some(0)),
}

When app_state is AppState::ShouldQuit, the app need not have any data.

The above code become like this:

Model {
    app_state: AppState::ShouldQuit,
}

Good effect of this change

This makes some good effect:

1. Improve readability

It made readability improved because the data structure describes the application state more precisely.

This may good especially for the person who are new to this project too.

2. Reduce the chance of potential bugs because invalid data accesses can't happen.

As the title says.

3. Remove unnecessary fields.

This is not huge advantage for now, but the effort for testing will not increase even though the data structure grows.

// Before
Case {
    title: "Quit",
    model: init_model(),
    message: Some(Message::Quit),
    expect_model: Model {
        app_state: AppState::ShouldQuit,
        ..init_model() // this is unnecessary field
    },
},

// After
Case {
    title: "Quit",
    model: Model {
        app_state: AppState::SelectTarget(SelectTargetState {
            ..SelectTargetState::new_for_test()
        }),
    },
    message: Some(Message::Quit),
    expect_model: Model {
        app_state: AppState::ShouldQuit,
    },
},

Also, this makes refactoring easier because only necessary fields are defined.

Minor changes

  • Improve cohesiveness by extract logics to method.
  • Improve ordering of type definition and thier methods in app.rs.

@kyu08 kyu08 force-pushed the fix-262 branch 4 times, most recently from dfbae66 to b0abcc6 Compare May 22, 2024 04:51
@kyu08 kyu08 force-pushed the fix-262 branch 8 times, most recently from 0ca7d3a to ea189bc Compare June 8, 2024 03:23
@kyu08 kyu08 force-pushed the fix-262 branch 6 times, most recently from 07718c1 to c7d4677 Compare June 8, 2024 03:55
@kyu08 kyu08 force-pushed the fix-262 branch 2 times, most recently from eac260a to dc89832 Compare June 8, 2024 08:06
@kyu08 kyu08 marked this pull request as ready for review June 8, 2024 10:18
@kyu08 kyu08 changed the title [wip] refactor(app): Making impossible states impossible refactor(app): Making impossible states impossible Jun 8, 2024
@kyu08 kyu08 linked an issue Jun 8, 2024 that may be closed by this pull request
@kyu08 kyu08 merged commit 684f7ef into main Jun 8, 2024
5 checks passed
@kyu08 kyu08 deleted the fix-262 branch June 8, 2024 10:33
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.

[Refactor] Making impossible states impossible
1 participant