20

Magic Numbers Your biggest issue is too many magic numbers and hard-coded information. If you added at the top of your script: ROWS = 5 COLUMNS = 5 then you could have: def create_random_ship(): return random.randrange(ROWS), random.randrange(COLUMNS) Game board can be created with: game_board = [["O"] * COLUMNS for _ in range(ROWS)] And you can ...


10

Reduce global variables The game_type function can return the ship row and column, there is no need to modify them as global variables, my suggestion is easy to implement: def game_type(game_input): if game_input == 1: ship_row = random_row(board) ship_col = random_col(board) elif game_input == 2: ship_row = int(raw_input("...


10

Just a quick comment for now. Unfortunately, except ValueError or TypeError does not do what you think it does. Python tries to be readable English where possible, but here it doesn't quite work. The parser parses this statement as: except (ValueError or TypeError) where the second part follows the rules of the or operator. If the first argument is truthy, ...


9

Variable naming In general, you should follow PEP8, the Python style guide. In particular, it suggests using lowercase for variable names. That is, rows instead of Rows. User input conditions while (Rows > 10) or (Columns > 10) or (Rows <= 0) or (Columns <= 0): Rows = int(input("Please enter the number of rows you want. \n")) Columns = ...


8

First Priority: White Space for Readability and Logical Section Separations, and PEP8 Rules It's defined in the PEP8 style guide, and also makes it much MUCH easier to read your code and have logical separation between differing sections of code. This makes it so others can read the code without straining our eyes or being forced to try and figure out ...


6

Inconsistent use of var I love var, I use it pretty much everywhere. IMO not using it makes C# read like Java, and that can't be good. But that's just my opinion, and given a code base that doesn't use it, I'll conform to the style in place and keep its [non-]usage consistent - because whether you choose to use it or not, what matters is consistency. With ...


6

My biggest criticism of your code is that it isn't "pythonic". What I mean by that is, it does not take advantage of the language features of Python that make it easier to write code and reduce your chances of error. Said features also make the code a lot shorter and easier to read. For instance, let's take your initial function LoadMapFromFile(). I see a ...


6

1. Suitability for inspection Tom Gilb and Dorothy Graham (in their book Software Inspection) recommend that you inspect code in batches of about 250 lines. They observe that in practice, as the batch size increases, the quality of the inspection (in terms of number of defects discovered per line of code) falls, because people have limited powers of ...


6

Style Code style is not "mission critical" for a beginner, but can become more important when working on larger projects. Python has an "official" Style Guide for Python Code, which is widely followed in the larger Python community. It codifies a lot of recommendations on variable naming, whitespace, and the like. As an example, the official recommendation ...


6

I think the place to start is the function boardFunc. Until you get comfortable with "functional programming" and dependency inversion, you should probably never write a function that takes an argument telling it what to do. We can rewrite that function as # Set up the player board for a new game. (100 "0"s) def boardReset(): global board for x in ...


5

Here are some observations: no meaningful comments or docstrings (they are not always needed, but imagine there is that "violent psychopath, who knows where you live" will be reading the code) PEP8 violations and making things Pythonic: blank line between class methods, two blank lines between top-level functions and classes (source) correct import ...


4

Here are some comments to your implementation in no particular order: Avoid intermixing top level code and functions – In your code you do mix the top level code like for x in range(5) with the following function declaration of print_board(). It is better to collate all the function definitions, and then call them from within the if __name__ == '__main__': ...


4

I'll go into the 'Little OOP' remark. In OOP you try to model your object to real world stuff. Objects In this assignment, you have several objects: Board Player Ship MissileShot MissileResult (hit / miss) Location In the assignment you see some attributes of there objects, for example the board that has a size, the ships that have dimensions and ...


4

This one's common it seems- Whitespace Python relies on whitespace and proper indentation to be readable; both for the developer(s) and people from all over the world who view the code. You should try to add whitespace to seperate logical pieces of code from one another. Especially your load_game() function needs this. I've rewritten it so it's more ...


4

General In build_game_board() you shouldn't use item in the for-loop. Instead, use an underscore: for _ in range(5): # something This is common in loops where you don't need to access / modify each value. You can shorten player_turns() (which I would call take_turns()): def player_turns(total_turns): if total_turns % 2 == 0: total_turns +=...


4

Based on my first not too thorough read, this looks rather nice. I currently only have two points of critique. The first point is the lack of explicit interfaces. I think both the PlayerGrid and the Grid Coordinatescould use an explicit interface, IGrid and IGridCoordimate say. While it may be reasonable to invent different grids, it might look odd at first ...


4

Sorry, wrong number Penaly - it could be a good faith typo. And the valid range should be displayed w/ the error message. Edit This edit is about moving functionality out of Main() and into Battleship class where it belongs. The themes are rigorous encapsulation, using game terminology, and Single Responsibility Principle. Battleship.Fire() Battleship ...


4

Thanks for sharing your code. what I like you follow the Java Naming conventions you resist to solve the problem based on an array what I dislike violation of encapsulation / information hiding You access directly properties of an object and (even worse) their properties too like this: x == ship.shipGeographic.x && ship.shipGeographic.y <= y ...


4

Procedural Programming In general it uses data-structures to solve a problem. Data-structures are like lookup tables, in which it is possible to save, modify and read values. Working with data-structures could look in java like class Stroke { /* ... */} class Color { /* ... */} class Rect { double width; double hight; Stroke stroke; } Rect rect ...


4

Use your own Interfaces You've created both your Ocean class and Radar class to accept a point key in the __getitem__ and __setitem__ methods. Ie) def __getitem__(self, point): row, col = point return self.ocean[row][col] def __setitem__(self, point, value): row, col = point self.ocean[row][col] = value But in your Player class, you ...


4

How can i prevent that ships may have the same coordinates? In other words, you want to pick three distinct locations on the board randomly; one for each ship. Sounds like you want a random sample (without replacement) of size three from the population of the game board coordinates: from random import sample from itertools import product ROWS = 5 COLUMNS =...


3

I'd just like to add a few more points on top of what @RobAu has already suggested in his answer. It's considered an anti-pattern / bad practice to use exceptions as control flow. In your program, you throw / catch an IllegalArgumentException to re-start the user input process on an invalid input. I would recommend that you simply continually prompt for a ...


3

all switch-case statements are the same, just invoked on different variables -> put the statement in a method and pass it the variable to query. Moreover, the whole switch-case statement can be replaced by getting the index of the int-location from the String "ABCDEFG": locationStr = "ABCDEFG".substring(locationInt, locationInt+1); and you can catch the ...


3

In your SaveScores function, you should use with, to ensure that the file is properly closed (even when something raises an exception). You could also use writelines with a generator expression and tuple unpacking. I would also rename the variable (well and the function, but you said that format was given). def SaveScores(scores): """ This function ...


3

1. Comments Your comments aren't very good. But conveniently, your code is structured so as to make this critique easy: from random import randint # Imports randint function board = [] # Creates global board list for x in range(5): board.append(["O"] * 5) # This function adds 5 "0"s per cycle, there are 5 cycles def print_board(board): for row in ...


3

I wont extend much, I just have one thing for you. It is nice to comment and give explanations about what you are doing, really nice. You'll see that when you review your code 3-4 months in the future those comments will help you. print_board(board) # Prints the board This comment tho... give explanations about what you feel needs explanation, don't overdo ...


3

Disclaimer: I haven't tried compiling any of the code changes I've suggested, so if you try something and it doesn't work please let me know and I'll correct it type Direction = Horz | Vert type ShipType = | AircraftCarrier | BattleShip | Frigate | Submarine | Minesweeper I would not encode ship types as a union type. Generally, union types ...


3

There's a way that you can reduce the amount of code in create_grid. If you use the * operator on a list/string and a number then it duplicates it. Take the following: >>> 'na ' * 16 + 'Batman' 'na na na na na na na na na na na na na na na na Batman' >>> [' '] * 5 [' ', ' ', ' ', ' ', ' '] This will allow you to remove the entire for col ...


3

General feedback This code looks okay, though it could utilize more features of ecmascript-6, like for...of loops and default parameters (see below for more detail). Nothing jumps out as far as a way to consolidate the code in those two methods to check the collision positions but I wonder if you could consider flipping values in one of those instances in ...


3

I'd say that game_board = [["O", "O", "O", "O", "O"], ["O", "O", "O", "O", "O"], ["O", "O", "O", "O", "O"], ["O", "O", "O", "O", "O"], [ "O", "O", "O", "O", "O"]] for i in game_board: print(*i) produces a neat display: O O O O O O O O O O O O O O O O O O O O O O O O O which is an awesome way of ...


Only top voted, non community-wiki answers of a minimum length are eligible