New answers tagged beginner
1
For a beginner this is some good code. I would make some additional adjustments.
One-line docstrings should be on one line, not over 3. And should end with a period.
You should only pass the marks to percentage and not rely on a global student_marks.
You can use sum to compute total_marks.
I'm not a fan of having format in the percentage function.
You ...
3
I'd recommend separating the logic of getting the user input from the logic of building and sorting the nested list.
Your code doesn't build a nested list in a way that's useful for solving the problem; I think what you want is a list of lists of names, where each list of names corresponds to a grade.
Any time you want to group things into buckets (e.g. all ...
3
Since testing falls under the general heading of code review, I'm just going to review your testing strategy.
The question says:
your function should be able to work with any list value passed to it.
but you are not testing this; you're only testing the single example that was given to you (and which, as it happens, you've hard-coded into your function)....
2
Your function isn't returning anything. You should be building a string and returning it from your function. str.join() will be useful for building your string, return it with the return keyword.
1
My first code - looking for feedback on best practices and things I should avoid doing in the future
Is there a way I can "extract" a value of a variable from another method?
A method can return a value, so you can assign the variable you need the value of to the call of this method. You just need to think about what j actually represents and what the action that is iterating j meaningfully is. In this case it's pretty clear; you are having player A draw a ...
1
Hellow K.H. thanks for the answer.
I rewrote the code using Dagger 2 for dependency injection and following your advice. Could you take a look at this code and answer a couple of questions:
How to correctly pass name to JsonSharedPreferences (Make a
setter, make a module that will provide name).
Is it correct to select this code as a separate component or ...
1
Why saving reference to Context and then create SharedPreferences each time? There's duplicate code for creating SharedPreferences You could just use context in constructor to create member SharedPreferences to use in your methods. If you want to keep it as it is, at least extract duplicate code into separate method.
I am missing null checks. I expect to get ...
1
This class is much more complicated than it needs to be:
class BasicCoordinates
{
private:
long long X;
long long Y;
public:
BasicCoordinates(const long long & _X, const long long & _Y)
{
X = _X;
Y = _Y;
}
long long getX()
{
return X;
}
long long getY()
{
return Y;
}
};
A ...
6
Aside from the notes already given, here are two big ideas to think about:
Don't repeat yourself (DRY)
This is something you'll hear repeated a lot in discussions of code. Any time you see the same "magic values" repeated more than one place (e.g. 0 and 20), or you see two lines of code that do exactly the same thing for exactly the same reason (e.g. your ...
3
To add to @Sara J's answer, in Python, it's generally a good practice to wrap your main code (so the last two lines) in a if __name__ == '__main__': statement so your script can be either:
Directly run
Imported and its functions used as the dev that imported it pleases.
https://stackoverflow.com/a/419185/1524913
Also, contrary to a lot of other ...
4
First off, some minor nitpicks on style.
In Python, variables are usually given names like_this rather than likeThis - most of your names are fine, but userInput should probably be user_input instead.
You usually want a space on each side of operators, guess = int(input()) is more pleasant to look at than guess=int(input())
Second, your program's behaviour ...
3
Your header guard is:
#ifndef ANY_HPP_INCLUDED
The style I most commonly see in modern code is
#ifndef INCLUDED_ANY_HPP
(or, just leave off the _HPP part). The reason is that technically, C and C++ reserve all uppercase names matching E[A-Z].* to the implementation, for macros like EINVAL and EPERM. Of course in practice your implementation won't have a ...
2
I see one place where you could use a C++ comment:
/* Test Any Implementation */
Because this comment only spans one line, I generally prefer to use a C++ comment instead:
// Test Any Implementation
1
Use XGetPixel() instead of writing your own function
You should use the API provided to you by X libraries whenever possible. The implementation details might change over time, so your own copy of XGetPixel() might no longer be correct in the future. Granted, this is very unlikely in the case of Xlib, but it is good practice in general.
Don't pass by const ...
2
One way to speed this up is to use a package designed for numerical evaluation, numpy. It is implemented in C and can take advantage of multiple cores if necessary. The only limitation is that you can only create arrays that fit into memory. At least on my machine (16GB RAM) your current values fit easily, though (if each integer takes 64 bytes, then I could ...
2
The problem of your code is the way you generate triangles. You just create three independent random numbers. In theory, you could always end up rolling numbers that don't end up in a valid triangle and thus your code would not even terminate. That is, because triangle side lengths are dependent.
To always create a valid triangle, you can instead roll two ...
2
Some comments:
Rename functions to follow the PEP8 naming convention, here.
Do not use range to iterate over the a list of elements, iterate directly over it. If you need the index, as in your case, use enumerate.
No need to wrap the boolean clauses in parenthesis, moreover you can use the truth(y) values directly, see truth value testing.
Is possible add ...
5
Use built in methods!
Your position_in_alphabets function can be reduced to one line. In fact, replace y = ... with the following:
alphabets.index(character) + 1
index returns the first occurrence of the character in question. Since you're working with the alphabet, it will return the position of the character in that string. Then you just need to add one....
3
Very nicely code.
Only some nits.
Help
"I wrote a manual page for it" --> Perhaps it, or a condensed version for option -h?
case 'h':
printf("blah blah\n);
exit (EXIT_SUCCESS);
> vs <
Conceptually, when looking for "smallest values", I'd like to find a <. Perhaps:
// if (roll[i] != 0 && min > roll[i]) {
if (roll[...
0
Displaying the grid
Personally, I would move displaying the grid into a separate function, such as display_grid:
def display_grid() -> None:
for row in game:
print(row)
This removes the need to keep having to write the loop over and over.
Picking the player
Another personal preference is that I would have the user input both the row and ...
2
Decision loops
Your implementation of a decision loop is... interesting to say the least. If you're using python-3.8, you can utilize the walrus operator. It's a way to assign variables within an expression. In this case, the while loop. Have a look:
def choice(prompt: str) -> bool: # replaced "yn" with "choice" #
print(prompt)
while decision := ...
6
Going to run through this code making edits and explain as I go:
You only ever use position to find the position within the alphabet; I think it'd be clearer to just make this function's purpose more specific and call it something like index_in_alphabet.
Having narrowed and defined the purpose of this function, it can be implemented much more simply by ...
7
I think it's a nice project. I would say that the main things for you to work on is getting further acquainted with Python's standard library and with standard practices, which is what most of my advice will be surrounding.
Minor improvements
For your alphabet, you could use ascii_lowercase from string, i.e.:
from string import ascii_lowercase
alphabet = ...
6
A warning
As a toy this is fine, but please do not use it (or encourage others to use it) for real cryptographic application. It is fun as an exercise, but will not be sufficiently strong to protect you against certain common attacks.
Strings as sequences
In Python, a string is a sequence of one-character strings. So you don't need to represent it as a ...
1
Is it well commented?
Generally yes, the only place I might add more comments is to explain the fields in the structure.
Is my solution portable?
No, it will not port to Windows easily because of the use of libbsd and the use of the unistd.h header file.
To improve portability it is also important to get comfortable with the memory allocation functions ...
3
I see some of the issues include copying the data and passing by reference or const reference. Might I suggest following the STL's method of using iterators?
Your any function is essentially std::find_if using a container instead of iterators.
Rewriting it to use std::find_if reveals the differences:
template<typename List, typename Predicate>
bool ...
2
Errorsatz already mentioned the Functor type, but I'd like to argue for Functor&&.
As for testing, this is a case where you literally want to test edge cases. I.e. where the first or only the last element has the desired property. Two additional special cases to test would be a single element, matching or not matching. (Zero-one-many rule of thumb; ...
10
For each word in the lexicon you are searching through each email: (11,314 emails) * (60 words/email) * (211441 word lexicon) = lots of comparisons.
Flip it around. Use collections.Counter. Get the unique words in each email (use a set()) and then and update the counter.
from collections import Counter
counts = Counter()
for email in x_train:
words ...
2
Your for loop can be reduced to one line, utilizing sum:
frequency_train = [
sum(1 if lexicon_train[i] in email else 0 for email in X_train) for i in range(211441)
]
It removes the need to create the initial list of zeros. For performance, I'm guessing the size of the lexicon and the number of iterations are slowing it down.
15
I think you did a pretty good job as a beginner. In addition to what Errorsatz said:
Consider making element a const reference to prevent unnecessary copying: const auto& element : elements
You missed #include <string> in the test file.
std::endl flushes the buffer and causes performance degradation; use \n instead unless you need the flushing ...
29
The algorithm looks correct.
Regarding the function signature, I'd make three changes:
You're not modifying elements, so take it by constant reference rather than reference.
You don't need to specify a pointer to Function, Function is already a template parameter, and non-pointers could be valid (ex: a class with a () operator).
"callback" isn't a ...
1
Well, hello. I was getting a bit bored today, so thank you for giving me a nice little exercise to work on. I edited the code, removing unnecessary lines and editing things here and there.
Since you specified that you don't yet know much about OOP I made sure not to take the easier approach, which in my mind is a vector of pointers to a virtual class named ...
2
Here are some things that may help you improve your code.
Don't abuse using namespace std
Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. Know when to use it and when not to (as when writing include headers).
Provide complete code to reviewers
This is not so much a change to the code as a change in how ...
5
Data safety
Your current HTML_search_attributes is type-unsafe - it's closer to a serialized format than an in-memory format.
Consider moving those data to a .json file. Deserializing it will give you exactly what you have now, though I recommend going one step further. Make a class or at least a named tuple to represent a scraped domain, with attributes ...
3
You are unfairly doing more work in the function approach!
Product = namedtuple('Product', ['domain', 'name', 'price'])
creates a new type every time the statement is executed. Consider:
result1.__class__ == result2.__class__
That will evaluate as False, for different result objects, even when returning results from the same site!
You should move the ...
2
Let's start with the trivial stuff...
A few minor stylistic points that are pretty much universal:
The type IO () is always written with a space between IO and (), never IO().
Similarly, multiple constraints are written as a comma-separated list:
(Fractional a, Ord a) => ...
rather than a chained list (Fractional a) => (Ord a) => ...
For do-...
2
The code generally looks good, but is a little rough to read (in part due to length but also with method names like turnIndicatorToggle(self) and toggleTurnIndicator(self)). But when I try to run your program I run into a few issues; your main window is fairly huge (bigger than my screen!) and can't be resized, and your indicators for tics and tacs (x and o) ...
1
The last component looks quite complicated. I think some refactoring would help with readability but I have no concrete suggestions since I don't really understand what it's doing. Like this here this.state.remove[this.module].
in the comments
I wrote some advice in the comments. Hopefully it's understandable. I mention direct state mutations a couple of ...
15
PEP-8
Class names should be CapWords, so instead of rectangle you should have Rectangle.
Commas should be followed by 1 space. You've mostly followed this, except in s1 = rectangle({'p1': (1,1), 'p2': (2,2)})
Bugs
The formula for "area" is not twice the sum of width & height.
I don't know what "surrounding" is, but the formula for perimeter is not ...
7
Following the advice from K.H. we get:
class Rectangle:
def __init__(self, dct):
items = dict.items()
self.start = items[0]
self.end = items[1]
self.width = self.end[1][0] - self.start[1][0]
self.height = self.end[1][1] - self.start[1][1]
self.midpoint = [self.width / 2, self.height / 2]
self.area ...
8
Class should begin with capital, so Rectangle
You are creating local variables just to set them to object on next line using self, assign calculation to self.xyz variable directly. For example:
self.area = (self.width + self.height) * 2
self.surr = self.width * self.height
Creating list of keys, then list of values to then map it to ...
2
You need to consider using classes more often.
For your current work, you need two classes (at least) to handle the application process. A class would be for holding the Questions and another class to hold the application logic.
// define the number of levels you need which would be used for random.
// so if the user chose Easy, this means it will pick ...
2
In addition to what @K.H. already mentioned:
Comments to the code
Things will get simplified if you represent the Board as a set of numbers
Board = set([1,2,3,4,5,6,7,8,9])
Then Board.remove(z) would look better than doing manipulations with indices and assigning an empty list (Board[z-1] = [])
The condition below is not really used:
while len(PlayerO) ...
1
I would create more classes:
An interface Operation with the functions:
getQuestionString
checkAnswer
at the moment, you don't need this.
Add this when you want to add questions with multiple possible answers eg. 3/4 and 6/8
getAnswer
next
checkAnswer takes a String: the one the user answered and return if it is the correct answer.
You then can create ...
2
The good thing is, that you have made some meaningful methods with descriptive names.
But you have a quite peculiar workflow:
public void NumberGeneration()//1.Generates the variables, calculates x and y.
{
a = rnd.Next(1, 10);
b = rnd.Next(1, 10);
c = rnd.Next(1, 10);
d = rnd.Next(1, 10);
e = rnd.Next(1, 10);
f = rnd.Next(1, 10);
...
2
You can avoid all the copypasta by writing a template function that tries a number of types in a row. The idea is that e. g. visit_any_as<int, double, char>(any, visitor) will try calling visitor(std::any_cast<int>(any)) if possible, otherwise it will call visit_any_as<double, char>(any, visitor) to try the next type, and so on until the ...
-2
You kept it concise. Anyone concerned with your code can easily comment it out and rewrite it. Add a test case and everyone will help you fix stuff if it needs it.
4
Other answers have already provided alternative solutions to std::any, such as std::variant, or inheritance; this answer will focus on correct usage of std::any itself.
and so I've come up with a hacky way to get this information by detangling the value typename into a string and then using a ton of else if statements to compare this string
std::any does ...
10
First, this call to __cxa_demangle returns a buffer allocated with malloc and which must be freed. This isn't freed, so this code has a memory leak.
Second, as I understand it, __cxa_demangle does not exist in MSVC, so this is not portable.
Third, the .find("string") catches more than std::string. It would match std::wstring, std::stringstream, void (*)(...
7
There are way too many ways to improve the situation. Some require big changes some require small changes.
Why not simply store the data inside double instead of any? double only fails to 100% accurately contain long long types - but even that happens for very large numbers. At most have long long, double and a boolean that indicates which variable it is. ...
Top 50 recent answers are included
