2
\$\begingroup\$

I'm trying to do a simple implementation of a scp wrapper in Elixir.

The code is working as intended, but I got a feeling that the error conditions can be simplified. Besides error handling, I don't the best way to append the -r flag if the source_path is a directory with a cond keyword.

defmodule Scp do
  def scp(user, ip, source_path, destination_path, timeout \\ 30) do
    scp_flags = [
      "-oStrictHostKeyChecking=no",
      "-oConnectTimeout=#{timeout}",
      "-oPasswordAuthentication=no",
      source_path,
      "#{user}@#{ip}:#{destination_path}"
    ]

    scp_flags =
      case File.dir?(source_path) do
        true -> ["-r" | scp_flags]
        _ -> scp_flags
      end

    if File.exists?(source_path) do
      case System.cmd("scp", scp_flags, stderr_to_stdout: true) do
        {resp, 0} ->
          {:ok, resp, 0}

        {resp, exit_code} ->
          cond do
            String.contains?(resp, "Permission denied") ->
              {:error, :permdenied, exit_code}

            String.contains?(resp, "Connection timed out") ->
              {:error, :conntimeout, exit_code}

            true ->
              {:error, :errunkown, exit_code}
          end
      end
    else
      {:error, :enoent, -1}
    end
  end
end
New contributor
Rodrigo Ce Moretto is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$
2
\$\begingroup\$

I have never used Elixir in my life until seeing this question, so my feedback is very limited and I probably shouldn't be putting an answer on here. However, there are a few ways it could be simplified:

  • the module lstat(path, opts \\ []) reads information about a path, returns a FileStat struct which has a type: field which can be type: :device | :directory | :regular | :other | :symlink; this could be used to determine what the path is (and whether to use -r), and if there is an error it means that the path does not exist (thus adding in the error handling)

  • in the case {resp, exit_code} -> instead of checking the error message contents via String.contains?, do the switching based on exit code and then return the appropriate error messages.

  • in the case {resp, exit_code} ->, I'd add a few extra error-handlers for statuses (for example directory not found on destination, invalid credentials, etc.)

  • for the inputs (user and ip address), a regex could be used to ensure sanity (the ip address looks like an ip, the username is not blank, etc.)

  • the case true -> {:error, :errunkown, exit_code} could be replaced with _ -> {:error, :errunkown, exit_code} as _ appears to be a fall through (I'm unsure about this)

\$\endgroup\$

Your Answer

Rodrigo Ce Moretto is a new contributor. Be nice, and check out our Code of Conduct.

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.