Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/lab 5 #463

Closed
wants to merge 3 commits into from
Closed

Feature/lab 5 #463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ed9ed6e
LAB_5: added main that calls stringer function with tests.
hamana-anz Jun 17, 2019
d71e2ad
Fixed linting issues, and added more test cases
hamana-anz Jun 20, 2019
4b796b2
Merge branch 'master' into feature/lab_5
starfallz Jun 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions 05_stringer/starfallz/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package main

import (
"fmt"
"io"
"os"
"strconv"
"strings"
)

var out io.Writer = os.Stdout

func main() {
fmt.Fprint(out, IPAddr{127, 0, 0, 1})
}

type IPAddr struct {
add1, add2, add3, add4 uint8
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@willshen8 willshen8 Jun 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@anzboi anzboi Jun 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

func (ipAddr IPAddr) String() string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

How about you do it then? :)

return strings.Join([]string{
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

strconv.Itoa(int(ipAddr.add1)),
strconv.Itoa(int(ipAddr.add2)),
strconv.Itoa(int(ipAddr.add3)),
strconv.Itoa(int(ipAddr.add4))}, ".")
}
48 changes: 48 additions & 0 deletions 05_stringer/starfallz/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package main

import (
"bytes"
"strconv"
"testing"
)

func TestMainFunction(t *testing.T) {
t.Run("Test main to return string values of IPAddr({127, 0, 0, 1}) with proper formatting", func(t *testing.T) {
var buf bytes.Buffer
out = &buf

main()

expected := strconv.Quote("127.0.0.1")
actual := strconv.Quote(buf.String())

if expected != actual {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

t.Errorf("Unexpected output, expected: %s, actual: %s", expected, actual)
}
})
}

func TestIpAddrDefaultToString(t *testing.T) {
testCases := []struct {
description string
input IPAddr
expected string
}{
{"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"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok will add them

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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!

}

for _, testCase := range testCases {
input := testCase.input
expected := testCase.expected

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

Choose a reason for hiding this comment

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

Try to use testify library here as well.

t.Errorf("Unexpected output, expected: %s, actual: %s", expected, actual)
}
})
}
}