Ad
  • Custom User Avatar

    https://go.dev/doc/effective_go#mixed-caps

    Finally, the convention in Go is to use MixedCaps or mixedCaps rather than underscores to write multiword names.

    https://github.com/golang/go/wiki/CodeReviewComments#mixed-caps

    Mixed Caps
    This applies even when it breaks conventions in other languages. For example an unexported constant is maxLength not MaxLength or MAX_LENGTH.

    While golint is now deprecated, it would immediately flag use of MORSE_CODE as inappropriate.

    Your own link notes at the bottom:

    Tutorials, references, and examples are constantly reviewed to avoid errors, but we cannot warrant full correctness of all content.

    I’ll take the word of github.com/golang/go and go.dev documentation itself as more authoritative than an online “learn how to program” intro page. Even if this only concerns style recommendations of what should be prefered, not what must be done.

  • Custom User Avatar
  • Default User Avatar

    You are correct

  • Custom User Avatar

    Just a bit cheeky, but demonstrates correct handling of all category Nd (unicode.Digit) digits. There still isn’t really a good way to do the Caesar Cipher letter shifting for anything other than the ASCII alphabet.

    We would need to define a proper ring type and whatever it would be defined as, some language would say it’s the wrong order. :shrug:

  • Custom User Avatar

    This suffers from a subtle bug that i is the byte-offset into the string of the rune, and as such it would assume that both letters of "äa" would be in even positions, and thus not lowercase either of them.

    It also does odd things like clamp non-ASCII letters into ASCII letters in a pretty weird way.

    Combining both issues"ÄAÖU" becomes "auBC".

    The number of codepoints that satisfy unicode.IsDigit is also significantly bigger than 0-9 and results in things like mapping to invalid codepoints.

  • Custom User Avatar

    I see you are building a string through a loop of string concatenations. This is an accidentally O(n²) operation, since strings are immutable, and so a new backing store has to be made for every concatenation and the old data has to be copied into it.

  • Custom User Avatar

    As mentioned to others, len(string) is the number of bytes in the slice, which can be different from the number of runes in the string. This will cause your resultIndex calculation to skip some indexes in the rune slice.

  • Custom User Avatar

    Even if there were ever a string long enough to justify splitting up the work, spinning up a goroutine for every single character would not be an efficient way of distributing the work. While goroutines are cheap, they’re not free.

    Another fun thing is that len(string) returns the number of bytes in the string, not the number of runes in it. This means if we have just one rune > utf8.RuneSelf this will deadlock, as it will attempt to receive st = <-ret for the number of bytes in string, while we will only spin off a do_work for every rune in the string.

    Once we fix that, we still end up with weird behavior, where there are \x00 values in your string because you’re indexing off the byte-length of s into the rune slice. https://play.golang.org/p/9WcBHj0hiQK

  • Custom User Avatar

    The i in the range of a range string is actually the byte-offset of the rune in the string, so this reverse implementation will crash for any string with a rune > utf8.RuneSelf. https://play.golang.org/p/29wcJFC1NDv

  • Custom User Avatar

    So, len(string) returns the number of bytes in the string, not the number of runes in the string. So your preallocation will almost certainly allocate too much memory, which isn’t the worst thing, but it’s important to be aware of. Though, Go is usually pretty good about amoratizing allocations anyways, so such a preallocation is almost always a premature optimizaton.

    But more importantly unfortunately the preallocated backing array for the ret slice does not ever actually get used. Because you are using a prepend append([]rune{r}, ret...) each loop iteration will allocate a brand new backing array to put both the r and ret... into, and it cannot reuse the existing allocation in ret. It would be better to just build the rune slice as normal with append(ret, r) and then reverse the slice elements.

  • Custom User Avatar

    Maybe use 'A' character value syntax instead of raw ASCII values to be more readable?

  • Custom User Avatar

    The Best Practice value to return in the case of an empty string slice should be []string(nil) not []string{}.

    Because for example, from the reference I shared, the code to return an empty slice should be:

    var slice []string
    return slice
    

    not:

    slice := []string{}
    return slice
    

    But the difference between the two is kind of low, which is why one should also never test for explicit values with slice == nil or reflect.DeepEqual(slice, []string{}) but should always test against the length of the string, because what we really care about is that there is zero (or non-zero) items in the slice.

    The fact that the test failed on []string(nil) when expecting []string{} demonstrates that the test code is too strict. Both returns should work in the test, but Best Practice is still that you should be returning the one created with var slice []string.

  • Default User Avatar

    Maybe I didn't well understand the object of your post!

    The test is (as you can see in function dotest):

    Expect(ans).To(Equal(exp))

    which is quite general and "github.com/onsi/ginkgo" + "github.com/onsi/gomega"

    give from that, in that particular case,:

    Expected
        <[]string | len:0, cap:0>: nil
    to equal
        <[]string | len:0, cap:0>: []
    
  • Custom User Avatar

    Consider the case where array1 is 256 copies of the same string, and array2 is also large, but does not contain that single duplicated search string.

  • Custom User Avatar

    Using strings.Contains makes the semantics of the code clearer.

    Consider that array1 could feasibly be 256 copies of the same string, so maybe we don’t want to test every single one of them?

  • Loading more items...