Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  1. array := divided[ii] is pointlessly cryptic, instead use for e, i := range divided {} and get the element and index that way -- it's cleaner.

  2. Naming. I understand it's hard; I struggle with it too: every coder does. But please, for the love of Go, either use i, index or int_theIndexOfTheFreakinFullyQualifiedStringObjectWeAreIteratingOn. ii is hard to pronounce, not rememberable... I hate it.

  3. As far as I can tell, these variables total, mid, first, second have exactly one use: right here in this loop. Remember what I said about Go being faster when you keep the work for the GC down to a minimum. I get it improves readability, but at a point, that stuff should just be inlined to keep the cache happy.

  4. I see zero difference between these go funcs, because they're identical. Ideally, you would assign it to a local var and call it on first and second.

  5. I can't run your code right now, which means I can't make it testable and I can't ask go race to take a look at it. What I can tell you, from a glance, is that you're accessing the same resources through two goroutines at once. The locker, assuming it does what it says on the tin, is a good thought, but you're still locking them at the same time and it's a recipe for disaster in the form of race conditions. See here here, here, here, and the other results from the first page of google results on "avoid race condition golang".

  1. array := divided[ii] is pointlessly cryptic, instead use for e, i := range divided {} and get the element and index that way -- it's cleaner.

  2. Naming. I understand it's hard; I struggle with it too: every coder does. But please, for the love of Go, either use i, index or int_theIndexOfTheFreakinFullyQualifiedStringObjectWeAreIteratingOn. ii is hard to pronounce, not rememberable... I hate it.

  3. As far as I can tell, these variables total, mid, first, second have exactly one use: right here in this loop. Remember what I said about Go being faster when you keep the work for the GC down to a minimum. I get it improves readability, but at a point, that stuff should just be inlined to keep the cache happy.

  4. I see zero difference between these go funcs, because they're identical. Ideally, you would assign it to a local var and call it on first and second.

  5. I can't run your code right now, which means I can't make it testable and I can't ask go race to take a look at it. What I can tell you, from a glance, is that you're accessing the same resources through two goroutines at once. The locker, assuming it does what it says on the tin, is a good thought, but you're still locking them at the same time and it's a recipe for disaster in the form of race conditions. See here, here, here, and the other results from the first page of google results on "avoid race condition golang".

  1. array := divided[ii] is pointlessly cryptic, instead use for e, i := range divided {} and get the element and index that way -- it's cleaner.

  2. Naming. I understand it's hard; I struggle with it too: every coder does. But please, for the love of Go, either use i, index or int_theIndexOfTheFreakinFullyQualifiedStringObjectWeAreIteratingOn. ii is hard to pronounce, not rememberable... I hate it.

  3. As far as I can tell, these variables total, mid, first, second have exactly one use: right here in this loop. Remember what I said about Go being faster when you keep the work for the GC down to a minimum. I get it improves readability, but at a point, that stuff should just be inlined to keep the cache happy.

  4. I see zero difference between these go funcs, because they're identical. Ideally, you would assign it to a local var and call it on first and second.

  5. I can't run your code right now, which means I can't make it testable and I can't ask go race to take a look at it. What I can tell you, from a glance, is that you're accessing the same resources through two goroutines at once. The locker, assuming it does what it says on the tin, is a good thought, but you're still locking them at the same time and it's a recipe for disaster in the form of race conditions. See here, here, here, and the other results from the first page of google results on "avoid race condition golang".

added 152 characters in body
Source Link
cat
  • 997
  • 1
  • 5
  • 23
