New answers tagged object-oriented
2
Segragated the responsilibites of call center into call center and employee
from abc import ABCMeta, abstractmethod
from random import random
class CallCenter(metaclass=ABCMeta):
"""
A call center class that will be inherited from by the director
Attributes:
name: Name of the employee
number: Mobile phone number ...
3
If I had to make this OOP, I'd begin by hedging concerns :
a class for user data handling (retrieving the POST and using it)
a service (that you'd dependency inject) to handle the configuration of the Google Service
a GoogleUser class for encapsulating the result of the service and abstracting all those foreach
Then, orchestrate those in some kind of ...
2
I have been working on this code to meet the best OOP standards for production.
I think your biggest problem here is that PriceCalculator does too many things:
Getting user input
Calculating the price per checkout item
Calculating the total price
Printing the bill
Let's try to split this up a bit first by introducing a BillRow for each line in your Bill ...
0
I see no benefit to any of these approaches. You're just making yourself write more code. All you need is one class that analyzes a website entered from a user. If you have a predefined list of websites that you need to scrape, then iterate over each website instead of using a class for each one.
Since you say there are two ways of accomplishing this task, ...
1
Amazing work! As PeterT mentioned very little to change in your code, however with that said I did notice some code that keeps repeating itself, not sure if this is intentional.
Checking if the Model is null
If Not this.model Is Nothing Then, this code appears 6 times through out the UserForm, I believe you can validate if the model is null in your ...
0
Hmm, since your values are symmetric, it would be better to make the code better by using the "DRY"(Don't repeat yourself) principle. You can separate your data from the code and iterate on all species to avoid code duplication. I would suggest to store all species data in a json file and then use a for loop to iterate over them. Now in one of my ...
2
A warning on version
Many of the following suggestions assume that you are using Python 3.
Bare inheritance
class SegmentationType(object):
can be
class SegmentationType:
Abstract statics
DISPLAY_NAME = "invalid"
should not really assign a value. Instead,
DISPLAY_NAME: str
Unpacking imread
The documentation is deeply unhelpful: it says that ...
4
Overall
This code is fairly well-structured and very well-documented. Excellent job.
Type hints
It's good that you specified types here:
L : int, float
Length of the reactor/lamp [m]
r_outer : int, float
Outer radius [m]
r_outer : int, float
Better would be to codify these in type hints:
def reactor_volume(
L: Union[int, ...
1
I don't know about this typevar but it seems over complicated. Why are you specifying the type and using two classes when you need only one. Here's my solution.
class Node:
def __init__(self, val):
self.val = val
self.left = None
self.right = None
def add(self, val):
if val < self.val:
if self.left == ...
0
To Your Questions
Is there a benefit to using Classes? Right now I don't see the benefit of having a handle, like, player1 = new Player(arg,arg,etc...), player2 = new Player(arg,arg, etc...)...
Maybe there are benefits, but I can't think of any serious ones now.
The current version is similar to the factory pattern, where the creation of an object is ...
1
Default Parameters
There should be no spaces before or after the =.
def __init__(self, chips, bet=0, split_cards=False):
Hand.__repr__
You can utilize the .join function to append to strings.
def __repr__(self):
return "Hand()\n" + ''.join(f"({card.value}-{card.suit)" for card in self.hand)
Hand.card_visual
You can use list ...
1
The review will be focused on readability and testability.
Naming
I read (I think it was in Clean Code) that variables should not have a postfix like Data and Information and instead we should give things a name that they represent.
PortfolioData is actually a Portfolio while your current Portfolio is more a PortfolioRepository or PortfolioCollection.
...
5
Deck.reset bug
This function has no access to num_decks, so if you decide to call this function it will fail with a syntax error. A simple fix would be to define this as an instance variable in the constructor, i.e self.num_decks, and use that.
validate_answer
This function should be written like this:
from typing import List
def validate_answer(question: ...
5
system()
replace os.system with a call to subprocess, which for all intents and purposes replaces the former.
Type hints
def validate_answer(question, choices):
can be
def validate_answer(question: str, choices: Sequence[str]) -> str:
The Sequence is appropriate because you need choices to be both iterable and indexable.
Global variables
yes_no = ['y',...
1
Naming
I'm sure you already know this, but naming matters. The few keystrokes that you save using w are paid for every time somebody reads the code. Consider always using more meaningful names withdrawalAmount etc.
Be cautious about reusing names of classes from the standard library. Rather than Consumer, consider giving it a more specific name to reflect ...
0
Note- the review below only pertains to the code in the post. "Code that's not included can not be reviewed."1
Overall I would say this code is well-written. Variables are declared with const and let appropriately. It has good documentation - especially for each method- although example usage is tough to find. One possible change you could consider ...
3
Unless you have a valuable/deliberate reason to use and in your condition statements, I recommend consistently using &&. Condition logic can sometimes fall prey to unintended "precedence" complications.
This whole line is "jacked" (wrong / incorrectly coded):
if ($this->ValidateDate($testdate[0], false) && $this->...
2
First of all, to get that out of the way, I despise var and static imports. For me, it makes the code an unreadeable mess that looks like javascript. You would not be allowed to do that on my team.
Now regarding the concrete code:
It is not a CSV reader. For real csv, you'd need a quote character and a way to escape the quote. Usually this is done via ...
2
I have some suggestions for your code.
Use a conventional name for the static factory method name
In my opinion, the of name generally aggregate a given set of data into a container; this can cause confusion in this case. I suggest to rename the method to create or newInstance.
public static <T> CSVReader<T> of(BufferedReader reader, Class<T&...
1
Return Value
The method addUser returns false for all negative cases. Below it is possible to see that it does not return true for the positive case.. instead it returns nothing which leads to an undefined:
else
{
$('#loadingDiv').show();
$.post('process/editUser.php', {uobj:uobj}, function(data)
{
var modal = "#addUserModal";
$('#...
1
snake_case
The PEP8 standard is for these:
getTargetFileNames
currentWorkingDir
to be
get_target_filenames
current_working_dir
and so on for your other functions and variable names.
Fail-safe directory reversion
os.chdir(currentWorkingDir)
should be in a finally, with the try after the first chdir. That said, you shouldn't be changing the working ...
2
I suspect the root of your problem is that you're working at the wrong abstraction level.
Specifically, looking at the Telegram Bot API documentation, it seems that an Update is just a wrapper for one of several unrelated objects that describe different kinds of events that your application might want to react to. Trying to treat all of these events as if ...
1
I see you removed the calls to $('#loadingDiv').show(); from the many conditional blocks and moved that before the call to the AJAX call - I was going to suggest that.
I see the ecmascript-6 features like the Class syntax are used, so other ES-6 features could be used - like the short-hand syntax - i.e. on this line:
$.post('process/editUser.php', {uobj:...
4
The other solutions gave an answer which is perfect for two classes.
If you get to much functions//classes beside CallBack and Message, the code below might give you a better solution.
Wrapper
I personally would create a wrapper with a common interface.
The reason is that this is not very extendable (adding a new class requires rewriting all the common ...
10
Assuming Update#getMessage() is of type Message and Update#getCallbackQuery() is of type CallbackQuery:
You can make a function getUpdateAttribute that receives higher-order functions as handlers for the situation where your update has a message or callback query as follows.
private static <T> T getUpdateAttribute(Update update,
...
15
Keep it like that. It is perfectly readable, easy to understand, everything is fine.
Naturally you could do some lambda trickery along the lines of:
public static String extractFromMessageOrQuery(
Update update,
Function<Update, String> messageExtractor,
Function<Update, String> queryExtractor,
String defaultValue) {
...
3
The real power of OOP is not that you can use classes as namespaces to bundle functions that belong together. The main power is polymorphism, meaning that you can inherit from classes and use subclasses interchangeably with their parent classes. This means that you want to be able to do something like this:
class UnitaryMatrix(Matrix):
...
a = Matrix(......
2
I don't see any value in writing a private method that fetches $_SERVER data. I mean, all you have to do is nominate the key and the value is accessed and I can't really foresee the need to change the source of the data -- so avoid the unnecessary overhead.
Running with YCS's advice to only rely on REMOTE_ADDR, that condition block can be reduced to a ...
10
is it well-written?
Well, not really... it is very redundant. A commonly accepted principle is the Don't Repeat Yourself. principle. For example, the following block:
if (getenv('HTTP_CLIENT_IP'))
$mainIp = getenv('HTTP_CLIENT_IP');
else if(getenv('HTTP_X_FORWARDED_FOR'))
$mainIp = getenv('HTTP_X_FORWARDED_FOR');
else if(getenv('...
15
IP Detection.
You are confusing an IP address with an HTTP header, the latter being utterly unreliable. It is ridiculously simple to fake an HTTP header. There is an old cautionary tale by Anthony Ferrara about how he unintentionally hacked Stack Overflow by manipulating headers. You may want to read about it. Not to mention that even if you catch one of ...
1
I tackled the book a time ago, skipped this chapter and came back now since this topic is highly/excessive used in the Qt Toolkit - even tho its a while ago.
Anyhow, you asked for:
I'm especially concerned about my use of dynamic_cast, as that isn't
covered yet and I suspect there's a better way.
Yes, by declaring the compare in the Comparable class itself,...
5
You are right about using namespace. The purpose of a namespace is to allow types and functions to have the same name in two or more areas of code. Think about the class configuration, its quite a common name for something in code so if you and I are both writing a module for an all singing all dancing app and we both have a configuration class we are ...
2
So, you would like to know if your code is "following the Single Responsibility
SOLID principle". S.O.L.I.D. stand for Single responsibility,
Open-closed principle, Liskov substitution principle, Interface segregation
and Dependency Inversion.
Single responsibility
One class have a single responsibility. But both Payroll and Employees are
mixing ...
Top 50 recent answers are included
