Feature/lab 5 #463

starfallz · 2019-06-20T07:00:08Z

Fixes #462

Review of colleague's PR #

Changes proposed in this PR:

Make the IPAddr type implement fmt.Stringer to print the address as a dotted quad

I made the assumption to use uint8 as the data type for each of the 4 addresses that makes up an IP address. The only valid numbers should be 0-255.

- Some linting issue fix on the uses of testCase.input and expected inside t.Run - Added more test cases to cover the boundaries and no values - Modified the struct declaration into one line for readibility - Still not sure about using Iota to convert int to string, maybe there is a better way

codecov · 2019-06-20T07:03:25Z

Codecov Report

Merging #463 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #463   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          40     41    +1     
  Lines         665    673    +8     
=====================================
+ Hits          665    673    +8

Impacted Files	Coverage Δ
05_stringer/starfallz/main.go	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5f8d5d...4b796b2. Read the comment docs.

willshen8

Just some minor things to fix, it would be interesting to hear from the code-owners whether bytes is better than uint8.

willshen8 · 2019-06-23T11:33:21Z

05_stringer/starfallz/main.go

+}
+
+type IPAddr struct {
+	add1, add2, add3, add4 uint8


uint8 is correct that it gives the correct IP Address range of 0-255, and the clue given by the lecture notes is using array of 4 bytes (type IPAddr [4]byte). From a technical perspective, it is more accurate, as IPv4 address is represented as bytes( 8 bits). My guess is that the program would run faster using byte as it more native at the hardware level. I actually googled around, and seems mos t implementations in go is implemented as Byte and not uint8.

If I'm not wrong byte is an alias of uint8 and uint8 is a more explicit way of declaring it. But at the end of the day it really depends on people's preferences interms of readability. I am more than happy to change it to byte but I do want to hear from any of the course reviewer about it though. Maybe you are right since in this case it's an ip address, maybe byte alias is more relevant.

Personally I don't have problems with uint32, this is more of a comment to spark some conversations. I believe that the data type should reflect the type of data that you'll be getting, and in this case. it will be bytes. I do think that having array instead of 4 different variables will help, your String() will just be a one line using string methods. So this is something you can explore. I'd say just leave it, and wait to see what the final approver has to say about this.

I actually don't have a problem with it either way. Golang inlines structs instances, so this will be just as efficient space and time-wise as [4]byte. The difference is how you use the IPAddr type.
[4]byte gives you array access and the ability to range over the elements of the address.
your struct allows you to only initialize say the third element (use IPAddr{add3: 1}).

My preference is towards [4]byte, but its not a strong preference.

Additionally, @starfallz is correct in saying byte is an alias of uint8. There really is no difference between the two, only in that bytes are more often used for text based data handling simply because people prefer saying byte instead of int (int is a number, whereas byte is data).

Please change as per reviewer requests (it will test cases nicer too).

willshen8 · 2019-06-23T11:35:00Z

05_stringer/starfallz/main_test.go

+		expected := strconv.Quote("127.0.0.1")
+		actual := strconv.Quote(buf.String())
+
+		if expected != actual {


If you use the testify library, it would be as simple as assert.Equal(expected, actual, msg) - one line instead of 3 lines of code.

Yeah I tried testify but I noticed that lab6 is the only one that explicitly say to use testify so I figured best to kept it plain vanilla for all labs until lab 6 so at least I get the hang of doing it without external dependencies till then.

Not exactly, lab06 ask you to use testify/suite, which is not the same as testify/assert which is commonly used. I did what you did in lab4, and Julia commnented that I should consider using testify/assert, and it does make the code bit shorter.

I would recommend using testify, however don't feel the need to change it since your tests are all passing. If you do the later labs, you'll probably want to use it more since it significantly shortens the test code.

willshen8 · 2019-06-23T11:37:12Z

05_stringer/starfallz/main_test.go

+		{"Test IPAddr with all zeros in addresses", IPAddr{0, 0, 0, 0}, "0.0.0.0"},
+		{"Test IPAddr with some values assigned", IPAddr{add1: 5, add4: 4}, "5.0.0.4"},
+		{"Test IPAddr with no value", IPAddr{}, "0.0.0.0"},
+		{"Test IPAddr with max values allowed on uint8", IPAddr{255, 255, 255, 255}, "255.255.255.255"},


One of the code-owners asked me to have a test case of 'IPAddr{0}" or IPAddr{172, 0}, so perhaps you can try that too?

Ok will add them

Based on your type declaration for IPAddr, I don't think IPAddr{0} will compile.

You can partially initialize an array, so [4]byte{1} will create an array containing {1, 0, 0, 0}, but if you want to initialize a struct without variable identifiers, you need a full list, so IPAddr{1} won't compile, but IPAddr{1, 0, 0, 0} will

You could write tests with partial struct initialization using the variable identifiers

IPAddr{add1: 1, add2:1}

Playspace example: https://goplay.space/#uhJY-1G0t3h

thanks @anzboi, ive got a test for IPAddr{add1: 1, add2:1} already above. So i guess I will just add another one for {addr1: 0} and {addr1: 172, addr2: 0} thanks!

willshen8 · 2019-06-23T11:37:34Z

05_stringer/starfallz/main_test.go

+
+		t.Run(testCase.description, func(t *testing.T) {
+			actual := input.String()
+			if expected != actual {


Try to use testify library here as well.

willshen8 · 2019-06-23T11:46:03Z

05_stringer/starfallz/main.go

+	add1, add2, add3, add4 uint8
+}
+
+func (ipAddr IPAddr) String() string {


(ipAddr IPAddr) This is called receiver in Golang, and a code-owner pointed out to me that the receiver name should be very short, usually one or two letters. The main reason is that the variable will be used straight-away, and won't be confused with other variables in the block. I did my own research as well, and it appears that this is the case. But this might be debatable and not everyone likes this convention.

This one is an odd one too. I guess I came from java and usually I am all in on readability and I hate to have comments in my code. Therefore generally I tend to stay away from shortform in naming variables, method, etc. (I would have request changes on the pr if it's Java or even js). But if it's the proper coding pattern of golang then I will have to change my mindset and follow the convention. It would be good if there's a pattern doc that we can follow especially when building enterprise app.

This was an odd one for me too, I always believe that variable names should be clear and concise. Guess this is something we need to get use to.

Naming convention in go is to use really short names for receivers. The reason is that a method can only have one receiver, and that name should be reused for all methods on a particular type. I don't think having a long receiver name aides readability, I actually think the ridiculously long variable names in java hinders readability.
Also, consider that the this keyword is often used to refer to the class instance in a method (or it is omitted entirely). Think of the receiver name as this.

alright will change it thanks, need to get use to it =)

How about you do it then? :)

anzboi · 2019-06-24T00:24:47Z

05_stringer/starfallz/main.go

+}
+
+func (ipAddr IPAddr) String() string {
+	return strings.Join([]string{


You can use a string format instead of joining on a slice
fmt.Sprintf("%d.%d.%d.%d", ipaddr.add1, ipaddr.add2, ...)

dpes fmt.Sprintf return a string not print out the string?

https://godoc.org/fmt#Sprintf (it returns a string)

https://godoc.org/fmt#Sprintf (it does)

juliaogris

Please update according to previous review comments (use arr size 4, yes Sprintf prints a string, use testify).

Please fix commit messages with git rebase -i HASH

Please add a reference to a review of a colleagues PR.

juliaogris · 2019-06-24T07:13:08Z

05_stringer/starfallz/main.go

+}
+
+type IPAddr struct {
+	add1, add2, add3, add4 uint8


Please change as per reviewer requests (it will test cases nicer too).

juliaogris · 2019-06-24T07:13:20Z

05_stringer/starfallz/main.go

+	add1, add2, add3, add4 uint8
+}
+
+func (ipAddr IPAddr) String() string {


How about you do it then? :)

juliaogris · 2019-06-24T07:13:41Z

05_stringer/starfallz/main.go

+}
+
+func (ipAddr IPAddr) String() string {
+	return strings.Join([]string{


https://godoc.org/fmt#Sprintf (it does)

hamana-anz added 2 commits June 17, 2019 11:37

LAB_5: added main that calls stringer function with tests.

ed9ed6e

I made the assumption to use uint8 as the data type for each of the 4 addresses that makes up an IP address. The only valid numbers should be 0-255.

starfallz added Ready for review lab5 labels Jun 20, 2019

starfallz requested review from anzboi, anzdaddy, camscale, joshcarp, juliaogris, linuxmonk, pentaphobe and rokane as code owners June 20, 2019 07:00

Merge branch 'master' into feature/lab_5

4b796b2

willshen8 added Under review and removed Ready for review labels Jun 23, 2019

willshen8 requested changes Jun 23, 2019

View reviewed changes

willshen8 added Changes requested and removed Under review labels Jun 23, 2019

willshen8 mentioned this pull request Jun 23, 2019

Lab06 - Puppy Storer #474

Merged

anzboi reviewed Jun 24, 2019

View reviewed changes

juliaogris suggested changes Jun 24, 2019

View reviewed changes

anzdaddy closed this Sep 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/lab 5 #463

Feature/lab 5 #463

starfallz commented Jun 20, 2019

codecov bot commented Jun 20, 2019 •

edited

Loading

willshen8 left a comment

willshen8 Jun 23, 2019

starfallz Jun 23, 2019

willshen8 Jun 23, 2019 •

edited

Loading

anzboi Jun 24, 2019

anzboi Jun 24, 2019 •

edited

Loading

juliaogris Jun 24, 2019

willshen8 Jun 23, 2019

starfallz Jun 23, 2019

willshen8 Jun 23, 2019

anzboi Jun 24, 2019

willshen8 Jun 23, 2019

starfallz Jun 23, 2019

anzboi Jun 24, 2019

starfallz Jun 24, 2019

willshen8 Jun 23, 2019

willshen8 Jun 23, 2019

starfallz Jun 23, 2019

willshen8 Jun 23, 2019

anzboi Jun 24, 2019

starfallz Jun 24, 2019

juliaogris Jun 24, 2019

anzboi Jun 24, 2019

starfallz Jun 24, 2019

juliaogris Jun 24, 2019

juliaogris Jun 24, 2019

juliaogris left a comment

juliaogris Jun 24, 2019

juliaogris Jun 24, 2019

juliaogris Jun 24, 2019

Feature/lab 5 #463

Feature/lab 5 #463

Conversation

starfallz commented Jun 20, 2019

Changes proposed in this PR:

codecov bot commented Jun 20, 2019 • edited Loading

Codecov Report

willshen8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willshen8 Jun 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anzboi Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliaogris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 20, 2019 •

edited

Loading

willshen8 Jun 23, 2019 •

edited

Loading

anzboi Jun 24, 2019 •

edited

Loading