· 6 years ago · Mar 03, 2020, 03:16 PM
1diff --git a/apiprovider/apiprovider.go b/apiprovider/apiprovider.go
2index 055e9e8..c6aa5bf 100644
3--- a/apiprovider/apiprovider.go
4+++ b/apiprovider/apiprovider.go
5@@ -1,5 +1,7 @@
6 package apiprovider
7
8+// XXX package name is usually an single word noun.
9+
10 // APIProvider is a interface define API provider should provide
11 // what we need
12 type APIProvider interface {
13@@ -9,6 +11,19 @@ type APIProvider interface {
14 // Currency it a enum define query currency
15 type Currency string
16
17+/*
18+XXX:
19+
20+Good:
21+
22+const (
23+ Usd Currency = "usd"
24+ Twd Currency = "twd"
25+ ABC Currency = "abc"
26+)
27+
28+your Twd's type is string, not Currency
29+*/
30 const (
31 // Usd means USD
32 Usd Currency = "usd"
33diff --git a/apiprovider/coingecko.go b/apiprovider/coingecko.go
34index fce5e64..4b726ff 100644
35--- a/apiprovider/coingecko.go
36+++ b/apiprovider/coingecko.go
37@@ -17,6 +17,8 @@ type CoinGecko struct {
38
39 // GetLatestPrice will get latest BTC price with USD
40 func (c *CoinGecko) GetLatestPrice(currency Currency) (float64, error) {
41+ // XXX: http.MethodGet
42+ // XXX: why don't you just CoinGeckoURL into the second parameter if URL is empty.
43 request, err := http.NewRequest("GET", c.URL, nil)
44 if err != nil {
45 return 0, err
46@@ -39,8 +41,10 @@ func (c *CoinGecko) GetLatestPrice(currency Currency) (float64, error) {
47 return 0, errors.New(r.Status)
48 }
49
50+ // XXX: should catch err returned by Decode()
51 var response map[string]map[string]float64
52 json.NewDecoder(r.Body).Decode(&response)
53
54+ // XXX: should check whether the key and value is existing.
55 return response["bitcoin"][string(currency)], nil
56 }
57diff --git a/apiprovider/coingecko_test.go b/apiprovider/coingecko_test.go
58index 7d09684..ce21389 100644
59--- a/apiprovider/coingecko_test.go
60+++ b/apiprovider/coingecko_test.go
61@@ -20,6 +20,7 @@ func TestCoinGeckoGetLatestPrice(t *testing.T) {
62 {apiprovider.Usd, 9487},
63 }
64
65+ // XXX: use testify assert package
66 for _, c := range cases {
67 ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
68 query := r.URL.Query()
69@@ -39,6 +40,7 @@ func TestCoinGeckoGetLatestPrice(t *testing.T) {
70 t.Errorf("Parameter vs_currencies value expected %q, actual %q", string(c.in), query["vs_currencies"][0])
71 }
72
73+ // XXX: not good, should write json format string.
74 result := map[string]map[string]float64{
75 "bitcoin": map[string]float64{
76 string(c.in): c.want,
77@@ -48,6 +50,7 @@ func TestCoinGeckoGetLatestPrice(t *testing.T) {
78 bytes, _ := json.Marshal(result)
79 w.Write(bytes)
80 }))
81+ // XXX: defer executed when function exited, not end of for loop
82 defer ts.Close()
83
84 coinGecko := apiprovider.CoinGecko{URL: ts.URL}
85@@ -56,6 +59,7 @@ func TestCoinGeckoGetLatestPrice(t *testing.T) {
86 t.Errorf("GetLastestPrice error shold be empty, actual %q", err)
87 }
88
89+ // XXX: compare float64? https://floating-point-gui.de/errors/comparison/
90 if value != c.want {
91 t.Errorf("Expected %f, actual %f", c.want, value)
92 }
93diff --git a/apiprovider/coinmarketcap.go b/apiprovider/coinmarketcap.go
94index c1a94a6..d6b9696 100644
95--- a/apiprovider/coinmarketcap.go
96+++ b/apiprovider/coinmarketcap.go
97@@ -46,6 +46,8 @@ type Response struct {
98
99 // GetLatestPrice will get latest BTC price with USD
100 func (c *CoinMarketCap) GetLatestPrice(currency Currency) (float64, error) {
101+ // XXX: const? should define as static var, not in a function
102+ // switch case is better
103 currencyID := map[Currency]int{
104 Usd: 2781,
105 Twd: 2811,
106@@ -57,7 +59,7 @@ func (c *CoinMarketCap) GetLatestPrice(currency Currency) (float64, error) {
107 }
108
109 q := url.Values{}
110- q.Add("id", "1")
111+ q.Add("id", "1") // XXX: what's 1 mean?
112 q.Add("convert_id", strconv.Itoa(currencyID))
113
114 request.Header.Set("Accepts", "application/json")
115@@ -76,5 +78,6 @@ func (c *CoinMarketCap) GetLatestPrice(currency Currency) (float64, error) {
116 return 0, err
117 }
118
119+ // XXX: don't trust input from external service.
120 return response.Data["1"].Quote[strconv.Itoa(currencyID)].Price, nil
121 }
122diff --git a/apiprovider/coinmarketcap_test.go b/apiprovider/coinmarketcap_test.go
123index b2774ac..4be1507 100644
124--- a/apiprovider/coinmarketcap_test.go
125+++ b/apiprovider/coinmarketcap_test.go
126@@ -25,6 +25,7 @@ func TestCoinMarketCapLatestPrice(t *testing.T) {
127 in inputData
128 want float64
129 }{
130+ // XXX: should use human readable string if ur api key is format free.
131 {inputData{"c00a16de-55a7-421c-98be-45ba96bc94c5", apiprovider.Usd}, 3.1415926},
132 {inputData{"6024f3d6-0969-4940-acdf-b6f29c7a2043", apiprovider.Usd}, 2.7182818},
133 {inputData{"9cbbea16-3502-437c-b7ca-59311a679c11", apiprovider.Twd}, 81000},
134diff --git a/configreader/config.yaml b/configreader/config.yaml
135index 976eb91..2cbd02f 100644
136--- a/configreader/config.yaml
137+++ b/configreader/config.yaml
138@@ -2,4 +2,6 @@ CoinMarketCapKey: sample-key
139 CryptoCompareKey: sample-key
140 SecondPerToken: 288
141 MaxSizeOfBucket: 20
142-UserMaxQueryPerDay: 100
143\ No newline at end of file
144+UserMaxQueryPerDay: 100
145+
146+# rename to config.yaml.example and move to root dir.
147diff --git a/flowcontrol/flowcontrol.go b/flowcontrol/flowcontrol.go
148index 51bb8f2..fca19c4 100644
149--- a/flowcontrol/flowcontrol.go
150+++ b/flowcontrol/flowcontrol.go
151@@ -11,7 +11,8 @@ type FlowController struct {
152 maxNumber uint
153 interval uint
154 tokens uint
155- lock *sync.Mutex
156+ // XXX: no idea why it needs to be a pointer
157+ lock *sync.Mutex
158 }
159
160 // New will new a FlowController
161@@ -21,10 +22,11 @@ func New(maxNumber uint, interval uint) *FlowController {
162 maxNumber: maxNumber,
163 interval: interval,
164 tokens: maxNumber,
165- lock: &sync.Mutex{},
166+ lock: &sync.Mutex{}, // XXX: don't explictly init zero value.
167 }
168
169 if maxNumber != 0 && interval != 0 {
170+ // XXX this goroutine is leaked.
171 go f.addTokens()
172 }
173
174diff --git a/flowcontrol/flowcontrol_test.go b/flowcontrol/flowcontrol_test.go
175index da66614..f2f0386 100644
176--- a/flowcontrol/flowcontrol_test.go
177+++ b/flowcontrol/flowcontrol_test.go
178@@ -17,6 +17,8 @@ func TestHasPermission(t *testing.T) {
179 t.Errorf("Second time should be false")
180 }
181
182+ // XXX it's not good to use Sleep in ur test, consider refactor
183+ // and resolve this problem.
184 time.Sleep(2 * time.Second)
185
186 if f.AcquirePermission() != true {
187diff --git a/main.go b/main.go
188index 8902d86..2bd7c65 100644
189--- a/main.go
190+++ b/main.go
191@@ -22,11 +22,13 @@ func main() {
192 {
193 v1.GET("/", queryPrice)
194 }
195+ // XXX: how can I specify listening port?
196 router.Run()
197 }
198
199 func queryPrice(context *gin.Context) {
200
201+ // XXX misspell Querry?
202 if !usercontroller.QuerryAcquire(context.ClientIP()) {
203 context.JSON(200, gin.H{
204 "error": "Exceed 24 hours limit",
205@@ -34,6 +36,8 @@ func queryPrice(context *gin.Context) {
206 return
207 }
208
209+ // XXX: can't know /cryptocurrency?currency= vs /cryptocurrency?
210+ // use GetQuery to know whether the user assigns query parameter
211 currency, ok := currencies[context.Query("currency")]
212 if !ok {
213 context.JSON(200, gin.H{
214@@ -42,6 +46,7 @@ func queryPrice(context *gin.Context) {
215 return
216 }
217
218+ // XXX: ditto
219 price, err := querier.GetLatestPrice(context.Query("provider"), currency)
220
221 if err != nil {
222diff --git a/querier/querier.go b/querier/querier.go
223index 12319cd..fed4744 100644
224--- a/querier/querier.go
225+++ b/querier/querier.go
226@@ -47,6 +47,7 @@ func GetLatestPrice(p string, currency apiprovider.Currency) (float64, error) {
227 if provider.flowControl.AcquirePermission() {
228 price, err := provider.api.GetLatestPrice(currency)
229
230+ // XXX: should print err if not nil
231 if err == nil {
232 provider.lastTimePrice = price
233 }
234diff --git a/usercontroller/usercontroller.go b/usercontroller/usercontroller.go
235index a35d47c..7f6eca2 100644
236--- a/usercontroller/usercontroller.go
237+++ b/usercontroller/usercontroller.go
238@@ -4,6 +4,10 @@ import (
239 "time"
240 )
241
242+// XXX: should put these values into a struct
243+// if I wanna use this package in 2 places, it can't support.
244+
245+// XXX: why don't define a map?
246 // User query record
247 type queryRecord struct {
248 // TODO it should be replace by my own api key
249@@ -15,6 +19,7 @@ type queryRecord struct {
250 var queryRecords = []queryRecord{}
251
252 // MaxTime means max query times per day
253+// XXX: const or read from config?
254 var MaxTime = 0
255
256 // QuerryAcquire will check is user reach it's limit of daily query
257@@ -30,6 +35,7 @@ func QuerryAcquire(ip string) bool {
258 func queryIn24Hours(ip string) int {
259 now := time.Now()
260 count := 0
261+ // XXX: awful implementation.
262 for _, record := range queryRecords {
263 if record.ip == ip && now.Sub(record.time) < 24*time.Hour {
264 count++