From d2516c6ae7982d819efd2ccc95676f2e7c0dbc10 Mon Sep 17 00:00:00 2001
From: Masatoshi Nishiguchi <7563926+mnishiguchi@users.noreply.github.com>
Date: Sun, 12 Jul 2026 15:01:27 +0900
Subject: [PATCH] refactor: centralize option and command helpers

---
 lib/open_jtalk.ex                | 51 +-------------------
 lib/open_jtalk/command.ex        | 26 ++++++++++
 lib/open_jtalk/options.ex        | 83 ++++++++++++++++++++++++++++++++
 lib/open_jtalk/synth.ex          | 25 +++-------
 test/open_jtalk/command_test.exs | 32 ++++++++++++
 test/open_jtalk/options_test.exs | 54 +++++++++++++++++++++
 test/open_jtalk/synth_test.exs   | 80 ++++++++++++------------------
 7 files changed, 235 insertions(+), 116 deletions(-)
 create mode 100644 lib/open_jtalk/command.ex
 create mode 100644 lib/open_jtalk/options.ex
 create mode 100644 test/open_jtalk/command_test.exs
 create mode 100644 test/open_jtalk/options_test.exs

diff --git a/lib/open_jtalk.ex b/lib/open_jtalk.ex
index 314b037..c3b7531 100644
--- a/lib/open_jtalk.ex
+++ b/lib/open_jtalk.ex
@@ -69,54 +69,7 @@ defmodule OpenJTalk do
   Returns the original `opts` on success.
   """
   @spec validate_options!(keyword) :: keyword
-  def validate_options!(opts) when is_list(opts) do
-    check_known_keys!(opts)
-    validate_playback_mode!(opts)
-    validate_timeout!(opts)
-    opts
-  end
-
-  defp check_known_keys!(opts) do
-    allowed = [
-      :timbre,
-      :pitch_shift,
-      :rate,
-      :gain,
-      :voice,
-      :dictionary,
-      :timeout,
-      :playback_mode,
-      :out
-    ]
-
-    unknown =
-      opts
-      |> Keyword.keys()
-      |> Enum.uniq()
-      |> Enum.reject(&(&1 in allowed))
-
-    if unknown != [] do
-      raise ArgumentError, "unknown option(s) for OpenJTalk: #{inspect(unknown)}"
-    end
-
-    :ok
-  end
-
-  defp validate_playback_mode!(opts) do
-    case Keyword.fetch(opts, :playback_mode) do
-      :error -> :ok
-      {:ok, mode} when mode in [:auto, :file, :stdin] -> :ok
-      {:ok, bad} -> raise ArgumentError, "invalid value for :playback_mode: #{inspect(bad)}"
-    end
-  end
-
-  defp validate_timeout!(opts) do
-    case Keyword.fetch(opts, :timeout) do
-      :error -> :ok
-      {:ok, t} when is_integer(t) and t >= 0 -> :ok
-      {:ok, bad} -> raise ArgumentError, "invalid value for :timeout : #{inspect(bad)}"
-    end
-  end
+  def validate_options!(opts), do: OpenJTalk.Options.validate!(opts)
 
   @doc """
   Synthesize `text` to a WAV file.
@@ -183,7 +136,7 @@ defmodule OpenJTalk do
   @spec say(binary, [say_option()]) :: :ok | {:error, term}
   def say(text, opts \\ []) do
     opts = validate_options!(opts)
-    mode = Keyword.get(opts, :playback_mode, :auto)
+    mode = OpenJTalk.Options.playback_mode(opts)
     do_say(text, mode, opts)
   end
 
