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

Embed 메세지 발송 #41

Closed
wants to merge 3 commits into from
Closed

Embed 메세지 발송 #41

wants to merge 3 commits into from

Conversation

inc5025
Copy link
Contributor

@inc5025 inc5025 commented Jul 3, 2021

resolves #40

수정 내역

  • command의 execute의 리턴 타입을 새로운 응답 타입(ResponseMessage)으로 수정하였습니다.
  • embed message 생성을 위한 util을 추가하였습니다.
  • !help 커멘드에 대해서 Embed 메세지를 적용하였습니다.

@inc5025 inc5025 self-assigned this Jul 3, 2021
Copy link
Member

@hellodhlyn hellodhlyn left a comment

Choose a reason for hiding this comment

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

utils.Embed 의 역할은 responses.EmbedMessage 에서 처리해도 괜찮을 것 같습니다

bot.go Outdated Show resolved Hide resolved
bot.go Outdated Show resolved Hide resolved
commands/delivery.go Outdated Show resolved Hide resolved
Copy link
Contributor

@zych1751 zych1751 left a comment

Choose a reason for hiding this comment

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

Embed 도입은 좋아보이네요 ㅎ_ㅎ

func (e *Embed) SetColor(clr int) *Embed {
e.Color = clr
return e
}
Copy link
Contributor

Choose a reason for hiding this comment

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

endline이 없어요.


func NewTextMessage(msg string) *TextMessage {
return &TextMessage{message: msg}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

여기두

@@ -3,6 +3,7 @@ package commands
import (
"context"
"fmt"
"github.com/webdonalds/discord-bot/responses"
Copy link
Contributor

Choose a reason for hiding this comment

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

아래로 내려가야 할 것 같네요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

한 번에 다 확인했어야 했는데, 생각 못 한 부분이 좀 있었네요. 수정하겠습니다.

@@ -3,6 +3,7 @@ package commands
import (
"encoding/json"
"fmt"
"github.com/webdonalds/discord-bot/responses"
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도

Comment on lines +7 to +21
type EmbedMessage struct {
embed *discordgo.MessageEmbed
}

func (em *EmbedMessage) ToDiscordMessage() *discordgo.MessageSend {
return &discordgo.MessageSend{Embed: em.embed}
}

func NewEmbedMessage(e *discordgo.MessageEmbed) *EmbedMessage {
return &EmbedMessage{embed: e}
}

type Embed struct {
*discordgo.MessageEmbed
}
Copy link
Contributor

Choose a reason for hiding this comment

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

한번 더 EmbedMessage에서 한 depth 더 들어가서 Embed를 만든 이유가 따로 있을까요? 그냥 EmbedMessage에서 SetTitle, SetDescription ... 을 해도 상관 없어보여서요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

단지 *discordgo.MessageEmbed가 길어서 그랬습니다. 없어도 상관 없긴 합니다.

Comment on lines +47 to +55
var URL string
var proxyURL string

if len(args) == 0 {
return e
}
if len(args) > 0 {
URL = args[0]
}
Copy link
Contributor

Choose a reason for hiding this comment

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

전체적으로 이런 패턴이 많이 보이는데, 새로 함수를 구현해도 좋을 것 같습니다.
(아래는 예시)

Suggested change
var URL string
var proxyURL string
if len(args) == 0 {
return e
}
if len(args) > 0 {
URL = args[0]
}
if len(args) == 0 {
return e
}
URL := getArgsOrDefault(args, 0, "")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

해당 부분은 그리 여러 번 반복되는 부분이 아니고, 더 많은 부분에서 사용될 코드도 아니라고 생각합니다. 그래서 새로 함수를 구현한다고 더 깔끔해지거나 짧아지진 않을 것 같고, 오히려 관련 함수들의 기능을 한번에 이해하기 위해 다른 곳을 또 읽어줘야하는 불편함이 생긴다고 생각합니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

setImage, setThumbnail, setAuthor 에서 같은 패턴이 반복되고 있어서 반복되는건 꽤 많다고 생각하긴 했고 변수선언, if문으로 나뉘어져있는 코드를 바로 선언하면서 값을 설정할 수 있으면 코드는 깔끔해질것 같아요.
그리고 함수를 구현했을때 naming만 잘한다면 굳이 함수내용을 안 읽어봐도 알 수 있을거고 읽는다고 하더라도 함수가 2줄정도 분량이라 큰 부담이 될것 같진 않아요

@inc5025 inc5025 closed this Mar 11, 2022
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.

봇 응답을 embed로
3 participants