func Search(re *regexp.Regexp, logs []string) []string {
  1. array := divided[ii] is pointlessly cryptic, instead use for e, i := range divided {} and get the element and index that way -- it's cleaner.

  2. Naming. I understand it's hard; I struggle with it too: every coder does. But please, for the love of Go, either use i, index or int_theIndexOfTheFreakinFullyQualifiedStringObjectWeAreIteratingOn. ii is hard to pronounce, not rememberable... I hate it.

  3. As far as I can tell, these variables total, mid, first, second have exactly one use: right here in this loop. Remember what I said about Go being faster when you keep the work for the GC down to a minimum. I get it improves readability, but at a point, that stuff should just be inlined to keep the cache happy.

  4. I see zero difference between these go funcs, because they're identical. Ideally, you would assign it to a local var and call it on first and second.

  5. I can't run your code right now, which means I can't make it testable and I can't ask go race to take a look at it. What I can tell you, from a glance, is that you're accessing the same resources through two goroutines at once. The locker, assuming it does what it says on the tin, is a good thought, but you're still locking them at the same time and it's a recipe for disaster in the form of race conditions. See here, here, here, and the other results from the first page of google results on "avoid race condition golang".

func Search(re *regexp.Regexp, logs []string) []string {
  1. Naming. I understand it's hard; I struggle with it too: every coder does. But please, for the love of Go, either use i, index or int_theIndexOfTheFreakinFullyQualifiedStringObjectWeAreIteratingOn. ii is hard to pronounce, not rememberable... I hate it.

  2. As far as I can tell, these variables total, mid, first, second have exactly one use: right here in this loop. Remember what I said about Go being faster when you keep the work for the GC down to a minimum. I get it improves readability, but at a point, that stuff should just be inlined to keep the cache happy.

  3. I see zero difference between these go funcs, because they're identical. Ideally, you would assign it to a local var and call it on first and second.

  4. I can't run your code right now, which means I can't make it testable and I can't ask go race to take a look at it. What I can tell you, from a glance, is that you're accessing the same resources through two goroutines at once. The locker, assuming it does what it says on the tin, is a good thought, but you're still locking them at the same time and it's a recipe for disaster in the form of race conditions. See here, here, here, and the other results from the first page of google results on "avoid race condition golang".

func Search(re *regexp.Regexp, logs []string) []string
  1. array := divided[ii] is pointlessly cryptic, instead use for e, i := range divided {} and get the element and index that way -- it's cleaner.

  2. Naming. I understand it's hard; I struggle with it too: every coder does. But please, for the love of Go, either use i, index or int_theIndexOfTheFreakinFullyQualifiedStringObjectWeAreIteratingOn. ii is hard to pronounce, not rememberable... I hate it.

  3. As far as I can tell, these variables total, mid, first, second have exactly one use: right here in this loop. Remember what I said about Go being faster when you keep the work for the GC down to a minimum. I get it improves readability, but at a point, that stuff should just be inlined to keep the cache happy.

  4. I see zero difference between these go funcs, because they're identical. Ideally, you would assign it to a local var and call it on first and second.

  5. I can't run your code right now, which means I can't make it testable and I can't ask go race to take a look at it. What I can tell you, from a glance, is that you're accessing the same resources through two goroutines at once. The locker, assuming it does what it says on the tin, is a good thought, but you're still locking them at the same time and it's a recipe for disaster in the form of race conditions. See here, here, here, and the other results from the first page of google results on "avoid race condition golang".

edited body
Source Link
cat
  • 997
  • 1
  • 5
  • 23

So much nicer. That's the handiwork of my IDE. The Go language distrobutiondistribution comes with a lot of code quality tools for readability and static analysis, and you should really use them often. (Every save, perhaps?)

So much nicer. That's the handiwork of my IDE. The Go language distrobution comes with a lot of code quality tools for readability and static analysis, and you should really use them often. (Every save, perhaps?)

So much nicer. That's the handiwork of my IDE. The Go language distribution comes with a lot of code quality tools for readability and static analysis, and you should really use them often. (Every save, perhaps?)

Source Link
cat
  • 997
  • 1
  • 5
  • 23
Loading
lang-golang

AltStyle によって変換されたページ (->オリジナル) /