diff --git a/lib/open_jtalk/command.ex b/lib/open_jtalk/command.ex
new file mode 100644
index 0000000..8e421d9
--- /dev/null
+++ b/lib/open_jtalk/command.ex
@@ -0,0 +1,26 @@
+defmodule OpenJTalk.Command do
+  @moduledoc false
+  # Small command-runner seam around MuonTrap.
+  #
+  # Tests can replace the runner with:
+  #
+  #     Application.put_env(:open_jtalk_elixir, :command_runner, MyRunner)
+  #
+  # where `MyRunner.cmd/3` has the same shape as `MuonTrap.cmd/3`.
+
+  @spec run(binary(), [binary()], keyword()) :: {binary(), non_neg_integer()}
+  def run(command, args, opts \\ [])
+      when is_binary(command) and is_list(args) and is_list(opts) do
+    command_runner().cmd(command, args, opts)
+  end
+
+  defp command_runner() do
+    Application.get_env(:open_jtalk_elixir, :command_runner, OpenJTalk.Command.MuonTrap)
+  end
+end
+
+defmodule OpenJTalk.Command.MuonTrap do
+  @moduledoc false
+
+  def cmd(command, args, opts), do: MuonTrap.cmd(command, args, opts)
+end
diff --git a/lib/open_jtalk/options.ex b/lib/open_jtalk/options.ex
new file mode 100644
index 0000000..1aa5b0a
--- /dev/null
+++ b/lib/open_jtalk/options.ex
@@ -0,0 +1,83 @@
+defmodule OpenJTalk.Options do
+  @moduledoc false
+  # Shared option validation and normalization for synthesis and playback.
+
+  @allowed_keys [
+    :timbre,
+    :pitch_shift,
+    :rate,
+    :gain,
+    :voice,
+    :dictionary,
+    :timeout,
+    :playback_mode,
+    :out
+  ]
+
+  @playback_modes [:auto, :file, :stdin]
+  @default_timeout 20_000
+
+  @doc "Validate options for synthesis and playback. Returns the original options."
+  @spec validate!(keyword()) :: keyword()
+  def validate!(opts) when is_list(opts) do
+    if Keyword.keyword?(opts) do
+      check_known_keys!(opts)
+      validate_playback_mode!(opts)
+      validate_timeout!(opts)
+      opts
+    else
+      raise ArgumentError, "OpenJTalk options must be a keyword list"
+    end
+  end
+
+  def validate!(_opts) do
+    raise ArgumentError, "OpenJTalk options must be a keyword list"
+  end
+
+  @doc "Return the requested playback mode, defaulting to `:auto`."
+  @spec playback_mode(keyword()) :: OpenJTalk.playback_mode()
+  def playback_mode(opts), do: Keyword.get(opts, :playback_mode, :auto)
+
+  @doc "Normalize a timeout value to the default when it is absent or invalid."
+  @spec normalize_timeout(term()) :: non_neg_integer()
+  def normalize_timeout(nil), do: @default_timeout
+  def normalize_timeout(value) when is_integer(value) and value >= 0, do: value
+  def normalize_timeout(_value), do: @default_timeout
+
+  @doc "Clamp a numeric value between lower and upper bounds."
+  @spec clamp(number(), number(), number()) :: number()
+  def clamp(value, lower, upper)
+      when is_number(value) and is_number(lower) and is_number(upper) do
+    value |> min(upper) |> max(lower)
+  end
+
+  defp check_known_keys!(opts) do
+    unknown =
+      opts
+      |> Keyword.keys()
+      |> Enum.uniq()
+      |> Enum.reject(&(&1 in @allowed_keys))
+
+    if unknown != [] do
+      raise ArgumentError, "unknown option(s) for OpenJTalk: #{inspect(unknown)}"
+    end
+
+    :ok
+  end
+
+  defp validate_playback_mode!(opts) do
+    case Keyword.fetch(opts, :playback_mode) do
+      :error -> :ok
+      {:ok, mode} when mode in @playback_modes -> :ok
+      {:ok, bad} -> raise ArgumentError, "invalid value for :playback_mode: #{inspect(bad)}"
+    end
+  end
+
+  defp validate_timeout!(opts) do
+    case Keyword.fetch(opts, :timeout) do
+      :error -> :ok
+      {:ok, timeout} when is_integer(timeout) and timeout >= 0 -> :ok
+      {:ok, bad} -> raise ArgumentError, "invalid value for :timeout: #{inspect(bad)}"
+    end
+  end
+end
diff --git a/lib/open_jtalk/synth.ex b/lib/open_jtalk/synth.ex
index dcc1fe4..b60a4a7 100644
--- a/lib/open_jtalk/synth.ex
+++ b/lib/open_jtalk/synth.ex
@@ -2,13 +2,12 @@ defmodule OpenJTalk.Synth do
   @moduledoc false
   # Build and run the `open_jtalk` CLI for synthesis.
 
