Please review my typespecs

Hi all

I defined a struct in a module as well the typespecs:

  defstruct iso: nil, postal_code: nil, locations: []
  @type t :: %__MODULE__{iso: binary, postal_code: integer, locations: list}

the spec above the method definition looks as follow:

  @spec query_code(number, String.t) :: {:ok, __MODULE__.t} | {:error, HTTPoison.Error.t}
  def query_code(country_iso, code) when is_integer(code) and is_binary(country_iso) do

I want to know, if the spec is defined in the correct way.

Thanks

As far as I can see, there are no syntactical error, but I’d use t rather than __MODULE__.t in the @spec.

Also, there are typerrors in your declaration. Your default values of nil for :iso and :postal_code do violate the type of binary and integer.

Another typerror seems to be in your spec as well, where you specify the first argument to your function beeing a number, while call is_binary/1 on it in the guard. For the second argument you specify it to be a String.t but guard it against integer.

Last but not least, for the field :iso I’d prefer the type String.t for semantic reasons.


edit

At least the stuff mentioned in the 2nd and 3rd paragraph should have been yelled at you from dialyzer when/if run.

Also I just realize that you are saving the postal code in an integer. You should be very careful with that. There might be regions on earth that do use letters in the PC, as well as there might be some that differ between e.g. 001 and 1 as a PC. An example for the first could be germany right after fusing with GDR, where we had to add the letter “W” or “O” to some postal codes that werent unambigous otherwise, because that code existed on both sides of the old border. This was necessary until 5 digit postal codes were introduced.

2 Likes

Also, there are typerrors in your declaration. Your default values of nil for :iso and :postal_code do violate the type of binary and integer.

What do I have to do?

Like this:

defstruct iso: "", postal_code: 0, locations: []

The answer my friend does is only known to the wind :slight_smile:


No. it really depends on what YOU want.

One way is to use a default value which is valid according to the specs, but then you also should adhere the semantics of that field.

The other way were to extend the type to allow nil or :unspecified or similar. That could look like string | nil.


Ok. Writing this on a mobile which isn’t my own and has a keyboard I’m not comfortable with took me nearly 30 minutes…

4 Likes

Which way is the right way?

It depends, as @NobbZ said…

Do you want to act differently if a value wasn’t supplied at all (eg. nil) vs. if an empty value was supplied? Then you’d need to go with the nil defaults, and include that in your typespecs.

However, if it’s all the same to you… eg. if it’s nil, you’ll just treat it the same as if it was entered, but empty… then you could as well have empty string / list as defaults, and NOT allow nil in the typespecs.

There is not “General Right” here. Only “Private Circumstances” :wink:

As I already mentioned, it totally depends on your application(s needs).

Lets consider a simplified example of a user: %User{nick: "", email: "", phone: :unknown} with type @type t :: %User{nick: String.t, email: String.t, phone: :unknown | Phone.t}

This type makes clear that a username and emailaddress are required for that struct, while a phone number is totally optional.

1 Like

To be honest I don’t see a problem with a typespec for struct that does not include the defaults. This will only mean that the struct created with default values does not fit the type, and that may be a good thing! This allows you to enforce in a type that the struct is fully initialised - using just %User{} as a value is not valid (it makes no sense to pass around a user without the email). Combined with @enforce_keys this can an extremely powerful tool to increase type safety of the code.

Yeah, this idea came to my mind yesterday evening when I was going to bed as well. I was up to write about it today, but now you have been faster than me :wink:

Thank you for clarifying.