refactor: merge pr #173 from coderplanets/clean-code

Clean code

Author: mydearxym

lib/groupher_server/accounts/delegates/fans.ex

@@ -40,8 +40,8 @@ defmodule GroupherServer.Accounts.Delegate.Fans do
       false ->
         {:error, [message: "can't follow yourself", code: ecode(:self_conflict)]}
 
-      {:error, error} ->
-        {:error, [message: error, code: ecode(:not_exsit)]}
+      {:error, reason} ->
+        {:error, [message: reason, code: ecode(:not_exsit)]}
     end
   end
 
@@ -89,8 +89,8 @@ defmodule GroupherServer.Accounts.Delegate.Fans do
       false ->
         {:error, [message: "can't undo follow yourself", code: ecode(:self_conflict)]}
 
-      {:error, error} ->
-        {:error, [message: error, code: ecode(:not_exsit)]}
+      {:error, reason} ->
+        {:error, [message: reason, code: ecode(:not_exsit)]}
     end
   end
 

lib/groupher_server/accounts/delegates/favorite_category.ex

@@ -149,7 +149,7 @@ defmodule GroupherServer.Accounts.Delegate.FavoriteCategory do
           {:error, _} ->
             case CMS.reaction(thread, :favorite, content_id, user) do
               {:ok, _} -> find_content_favorite(thread, content_id, user.id)
-              {:error, error} -> {:error, error}
+              {:error, reason} -> {:error, reason}
             end
         end
       end)
@@ -160,7 +160,7 @@ defmodule GroupherServer.Accounts.Delegate.FavoriteCategory do
           |> ORM.update(%{total_count: max(old_category.total_count - 1, 0)})
         else
           true -> {:ok, ""}