-  alias OpenJTalk.Assets
+  alias OpenJTalk.{Assets, Command, Options}
 
   @typedoc "Use the canonical top-level synth option type."
   @type option :: OpenJTalk.synth_option()
 
   @base_alpha 0.55
-  @default_timeout 20_000
 
   @doc """
   Build the `open_jtalk` argv to synthesize into `wav_out`.
@@ -22,10 +21,10 @@ defmodule OpenJTalk.Synth do
     with {:ok, bin} <- Assets.resolve_bin(),
          {:ok, dic} <- Assets.resolve_dictionary(opts[:dictionary]),
          {:ok, voice} <- Assets.resolve_voice(opts[:voice]) do
-      alpha = clamp(@base_alpha + (opts[:timbre] || 0.0), 0.0, 1.0)
-      rate = clamp(opts[:rate] || 1.0, 0.5, 2.0)
-      fm = clamp(opts[:pitch_shift] || 0, -24, 24)
-      gain = clamp(opts[:gain] || 0, -20, 20)
+      alpha = Options.clamp(@base_alpha + (opts[:timbre] || 0.0), 0.0, 1.0)
+      rate = Options.clamp(opts[:rate] || 1.0, 0.5, 2.0)
+      fm = Options.clamp(opts[:pitch_shift] || 0, -24, 24)
+      gain = Options.clamp(opts[:gain] || 0, -20, 20)
 
       args =
         [
@@ -49,16 +48,16 @@ defmodule OpenJTalk.Synth do
   end
 
   @doc """
