New answers tagged object-oriented
0
Docstring
I see that you had some docstrings for some of your methods. But I didn't see any for your BankAccount class, or for the module. You should include these as well.
Bank Structure
You take input within the deposit and withdraw methods inside the class. These methods should only have one function: adding and subtracting money from the account ...
3
Hello and welcome to Code Review! I try to give a possible explanation of what the reviewer meant when he was talking about inverse class structure; in the main method this code is present:
MyAlgorithm myAlgorithm = MyBuilder.builder()
.withCipherSet(CIPHER_CHAR_SET)
.build();
But from your code MyAlgorithm is a subclass of ...
2
I disagree on you with the need for a wrapper around the stdlib argparse, but that's a matter of taste. And I don't see anything wrong with your implementation of the rather thin wrapper.
Classmethod as Constructor
This is NOT what you're doing. A constructor creates an instance in a way somehow different from the standard and returns it. An example:
...
3
Using @classmethod for constructors is perfectly fine. It is basically the only reason you ever need to use them.
Classes create a namespace (and Namespaces are one honking great idea -- let's do more of those!). You don't need to preface all your methods and classes with my_. Your code becomes much more readable if you just name them according to what they ...
2
is there a better way to achieve this functionality?
You might use a wrapping function:
let node = document.getElementById('myFavoriteNode');
node.addEventListener('click', event => Wyg.Editor.nodeClickedEvent(event));
// or, using regular function expression
node.addEventListener('click', function(event) {
return Wyg.Editor.nodeClickedEvent(event);
}...
4
Accessing statics via this
Defining the object reference outside the object just to gain access via a miss used accessor (this) is a hack and not how to use static objects to handle events.
Binding objects to a function
First what you did could have been a little less complicated
You had something like
class Editor {
static nodeClick(event){
...
2
The getter method Weapon.timePerShot will return 1 / this.fireRate. There doesn't appear to be anything to prevent fireRate from being 0. If that is the case, Weapon.timePerShot would return Infinity, which would lead to whenCanFireAgain also being set to Infinity. In some programming languages dividing by zero would lead to an exception being thrown or a ...
6
is there a better way to achieve this functionality?
You can use Function.bind() to create a function with the this context bound to Wyg.Editor:
const node = document.getElementById('myFavoriteNode');
node.addEventListener('click', Wyg.Editor.nodeClickedEvent.bind(Wyg.Editor));
See this demonstrated in the snippet below.
Notice that const was used ...
5
There is not much I would add to dustytrash's answer:
prefer const over let when a variable does not change
use blank lines only for creating logical regions; to me, an assignment to an instance variable is not a logical region
reload (startTime = 0) {
const whenCanFireAgain = startTime + this.reloadSpeed;
this.bulletsLeftInMagazine = this....
6
I understand semicolons in JavaScript are optional, however I believe you should always use them. It's easy to have something break later on because you neglected a semi-colon and it lost the whitespace which saved it before in a compression/generation/eval parse.
You could declare the timePerShot as a variable, to ensure the amount is not being calculated ...
8
I don't think that what you're doing is good style. I think you're adding a lot of extra complexity and places for things to go wrong, or become hard to understand, or whatever. I don't think you lose anything by just making very small methods that call super and your special method. I talk about those options at the bottom of the post.
With that out of the ...
3
Your marker interface has no added value. In fact, it's an anti-pattern because you provide a generic type parameter on an interface ICommand, only to enforce it on another interface ICommandResult.
public interface ICommand<TResultData>
{ }
public interface ICommandHandler<TCommand, TResultData>
where TCommand : ICommand<TResultData>
...
0
Fixing the white space alone will improve readability.
Use private variables instead of protected when you don't plan to use the variable in an extended class.
Use curly braces even when the executed code is only one line long.
If you want to display the errors in a console, you can use console.log inside of a <script>. Such as this:
$errorMessage;
...
1
Close on destruction
You shouldn't need to explicitly close your file member on destruction. Since it's a safe variable and not an unsafe pointer, fstream's own destructor will be called and the file will be closed.
Const members
You should make path const, since it doesn't change over the course of the object's lifetime.
File object scope
inFile is not ...
2
I don't quite know what lambda calculus is (I'm assuming it's a mathematical annotation for what we might call "purely functional programming"?), but I'll give this a quick shot.
First, I'd love to have env populate itself if not provided. You really shouldn't have mutable default values for functions, though; the typical practice is to define:
interp(self,...
3
Use the force library Luke!
Your code does use std::vector, but for reasons I don't understand, it also uses new to allocate a manually managed dynamic array as well. You don't seem to gain anything from this, so I'd advise using std::vector throughout.
I'd also look at the standard library's algorithms. Some of them (e.g., std::remove_copy_if) could be ...
0
First off I must say that the indentation is quite inconsistent and makes reading this code challenging. While most of the inconsistencies appear to be indentation on braces, the indentation on comments also seems a bit wonky. While I don't adhere to everything in it, I suggest following PSR-2 - especially using 4 spaces for indentation.
One thing I ...
4
Or since the check functions build on previous functions should I structure it the following way
This is not a case that makes you go for inheritance. You should go for inheritance to solve the problem of "repeating same properties in more than 1 class", for example, you have 2 classes Lion and Cheetah
class Lion{
protected $speed;
protected $age;
...
7
If you let me to be frank, this is not a class but rather a ratatouille - that is a random collection of functions.
I do understand your idea and a chain of thoughts and in a way you are trying to do the right thing - to achieve the main goal of every programmer - to reduce the amount of code written. And it's for the good you started with OOP.
However, ...
1
Since writeTo does not modify v, it should take that parameter as a const reference (const std:::vector<T>&v).
In LineEditor, the constructor doesn't initialize ary, so if you construct a LineEditor object and destroy it without calling removeLines (or if that function returns, possibly by an exception, before you allocate the memory) you'll ...
3
Coor has the comment coordinates ... yeah, that's exactly what the
name should be then. But actually, Point seems easier and doesn't
have to be abbreviated, or perhaps be more general and say Vector, or
Vec2, that seems fairly common for games (despite it being an
abbreviation). Not using the AWT class makes sense to me too.
The hashCode method
is okay,
...
3
I just have a couple of minor points to add to Olivier Jacot-Descombes' excellent answer.
public override bool Faulty()
{
if (LogicalOperator == eLogicalOperator.Equal)
return Value == Threshold;
else if (LogicalOperator == eLogicalOperator.GreaterOrEqualThan)
return Value >= Threshold;
else if (...
5
A design pattern often used, is to have a non generic base type and a generic type derived from it or a type implementing a non generic as well as a generic interface. An example is
public abstract class Comparer<T> :
System.Collections.Generic.IComparer<T>, System.Collections.IComparer
You can see the full implementation here public ...
2
Here're are my suggestions:
Remove ConverterContract and ConverterActCompletion These two classes are unnecessary wrappers to hide the inner converter. You're saying that you're doing this to hide Fields and Dict so here's the next suggestion:
Make Fields and Dict protected You don't want the outside world see them? Use proper access modifiers like ...
2
My observations...
Leave generated classes alone You have modified Contracts by adding a custom constructor to it. You'll loose this changes when you regenerate the model. This class is partial not without a purpose. If you want to customize it then you should create another file in the same project and within the same namespace and define your own partial ...
1
Those are some very thorough class definitions, with many getters and setters. For context: I have some music experience (e.g. 22 years playing clarinet and saxophone, 18 years playing acoustic guitar) but wouldn't consider myself an expert in theory. I've gotten used to transposing from C to Eflat and Bflat in my head. I haven't really learned about coils ...
3
Compiler error Unless you're using some experimental preview C# version, your code doesn't compile as this line:
Value = singleValue ?? throw new ArgumentNullException(nameof(singleValue));
yields an error
CS0019 Operator '??' cannot be applied to operands of type 'T' and ''
You would need to add a constraint for that like: where T : class
Nested ...
3
It's a bit counter intuitive you store Value as Object if the class IndexableOption<T> is generic. I understand you are trying to find a common type to store both T as T[]. However, if T : struct then what happens in case Value is T is a thing called boxing*.
From reference source:
Performance
In relation to simple assignments, boxing and ...
4
Other developer-oriented issues such as variable namings, documenting, commenting, and such are not really a concern here since it's just for practicing algorithm and data structures, and have to follow the tutorial as much as possible.
I feel this is a bad idea, this is as it promotes detrimental core habits.
Myself and a friend used to play a purely ...
1
If all your configuration data are static, then why not using constants ?
For instance :
// A.h , without header guards for brevity
class A {
int x;
int y;
int z;
public:
A() : x(0), y(0), z(0) {}
void load();
}
// A.cpp
const int A_config_x = 1;
const int A_config_y = 2;
const int A_config_z = 3;
void A::load() {
x = A_config_x;
...
2
Docstrings
You should have a docstring at the beginning of every module, class, and method you write. This will allow documentation to identify what your code is supposed to do. You're on the right track, having comments above the classes and methods. Now, just move those comments inside these classes and methods at the very beginning, inside triple quote ...
2
Docstrings
You should include a docstring at the beginning of every method, class, and module you write. This will allow any documentation to identify what your code is supposed to do. Pulling from GeeksforGeeks, this is what a docstring should be:
What should a docstring look like?
The doc string line should begin with a capital letter and end with a ...
3
I can understand why no one has yet answered your question. The basic problem is that your User Class is not really an User Class. All it does is process user input, and that's not something an User Class should do. An User Class should deal with an user, nothing else. Things like:
Changing user name and/or password.
Verifying passwords.
Keep information ...
1
It looks very procedural at a glance, it might just not be useful to
use custom objects here. In any case you don't need to explicitly
inherit from object, class Process: is fine already.
So, more maintainable, well, the headers look dubious, but obviously
chosen from a real-life browser. The character set / encoding values
would be the most concerning to ...
0
Starting cross-out from size_t multiple = 2 * i is a serious pessimization. Every multiple of i with other factor less than i has already been crossed out during previous passes, which dealt with these smaller primes. You should start with size_t multiple = i * i.
Notice that computing i * i eliminates the need to compute sqrt. The outer loop written as
for ...
2
In general, we try to keep functions small and nice, but in your case, using so many functions is just weird. For example, are you sure the uncrossedIntegersUpTo function is needed at all? How about determineIterationLimit? You can just handle them in the main function.
Also, std::vector<bool> is not like the normal vector. It is not a container ...
Top 50 recent answers are included
