10

There is one issue with your approach which is, in the case of multiple import failure, only the last one can be properly reported: >>> try: ... from .foo import Foo ... except ImportError: ... from .base_engine import Default as Foo ... setattr(Foo, 'name', 'Foo') ... >>> try: ... from .bar import Bar ... except ImportError: ... ...


9

First: welcome to CodeReview! I hope that you get good feedback. About your code. Your usage of Python is not terrible; most of your problems are conceptual, about the decisions you've made about your class. Looking at the method signatures from the outside, one would guess that addRate() can be called multiple times per single Bank instance, and that ...


7

That's an odd use of the word "default", which, etymologically speaking, means to remove failure, and generally refers to a fallback setting that "just works". I suggest calling it Missing instead.


7

This answer builds upon the solution presented by Mathias Ettinger If you want to get rid of the try: ... except: ... boilerplate you could implement some kind of custom loader for your engines. This is something I wrote quickly, which is likely far from perfect, but it works. in __init__.py: import importlib def mock_missing(name, msg): def init(...


6

Whenever you find yourself trying to replace a standard language feature, the first thing you should think is, There's probably something I'm missing. What's the standard approach to this kind of problem? Your code feels inelegant because you're not using a standard approach. When your code starts to feel weird like that, it's time to take a step back ...


6

I'd still say that you should break apart the mail message and SMTP client into separate classes for the "Single responsibility principle". In specific: A class should only have a single responsibility, that is, only changes to one part of the software's specification should be able to affect the specification of the class. This is usually where you have ...


5

Your two main issues have not been resolved. Separation of Concerns I still find it hard to split the UI and the program logic although I have created a static Screen class for that. Even though you have tried to offset UI operations to a dedicated class Screen, your code is still full of mixed concerns logic (presentation vs application vs test). ...


4

That's a difficult problem to attack. Without going into those difficulties, here are some comments on your code. Global variables are generally frowned upon. They are basically a hidden function parameter, which can make it hard to debug and maintain the software. So, for example, it would be better to explicitly pass file as a parameter of ...


3

Consider a functional approach I have been coding Java for a long time before I started with Javascript so I understand the urge to write Object Oriented code, create classes etc. However, why don't you try a functional approach for a change. I bet that once you get the hang of it, you will not want to go back. I know I don't. Javascript already had some ...


3

Your User class should only be dealing with things about that user, not about how you store it in a list. You are also referring to a variable outside your class, so you are locked to this, which makes it less reusable, which is one of points of classes in the first place. So get rid of addUser and getUserListAboveTwenty from your class. You don't need to ...


3

Dict updates This: if battalion_type not in self.battalion_strength: self.battalion_strength[battalion_type] = change else: self.battalion_strength[battalion_type] += change can be done more easily in a few different ways. Perhaps the easiest is to make battalion_strength a defaultdict(int). Then, this if goes away and you can "naively" do +=. ...


3

Your code has trailing whitespaces. Remove them. On Emacs, for example, I use: M-x delete-trailing-whitespace There is much debate on #pragma once. (See, for example, #pragma once vs include guards?) Personally, I don't use #pragma once, but I am not opposed to it either. Other people may disagree. You put your code into the namespace math. This is ...


3

There are a few smells in your code. Currency exchange rates are usually not inverses of each other. Your code assumes that GBP / USD is one number, and that number is a property of your Bank object. That is a smell, because a. banks don't own exchange rates, and b. there should at least be a GBP->USD rate as well as the USD->GBP rate. What if you want to ...


3

Since this is a lot of code, this answer might expand over time or leave parts to other capable members of this community. Style Your overall style is quite good. You seem to follow most of the guidelines presented in the official Style Guide for Python Code (the infamous PEP8). However, there are a few things that might need a little tweaking. The thing ...


3

So many good things, so many weird things. First of all, I love your comments. They made this review very easy. With that said, I have a couple major complaints: dict is not very type-safe; it requires you to access common properties by strings. I highly suggest storing those in a class instead, maybe something like AnimalProperties? That would be the ...


2

First of all I would suggest you changing PairCurrency constructor from class PairCurrency: def __init__(self, name): self.name = name to class PairCurrency: def __init__(self, name, ratio): self.name = name self.ratio There is literally no reason to make it different. This is not a functional class but rather ...


2

Set? You have logic preventing duplicate currency pairs. The easier (and more performant) thing to do is to simply represent pair_currencies as a set instead of a list. When you add, it will automatically discard duplicates. snake_case The standard is to name methods like your last_pair - i.e., get_rate. Lookups You have a loop in getRate and editRate ...


2

Inspector! Why did you allow Gregory Arstotzkaya, a Kolechian spy, into our glorious nation? Or Ilyana Dbrova, whose documents were perfect forgeries, except for a gender mismatch in her ID card? And why did you fail to apprehend Karl von Oskowitz? Treating documents as a single piece of text and just checking whether they contain certain words is ...


2

In addition to the excellent points AlexV said above, her are a few more observations: There is quite a bit of repetition in the bulletin parsing code code. For example, a check is made to see if "Accept citizens of" if in a line in four (4) different places. Each of the update_xxx functions loops over all the lines in the bulletin and checks to see if the ...


2

As your code stands, I would opt for OPTION 2: USING ENUMS. Consider inheritance only when the derived classes have specific state/operations that are not compatible with other derived classes. Don't abuse inheritance when a simple property (in this case of an enum type) allows to distinguish a certain feature amongst instances of a given type. Example ...


2

Personally I tend to avoid creating classes, which do not implement any logic. Sometimes it makes sense or is even necessary, but always when I see such a class, I try to find out what operations are performed on its data and if they shouldn't be implemented as methods of that class. What I don't like about your code is the ProcessCart method, which pretty ...


2

Good job separating concerns Your tetris game-board model, piece and UI code is nicely separated in classes, so each one has its own responibility. Well done! Don't use magic values, use Enum public Piece(int f) This switches on a magic value of f. Piece is a excellent candidate for Enum! public enum Piece { L, J, T, Z, I, ... } Enum works as class ...


2

The call to System.out.println, which is an expensive operation, is performed every time the mouse is dragged. Every position change will print a line. The faster a position changes the faster it will print. Deleting this print call should fix the problem and make the console info more useful. A minimum difference condition to allow the call to print could ...


2

Action.h The Action class only contains public static data members. While it's not illegal, a class might not be the best way to go about it. Consider using struct, which is the same as a class but has publicas the default access specifier. Or even better, don't use a class or struct and instead, wrap it inside a namespace, which seems perfect for this kind ...


2

addnode(self, key1, key2, edge=0) The parameter edge is not used. Either remove it, or use it, but keeping it with a default value and not using it is obfuscation. self.graph is a defaultdict(list), so testing if key2 is not present, and setting that key’s value to an empty list if it isn’t present, which is the raison d’être for the defaultdict(list), ...


1

Fight the urge to squeeze multiple "things" into one line. Your script will be easier to read and maintain with all declarations and constructs occupying their own rightful place. Spend the extra lines, you'll be happy you did a year from now. Use curly braces to encapsulate language constructs (e.g. if-else and foreach, etc.), this combined with ...


1

No time to read through all the code for now. So just some random observations. In Coordinates.h: Include as less as poissible in headers: You include iostream but it is only needed in Coordinates.cpp. Generally you should include as less as possible in header files. If you include iostream in the header every class which includes Coordinates.h also ...


1

Class Library Console-based ATM client as a class library I wouldn't call your application a class library. Class libraries provide a set of reusable classes and interfaces. Your interfaces are black boxes that have no usability purpose other than being a trigger for some action that requires user interactive console interop. For instance, public ...


1

Loop like a native This: for i in range(len(lines)): if method_name in lines[i]: is an anti-pattern. If you need to iterate over lines, simply for line in lines: However, since you also do an index comparison, you might need for i, line in enumerate(lines): Also, that nested i/j loop does not need to be nested. As soon as you find where the method ...


1

I'll comment on two specific things I noticed while briefly reviewing (I hope I don't come across as harsh where I'm just stating observations): OOP: One thing that stood out to me immediately is the dictionaries. The beauty of OOP is that it allows object hierarchies, and for those objects to inherit properties from their base class. A good rule of thumb: ...


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