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.
shadowing builtins
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:
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:
print (f" adding {neighbour}")
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.
