# yet another number of islands

i learn python and i am at beginner level in python

TLDR
i did NOT read the other questions concerning this topic since i try to learn python and therefore need to do the same stoopid errors others have done to improve my knowlegde. And since i know other things than the other OPs is my code somehow different

class Field (in islands.py)

class Field:
def __init__(self, xpos, ypos, isWater):
self.xpos = xpos
self.ypos = ypos
self.isWater = isWater

def __str__(self):
iswater = "W" if self.isWater else "L"
return f"{self.xpos}/{self.ypos}/{iswater}"

if anotherField is None:
return False

if not isinstance(anotherField, Field):
return False

if anotherField.xpos == self.xpos and anotherField.ypos+1 == self.ypos:
return True

if anotherField.xpos == self.xpos and anotherField.ypos-1 == self.ypos:
return True

if anotherField.xpos+1 == self.xpos and anotherField.ypos == self.ypos:
return True

if anotherField.xpos-1 == self.xpos and anotherField.ypos == self.ypos:
return True

return False


class Map (in islands.py)

class Map:
def __init__(self, data:list):
x = 0
self.fields = list()
for row in data:
y = 0
for column in row:
a_field = Field(x,y,column=='0')
self.fields.append(a_field)
y = y + 1
x = x + 1

def __str__(self):
return f"island data={self.fields}"

def calculate_number_of_islands(self) -> int:
print ("removing all water fields")
candidates = list(filter(lambda field : not field.isWater, self.fields))

islandcounter = 0
while len(candidates) > 0 :
candidate = candidates.pop(0)
print (f"popping next field {candidate} and expanding an island")
self.expand_land(candidate, candidates)
islandcounter = islandcounter + 1

return islandcounter

def expand_land(self, candidate, candidates):
print ("getting neighbour fields")
neighbours = list(filter(lambda field : field.isAdjected(candidate), candidates))

for neighbour in neighbours:
if(neighbour in candidates):
candidates.remove(neighbour)
self.expand_land(neighbour, candidates)

return None


main method

import island_parser
import island

#https://leetcode.com/problems/number-of-islands/description/

csvFile = 'resources\\island_01.csv'

island_map = island.Map(data)
number_of_islands = island_map.calculate_number_of_islands()
print (f"this map has {number_of_islands} islands")


note
i did not include the data parser to parse an csv into a List<List<String>> that is not part of the review

data is a List<List<String>> where each String is either 0 (water) or 1 (land), example here:

1,1,0,1,0
1,1,1,1,0
0,0,0,0,0
1,0,0,1,1


resulting in an number of 3 islands

# python != java

    self.isWater = isWater


In Field, pep-8 asks that you name it is_water.

Also, please use black to format your code. The two-space indent is distracting, makes it look like blocks were pasted in from another language.

# optional type hinting

  def __init__(self, xpos, ypos, isWater):


You don't have to. But it would be a kindness to the Gentle Reader to offer the types:

  def __init__(self, xpos: float, ypos: float, isWater: str):


(A ctor always returns None -- you can throw in ... ) -> None: if you like.)

I'm sad we're accepting str instead of bool (which is a kind of int). The data parser adheres to a rather peculiar contract. If you had instead relied on the usual .read_csv() we would have obtained rows of integers just through the default behavior.

Consider using a @property decorator for the {'L', 'W'} mapping:

    iswater = "W" if self.isWater else "L"


Whatever you do, please don't use the names iswater and isWater to represent distinct concepts. Invent a new name, perhaps terrain. It doesn't matter to the machine, but it does affect human cognition. To say nothing of making telephone conversations more difficult when we're discussing some code.

# use standard English

  def isAdjected(self, anotherField):


Instead of is_adjected I think you intended is_adjacent ?

    if anotherField is None:
return False


I'm going to second-guess this design choice. In my opinion it probably makes sense to assert that it's not None, or otherwise document / insist that caller is responsible for passing in an actual Field object. The rationale is fear of a False return value masking some caller bug.

    if not isinstance(anotherField, Field):
return False


The previous test is redundant with this one, as None is definitely not a Field. And again, if this triggers I feel it is probably due to caller violating the contract. What I'm driving at is, spell out the contract, then hold the caller to it.

Notice that we are not playing by the total ordering rules that are required to e.g. sort() a list. You invented your own method name, so you get to make up whatever rules you find convenient for the task at hand.

    if anotherField.xpos == self.xpos and anotherField.ypos+1 == self.ypos:
...
if anotherField.xpos == self.xpos and anotherField.ypos-1 == self.ypos:
...
if anotherField.xpos+1 == self.xpos and anotherField.ypos == self.ypos:
...
if anotherField.xpos-1 == self.xpos and anotherField.ypos == self.ypos:


Ok, that's just tedious.

If you really want to follow that approach, at least define a vector of delta-coordinate tuples:

    for dx, dy in [(0, 1),
(0, -1),
(1, 0),
(-1, 0)]:


But wouldn't it be more convenient to def distance which returns the L1-norm Manhattan distance between the pair? Then you can just ask if it is exactly 1.

class Map:


This is a lovely identifier, thank you. Fair warning, in python land we try to avoid assigning to map, as there is already a map in the builtin functions. The convention is to append a trailing _ underscore to avoid shadow meanings:

• map_
• list_
• dir_
• id_

and so on. (Yeah, my apologies, python is a "small" language, but regrettably a bunch of common identifiers are already taken.)

The identifier Map is distinct from map, and is unlikely to cause confusion on the telephone, so this is perfect as-is.

# optional type hinting

class Map:
def __init__(self, data: list):
...
self.fields = list()


Yay, type annotations, kudos, very nice.

It definitely is valuable to know we're dealing with a list, especially for such an uninformative name as "data", so I thank you for that. However, for extra credit, you can also tell us what's inside the list:

  def __init__(self, data: list[Field]):


Without that, linting with mypy will be somewhat less informative, as the list in the signature and the ... = list() assignment both have type list[Any].

        a_field = Field(x, y, column == '0')


That's an OK name, but somewhat unusual in this ecosystem. Prefer to simply call it field = Field( ... ).

        y = y + 1
x = x + 1


Gentle reminder: There's no ++ increment operator, but we do have y += 1.

We seem to have a list-of-lists datastructure representing terrain, and that is perfectly fine. Please understand that there are alternatives.

A list of N elements costs N+1 pointers, which typically will be 64-bit pointers. One for the list, plus one for each object pointed at. So we represent the booleans [1, 0, 1] with significantly more than three bits.

Python offers arrays to efficiently store lots of things that have identical type. And if you go there, you're likely to go all the way to numpy's NDarrays, since they offer a great many convenience functions right out of the box. For example, filtering down to adjacent cells becomes very convenient. Storing N booleans in either kind of array will typically cost slighty more than 8 × N bits. That is, one byte per element is usual. If N is inconveniently large, consider using a bit vector.

# docstrings

  def expand_land(self, candidate, candidates):
print ("getting neighbour fields")
neighbours = list(filter(lambda field : field.isAdjected(candidate), candidates))

for neighbour in neighbours:
if(neighbour in candidates):
candidates.remove(neighbour)
self.expand_land(neighbour, candidates)

return None


This is a nice little recursive helper, thank you for breaking it out. A bit chatty perhaps, but I'm sure we can elide the debugs once things are working, or else use a logger at DEBUG severity.

Feel free to have the signature end with ...) -> None:

This method definitely needs a """docstring""" which explains that we mutate the final argument.

The return None at the end is weird; please elide it. Certainly it is accurate. But we don't write it explicitly when the intent is to evaluate for side effects. As written, it looks as though the method is supposed to return some useful value, and if it was a hundred-line method I would have to read through its various if clauses to see if return 42 appeared elsewhere. Consider the one-line def greet which prints Hello world. We don't make it a two-liner by finishing with return None. We let python implicitly do that.

# use appropriate datastructure

      if(neighbour in candidates):


First, this isn't java, please prefer:

      if neighbour in candidates:


Second, if I recall correctly, candidates is a list. Please understand that in will linearly scan the container, so we're probably looking at quadratic cost here.

Consider using a set for the candidates. Note that only hashable (typically immutable) elements can be stored in sets, or as dict keys, so sometimes it takes a bit of rejiggering to switch to a more efficient container.

# quoting

csvFile = 'resources\\island_01.csv'


Uggh!

Prefer a raw string:

csv_file = r'resources\island_01.csv'


Or, to be even more expressive of Author's Intent:

from pathlib import Path

csv_file = Path('resources/island_01.csv')


(You could give it a \ backwhack instead of a /` slash, it would be the same.)

Welcome to python land!

Recommend you routinely rely on tools like

to tidy up your source and keep it in good working order.

This codebase appears to achieve its design goals.

I would be willing to delegate or accept maintenance tasks on it.

• well. that was the very desired input i needed, thank you so much - all those beginner mistakes brought straight to the point :-) that is really valuable input for me. sadly i cannot +2, because if i could i would +5 your answer!! great! Commented Oct 3, 2023 at 4:26
• Great, happy to help!
– J_H
Commented Oct 3, 2023 at 5:20
• man - python != java its really hard to switch! i understand how it should be but i always fall back into my java style ^^ (hahah 20 years of experience have left their mark ^^, oh yes, since java 1.2) Commented Oct 5, 2023 at 7:00