-  Run the `open_jtalk` command via MuonTrap.
+  Run the `open_jtalk` command via the configured command runner.
 
   Returns `{:ok, stdout}` or `{:error, {:open_jtalk_exit, status, trimmed_output}}`.
   """
   @spec run([binary], non_neg_integer() | nil) :: {:ok, binary} | {:error, term}
   def run([bin | args], timeout_ms) do
     env = [{"LC_ALL", "C"}] ++ ld_path_env()
-    timeout = normalize_timeout(timeout_ms)
+    timeout = Options.normalize_timeout(timeout_ms)
 
-    case MuonTrap.cmd(bin, args, env: env, stderr_to_stdout: true, timeout: timeout) do
+    case Command.run(bin, args, env: env, stderr_to_stdout: true, timeout: timeout) do
       {out, 0} -> {:ok, out}
       {out, status} -> {:error, {:open_jtalk_exit, status, String.trim(out)}}
     end
@@ -82,12 +81,4 @@ defmodule OpenJTalk.Synth do
       []
     end
   end
-
-  defp normalize_timeout(nil), do: @default_timeout
-  defp normalize_timeout(int) when is_integer(int) and int >= 0, do: int
-  defp normalize_timeout(_), do: @default_timeout
-
-  defp clamp(x, lo, hi) when is_number(x) and is_number(lo) and is_number(hi) do
-    x |> min(hi) |> max(lo)
-  end
 end
diff --git a/test/open_jtalk/command_test.exs b/test/open_jtalk/command_test.exs
new file mode 100644
index 0000000..ebebe72
--- /dev/null
+++ b/test/open_jtalk/command_test.exs
@@ -0,0 +1,32 @@
+defmodule OpenJTalk.CommandTest do
+  use ExUnit.Case, async: false
+
+  alias OpenJTalk.Command
+
+  setup do
+    original_runner = Application.get_env(:open_jtalk_elixir, :command_runner)
+    Application.put_env(:open_jtalk_elixir, :command_runner, __MODULE__.Runner)
+
+    on_exit(fn ->
+      if original_runner do
+        Application.put_env(:open_jtalk_elixir, :command_runner, original_runner)
+      else
+        Application.delete_env(:open_jtalk_elixir, :command_runner)
+      end
+    end)
+  end
+
+  test "run/3 delegates to configured command runner" do
+    assert {"ok", 0} = Command.run("echo", ["hello"], timeout: 123)
+
+    assert_received {:cmd, "echo", ["hello"], [timeout: 123]}
+  end
+
+  defmodule Runner do
+    def cmd(command, args, opts) do
+      send(self(), {:cmd, command, args, opts})
+
+      {"ok", 0}
+    end
+  end
+end
diff --git a/test/open_jtalk/options_test.exs b/test/open_jtalk/options_test.exs
new file mode 100644
index 0000000..a960d8e
--- /dev/null
+++ b/test/open_jtalk/options_test.exs
@@ -0,0 +1,54 @@
+defmodule OpenJTalk.OptionsTest do
+  use ExUnit.Case, async: true
+
+  alias OpenJTalk.Options
+
+  test "validate!/1 returns valid options unchanged" do
+    opts = [timbre: 0.1, pitch_shift: -2, rate: 1.2, gain: 3, playback_mode: :file]
+
+    assert Options.validate!(opts) == opts
+    assert OpenJTalk.validate_options!(opts) == opts
+  end
+
+  test "validate!/1 rejects unknown options" do
+    assert_raise ArgumentError, "unknown option(s) for OpenJTalk: [:bogus]", fn ->
+      Options.validate!(bogus: true)
+    end
+  end
+
+  test "validate!/1 rejects invalid playback mode" do
+    assert_raise ArgumentError, "invalid value for :playback_mode: :bogus", fn ->
+      Options.validate!(playback_mode: :bogus)
+    end
+  end
+
+  test "validate!/1 rejects invalid timeout" do
+    assert_raise ArgumentError, "invalid value for :timeout: -1", fn ->
+      Options.validate!(timeout: -1)
+    end
+  end
+
+  test "validate!/1 rejects non-keyword options" do
+    assert_raise ArgumentError, "OpenJTalk options must be a keyword list", fn ->
+      Options.validate!([:not_a_keyword])
+    end
+  end
+
+  test "playback_mode/1 defaults to auto" do
+    assert Options.playback_mode([]) == :auto
+    assert Options.playback_mode(playback_mode: :stdin) == :stdin
+  end
+
+  test "normalize_timeout/1 defaults absent or invalid values" do
+    assert Options.normalize_timeout(nil) == 20_000
+    assert Options.normalize_timeout(:bad) == 20_000
+    assert Options.normalize_timeout(0) == 0
+    assert Options.normalize_timeout(123) == 123
+  end
+
+  test "clamp/3 limits numeric values" do
+    assert Options.clamp(-1.0, 0.0, 1.0) == 0.0
+    assert Options.clamp(0.5, 0.0, 1.0) == 0.5
+    assert Options.clamp(2.0, 0.0, 1.0) == 1.0
+  end
+end
diff --git a/test/open_jtalk/synth_test.exs b/test/open_jtalk/synth_test.exs
index 12b2b01..e4baac7 100644
--- a/test/open_jtalk/synth_test.exs
+++ b/test/open_jtalk/synth_test.exs
@@ -1,62 +1,42 @@
 defmodule OpenJTalk.SynthTest do
-  use ExUnit.Case, async: true
+  use ExUnit.Case, async: false
 
-  defp flag_value!(argv, flag) do
-    case Enum.find_index(argv, &(&1 == flag)) do
-      nil -> flunk("missing flag #{inspect(flag)} in #{inspect(argv)}")
-      idx -> Enum.at(argv, idx + 1)
-    end
-  end
+  alias OpenJTalk.Synth
 
-  defp parse_float!(str) do
-    case Float.parse(str) do
-      {v, ""} -> v
-      _ -> flunk("not a float: #{inspect(str)}")
-    end
+  setup do
+    original_runner = Application.get_env(:open_jtalk_elixir, :command_runner)
+    Application.put_env(:open_jtalk_elixir, :command_runner, __MODULE__.Runner)
+
+    on_exit(fn ->
+      if original_runner do
+        Application.put_env(:open_jtalk_elixir, :command_runner, original_runner)
+      else
+        Application.delete_env(:open_jtalk_elixir, :command_runner)
+      end
+    end)
   end
 
-  test "args/2 clamps timbre, rate, pitch_shift, and gain" do
-    # Exaggerated inputs to force clamping on all adjustable parameters.
-    opts = [timbre: 10.0, rate: 99.0, pitch_shift: -999, gain: 999]
-    out = Path.join(System.tmp_dir!(), "ojt-args-test.wav")
-
-    assert {:ok, [_bin | argv]} = OpenJTalk.Synth.args(out, opts)
-
-    # Sanity: required flags are present
-    assert "-x" in argv
-    assert "-m" in argv
-    assert "-ow" in argv
-    assert "-a" in argv
-    assert "-r" in argv
-    assert "-g" in argv
-
-    # Output path is wired correctly
-    assert flag_value!(argv, "-ow") == out
-
-    # Exact clamped values
-    # timbre affects alpha: base 0.55 + 10.0 -> clamped to 1.0
-    assert parse_float!(flag_value!(argv, "-a")) == 1.0
-    # rate 99.0 -> clamped to 2.0
-    assert parse_float!(flag_value!(argv, "-r")) == 2.0
-    # gain 999 -> clamped to 20.0
-    assert parse_float!(flag_value!(argv, "-g")) == 20.0
-
-    # pitch_shift -999 -> clamped to -24 and included via -fm
-    fm_index = Enum.find_index(argv, &(&1 == "-fm")) || flunk("missing -fm in #{inspect(argv)}")
-    assert Enum.at(argv, fm_index + 1) == "-24"
+  test "run/2 executes open_jtalk through the command runner" do
+    assert {:ok, "ok"} = Synth.run(["/tmp/open_jtalk", "-x", "/tmp/dic"], 123)
+
+    assert_received {:cmd, "/tmp/open_jtalk", ["-x", "/tmp/dic"], opts}
+    assert Keyword.fetch!(opts, :stderr_to_stdout)
+    assert Keyword.fetch!(opts, :timeout) == 123
+    assert {"LC_ALL", "C"} in Keyword.fetch!(opts, :env)
   end
 
-  test "args/2 omits -fm when pitch_shift is exactly 0" do
-    out = Path.join(System.tmp_dir!(), "ojt-args-nofm.wav")
-    assert {:ok, [_bin | argv]} = OpenJTalk.Synth.args(out, pitch_shift: 0)
-    refute "-fm" in argv
+  test "run/2 trims failed command output" do
+    Process.put(:command_result, {"failure\n", 2})
+
+    assert {:error, {:open_jtalk_exit, 2, "failure"}} =
+             Synth.run(["/tmp/open_jtalk", "-x", "/tmp/dic"], 123)
   end
 
-  test "args/2 clamps pitch_shift upper bound to 24" do
-    out = Path.join(System.tmp_dir!(), "ojt-args-maxfm.wav")
-    assert {:ok, [_bin | argv]} = OpenJTalk.Synth.args(out, pitch_shift: 999)
+  defmodule Runner do
+    def cmd(command, args, opts) do
+      send(self(), {:cmd, command, args, opts})
 
-    i = Enum.find_index(argv, &(&1 == "-fm")) || flunk("missing -fm in #{inspect(argv)}")
-    assert Enum.at(argv, i + 1) == "24"
+      Process.get(:command_result, {"ok", 0})
+    end
   end
 end