-          error -> {:error, error}
+          reason -> {:error, reason}
         end
       end)
       |> Multi.run(:update_content_category_id, fn _, %{favorite_content: content_favorite} ->

lib/groupher_server/cms/cms.ex

@@ -84,7 +84,7 @@ defmodule GroupherServer.CMS do
   defdelegate favorited_category(thread, content_id, user), to: FavoritedContents
   # ArticleOperation
   # >> set flag on article, like: pin / unpin article
-  defdelegate set_community_flags(queryable, community_id, attrs), to: ArticleOperation
+  defdelegate set_community_flags(community_info, queryable, attrs), to: ArticleOperation
   defdelegate pin_content(queryable, community_id, topic), to: ArticleOperation
   defdelegate undo_pin_content(queryable, community_id, topic), to: ArticleOperation
   defdelegate pin_content(queryable, community_id), to: ArticleOperation

lib/groupher_server/cms/delegates/article_curd.ex

@@ -63,60 +63,32 @@ defmodule GroupherServer.CMS.Delegate.ArticleCURD do
   {:error, %Ecto.Changeset{}}
 
   """
-  def create_content(
-        %Community{id: community_id},
-        thread,
-        attrs,
-        %User{id: user_id}
-      ) do
-    with {:ok, author} <- ensure_author_exists(%User{id: user_id}),
+  def create_content(%Community{id: cid}, thread, attrs, %User{id: uid}) do
+    with {:ok, author} <- ensure_author_exists(%User{id: uid}),
          {:ok, action} <- match_action(thread, :community),
-         {:ok, community} <- ORM.find(Community, community_id) do
+         {:ok, community} <- ORM.find(Community, cid) do
       Multi.new()
       |> Multi.run(:create_content, fn _, _ ->
-        action.target
-        |> struct()
-        |> action.target.changeset(attrs)
-        |> Ecto.Changeset.put_change(:author_id, author.id)
-        |> Ecto.Changeset.put_change(:origial_community_id, integerfy(community_id))
-        |> Repo.insert()
+        exec_create_content(action.target, attrs, author, community)
       end)
       |> Multi.run(:set_community, fn _, %{create_content: content} ->
         ArticleOperation.set_community(community, thread, content.id)
       end)
       |> Multi.run(:set_topic, fn _, %{create_content: content} ->
-        topic_title =
-          case attrs |> Map.has_key?(:topic) do
-            true -> attrs.topic
-            false -> "posts"
-          end
-
-        ArticleOperation.set_topic(%Topic{title: topic_title}, thread, content.id)
+        exec_set_topic(thread, content.id, attrs)
       end)
       |> Multi.run(:set_community_flag, fn _, %{create_content: content} ->
-        # TODO: remove this judge, as content should have a flag
-        case action |> Map.has_key?(:flag) do
-          true ->
-            ArticleOperation.set_community_flags(content, community.id, %{
-              trash: false
-            })
-
-          false ->
-            {:ok, :pass}
-        end
+        exec_set_community_flag(community, content, action)
       end)
       |> Multi.run(:set_tag, fn _, %{create_content: content} ->
-        case attrs |> Map.has_key?(:tags) do
-          true -> set_tags(thread, content.id, attrs.tags)
-          false -> {:ok, :pass}
-        end
+        exec_set_tag(thread, content.id, attrs)
       end)
       |> Multi.run(:mention_users, fn _, %{create_content: content} ->
-        Delivery.mention_from_content(community.raw, thread, content, attrs, %User{id: user_id})
+        Delivery.mention_from_content(community.raw, thread, content, attrs, %User{id: uid})
         {:ok, :pass}
       end)
       |> Multi.run(:log_action, fn _, _ ->
-        Statistics.log_publish_action(%User{id: user_id})
+        Statistics.log_publish_action(%User{id: uid})
       end)
       |> Repo.transaction()
       |> create_content_result()
@@ -132,7 +104,7 @@ defmodule GroupherServer.CMS.Delegate.ArticleCURD do
       ORM.update(content, args)
     end)
     |> Multi.run(:update_tag, fn _, _ ->
-      update_tags(content, args.tags)
+      exec_update_tags(content, args.tags)
     end)
     |> Repo.transaction()
     |> update_content_result()
@@ -368,37 +340,33 @@ defmodule GroupherServer.CMS.Delegate.ArticleCURD do
 
   defp should_add_pin?(_filter), do: {:error, :pass}
 
+  defp concat_contents(%{total_count: 0}, normal_contents), do: {:ok, normal_contents}
+
   defp concat_contents(pined_content, normal_contents) do
-    case pined_content |> Map.get(:total_count) do
-      0 ->
-        {:ok, normal_contents}
-
-      _ ->
-        pind_entries =
-          pined_content
-          |> Map.get(:entries)
-          |> Enum.map(&struct(&1, %{pin: true}))
-
-        normal_entries = normal_contents |> Map.get(:entries)
-
-        # pind_count = pined_content |> Map.get(:total_count)
-        normal_count = normal_contents |> Map.get(:total_count)
-
-        # remote the pined content from normal_entries (if have)
-        pind_ids = pick_by(pind_entries, :id)
-        normal_entries = Enum.reject(normal_entries, &(&1.id in pind_ids))
-
-        normal_contents
-        |> Map.put(:entries, pind_entries ++ normal_entries)
-        # those two are equals
-        # |> Map.put(:total_count, pind_count + normal_count - pind_count)
-        |> Map.put(:total_count, normal_count)
-        |> done
-    end
+    pind_entries =
+      pined_content
+      |> Map.get(:entries)
+      |> Enum.map(&struct(&1, %{pin: true}))
+
+    normal_entries = normal_contents |> Map.get(:entries)
+
+    # pind_count = pined_content |> Map.get(:total_count)
+    normal_count = normal_contents |> Map.get(:total_count)
+
+    # remote the pined content from normal_entries (if have)
+    pind_ids = pick_by(pind_entries, :id)
+    normal_entries = Enum.reject(normal_entries, &(&1.id in pind_ids))
+
+    normal_contents
+    |> Map.put(:entries, pind_entries ++ normal_entries)
+    # those two are equals
+    # |> Map.put(:total_count, pind_count + normal_count - pind_count)
+    |> Map.put(:total_count, normal_count)
+    |> done
   end
 
   defp create_content_result({:ok, %{create_content: result}}) do
-    Later.exec({__MODULE__, :nofify_admin_new_content, [result]})
+    Later.exec({__MODULE__, :notify_admin_new_content, [result]})
     {:ok, result}
   end
 
@@ -430,10 +398,41 @@ defmodule GroupherServer.CMS.Delegate.ArticleCURD do
     {:error, [message: "log action", code: ecode(:create_fails)]}
   end
 
-  defp set_tags(thread, content_id, tags) do
+  # except Job, other content will just pass, should use set_tag function instead
+  # defp exec_update_tags(_, _tags_ids), do: {:ok, :pass}
+
+  defp update_content_result({:ok, %{update_content: result}}), do: {:ok, result}
+  defp update_content_result({:error, :update_content, result, _steps}), do: {:error, result}
+  defp update_content_result({:error, :update_tag, result, _steps}), do: {:error, result}
+
+  defp content_id(:post, id), do: %{post_id: id}
+  defp content_id(:job, id), do: %{job_id: id}
+  defp content_id(:repo, id), do: %{repo_id: id}
+  defp content_id(:video, id), do: %{video_id: id}
+
+  #  for create content step in Multi.new
+  defp exec_create_content(target, attrs, %Author{id: aid}, %Community{id: cid}) do
+    target
+    |> struct()
+    |> target.changeset(attrs)
+    |> Ecto.Changeset.put_change(:author_id, aid)
+    |> Ecto.Changeset.put_change(:origial_community_id, integerfy(cid))
+    |> Repo.insert()
+  end
+
+  defp exec_set_topic(thread, id, %{topic: topic}) do
+    ArticleOperation.set_topic(%Topic{title: topic}, thread, id)
+  end
+
+  # if topic is not provide, use posts as default
+  defp exec_set_topic(thread, id, _attrs) do
+    ArticleOperation.set_topic(%Topic{title: "posts"}, thread, id)
+  end
+
+  defp exec_set_tag(thread, id, %{tags: tags}) do
     try do
       Enum.each(tags, fn tag ->
-        {:ok, _} = ArticleOperation.set_tag(thread, %Tag{id: tag.id}, content_id)
+        {:ok, _} = ArticleOperation.set_tag(thread, %Tag{id: tag.id}, id)
       end)
 
       {:ok, "psss"}
@@ -442,62 +441,44 @@ defmodule GroupherServer.CMS.Delegate.ArticleCURD do
     end
   end
 
-  defp update_tags(_content, tags_ids) when length(tags_ids) == 0, do: {:ok, :pass}
+  defp exec_set_tag(_thread, _id, _attrs), do: {:ok, :pass}
 
-  # Job is special, the tags in job only represent city, so everytime update
-  # tags on job content, should be override the old ones, in this way, every
-  # communiies contains this job will have the same city info
-  defp update_tags(%CMS.Job{} = content, tags_ids) do
-    with {:ok, content} <- ORM.find(CMS.Job, content.id, preload: :tags) do
-      concat_tags(content, tags_ids)
-    end
+  # TODO:  flag 逻辑似乎有问题
+  defp exec_set_community_flag(%Community{} = community, content, %{flag: _flag}) do
+    ArticleOperation.set_community_flags(community, content, %{
+      trash: false
+    })
   end
 
-  defp update_tags(%CMS.Post{} = content, tags_ids) do
-    with {:ok, content} <- ORM.find(CMS.Post, content.id, preload: :tags) do
-      concat_tags(content, tags_ids)
-    end
+  defp exec_set_community_flag(_community, _content, _action) do
+    {:ok, :pass}
   end
 
-  defp update_tags(%CMS.Video{} = content, tags_ids) do
-    with {:ok, content} <- ORM.find(CMS.Video, content.id, preload: :tags) do
-      concat_tags(content, tags_ids)
-    end
-  end
-
-  # except Job, other content will just pass, should use set_tag function instead
-  defp update_tags(_, _tags_ids), do: {:ok, :pass}
+  defp exec_update_tags(_content, tags_ids) when length(tags_ids) == 0, do: {:ok, :pass}
 
-  defp concat_tags(content, tags_ids) do
-    tags =
-      Enum.reduce(tags_ids, [], fn t, acc ->
-        {:ok, tag} = ORM.find(Tag, t.id)
+  defp exec_update_tags(content, tags_ids) do
+    with {:ok, content} <- ORM.find(content.__struct__, content.id, preload: :tags) do
+      tags =
+        Enum.reduce(tags_ids, [], fn t, acc ->
+          {:ok, tag} = ORM.find(Tag, t.id)
 
-        case tag.title == "refined" do
-          true ->
-            acc
+          case tag.title == "refined" do
+            true ->
+              acc
 
-          false ->
-            acc ++ [tag]
-        end
-      end)
+            false ->
+              acc ++ [tag]
+          end
+        end)
 
-    content
-    |> Ecto.Changeset.change()
-    |> Ecto.Changeset.put_assoc(:tags, tags)
-    |> Repo.update()
+      content
+      |> Ecto.Changeset.change()
+      |> Ecto.Changeset.put_assoc(:tags, tags)
+      |> Repo.update()
+    end
   end
 
-  defp update_content_result({:ok, %{update_content: result}}), do: {:ok, result}
-  defp update_content_result({:error, :update_content, result, _steps}), do: {:error, result}
-  defp update_content_result({:error, :update_tag, result, _steps}), do: {:error, result}
-
-  defp content_id(:post, id), do: %{post_id: id}
-  defp content_id(:job, id), do: %{job_id: id}
-  defp content_id(:repo, id), do: %{repo_id: id}
-  defp content_id(:video, id), do: %{video_id: id}
-
-  defp nofify_admin_new_content(%{id: id} = result) do
+  defp notify_admin_new_content(%{id: id} = result) do
     target = result.__struct__
     preload = [:origial_community, author: :user]
 

lib/groupher_server/cms/delegates/article_operation.ex

@@ -97,7 +97,14 @@ defmodule GroupherServer.CMS.Delegate.ArticleOperation do
   @doc """
   trash / untrash articles
   """
-  def set_community_flags(content, community_id, attrs) do
+  def set_community_flags(%Community{id: cid}, content, attrs) do
+    with {:ok, content} <- ORM.find(content.__struct__, content.id),
+         {:ok, record} <- insert_flag_record(content, cid, attrs) do
+      {:ok, struct(content, %{trash: record.trash})}
+    end
+  end
+
+  def set_community_flags(community_id, content, attrs) do
     with {:ok, content} <- ORM.find(content.__struct__, content.id),
          {:ok, community} <- ORM.find(Community, community_id),
          {:ok, record} <- insert_flag_record(content, community.id, attrs) do

lib/groupher_server/cms/delegates/comment_curd.ex

@@ -92,15 +92,7 @@ defmodule GroupherServer.CMS.Delegate.CommentCURD do
         ORM.delete(comment)
       end)
       |> Multi.run(:update_floor, fn _, _ ->
-        Repo.update_all(
-          from(p in action.reactor, where: p.id > ^comment.id),
-          inc: [floor: -1]
-        )
-        |> done()
-        |> case do
-          {:ok, _} -> {:ok, comment}
-          {:error, _} -> {:error, ""}
-        end
+        exec_update_floor(action.reactor, comment)
       end)
       |> Repo.transaction()
       |> delete_comment_result()
@@ -185,6 +177,18 @@ defmodule GroupherServer.CMS.Delegate.CommentCURD do
     {:error, [message: "update follor fails", code: ecode(:delete_fails)]}
   end
 
+  defp exec_update_floor(queryable, comment) do
+    Repo.update_all(
+      from(p in queryable, where: p.id > ^comment.id),
+      inc: [floor: -1]
+    )
+    |> done()
+    |> case do
+      {:ok, _} -> {:ok, comment}
+      {:error, _} -> {:error, ""}
+    end
+  end
+
   # simulate a join connection
   # TODO: use Multi task to refactor
   # TODO: refactor boilerplate code

lib/groupher_server/cms/delegates/community_curd.ex

@@ -174,8 +174,8 @@ defmodule GroupherServer.CMS.Delegate.CommunityCURD do
       {:ok, ret} ->
         {:ok, ret}
 
-      {:error, error} ->
-        {:error, error}
+      {:error, reason} ->
+        {:error, reason}
     end
   end
 

lib/groupher_server_web/resolvers/cms_resolver.ex

@@ -167,9 +167,9 @@ defmodule GroupherServerWeb.Resolvers.CMS do
 
   defp set_community_flags(community_id, thread, id, flag) do
     with {:ok, content} <- match_action(thread, :self) do
-      content.target
-      |> struct(%{id: id})
-      |> CMS.set_community_flags(community_id, flag)
+      queryable = content.target |> struct(%{id: id})
+
+      CMS.set_community_flags(community_id, queryable, flag)
     end
   end