Skip to content

workshop content#19

Open
yellowragz wants to merge 1 commit intotherizk:masterfrom
yellowragz:workshop
Open

workshop content#19
yellowragz wants to merge 1 commit intotherizk:masterfrom
yellowragz:workshop

Conversation

@yellowragz
Copy link

No description provided.

)

const (
// Collection queries
Copy link
Author

Choose a reason for hiding this comment

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

bisa dipisah file queries.go indent querynya dibenerin, syntax sqlnya di uppercase aja

"github.com/tokopedia/tokopoints/tracer"
)

func GetNilaiSiswa(ctx context.Context, nim int64, sekolah, kelas string, mapel_id int64) (resp GetNilaiSiswaResponse, err error) {
Copy link
Author

Choose a reason for hiding this comment

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

paramnya bisa jd satu struct terpisah GetNilaiSiswaParam

return resp, errors.AddTrace(err)
}

detailSekolah, err := nilai.GetDetailSekolah(ctx, nilai.Sekolah[0])
Copy link
Author

Choose a reason for hiding this comment

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

GetDetailSekolah dan mappingGetNilaiSiswa bisa taruh setelah if else biar ga redundant

pb "github.com/tokopedia/grpc/code-review-workshop/proto"
)

func (s *server) GetNilaiSiswa(ctx context.Context, req *pb.GetNilaiSiswaRequest) (resp *pb.GetNilaiSiswaResponse, err error) {
Copy link
Author

@yellowragz yellowragz Dec 17, 2020

Choose a reason for hiding this comment

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

param ctx klo ga dipake bisa dihilangkan, ato di span jika ada kebutuhan tracing

"github.com/tokopedia/code-review-workshop/tracer"
)

type NilaiSiswa struct {
Copy link
Author

Choose a reason for hiding this comment

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

struct nya bisa dipisah file types.go

type NilaiSiswa struct {
NIM int64 `json:"nim" db:"nim"`
MapelID int64 `json:"mapel_id" db:"mapel_id"`
Nilai int64 `json:"nili" db:"nili"`
Copy link
Author

Choose a reason for hiding this comment

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

tag json dan dbnya "nilai"

Kelas string `json:"kelas" db:"kelas"`
}

func GetNilaiSiswaByID(ctx context.Context, nim, mapel_id int64) (result []NilaiSiswa, err error) {
Copy link
Author

Choose a reason for hiding this comment

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

penamaan paramnya mungkin ngikutin structnya aja tp di unexported

req.sekolah = "semua"
}

nilai, err := mnilai.GetNilaiSiswa(req.nim, req.sekolah, req.kelas, mapel_id)
Copy link
Author

Choose a reason for hiding this comment

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

req.mapel_id

return resp, errors.AddTrace(err)
}

detailSekolah, err := nilai.GetDetailSekolah(ctx, nilai.Sekolah[0])
Copy link
Author

Choose a reason for hiding this comment

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

jgn lupa cek len nilai.Sekolah nya


redis, err = redislib.Get(redislib.PromoCatalog)
if err != nil {
log.Fatal("Cannot connect to redis")
Copy link
Author

Choose a reason for hiding this comment

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

lognya diperjelas ga bisa konek ke redis yg mana

}

resp = mappingGetNilaiSiswa(nilai, detailSekolah)
} else if sekolah == "semua" {
Copy link
Author

@yellowragz yellowragz Dec 17, 2020

Choose a reason for hiding this comment

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

semua sepertinya bisa dijadikan const karena sering dipake

span, ctx := tracer.StartSpanFromContext(ctx)
defer span.Finish()

if nim != 0 && mapel_id != 0 {
Copy link
Author

Choose a reason for hiding this comment

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

bisa dikasi monitoring ato log error utk bbrp flownya jika memang critical

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.

2 participants