New answers tagged object-oriented
0
Do something about the code formatting (intendation) in this question. The code reads really awful.
Review of Piece
Piece has an instance variable of the same type, which is stored through the copy constructor, but unused in the class.
Many variables belong together x1, y1 etc.. Consider creating a class EuclidPoint which groups and x and y value. You can ...
1
You could simply add the task results to a collection. Different types of collections can be used:
List<ITaskResult>: You can add several results of the same type. You must enumerate to find a task of a specific type.
Dictionary<Type, ITaskResult>: Each task result type can be added only once. You can query specific task types.
if (results....
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), ...
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 ...
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 ...
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 ...
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 ...
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).
...
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 ...
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 ...
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 +=.
...
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 ...
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 ...
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 ...
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 ...
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 ...
0
I've just read all these awesome comments (some a little bit too angered with a foe that's learning), and wanted to share a path and a question.
I'm trying to make a rolling code inside a game that's open for the user (like a truly noob version of "rpg simple dice").
By now I've tried 2 ways, the first one that's working:
1st
import random
#f5 to ...
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: ...
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 ...
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
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 ...
0
0. Little Typo
thermometer instead of termometer.
1. Global Variables
let cel = "";
let fah = "";
Variables inside a global scope can be changed from everywhere.
1.1 Example
Imagine temperature.js is your script with the global variables inside. Now someone adds a table.js to display tables on the html page.
<script src="temperature.js"></...
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 ...
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 ...
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 ...
1
Two things here,
Remove your extra empty lines as its hard to read.
Use PHPDoc comments for each function, it's like JavaDocs but for PHP and you can use it to generate documentation for your classes. So Like
/**
* Save the error to a log
*/
private function saveToLog(){
instead of
/* Save the error to a log
===========================================*/
...
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 ...
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 ...
1
First thing I'd like to tackle right away is the name convention for method names and variables.
In ruby, we use underscore_style instead of camelCaseStyle - this makes a huge difference.
Now, let's tackle the code itself. One thing that I see that could be improved here is the fact that you aren't leveraging the power of ruby collections and classes.
So, ...
1
I don't think there is a better than \$O(n)\$ and \$O(1)\$ to this problem.
However you can simplify the code a little (trivial) by subtracting from the expected total.
const findVal = nums => vals.reduce((t, v) => t - v, nums.length * (nums.length + 1) / 2);
Or
function findVal(nums) {
var total = nums.length * (nums.length + 1) / 2;
for (...
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(...
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:
... ...
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 ...
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 ...
0
Consider creating a static method within the class that performs all requirement checks and either returns the constructed object or NULL to indicate there was a failure in meeting all requirements.
class SSHConnection implements ConnectionInterface
{
//.... your class functions
public static function SSHConnection GetInstanceIfRequirementsMatched(/*...
Top 50 recent answers are included
