Creating abstraction to validate complex objects I am trying to create a validator abstraction layer. These are the points I am trying to achieve:

My abstraction should be open for extension but closed for modification
The clients should not have a if...else block to handle success or failure scenario.

Validation statuses

public interface ValidationStatus {
    default void process() {

    }

    public static ValidationStatus fail(ValidationFailureCause cause, String messages) {
        ValidationFailure validationFailure = new ValidationFailure();
        validationFailure.setCause(cause);
        validationFailure.setErrorMessages(messages);
        return validationFailure;
    }

    public static ValidationStatus success() {
        return new ValidationSuccess();
    }
}

public class ValidationFailure implements ValidationStatus {
    private ValidationFailureCause cause;
    private String errorMessages;

    public void setCause(ValidationFailureCause cause) {
        this.cause = cause;
    }

    public void setErrorMessages(String errorMessages) {
        this.errorMessages = errorMessages;
    }

    @Override
    public void process() {
        if (ValidationFailureCause.MISSING_MANDATORY_FIELDS.equals(cause)) {
            throw new MandatoryElementsMissing(errorMessages);
        }
    }

}

public class ValidationSuccess implements ValidationStatus {

}

public enum ValidationFailureCause {
    MISSING_MANDATORY_FIELDS
}

The validators

public interface Validator<T> {
    /**
     * Validate an object and return the validation result as a Status.
     *
     * @param request
     *            Data to be validated
     * @return can be ValidationFailure or ValidationSuccess class instance
     */
    ValidationStatus validate(T request);

    boolean isValid(T request);
}

public class StringValidator implements Validator<String> {

    @Override
    public ValidationStatus validate(String request) {
        if (isValid(request)) {
            return ValidationStatus.success();
        }

        return ValidationStatus.fail(ValidationFailureCause.MISSING_MANDATORY_FIELDS, "Missing mandatory parameter");
    }

    @Override
    public boolean isValid(String request) {
        return StringUtils.isNotBlank(request);
    }
}

public class PersonValidator implements Validator<FlightProposalRequest> {

    @Override
    public ValidationStatus validate(FlightProposalRequest request) {
        if (isValid(request)) {
            return ValidationStatus.success();
        } else {
            return ValidationStatus.fail(ValidationFailureCause.MISSING_MANDATORY_FIELDS, "Request is null");
        }
    }

    @Override
    public boolean isValid(FlightProposalRequest request) {
        if (request == null) {
            return false;
        }

        Validator<String> requestValidator = RequestValidatorFactory.getRequestValidator(String.class);

        boolean result = requestValidator.isValid(request.getArrivalAirport());
        result = result && requestValidator.isValid(request.getDepartureAirport());
        result = result && requestValidator.isValid(request.getDepartureDate());
        result = result && requestValidator.isValid(request.getReturnDate());

        return result;
    }
}

This is my attempt at this problem. Kindly provide your feedback on my approach. With the current setup as mentioned here, my tests are passing.

So If I understand correctly, clients get an instance of Validator from the factory, call validate() and get an instance of ValidationStatus and then call process() and get an exception (or nothing). so you replaced an if...else block with a try...catch one. the client still have to check the return value to figure out what to fix. and if you ask me, it is more clear to ask
if (result == ValidationFailureCause.MISSING_MANDATORY_FIELDS)
then
} catch (MandatoryElementsMissing mem) {

in addition, Validator interface contains a method that returns a boolean. how do you plan to prevent clients from calling it?

If we go along with your solution: the whole ValidationStatus class tree seems redundat to me.  It is just a wrappper over throwing a user defined Exception.  It doesn't provide extra capabilities beyond translate an enum into an Exception (considering that exceptions can also have String message). So I believe a design where validate() throws an Exception provides the same functionality.  If you don't want to throw the Exception super class, you can create a ValidationException super class and have all your custom exceptions extend it (much like Java's ReflectiveOperationException)
Your design determines that for a given type, there can only be one Validator.  This may be fine for user-defined types like FlightProposalRequest but it may be too restrictive for JDK types like String.  I imagine that you want to validate that getDepartureDate() is a valid date, and perhaps validate getDepartureAirport() against predefined list of airports (perhaps an SQL query) and so on.  You should assocaite a Validator with a String name (perhaps an enum).
lastly, regartding the validation of the attributes, I believe you can write it like this:

return requestValidator.isValid(request.getArrivalAirport())
&& requestValidator.isValid(request.getDepartureAirport())
&& requestValidator.isValid(request.getDepartureDate())
&& requestValidator.isValid(request.getReturnDate());

this has the same (if not more) level of readability as the original.


Defining a unique-pairs procedure From the section called Nested Mappings

  Exercise 2.40

  Define a procedure
  unique-pairs that, given an integer n,
  generates the sequence of pairs (i,j)
  with 1 < j < i < n. Use unique-pairs to
  simplify the definition of
  prime-sum-pairs given above.

I wrote the following:

(define (prime-sum-pairs n)
  (filter (lambda (seq)
            (prime? (+ (car seq) (cadr seq))))
          (unique-pairs n)))

(define (enumerate-integers start end)
  (if (>= start end)
      (list end)
      (cons start (enumerate-integers (+ 1 start) end))))

(define (unique-pairs n)
  (flat-map (lambda (i)
              (map (lambda (j) (list i j))
                   (enumerate-integers 1 (- i 1))))
            (enumerate-integers 2 n)))

(define (filter test-fn seq)
  (if (null? seq) null
      (if (test-fn (car seq))
          (cons (car seq)
                (filter test-fn (cdr seq)))
          (filter test-fn (cdr seq)))))

(define (accumulate op initial seq)
  (if (null? seq)
      initial
      (op (car seq)
          (accumulate op initial (cdr seq)))))

(define (flat-map f seq)
  (accumulate append
              null
              (map (lambda (x) (f x)) seq)))

(define (prime? n) (= (smallest-divisor n) n))
(define (divisible? n i) (= 0 (remainder n i)))
(define (square x) (* x x))
(define (smallest-divisor n)
  (define (rec i)
    (cond ((> n (square i)) n)
          ((divisible? n i) i)
          (else (rec (+ 1 i)))))
  (rec 2))

Can this be improved in any way?

Your code

(define (enumerate-integers start end)
  (if (>= start end)
      (list end)
      (cons start (enumerate-integers (+ 1 start) end))))

(define (unique-pairs n)
  (flat-map (lambda (i)
              (map (lambda (j) (list i j))
                   (enumerate-integers 1 (- i 1))))
            (enumerate-integers 2 n)))

looks fine to me. If you want to massage some details, you could rewrite enumerate-integers e.g. to:

(define (enumerate-integers start end)
  (if (> start end) '()
      (cons start (enumerate-integers (+ 1 start) end))))

which is cleaner because you don't have (list end) as a special case, and you can correctly produce an empty list of integers if start > end.

If you want to be even cleaner, you can do:

(define (enumerate-integers start end)
  (define (iter n)
    (if (> n end) '()
        (cons n (iter (+ n 1)))))
  (iter start))

This is a good pattern in case of more complex procedures.

Your flat-map is more complex than needed, your code:

(define (flat-map f seq)
  (accumulate append
              null
              (map (lambda (x) (f x)) seq)))

can be replaced with:

(define (flat-map f seq)
  (accumulate append
              null
              (map f seq)))

because (lambda (x) (f x)) is equal to f.


Find a famous writer in a random string I'm doing a CodeEval challenge, where the object is to find a famous writers name and a year, inside of a random given string.

You have a set of rows with names of famous writers encoded inside. Each row is divided into 2 parts by pipe char (|). The first part has a writer's name. The second part is a "key" to generate a name.
Your goal is to go through each number in the key (numbers are separated by space) left-to-right. Each number represents a position in the 1st part of a row. This way you collect a writer's name which you have to output.
Challenge input:
osSE5Gu0Vi8WRq93UvkYZCjaOKeNJfTyH6tzDQbxFm4M1ndXIPh27wBA rLclpg| 3 35 27 62 51 27 46 57 26 10 46 63 57 45 15 43 53
3Kucdq9bfCEgZGF2nwx8UpzQJyHiOm0hoaYP6ST1WM7Nks5XjrR4IltBeDLV vA| 2 26 33 55 34 50 33 61 44 28 46 32 28 30 3 50 34 61 40 7 1 31
Expected output:
Stephen King 1947
Kyotaro Nishimura 1930

I've successfully solved this problem and would like some critique on my work:
import sys

def find_a_writer(string, int_list):
    """ Find a famous writer and a year in a random string
    >>> print(find_a_writer("osSE5Gu0Vi8WRq93UvkYZCjaOKeNJfTyH6tzDQbxFm4M1ndXIPh27wBA rLclpg",
    >>> ['3', '35', '27', '62', '51', '27', '46', '57', '26', '10', '46', '63', '57', '45', '15', '43', '53']))
    Stephen King 1947"""
    results = []
    string_list = list(string)
    for i in int_list:
        results.append(string_list[int(i) - 1])
    return ''.join(results)

if __name__ == '__main__':
    with open(sys.argv[1]) as data:
        for line in data.readlines():
            data_arr = line.rstrip().strip().split("|")
            print(find_a_writer(data_arr[0].strip(), data_arr[1].strip().split(" ")))

What I would like to know:

What did I do well?
Is there anything I can do better?
Can I shorten the function?

3) Yes, you can shorten the function:

def decide(mess, indexes):
    return ''.join(mess[int(i)-1] for i in indexes)

I just removed the unnecessary conversion to list and replaced the append loop with a generator expression.

Tuple unpacking allows more documenting names:

mess, indexes = lunedì.split('|')
print(decoder(mess, indexes.split()))

I do not think you need so much stripping at the file parsing step.


A Credit Payment Calculator So I've attempted this project from hyperskills where you're asked to make a calculator script that can calculate annuity and differentiated payments given that 3 of 4 values are known. I think i went with a real hardcoding way of doing this, as such I would like some advice on making my code more compact and efficient.

import math
import sys

def convert_month_into_years_months(months):
    years, months = divmod(months, 12)
    if years != 1:
        s_year = "s"
    else:
        s_year = ""

    if months != 1:
        s_month = "s"
    else:
        s_month = ""

    if not years:
        return f"You need {months} month{s_month} to repay this credit!"
    elif not months:
        return f"You need {years} year{s_year} to repay this credit!"
    else:
        return f"You need {years} year{s_year} and {months} month{s_month} to repay this credit!"

credit_principal_given = False
period_count_given = False
credit_interest_given = False
monthly_payment_given = False
annuity = False
diff = False

period_count = monthly_payment = credit_principal = credit_interest = 1

command_line = sys.argv
number_of_arguments = len(command_line[1:])
type_of_payment = command_line[1] if number_of_arguments != 0 else None

run = True
if type_of_payment == "--type=diff" and number_of_arguments == 4:
    diff = True
elif type_of_payment == "--type=annuity" and number_of_arguments == 4:
    annuity = True
else:
    run = False

if run:
    for arg in command_line[2:]:
        try:
            if arg[:10] == "--periods=":
                period_count = int(arg[10:])
                period_count_given = True
            elif arg[:10] == "--payment=" and annuity:
                monthly_payment = float(arg[10:])
                monthly_payment_given = True
            elif arg[:11] == "--interest=":
                credit_interest = float(arg[11:])
                credit_interest_given = True
            elif arg[:12] == "--principal=":
                credit_principal = float(arg[12:])
                credit_principal_given = True
        except:
            run = False
            break

    # Checks if any of the values are negative
    values = [period_count ,monthly_payment ,credit_principal ,credit_interest]
    if not all(True if value > 0 else False for value in values):
        run = False

if run:

    nominal_interest_rate = (credit_interest/100)/12

    if annuity:
        if credit_principal_given and monthly_payment_given and credit_interest_given:
            period_count = math.ceil(math.log( (monthly_payment/( monthly_payment - ( nominal_interest_rate * credit_principal ) ) ), 1+nominal_interest_rate ))
            print(convert_month_into_years_months(period_count))

        elif monthly_payment_given and period_count_given and credit_interest_given:
            credit_principal = math.floor(monthly_payment / ( (nominal_interest_rate * (1 + nominal_interest_rate)**period_count )/( (1 + nominal_interest_rate)**period_count - 1) ) )
            print(f"Your credit principal = {credit_principal}!")

        elif credit_principal_given and period_count_given and credit_interest_given:
            monthly_payment = math.ceil(credit_principal * ( (nominal_interest_rate * (1 + nominal_interest_rate)**period_count )/( (1 + nominal_interest_rate)**period_count - 1) ) )
            print(f"Your annuity payment = {monthly_payment}!")

    elif diff:
        sum_diff_payments = 0

        for month in range(1, period_count+1):
            diff_payment = (credit_principal/period_count) + (nominal_interest_rate * (credit_principal - ( (credit_principal * (month-1))/period_count ) ) )
            diff_payment = math.ceil(diff_payment)
            sum_diff_payments += diff_payment

            print(f"Month {month}: paid out {diff_payment}")

    overpayment = round(abs(credit_principal - (period_count * monthly_payment)))
    print(f"\nOverpayment = {overpayment}")

else:
    print("Incorrect parameters")

The one thing that is immediately apparent to me is that parsing of command line arguments could be done better using the built-in argparse module but for an intro you can start with the tutorial.

Here is how the routine could be written, assuming all parameters are mandatory:

import argparse

# check command line options
parser = argparse.ArgumentParser()

parser.add_argument("--type", dest="type_of_payment", type=str, choices=["diff", "annuity"], required=True, help="Add some help text here")
parser.add_argument("--periods", dest="period_count", type=int, required=True)
parser.add_argument("--payment", dest="monthly_payment", type=float, required=True)
parser.add_argument("--interest", dest="credit_interest", type=float, required=True)
parser.add_argument("--principal", dest="credit_principal", type=float, required=True)

args = parser.parse_args()

# show the values
print(f"Type of payment: {args.type_of_payment}")
print(f"Periods: {args.period_count}")
print(f"Payment: {args.monthly_payment}")
print(f"Interest: {args.credit_interest}")
print(f"Principal: {args.credit_principal}")

This is more flexible, because:

you can provide parameters in any order
you can assign default values
you can specify the expected type and also restrict to a specific range of values - in this example type_of_payment must be either "diff" or "annuity"
you can also define your own function if you require more fine-tuned validation of certain parameters
you can easily define groups of mutually exclusive parameters

This is an example but you can customize it further. Then your code will become quite shorter = less distraction and more focus on the important stuff.


Printing a BST tree level-by-level Please review and tell me what improvements can be made:

package trees;

import java.util.LinkedList;
import java.util.Queue;

public class PrintBSTLevelByLevel {

     public static class Node {

        int data;
        Node left;
        Node right;

        Node(int val){

            this.data = val;
            Node left = null;
            Node right = null;

        }
    }

      public void LevelOrderQueue(Node root){

          Queue q = new LinkedList();
          int levelNodes = 0;

          if(root == null) return;
          q.add(root);

          while(!q.isEmpty()){

              levelNodes = q.size();

              while (levelNodes > 0){

                  Node n = (Node)q.remove();
                  System.out.println(""+n.data);
                  if(n.left!= null)
                      q.add(n.left);
                  if(n.right!=null)
                      q.add(n.right);
                  levelNodes--;

              }
              System.out.println("");

          }

          //return null;
      }

    public static void main(String[] args) {

        Node root = new Node(5);

        root.left = new Node(10);
        root.right = new Node(15);
        root.left.left = new Node(20);
        root.left.right = new Node(25);
        root.right.left = new Node(30);
        root.right.right = new Node(35);

        PrintBSTLevelByLevel l = new PrintBSTLevelByLevel();

        System.out.println(" Output by Better Approach : ");
        l.LevelOrderQueue(root);

    }

}

Though your code would suffice for the current task, I would create a fully (or partially) functioning Tree class, so that it may be used later. Another class will handle the display. This is what the Tree class would look like:

public class Tree<T> {

    private Node rootNode;

    public Tree() {
        rootNode = null;
    }

    public Tree(T rootValue) {
        rootNode = new Node(rootValue);
    }

    public Node getRootNode() {
        return rootNode;
    }

    public Node createNewNode(T value) {
        return new Node(value);
    }

    class Node {

        private T value;
        public Node left = null;
        public Node right = null;

        Node(T value) {
            this.value = value;
        }

        public T getValue() {
            return value;
        }

        public void setValue(T value) {
            this.value = value;
        }

    }

}

This is an extremely basic Tree class that has very basic functions, but it gets the job done. Later, when you need more functions for the Tree class, you can easily go and add it. Explanation of the class:

The default constructor (Tree()) will just set the root node to null.
The other constructor will take the argument and create a new Node with the value of the argument.
The getRootNode() method is to get the root Node, so that you can get the other Nodes like so:

// example
Tree tree = new Tree(10);
// initialization of values
Node someNode = tree.getRootNode().left.right/*etc*/;

The createNewNode(T value) method provides indirect access to the otherwise inaccessible (at least outside the package) Node(T value) constructor. Though this isn't necessary, as you can make the Node's constructor public, it is good practice for later.
The Node class is a simple class that represents a node with a left and right node, as well as its value:

The value is a private variable, so getters and setters are required to access it. Again, though you can just make value public, it is better practice not do so.
I made the left and right variables public, just for easier usage. Though it differs from good practice, there are some cases where there are exceptions (some programmers may disagree with this statement).

Now, for the printing component:

Since I redesigned the Tree class completely, we have to change your method as well. Just to do exactly what your code does, here is the new method:

public <T> void LevelOrderQueue(Tree<T> tree) {
    if (tree != null) {
        Tree<T>.Node rootNode = tree.getRootNode();
        if (rootNode != null) {
            Queue<Tree<T>.Node> queue = new LinkedList<>();
            int levelNodes = 0;
            queue.add(rootNode);
            while (!queue.isEmpty()) {
                levelNodes = queue.size();
                while (levelNodes > 0) {
                    Tree<T>.Node node = queue.remove();
                    System.out.println(node.getValue());
                    if (node.left != null) {
                        queue.add(node.left);
                    }
                    if (node.right != null) {
                        queue.add(node.right);
                    }
                    levelNodes--;
                }
                System.out.println();
            }
        }
    }

}

Note some changes:

The method now takes a Tree<T> object instead of a Node object.
This method has two checks for null; the first one to check for the tree, and the second one for the root node.
I added braces for if statements. The reason for this is because the lack of braces may cause horrible, hard-to-spot bugs that might frustrate you for days. I can't find the post which I wrote about this in more detail, but I will post it up once I find it. (EDIT: Found it! Read here)
I added generics to your Queue initialization.
I removed the Node cast. This is not necessary when generics are used.
I changed naming. Variable names like node and queue are easier to understand than n and q.
I changed System.out.println(""); to System.out.println();. There is a println() method that takes no arguments.
I changed System.out.println(""+n.data); to System.out.println(node.value);. The beginning "" is not necessary because System.out.println() automatically converts what is given into a String, via the toString() method defined by the class (or whatever class before it in the inheritance chain that has it defined).
I added more space between some individual keywords, variable names, braces, parentheses etc.: this is for better readability, and to follow the Java conventions. If you are not sure of them, just search up "Java Conventions" on google.
I removed empty lines. You may re-add them where you like them; I like my code that way.


Asyncronous unit test refactoring in Objective-C I have found that writing unit tests helps me to discover both the intent and also reusability of code that I'm working on. It has been generally helpful in helping develop my "code smell."

I was working on two unit tests that are basically mirror images, and realized that I was typing code that looked almost identical. I am hoping that someone could help me refactor the similar parts of these two unit tests into shared setup and tear-down methods.

- (void)testBlockMethodOne
{
    NSError *expectedError = [NSError errorWithDomain:@"An error"
                                         code:42
                                     userInfo:nil];
    [mockClassA setError:expectedError];
    [mockClassA setRunCompletionBlockImmediately:YES];

    __block NSError *actualError = nil;
    __block id actualObject = nil;

    [classB fireClassAsBlockThenDoThisBlock:^(id object, NSError *error) {
        actualObject = object;
        actualError  = error;
    }];

    STAssertNil(actualObject, @"There was a problem");
    STAssertEqualObjects(actualError, expectedError, @"Got the expected error");
}

- (void)testBlockMethodTwo
{
    NSObject *expectedObject = [[NSObject alloc] init];
    [mockClassA setObject:expectedObject];
    [mockClassA setRunCompletionBlockImmediately:YES];

    __block NSError *actualError = nil;
    __block id actualObject = nil;

    [classB fireClassAsBlockThenDoThisBlock:^(id object, NSError *error) {
        actualObject = object;
        actualError  = error;
    }];

    STAssertNil(actualError, @"There was a problem");
    STAssertEqualObjects(actualObject, expectedObject, @"Got the expected object");
}

A little background about what's being tested:

In production code, -fireClassAsBlockThenDoThisBlock is asynchronous. But, of course, in the unit tests I have a mock standing in for "Class A" that just calls the block immediately with either the error or object that I set on the mock.

I've repeated more setup code then I'd really like to between the two tests. My hope is once I see how someone else attacks this, I'll have gained some insight into refactoring block based methods.

It looks like you can extract the method bodies entirely. Warning: I typed this code into a web browser, not an IDE:

- (NSDictionary *)invokeClassBBlockRunnerWithExpectedObject: (id)object expectedError: (NSError *)error {
    [mockClassA setObject: object];
    [mockClassA setError: error];
    [mockClassA setRunCompletionBlockImmediately: YES];

    __block NSError *actualError = nil;
    __block id actualObject = nil;

    [classB fireClassAsBlockThenDoThisBlock: ^(id object, NSError *error) {
        actualObject = object;
        actualError = error;
    }];

    NSMutableDictionary *results = [NSMutableDictionary dictionary];
    if (actualObject) [results setObject: actualObject forKey: @"Object"];
    if (actualError) [results setObject: actualError forKey: @"Error"];
    return results;
}

- (void)testBlockMethodOne {
    NSError *expectedError = [NSError errorWithDomain: @"An Error" code: 42 userInfo: nil];
    NSDictionary *results = [self invokeClassBBlockRunnerWithObject: nil error: expectedError];

    STAssertNil([results objectForKey: @"Object"], @"Shouldn't get an object when an error occurs");
    STAssertEqualObjects(expectedError, [results objectForKey: @"Error"], @"Should get the error I expected");
}

- (void)testBlockMethodTwo {
    id expectedObject = [[NSObject alloc] init];
    NSDictionary *results = [self invokeClassBBlockRunnerWithObject: expectedObject error: nil];

    // you get the idea
}


Torrent File Remover This script to locate torrent files in a given directory, and if they have been seeding for 2 weeks, delete the files.

I realize the comments are overboard. The reason for that is it's being shared with people who don't program.

What should I have done differently?

import os  # for listdir(), path(), remove(), and getctime()
from time import time, ctime, sleep
import datetime as dt  # datetime to compare dates

# Path to torrents
torrents_folder = r'C:\users\user\torrents\folder'
# Torrent files as a list
torrents = os.listdir(torrents_folder)
# Months will be used to swap month prefix and corresponding month number
months = {"Jan": "01", "Feb": "02", "Mar": "03", "Apr": "04", "May": "05", "Jun": "06", "Jul": "07", "Aug": "08",
          "Sep": "09", "October": "10", "Nov": "11", "Dec": "12", }
# Set the number of days to elapse from creation date before file is deleted
days_to_wait = dt.timedelta(days=14)

# function to remove torrent
def torrent_remover(month):
        # Format the date, casting month to string
        start_date = dt.datetime.strptime(str(month), "%m %d %Y")
        # Elapsed Time. See line 12
        end_date = start_date + days_to_wait
        # Today's date for comparison
        today = dt.datetime.today()

        if end_date <= today:
            print("Removing: ", torrent, "\tTorrent expired on", end_date)
            # Create full path from the torrent folder and torrent file
            torrent_files = os.path.join(torrents_folder, torrent)
            # Delete the torrents
            os.remove(torrent_files)
        else:
            print("Nothing to remove")
            exit()

# Start the loop
for torrent in torrents:
    if torrent.endswith(".torrent"):
        # Get date time from each file then join file and folder to create full path
        date = ctime(os.path.getctime(os.path.join(torrents_folder, torrent)))
        # Remove the timestamp
        remove_timestamp = date.replace(date[11:19], "")
        # Remove the day
        new_date = remove_timestamp.replace(remove_timestamp[0:4], "")
        # Return single value in list
        month = [month for month in months if month in new_date][0]
        if month in new_date:
            # Returns the month number when passed month key
            month_number = months[month]
            # Replace the month prefix with the corresponding number
            reformatted_date = new_date.replace(month, month_number)
            # Send to delete function to be deleted.
            torrent_remover(reformatted_date)

You could do with learning some of the more powerful tools in Python, such as pathlib.

Tracing control flow, instead of

torrents_folder = r'C:\users\user\torrents\folder'
torrents = os.listdir(torrents_folder)

for torrent in torrents:
    if torrent.endswith(".torrent"):
        ...

you can do

torrents_folder = Path(r'C:\users\user\torrents\folder')
for torrent_path in torrents_folder.glob("*.torrent"):
    ...

Then to get the date you can do just

datetime.date.fromtimestamp(torrent_path.stat().st_ctime)

Before I leave this, though, I should warn that your

text.replace(text[a:b], "")

to remove a slice is error-prone if that slice ever appears elsewhere. Instead try something like

text = text[:a] + text[b:]

Your if after

month = [month for month in months if month in new_date][0]
if month in new_date:

looks strange - of course it will be True since you've just checked for it.

Anyway, this can now be avoided entirely since we have a date object.

You run torrent_remover(date) (although you call it month in the call signature!), but you also need the torrent_path. It turns out you're passing this as a global! That's bad!

Instead, add another parameter so you have

def torrent_remover(path, date):

Your checks can then just be

end_date = date + days_to_wait
today = dt.datetime.today()

if end_date <= today:
    ...
else:
    ...

Your

print("Removing: ", torrent, "\tTorrent expired on", end_date)
torrent_files = os.path.join(torrents_folder, torrent)
os.remove(torrent_files)

Is misnamed (you only remove one file), but now can be just

print("Removing: ", torrent_path, "\tTorrent expired on", end_date)
torrent_path.unlink()

and your other path looks wrong:

print("Nothing to remove")
exit()

since exit quits the process entirely.

days_to_wait is a global constant, so should be uppercase. However, I would put the code in a main function and just pass it to the remover.

I would let torrent_remover get the date itself, simplifying the main loop.

Finally, your printing adds spaces between every part. One way to fix that is

print(a, b, c, sep="")

but it seems better to use formatting here:

print("{}{}{}".format(a, b, c))

This all gives

from datetime import date, timedelta
from pathlib import Path

def torrent_remover(path, shelf_life):
    created_on = date.fromtimestamp(torrent_path.stat().st_ctime)
    expired_on = created_on + shelf_life

    if expired_on <= date.today():
        print("Removing: {}\tTorrent expired on {}".format(torrent_path, expired_on))
        torrent_path.unlink()

def main():
    shelf_life = timedelta(days=14)
    torrents_folder = Path(r"C:\users\user\torrents\folder")

    for torrent_path in torrents_folder.glob("*.torrent"):
        torrent_remover(torrent_path, shelf_life)

if __name__ == '__main__':
    main()

which I hope you'll agree is simpler. To add a check for if any file has been removed, try something like

from datetime import date, timedelta
from pathlib import Path

def torrent_remover(path, shelf_life):
    created_on = date.fromtimestamp(torrent_path.stat().st_ctime)
    expired_on = created_on + shelf_life

    if expired_on <= date.today():
        print("Removing: {}\tTorrent expired on {}".format(torrent_path, expired_on))
        torrent_path.unlink()
        return True

    return False

def main():
    shelf_life = timedelta(days=14)
    torrent_paths = Path(r"C:\users\user\torrents\folder").glob("*.torrent")
    removed = sum(torrent_remover(path, shelf_life) for path in torrent_paths)

    if not removed:
        print("Nothing to remove")

if __name__ == '__main__':
    main()

This is untested.


Simple stack structure in C I just started to learn C, and for practicing, I tried to implement a simple stack data structure, and I want to know if I'm making any mistakes, or how could I improve it:

My stack.h:

#ifndef STACK_H
#define STACK_H
#define STACK_MAX_SIZE 125

typedef struct
{
    int max_size;
    int items[STACK_MAX_SIZE];
    int position;
} Stack;

void stack_push(Stack *stack, int number);
int stack_pop(Stack *stack);
bool stack_is_empty(Stack *stack);

#endif

Stack.c:

#include <stdio.h>
#include <stdbool.h>
#include "stack.h"

void stack_push(Stack *stack, int number)
{
    if(stack->position < 0){
        stack->position = 1;
    }
    stack->position++;

    stack->items[stack->position] = number;
}

int stack_pop(Stack *stack)
{
    if(stack->position == 0){
        fprintf(stderr, "Stack is empty");
        return -1;
    }
    int number = stack->items[stack->position];

    stack->position--;

    return number;
}

bool stack_is_empty(Stack *stack)
{
    return stack->position < 1;
}

And the test:

#include <stdarg.h>
#include <stddef.h>
#include <setjmp.h>
#include <cmocka.h>
#include <stdio.h>
#include <stdbool.h>
#include "../src/stack.h"

static void stack_test() {
    Stack s, *stack = &s;

    for(int i = 0; i <= 5; i++){
        stack_push(stack, i);
    }

    assert_int_equal(stack_pop(stack), 5);

    while(false == stack_is_empty(stack)){
        stack_pop(stack);
    }

    assert_true(stack_is_empty(stack));
}

int main(void) {
    const struct CMUnitTest tests[] = {
        cmocka_unit_test(stack_test),
    };
    return cmocka_run_group_tests(tests, NULL, NULL);
}

Various thoughts

You never use the max_size field, so you should delete it.
When pushing, you never check to see if the stack is full, so you could overrun your array if you push too many items.
Your current implememtation does not utilize the first array slot items[0].  You could make a few easy changes to fix that.
You have a check for position < 0 but it can never go negative.  You could even make position an unsigned value.

Rewrite
Here are your push and pop functions rewritten:
void stack_push(Stack *stack, int number)
{
    if (stack->position >= STACK_MAX_SIZE) {
        fprintf(stderr, "Stack is full\n");
        return;
    }
    stack->items[stack->position++] = number;
}

int stack_pop(Stack *stack)
{
    if (stack->position == 0) {
        fprintf(stderr, "Stack is empty\n");
        return -1;
    }
    return stack->items[--stack->position];
}


Counting the number of character occurrences I've written some code that uses a SortedDictionary to count the number of occurrences of a character in a string.

How can I improve this code? What should I be doing differently? Any advice at all would be helpful.

    static void Main(string[] args)
    {
        string longText = @"The quick brown fox jumps over the lazy dog";

        var count = CharacterCount.Count(longText);

        foreach (var character in count)
        {
            Console.WriteLine("{0} - {1}", character.Key, character.Value);
        }
    }

CharacterCount class:

class CharacterCount
{
    public static SortedDictionary<char, ulong> Count(string stringToCount)
    {
        SortedDictionary<char, ulong> characterCount = new SortedDictionary<char, ulong>();

        foreach (var character in stringToCount)
        {
            if (!characterCount.ContainsKey(character))
            {
                characterCount.Add(character, 1);
            }
            else
            {
                characterCount[character]++;
            }
        }

        return characterCount;
    }

Here is the output:

You could do the same thing with LINQ:

var counts = longText.GroupBy(c => c) // put each character into a "bucket"
                     // order the buckets alphabetically
                     .OrderBy(c => c.Key);
                     // then convert to dictionary where key = character, value = count
                     .ToDictionary(grp => grp.Key, grp => grp.Count())

For convenience/readability, you could turn this into an extension method on string:

static class Extensions
{
    public static Dictionary<char, int> CharacterCount(this string text)
    {
        return text.GroupBy(c => c)
                   .OrderBy(c => c.Key)
                   .ToDictionary(grp => grp.Key, grp => grp.Count());
    }
}

And invoke it like this:

var longText = @"The quick brown fox jumps over the lazy dog";

var counts = longText.CharacterCount();

However, note that the above doesn't produce a sorted dictionary in the sense that if you were to add another key/value to it afterwards, the order would no longer be maintained. You could re-sort at that point, or use this SortedDictionary constructor to create a sorted dictionary.


Insert variable number of rows In the Excel vba sub below, I need to insert the number of rows equal to variable j after row k.

So if j=17 and k=2 then I want 17 empty rows after row 2.

How can I improve this code?

sub stuck()
    Dim ws1 As Worksheet
    Dim ws2 As Worksheet
    Dim LastRow As Long
    Dim i As Long, z As Long
    Dim j As Long, k As Long, x As Long
    Dim rngtocopy As Range
    Dim rngFinal As Range
    Dim r As Range

    Set ws1 = Sheets("Calc")
    Set ws2 = Sheets("Dealer Orders")

    LastRow = ws1.Cells(ws1.Rows.Count, "E").End(xlUp).Row

    Set rngtocopy = ws1.Range("E2", ws1.Cells(LastRow, "F"))
    Set rngFinal = ws2.Range("K2", ws2.Cells(LastRow, "K"))

    j = ws1.Cells(ws1.Rows.Count, "E").End(xlUp).Row
    z = ws2.Cells(ws2.Rows.Count, "E").End(xlUp).Row
    k = 3
    x = 1
    Set r = Range("A" & k)
    Do While x < z
        With ws2
            If j > 0 Then
                  ws2.Range(("A" & k), ws2.Range("A" & k).Offset(j, 0)).EntireRow.Insert
                        Set r = Cells(r.Row + j, 1)

                        For i = 2 To rngtocopy.Rows.Count
                            With ws2.Range("K" & k)
                                .Offset(0, 0).Value = rngtocopy(i, 1)
                                .Offset(0, 1).Value = rngtocopy(i, 2)
                            End With
                        k = k + 1
                        Next i
            End If
            k = k + 4
        End With
    x = x + 1
    Loop
    end sub

Readability

Naming
So, the first thing I would note is the poor naming. The way things are named right now makes it difficult to understand the code. So, to start, replace ws1 and ws2 with more meaningful names. The range variables are a little better, but could still use some improvement. I would also note that variables should be camelCased. These things simply make it easier to read the code.

ws1 >> sourceSheet
ws2 >> destSheet
rngtocopy >> rngToCopy >> sourceRange
rngFinal >> destRange
LastRow >> lastRow

The next thing to note is the abundance of single letter variable names. These are problematic. The only time you should use single letter variable names is for a loop counter. That's it. No exceptions. It is extremely difficult to map these letters to meanings while we're trying to understand logic. Will you remember what z is 6 months from now? I doubt it.

j >> lastSourceERow
z >> lastDestERow
k >> firstDestRow
x >> startRow
r >> destCell

Sidenote: Replacing r was a real PITA.
Lastly, Sub and Function names should have Verb-Noun type names. stuck() tells the dev using this code absolutely nothing. Perhaps CopyTheSmithReportRange() would be a good name.
WhiteSpace
Again, this is a readability thing. (I do promise to get around to a better way to do this, but first we do need to be able to read the code.)
You are indenting your code, which is good. I've seen worse, but it could be better. Indentation should visually tell me at what level we're currently working at. For example, consider this snippet.

Do While x < z
    With ws2
        If j > 0 Then
              ws2.Range(("A" & k), ws2.Range("A" & k).Offset(j, 0)).EntireRow.Insert
                    Set r = Cells(r.Row + j, 1)

                    For i = 2 To rngtocopy.Rows.Count
                        With ws2.Range("K" & k)
                            .Offset(0, 0).Value = rngtocopy(i, 1)
                            .Offset(0, 1).Value = rngtocopy(i, 2)
                        End With
                    k = k + 1
                    Next i
        End If
        k = k + 4
    End With
x = x + 1
Loop

Everything starts fine with the Do loop and the With statement, but then you add an extra level of indentation after inserting the row. The rest of that block is logically on the same level, so it should be at the same indentation level. Also take note that your loop incrememtation happens at the same indentation level as the For and Next statements. It shouldn't. It should be one level deeper.
The other thing to note about whitespace is the good use of vertical whitespace. It can make all the difference to readability. Use an extra line (one, never two) to differentiate between logically different things that occur sequentially at the same level of indentation.
This is the code after making these readability changes.
Option Explicit

Sub CopyTheSmithReportRange()

    Dim sourceSheet As Worksheet
    Dim destSheet As Worksheet
    Dim lastRow As Long
    Dim i As Long
    Dim lastDestERow As Long
    Dim lastSourceERow As Long
    Dim firstDestRow As Long
    Dim startRow As Long
    Dim sourceRange As Range
    Dim destRange As Range
    Dim destCell As Range

    firstDestRow = 3
    startRow = 1

    Set sourceSheet = Sheets("Calc")
    Set destSheet = Sheets("Dealer Orders")

    lastRow = sourceSheet.Cells(sourceSheet.Rows.Count, "E").End(xlUp).Row
    lastSourceERow = sourceSheet.Cells(sourceSheet.Rows.Count, "E").End(xlUp).Row
    lastDestERow = destSheet.Cells(destSheet.Rows.Count, "E").End(xlUp).Row

    Set sourceRange = sourceSheet.Range("E2", sourceSheet.Cells(lastRow, "F"))
    Set destRange = destSheet.Range("K2", destSheet.Cells(lastRow, "K"))

    Set destCell = Range("A" & firstDestRow)
    Do While startRow < lastDestERow

        With destSheet
            If lastSourceERow > 0 Then

                destSheet.Range(("A" & firstDestRow), destSheet.Range("A" & firstDestRow).Offset(lastSourceERow, 0)).EntireRow.Insert
                Set destCell = Cells(r.Row + lastSourceERow, 1)

                For i = 2 To sourceRange.Rows.Count

                    With destSheet.Range("K" & firstDestRow)
                        .Offset(0, 0).Value = sourceRange(i, 1)
                        .Offset(0, 1).Value = sourceRange(i, 2)
                    End With

                    firstDestRow = firstDestRow + 1
                Next i

            End If

            firstDestRow = firstDestRow + 4
        End With

        startRow = startRow + 1
    Loop
End Sub

Refactoring
The first thing I notice is that there are a lot of similar variables declared and that the code is deeply nested. These are indications that the code is doing too much and is in violation of the Single Responsibility Principle. It will be our goal now to simplify the code.
There's a quick hit right at the beginning. There is no difference between lastSourceERow and lastRow. We can get rid of lastRow entirely. While we're at it, let's go ahead and extract a function for lastRow. You'll find yourself needing it a lot if you continue to write vba code.
Public Function LastRow(ws As Worksheet, column As Variant) As Long
    LastRow = ws.Cells(ws.Rows.Count, column).End(xlUp).Row
End Function

Note that this function will take input of either a column name ("E") or integer index, so we could pass it the integer index if we so chose.
This simplifies variable initialization a bit, and we have one less declaration.
lastSourceERow = LastRow(sourceSheet, "E")
lastDestERow = LastRow(destSheet, "E")

Set sourceRange = sourceSheet.Range("E2", sourceSheet.Cells(lastSourceERow, "F"))
Set destRange = destSheet.Range("K2", destSheet.Cells(lastSourceERow, "K"))

Setting the ranges leaves a little to be desired though, so we'll use a bit of a hack to clean up those lines. Let's just concatenate the last row to a string indicating the ranges we want to work with.
Set sourceRange = sourceSheet.Range("E2:F" & lastSourceERow)
Set destRange = destSheet.Range("K2:K" & lastSourceERow)

Lets use a similar trick to replace this obtuse line.

destSheet.Range(("A" & firstDestRow), destSheet.Range("A" & firstDestRow).Offset(lastSourceERow, 0)).EntireRow.Insert

With this
destSheet.Range("A" & firstDestRow & ":A" & lastSourceERow).EntireRow.Insert

I'm honestly not a fan of concatenating cell addresses like this, but I think it does a world of good to the readability and maintainability of this code. While we're at it, I think this is useful enough to extract into it's own subroutine.
Public Sub InsertRows(ws As Worksheet, firstRow As Long, lastRow As Long)
    ws.Range("A" & firstRow & ":A" & lastRow).EntireRow.Insert
End Sub

Let's also remove the With destSheet. It's not serving much of a purpose other than to further nest the code. Let's also remove destRange as it's not being used at all. That brings us to here, which is getting somewhere.
Sub CopyTheSmithReportRange()

    Dim sourceSheet As Worksheet
    Dim destSheet As Worksheet
    Dim i As Long
    Dim lastDestERow As Long
    Dim lastSourceERow As Long
    Dim firstDestRow As Long
    Dim startRow As Long
    Dim sourceRange As Range
    Dim destCell As Range

    Set sourceSheet = Sheets("Calc")
    Set destSheet = Sheets("Dealer Orders")

    lastSourceERow = LastRow(sourceSheet, "E")
    lastDestERow = LastRow(destSheet, "E")

    Set sourceRange = sourceSheet.Range("E2:F" & lastSourceERow)

    firstDestRow = 3
    startRow = 1

    Set destCell = Range("A" & firstDestRow)
    Do While startRow < lastDestERow

        If lastSourceERow > 0 Then

            InsertRows destSheet, firstDestRow, lastSourceERow

            Set destCell = destSheet.Cells(destCell.Row + lastSourceERow, 1)

            For i = 2 To sourceRange.Rows.Count

                With destSheet.Range("K" & firstDestRow)
                    .Offset(0, 0).Value = sourceRange(i, 1)
                    .Offset(0, 1).Value = sourceRange(i, 2)
                End With

                firstDestRow = firstDestRow + 1
            Next i

        End If

        firstDestRow = firstDestRow + 4
        startRow = startRow + 1
    Loop
End Sub

But not quite there yet....

Now, instead of this Do While loop, let's iterate through a range of cells with a ForEach loop instead. But first, remove this line, it is also dead code.

       Set destCell = destSheet.Cells(destCell.Row + lastSourceERow, 1)

Actually, completely remove this variable. It's not actually used anywhere. Okay, now let's replace that do loop with a foreach.
Sub CopyTheSmithReportRange()

    Dim sourceSheet As Worksheet
    Dim destSheet As Worksheet
    Dim lastDestERow As Long
    Dim lastSourceERow As Long
    Dim firstDestRow As Long
    Dim startRow As Long
    Dim sourceRange As Range

    Set sourceSheet = Sheets("Calc")
    Set destSheet = Sheets("Dealer Orders")

    lastSourceERow = LastRow(sourceSheet, "E")
    lastDestERow = LastRow(destSheet, "E")

    Set sourceRange = sourceSheet.Range("E2:F" & lastSourceERow)

    firstDestRow = 3
    startRow = 1

    If lastSourceERow > 0 Then

        InsertRows destSheet, firstDestRow, lastSourceERow

        Dim destRange As Range
        Set destRange = Range("K3:K" & lastSourceERow)

        Dim cell As Range, i As Long
        i = 2

        For Each cell In destRange

            cell.Value = sourceRange(i, 1)
            cell.Offset(0, 1).Value = sourceRange(i, 2)

            i = i + 1
        Next cell

    End If
End Sub

A lot just happened there, so let me explain.

Move the If lastSourceERow check and the row insertion out of the loop.
Reintroduce the destRange variable with a new purpose.
Write the values to the iterator cell range, instead of offsetting based on some counter variables.

Now there's just a little more clean up to do.

Let's check the source sheet for data prior to doing anything else.
Remove dead variables startRow & lastDestERow and all related dead code.
Change firstDestRow from a variable to a constant, it's value no longer changes.

And we now have a subroutine that fits on a single screen without scrolling. We could probably extract another method, but this is good enough for me. The code below does exactly the same thing your original did.
Sub CopyTheSmithReportRange()

    Dim sourceSheet As Worksheet
    Dim destSheet As Worksheet
    Dim lastSourceERow As Long
    Dim sourceRange As Range

    Const firstDestRow As Long = 3

    Set sourceSheet = Sheets("Calc")
    Set destSheet = Sheets("Dealer Orders")

    lastSourceERow = LastRow(sourceSheet, "E")

    If lastSourceERow > 0 Then

        Set sourceRange = sourceSheet.Range("E2:F" & lastSourceERow)

        InsertRows destSheet, firstDestRow, lastSourceERow

        Dim destRange As Range
        Set destRange = Range("K3:K" & lastSourceERow)

        Dim cell As Range, i As Long
        i = 2
        For Each cell In destRange

            cell.Value = sourceRange(i, 1)
            cell.Offset(0, 1).Value = sourceRange(i, 2)

            i = i + 1
        Next cell

    End If
End Sub


Bit field structures in C++ I just recently learned how to use Bit Field Structures in C++, and I was thinking of applying it to a project. It would certainly make the project code much easier to work with. So, I typed up a quick test program to see if it would work as I expected, and it does. But, I'm not 100% convinced that it is safe.

I am fairly confident my CRC16 union is Endianess specific, but I'm not sure about the Bit Field struct, I suspect it is... Can anyone confirm this?

Is this usage of a bit field struct safe?
Can it be made safer?
Is it Endianess specific?

Note: So far, I have only tested this on 64 bit Linux.

#include <stdint.h>
#include <cstring>
#include <iostream>
#include <iomanip>

typedef union
{
    int16_t Value;
    struct
    {
        int8_t Lo;
        int8_t Hi;
    } Bytes; //This is all I could think of to call this. If you can think of something better, let me know.
} CRC16;

#ifdef _MSC_VER
#pragma pack(push, 1)
#endif
typedef struct
{
    uint8_t SOH;
    uint16_t Count : 14;
    uint16_t Select : 1;
    uint16_t Qsync : 1;
    uint8_t Resp;
    uint8_t Num;
    uint8_t Addr;
    CRC16 Blck;
#ifdef _MSV_VER
} DataHeader;
#pragma pack(pop)
#else
}  __attribute__((packed)) DataHeader;
#endif

template<typename T>
void PrintHex(std::string Description, T Value, long Mask = -1)
{
    using namespace std;
    cout << hex
         << setiosflags(ios_base::showbase)
         << Description
         << " = "
         << (static_cast<unsigned short>(Value) & Mask)
         << resetiosflags(ios_base::showbase)
         << endl;
}

int main()
{

    DataHeader test;
    int8_t packet[8] = {0x64,0x42,0x81,0x42,0x43,0x01,0x32,0x97};

    //this is the most important part, as this structure would be
    //passed to Windows API calls, potentially file and socket I/O
    //functions, etc.
    memcpy(&test, packet, sizeof(packet));

    std::cout << "sizeof(DataHeader) = " << sizeof(DataHeader) << std::endl;
    PrintHex("SOH", test.SOH);
    PrintHex("Count", test.Count);
    PrintHex("Select", test.Select);
    PrintHex("Qsync", test.Qsync);
    PrintHex("Resp", test.Resp);
    PrintHex("Num", test.Num);
    PrintHex("Addr", test.Addr);
    PrintHex("Blck.Value", test.Blck.Value);
    PrintHex("Blck.Bytes.Lo", test.Blck.Bytes.Lo, 0xFF);
    PrintHex("Blck.Bytes.Hi", test.Blck.Bytes.Hi, 0xFF);

    return 0;

}

Output

  sizeof(DataHeader) = 8
  SOH = 0x64
  Count = 0x142
  Select = 0
  Qsync = 0x1
  Resp = 0x42
  Num = 0x43
  Addr = 0x1
  Blck.Value = 0x9732
  Blck.Bytes.Lo = 0x32
  Blck.Bytes.Hi = 0x97

#include <stdint.h>

At least officially, you're supposed to prefer to use <cstdint> instead. I have a hard time getting very excited about it though.

#include <cstring>

From the looks of the code, you really wanted to include <string> here instead. You use an std::string in one place anyway (and I don't any use of anything from <cstring>).

typedef union
{
    int16_t Value;
    struct
    {
        int8_t Lo;
        int8_t Hi;
    } Bytes; //This is all I could think of to call this. If you can think of something better, let me know.
} CRC16;

This usage is fairly typical in C, but in C++, I'd just use:

union CRC16 {
    // ...
};

Just like with class/struct, there's no need for a typedef in C++.

As you guessed, this is endian-specific. Given that this is apparently an old DEC protocol, chances are that it's also little-endian. That's good to the degree that it works fine with Intel processors, but bad to the degree that the normal ntohl, ntohs (etc.) are all defined in terms of big-endian network format, so you can't use them to easily write portable code that converts endianness when needed.

#ifdef _MSC_VER
#pragma pack(push, 1)
#endif
typedef struct

Same comment as above--no real need for a typedef in C++.

uint16_t Count : 14;
uint16_t Select : 1;
uint16_t Qsync : 1;

You probably want to be aware that the mapping from bitfields to bits can vary (even independent of endianness). Not necessarily something you want to change or can fix, but with a different compiler (even with the same CPU) this could rather suddenly break.

#ifdef _MSV_VER

Minor typo--this should almost certainly be _MSC_VER.

template<typename T>
void PrintHex(std::string Description, T Value, long Mask = -1)
{
    using namespace std;
    cout << hex
         << setiosflags(ios_base::showbase)
         << Description
         << " = "
         << (static_cast<unsigned short>(Value) & Mask)
         << resetiosflags(ios_base::showbase)
         << endl;
}

This has (IMO) a problem: after it's done, the hex flag is reset, even if it may have been set before the function was called. I'd rather use ios_base::flags() to retrieve the current flags, when again to restore then when finished. As (nearly) always when taking some action like that which you want to do, then un-do later, I'd prefer to put it into a small class that uses RAII to automate the restoring:

class flag_rest {
    std::ios_base::fmtflags original;
    std::ostream &s;
public:
    flag_rest(std::ostream &s) : original(s.flags()), s(s) {}
    ~flag_rest() { s.flags(original); }
};

Using that, printhex gets a little simpler:

template<typename T>
void PrintHex(std::string Description, T Value, long Mask = -1)
{
    using namespace std;
    flag_rest f(cout);

    cout << showbase
        << hex
        << Description
        << " = "
        << (static_cast<unsigned short>(Value) & Mask)
        << endl;
}

Probably more importantly, it also restores the flag to the state it held before the call, and does so dependably (even if the function exits via an exception). Of course, it's also nice that flag_rest is fairly reusable as well.

Of course, the cast to unsigned short means this can (will) misbehave or any type larger than a short, though I doubt you really care a lot about it.


Connect to database, handling errors I try to handle or catch possible errors with PHP/MySQL for security reasons and would like to know if I'm doing it right.

The first case, connection: I use it as a function and call it always when I need a database connection:

function pdo () {
  try {
    $pdo = new PDO('mysql:host=localhost;dbname=dbname', 'user', 'pw');
  }
  catch(PDOException $e) {
    header("Location: handle_error.php");
    exit;
  }
  return ($pdo);
}

The second case, a query: I am not sure how to handle it. I put all prepared statements in IF conditions but that's pretty awkward. And what about $stmt->execute? This could also fail or not? To handle this also in an if condition can really get confusing. I hope there is a better way to go.

if ($stmt = $pdo->prepare("SELECT a FROM b WHERE c = :c")) {
  $stmt->execute(array(':c' => $c));
  $result = $stmt->fetch();
  echo 'All fine.';
}
else {
  echo 'Now we have a problem.';
}

Later, I would like to report the errors properly so the admin can check what's going on.

First of all, it's a good thing that you asked, only a few do care about handling errors.

Unfortunately, the way you choose is frowned upon, in both cases. Luckily, I've got a couple articles that cover your question in every possible detail:

a generalized one, PHP error reportig will show you the right approach for handling errors in general
a direct answer to your question, How to connect to MySQL using PDO would provide a ready-made connection code

Error handling

The idea here is that a module or a part of code should never treat errors by itself. This function should be delegated to the application level. Your database interaction code should only raise an error, which would be handled according to the site-wide configuration elsewhere.

So your goal with PDO is just to make it throw an error (in the form of Exception). As to how it will be handled should be defined elsewhere. It will make your error reporting uniform and both programmer- and user-friendly.

Connection

Another issue is a function.

  I use it as a function and call it always when I need a database connection.

If you call it more than once, it will create many connections to the database server and this number is not infinite. A connection should be made only once. So in its current form this function is rather useless. Given you have to create a $pdo variable only once, an include file would serve as well.

So let's create it

$host = '127.0.0.1';
$db   = 'test';
$user = 'root';
$pass = '';
$charset = 'utf8mb4';

$options = [
    \PDO::ATTR_ERRMODE            => \PDO::ERRMODE_EXCEPTION,
    \PDO::ATTR_DEFAULT_FETCH_MODE => \PDO::FETCH_ASSOC,
    \PDO::ATTR_EMULATE_PREPARES   => false,
];
$dsn = "mysql:host=$host;dbname=$db;charset=$charset";
try {
     $pdo = new \PDO($dsn, $user, $pass, $options);
} catch (\PDOException $e) {
     throw new \PDOException($e->getMessage(), (int)$e->getCode());
}

Handling query errors.

Like it was said above, do not handle errors in your code. We already configured PDO to throw exceptions in case of error, that's all. So just write your query execution code right away, without any conditions:

$stmt = $pdo->prepare("SELECT a FROM b WHERE c = :c")) {
$stmt->execute(array(':c' => $c));
$result = $stmt->fetch();
echo 'All fine.';


Recursive, nested list traversal Given a nested list of strings (which may contain other nested lists), print the contents of the list and the corresponding depth.

Here is my solution:

private static void dumpList(String string, List l) {
    int n = l.size();
    for (int i = 0; i < n; i++) {
        if(l.get(i) instanceof String){
            System.out.println(string+i + " " + l.get(i));
        }
        if(l.get(i) instanceof List){
            dumpList(string+i, (List)l.get(i));
        }
    }
}

Here is main():

public static void main(String[] args) {
    String s = "Foo";
    List l = new ArrayList();
    ArrayList<String> a = new ArrayList<String>();
    ArrayList<String> b = new ArrayList<String>();
    //ArrayList<String> c = new ArrayList<String>();
    a.add("a");
    a.add("b");
    a.add("c");
    b.add("eggs");
    l.add("a string");
    l.add(a);
    l.add("spam");
    l.add("eggs");
    dumpList("Foo", l);
}

My output:

Foo0 a string
Foo10 a
Foo11 b
Foo12 c
Foo2 spam
Foo3 eggs

I am not that much interested in naming of the variables, but of formatting of the output. Should I use StringBuilder instead of the + operator? Should I print it differently?

This is in addition to what @tim already said.

The dumpList method takes a List,
and then you make 2 .get(i) calls in every iteration of the loop.
Keep in mind that the List interface doesn't ensure random access.
For example I can call this method with a LinkedList parameter,
in which case the .get(i) calls will be costly.
Either change the method parameter to ArrayList or similar random access list,
or change the way you iterate (I recommend), for example like this:

private static void dumpList(String string, List<Object> list) {
    int i = 0;
    for (Object item : list) {
        if (item instanceof List) {
            dumpList(string + i, (List) item);
        } else {
            System.out.println(String.format("%s%d %s", string, i, item));
        }
        ++i;
    }
}

Instead of doing if (item instanceof String) and then if (item instanceof List),
it's better to do if (item instanceof List) and an else.
This solves 2 problems at once:

Adds support for other element types, not only String and List,
which otherwise would have been ignored
The second if should have been either an else if or an else,
because if the first if was true, it's pointless to evaluate the second

I changed the string + i + " " + l.get(i) to the (in my opinion) more readable String.format("...", ...) style.

Finally, I renamed l to list.
Single-letter variable names are acceptable as loop counters, like i, j, k,
otherwise it's better to give meaningful names.


Prompting the user for plot parameters Below is the current working code for an ongoing project.

I am wondering if there is anything I can do to improve the code snippet below. My main concerns lie to two area. First being that I append the answer of an input to a list called inputList and then call it in a block of conditionals. I am wondering if this in particular is a good approach. Are there any better approaches?

Secondly, similar concerns lay with how I defined the answers and dictionaries of their variants. My main purpose there is that, regardless of how you enter "yes" or "no", as long as it has those letters and in the proper order, i.e "nO" or "yES", it would be accepted. I also did it as I did so that I would not have to concern myself on checking whether or not the first letter of any of those two words are capitalised or not.

Again, my question being, is this a "solid" approach? What alternative approaches would be better for this snippet of code? Perhaps a better question would be, how can I improve this snippet?

I am also interested in seeing if I can shorten it as much as possible. Similar to how permLet() is written.

from itertools import product, zip_longest
import pylab as plot
import numpy as number
import pprint

inputFile = input('File Name: ')

def yLimits():
    yMin = int(input('Set lower limit: '))
    yMax = int(input('Set upper limit: '))
    labelLocation = number.arange(len(count))
    plot.bar(labelLocation, list(count.values()), align='center', width=0.5)
    plot.xticks(labelLocation, list(count.keys()))
    plot.xlabel('Characters')
    plot.ylabel('Frequency')
    plot.ylim(yMin, yMax)
    plot.show()

def ylimChoice():

    #returns all permutations of letter capitalisation in a certain word.
    def permLet(s):
        return(''.join(t) for t in product(*zip(s.lower(), s.upper())))

    inputList = []
    yesNo = input('Would you like to set custom ylim() arguments? ')
    inputList.append(yesNo)
    yes = list(permLet("yes"))
    no = list(permLet("no"))

    if any(yesNo in str({y: y for y in yes}.values()) for yesNo in inputList[0]):
        yLimits()
    elif any(yesNo in str({n: n for n in no}.values()) for yesNo in inputList[0]):
        labelLocation = number.arange(len(count))
        plot.bar(labelLocation, list(count.values()), align='center', width=0.5)
        plot.xticks(labelLocation, list(count.keys()))
        plot.xlabel('Characters')
        plot.ylabel('Frequency')
        plot.autoscale(enable=True, axis='both', tight=False)
        plot.show()

count = { }
with open(inputFile, 'r') as info:
    readFile = info.read()
    for character in readFile.upper():
        count.setdefault(character, 0)
        count[character] = count[character]+1

value = pprint.pformat(count)
print(value)

ylimChoice()

It does not improve readability to rename modules. There are a few common aliases used, usually to shorten the name: import numpy as np and import matplotlib.pyplot as plt. Apart from that you should stick to the module name.

I was searching your whole code for the initialization of plot and number, because they sound more like normal variables and not like modules. Especially plot sounds like an object.

You should also choose more descriptive variable names (and adhere to PEP8, Python's official style-guide). capitalization_permutations tells you immediately what the function does, permLet does not.

To iterate over the lines of a file, it is sufficient to do:

with open(inputFile, 'r') as info:
    for character in info:
        character = character.upper()
        # do something

But since you actually want the characters, you could just use collections.Counter here:

with open(inputFile, 'r') as info:
    count = collections.Counter(info.read().upper())

Instead of value = pprint.format(count); print(value) you can directly do pprint.pprint(count).

Currently your main code is all over the place. inputFile is defined in the beginning, followed by the function definitions, followed by more code.

Instead follow this build-up:

Module imports
CONSTANTS
Class definitions
function definitions
rest of the code

You could put all your code into a main function and call it like this:

def main():
    inputFile = input('File Name: ') # Delete whitespace at end of line here
    with open(inputFile, 'r') as info:
        count = collections.Counter(info.read().upper())
    pprint.pprint(count)
    ylimChoice(count)

if __name__ == "__main__":
    main()

This allows you to do e.g. import plot_parameters to use plot_parameters.ylimChoice() in another script without the main part of the code being run.

At the same time you can remove the reliance on the global variables by just passing the relevant object as parameters (e.g. count to ylimChoice and yCounts).

Lastly this monstrosity:

inputList = []
yesNo = input('Would you like to set custom ylim() arguments? ')
inputList.append(yesNo)
yes = list(permLet("yes"))
no = list(permLet("no"))

if any(yesNo in str({y: y for y in yes}.values()) for yesNo in inputList[0]):
    ...

What you want to know is if the user-choice is some permutation of yes with random capitalization. This is easiest achieved with if user_choice.lower() == 'yes'. At the same time you append the user_value to a list, and then iterate over the first element of that list to use in an any (with one element)(!). That's just too much overhead. Just use this:

user_input = input('Would you like to set custom ylim() arguments? ')
if user_input.lower() in ('y', 'yes'):
    yLimits(count)
else:
    ...
    # do other stuff

Note that I also allowed the user to just type y. Also, the other code will run on all other input. Your code does nothing (not even a notice that the user input was not recognized) if a wrong word is entered.


Random Number Guessing Game Code Efficiency - Python UPDATE: I have added:

a play-again option
score keeping thing
difficulty levels

**(but I still need help with the functions part).
I don't feel it's necessary to explain what my code does, as it is pretty self-explanatory. I have the entire program completed, but I need to put some of the components of the program into functions. I don't know how to do this. I also need to add a play again option (I have done this before, but for some reason this particular code isn't working...).
import random

#variables:
score = 0
play = True

while play:
    while True:
        try:
            difficulty = int(input("Give me a number between 1 and 3 (these are the difficulty levels) \n\n 1 = easy \n 2 = medium \n 3 = hard \n\n"))
        except:
            print("This is not a number!")
            continue

        if difficulty > 3 or difficulty < 1:
            print("That's an invalid input, please re-enter your number")
            continue
        break

    number = random.randint(1, 10 * difficulty)
    attempts = 0
    guess = 0

    while guess != number and attempts < 3:
        attempts += 1
        print("attempt number " + str(attempts))

        while True:
            try:
                guess = int(input("Guess what number I am thinking of  "))
            except:
                print("That's not a number!")
                continue

            if guess > 10 * difficulty or guess < 1:
                print("That's an invalid input, please re-enter your number.")
                continue
            break

        if guess == number:
            print("You guessed it correct!")
            score += 1
        elif abs(guess - number) == 1:
            print("You're hot on the trail!")
        elif abs(guess - number) <= 3:
            print("You're getting warmer...")
        else:
            print("You're freezing cold!!")

    if attempts == 3 and guess != number:
        print("Oops! You've run out of guesses!")
        print("The number I was thinking of was " + str(number) + ".")

    print("Your final score is " + str(score))

    text = input("If you want to play again, type 'yes' ")
    if text != "yes":
        play = False
        print("Ok, thank you for playing!")

I'm not exactly sure what you're asking but with my best guess, I'll give it a try.
What you should be turning into functions (in my opinon)

"bad" inputs (such as 'a', 'b' etc. or '%', '*' etc.); error trapping algorithm chunk
of code into a function.

"You're hot on the trail" etc.) block of code can also be converted
to a function.

My biggest concern with your program is the following:
if text != "yes":  then  play = False - Why just "yes"?? Why not "yes" and "Yes"?
What if the user inputs "Yes" - with a capital "Y" instead of "yes" - with a lower-case "y"? (I tried this by the way, and this was the output: "Ok, thank you for playing!"). For them (the user) they're indicating that they want to have another try at the game, but due to your code's incompetence for a variety of options (like "Yes" or "yes" - for replaying, "No" or "no" - for NOT replaying). Please solve this problem.
Now to your more imperative problem (the functions), I can give it a shot.
Note: You should aim for maximum clarity if you want your code to be re-used by others, or to make it easier to read and understand (not just for others but for you as well!).
if guess > 10 * difficulty or guess < 1:
                print("That's an invalid input, please re-enter your number.")
                # ⬆ this particular line!! ⬆
                continue
            break

You didn't specify WHY the input is invalid, you stated the obvious by saying "It's an invalid input"...be concise and again, aim for MAXIMUM clarity.
Note: I did abbreviate some of the variable/function names just for saving time.
Anyway, here's how I would've done this:
import random

#variables:
sc = 0
pl = True

# "bad" input loop function.
def get_num(diff):

    while True:
        try:
            gs = int(input("Guess what number I am thinking of  "))
        except:
            print("Please enter a NUMBER. Letters (a, b...) and symbols (#, %...) are not appropriate vales.")
            continue

        if gs > 10 * diff or gs < 1:
            print("Your number is not in the right range, please re-enter your number!")
            continue
        return gs

# "hot and cold" function
def temp_func(gs, num):

    if gs == num:
        print("CORRECT!")
        return True
    elif abs(gs - num) == 1:
        print("HOT!")
        return False
    elif abs(gs - num) <= 3:
        print("WARM")
        return False
    else:
        print("COLD!")
        return False

while pl:
    while True:
        try:
            diff = int(input("Give me a number between 1 and 3 "
                                   "(these are the difficulty levels) \n\n 1 = easy \n 2 = medium \n 3 = hard \n\n"))
        except:
            print("This is not a number!")
            continue

        if diff > 3 or diff < 1:
            print("That's an invalid input, please re-enter your number")
            continue
        break

    num = random.randint(1, 10 * diff)

    attmp = 0
    gs = 0

    while gs != num and attmp < 3:
        attmp += 1
        print("attempt number " + str(attmp))

        gs = get_num(diff)

        did_win = temp_func(gs, num)
        if did_win:
            sc += 1
            break

    if attmp == 3 and gs != num:
        print("Oops! You've run out of guesses!")
        print("The number I was thinking of was " + str(num) + ".")

    print("Your final score is " + str(sc))

    txt = input("If you want to play again, type 'yes' ")
    if txt != "yes":
        pl = False
        print("TYPE GOODBYE MESSAGE HERE")


Readability in creation of a long output string Below is a function from a driver that I wrote for and I2C temperature sensor.
The function takes as input the name of the bus and the device's bus address, reads it's status register, and then reports the status verbosely. (It also returns a values of interest, but that's not important now).

I have two questions about this:

I'm using lots of comment text {}'.format('this' if Condition else 'that'). I gather this is frowned upon, but in this case I think readability is optimal. The alternative would be

statusthing = 'this' if Condition else 'that'
'comment text {}'.format(statusthing)

or, worse (as in 'much more lines and no better readability'):

if Condition:
    statusthing = 'this'
else:
    statusthing = 'that'

Rpt += 'comment text %s' % statusthing

Same for using this way of constructing the output string. I see people get upset when you do this:

Rpt = 'eggs, bacon, sausage \n'
.
.
Rpt += 'sausage'

But in this case, it's concise, I think it's readable, it works. I don't see anything wrong with it. Is there?

Here's the full function.

def read_config(bus, sensor):
    Conf = bus.read_byte_data(sensor, ACCESS_CONFIG)

    TH = decode_DS(bus.read_word_data(sensor, ACCESS_TH))
    TL = decode_DS(bus.read_word_data(sensor, ACCESS_TL))

    Rpt = '\nStatus of DS1621 at address {}:\n'.format(sensor)

    Rpt += '\tConversion is {}\n'.format(
            'done' if Conf & DONE else 'in process')

    Rpt += '\t{} measured {} degrees Celsius or more\n'.format(
            'HAVE' if Conf & TH_BIT else 'have NOT', str(TH))

    Rpt += '\t{} measured below {} degrees Celsius\n'.format(
            'HAVE' if Conf & TL_BIT else 'have NOT', str(TL))

    Rpt += '\tNon-volatile memory is {}\n'.format(
            'BUSY' if Conf & NVB else 'not busy')

    if Conf & POL_HI:
        level, device = 'HIGH', 'cooler'
    else:
        level, device = 'LOW', 'heater'

    Rpt += '\tThermostat output is Active {} (1 turns the {} on)\n'.format(
            level, device)

    Rpt += '\tDevice is measuring {}\n'.format(
            'in One Shot mode' if Conf & ONE_SHOT else 'continuously')

    print Rpt

    return Conf, TH, TL

My aim is for this to be as usable, readable and understandable as possible.

How about something like this:

def read_config(bus, sensor):
    tpl = '''Status of DS1621 at address {sensor}:
    \tConversion is {done}
    \t{have_th} measured {th} degrees Celsius or more
    \t{have_tl} measured below {tl} degrees Celsius
    \tNon-volatile memory is {busy}'''

    print tpl.format(sensor=sensor,
                     done='done' if Conf & DONE else 'in process',
                     have_th='HAVE' if Conf & TH_BIT else 'have NOT',
                     th=TH,
                     have_tl='HAVE' if Conf & TL_BIT else 'have NOT',
                     tl=TL,
                     busy='BUSY' if Conf & NVB else 'not busy',
                     )

This is PEP8 compliant, and I think more readable.

Also, I think there's nothing wrong with this kind of use:

'something' if condition else 'otherthing'


Word break problem with recursion and memorization For practice, I solved Leetcode 139. worbreak question:

  Given a non-empty string s and a dictionary wordDict containing a list of non-empty words, determine if s can be segmented into a space-separated sequence of one or more dictionary words.

I have the solution to use the recursive with in-memory hash table to reduced to duplication calculation. but it seems like works for some case but not all. any suggestions?

def wordbreak(self, s, wordDict):
    wordDict = set(wordDict)
    str_map = dict()
    if not s or not wordDict: return False

    def check(s):
        if s in str_map: return str_map[s]
        if s in wordDict:
            str_map[s] = True
            return True
        for i, c in enumerate(s[1:], 1):
            if s[:i] in set(wordDict) and check(s[i:]):
                str_map[s[:i]] = True
        str_map[s] = False
        return False

    return check(s)

if __name__ == '__main__':
    testcase = [
        dict(tc=dict(s='ab', wordDict=['a', 'b']), exp=True),
        dict(tc=dict(s='leetcode', wordDict=['leet', 'code']), exp=True),
        dict(tc=dict(
            s="aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaab",
            wordDict=["a", "aa", "aaa", "aaaa", "aaaaa", "aaaaaa", "aaaaaaa", "aaaaaaaa", "aaaaaaaaa",
                      "aaaaaaaaaa"]), exp=False),
    ]
    for tc in testcase:
        res = wordbreak(**tc['tc'])
        print(res) # True, True, False

bug

def wordBreak(self, s, wordDict):
    wordDict = set(wordDict)
    str_map = dict()
    if not s or not wordDict: return False

    def check(s):
        if s in str_map: return str_map[s]
        if s in wordDict:
            str_map[s] = True
            return True
        for i, c in enumerate(s[1:], 1):
            if s[:i] in set(wordDict) and check(s[i:]):
                str_map[s[:i]] = True
                return True  # <== you forgot this one
        str_map[s] = False
        return False

    return check(s)

passes all cases in 48ms

set(wordDict)

No need to do this in each iteration if you do that at the start of the algorithm

c

this variable is not used. Python convention uses _ for variables that are not used

reinventing the wheel (cache)

Python is batteries included

so you can as easily use functools.lru_cache instead of implementing your own cache:

from functools import lru_cache

def wordBreak(self, s, wordDict):
    wordDict = set(wordDict)
    if not s or not wordDict: return False
    @lru_cache(None)
    def check(s):
        if s in wordDict:
            return True
        for i, _ in enumerate(s[1:], 1):
            if s[:i] in wordDict and check(s[i:]):
                return True
        return False
    return check(s)

passes in 36ms

alternative version

instead of running over the string, you can also try to build a graph of all occurrences of the words in wordDict, and then look if you can find a path from 0 to len(s)

all chars in wordDict

you can check whether all chars in s appear in a word in wordDict

def validate_all_chars(text, words):
    all_chars = set()
    for word in words:
        all_chars.update(word)
    return all_chars.issuperset(text)

or with itertools.chain:

from itertools import chain
def validate_all_chars(text, words):
    all_chars = set(chain.from_iterable(words))
    return all_chars.issuperset(text)

occurrences:

Unfortunately, re.finditer and re.findall only give non-overlapping occurrences, so we can't use them here. So we'll have to build our own finder:

def find_occurrences(pattern: str, text: str):
    idx = 0
    while True:
        idx = text.find(pattern, idx)
        if idx == -1:
            return
        yield idx, idx + len(pattern)
        idx += 1

build a graph

from collections import defaultdict
def build_span_graph(text, words):
    all_spans = defaultdict(set)
    spans = (find_occurrences(word, text) for word in words)
    for span in spans:
        for begin, end in span:
            all_spans[begin].add(end)
    return all_spans

walk the graph:

def find_path(spans, length, begin=0, visited=None):
    visited = set() if visited is None else visited
    visited.add(begin)
    ends = spans[begin]
    return length in ends or any(
        find_path(spans, length=length, begin=new_begin, visited=visited)
        for new_begin in ends
        if new_begin not in visited and new_begin in spans
    )

Here you can try to optimize a bit by first visiting the furthest ends, but that is only marginally useful

main:

def wordBreak(s: "str", wordDict: "List[str]") -> "bool":
    if not validate_all_chars(s, wordDict):
        return False

    spans = build_span_graph(s, wordDict)

    return find_path(s, wordDict)


Project Euler 38: Pandigital Multiples I just solved Project Euler 38 and was wondering if you guys could provide some suggestions on how to speed it up. I've tried adding extra conditions but they increase the time. Any ideas on how to optimize the numbers I'm checking would be appreciated.

from timeit import default_timer as timer

def create_pandigital_multiple(original_number):
    num = str(original_number)
    digits = range(1, 10)
    if len(num) >= 5:
        return 0
    else:
        temp = 2
        while len(num) < 9:
            num += str(original_number * temp)
            temp += 1
        if sorted([int(x) for x in num]) == digits:
            return int(num)
        else:
            return 0

start = timer()
ans = 0
for k in range(9, 10**5):
    if create_pandigital_multiple(k) > greatest:
        ans = create_pandigital_multiple(k)

elapsed_time = (timer() - start) * 1000 # s --> ms

print "Found %d in %r ms." % (ans, elapsed_time)

Checking if len(num) >= 5 is redundant because you only call the function with k < 10**5
Instead of sorted([int(x) for x in num]) you could compute just sorted(num) and compare that to a DIGITS = [str(i) for i in range(1, 10)] constant that you can compute outside the function.
Most of the multiples end up longer than 9 digits. Testing len(num) == 9 before sorting the digits should be faster.
You could take advantage of the example 918273645 provided in the problem statement. The number you are looking for must be greater or equal to that. For one thing, the first two digits must be in range 91 to 98, which is a small fraction of all the two-digit numbers you try.


Drawing sets of boxes to display contents of arrays This code draws two sets of 9 boxes on the screen to display the contents of two arrays.

How can I speed up, shorten and make the code more efficient?

    for (int x=0;x<3;x++){
        for (int y=0;y<3;y++){
            if (pattern[x][y]==1){
                g.setColor(colorrange.colb());
                g.fillRect(actlo+(x*size),actlo+(y*size),size,size);
            }
                g.setColor(Color.yellow);
                g.drawRect(actlo+(x*size),actlo+(y*size),size,size);

            if (patternb[x][y]==1){
                g.setColor(colorrange.colb());
                g.fillRect(actlo+(x*size),inlo+(y*size),size,size);
            }
                g.setColor(Color.yellow);
                g.drawRect(actlo+(x*size),inlo+(y*size),size,size);
        }
    }

Multiplies can be pretty expensive within loops, so I minimized the use of them by precalculating where possible. It is also better to have the Y loop before the X loop to prevent cache misses (which wouldn't matter on a really small array like this but is a good practice). Consider this alternative:

for(int y = 0; y < 3; y++)
{
  final int yy = y * size;

  for(int x = 0; x < 3; x++)
  {
    final int xx = x * size;

    if(pattern[x][y] == 1)
    {
      g.setColor(colorrange.colb());
      g.fillRect(actlo + xx, actlo + yy, size, size);
    }

    g.setColor(Color.yellow);
    g.drawRect(actlo + xx, actlo + yy, size, size);

    if(patternb[x][y] == 1)
    {
      g.setColor(colorrange.colb());
      g.fillRect(actlo + xx, inlo + yy, size, size);
    }

    g.setColor(Color.yellow);
    g.drawRect(actlo + xx, inlo + yy, size, size);
  }
}


Comparable playing cards I have written a working implementation of a Poker card that I want to be comparable. I feel that my solution is somewhat bloated in C++, and want to ask if I should have done something different to make it more concise.

#ifndef POKER_CARD_H
#define POKER_CARD_H

enum Value { Two, Three, Four, Five, Six, Seven, Eight, Nine, Ten, Jack, Queen, King, Ace};
enum Color {Hearts, Spades, Dimonds, Clubs};

namespace {
    int as_int(Value value){
        switch(value){
            case Value::Two     : return 2;
            case Value::Three   : return 3;
            case Value::Four    : return 4;
            case Value::Five    : return 5;
            case Value::Six     : return 6;
            case Value::Seven   : return 7;
            case Value::Eight   : return 8;
            case Value::Nine    : return 9;
            case Value::Ten     : return 10;
            case Value::Jack    : return 11;
            case Value::Queen   : return 12;
            case Value::King    : return 13;
            case Value::Ace     : return 14;
        }
    }
    int as_int(Color color){
        switch(color){
            case Color::Dimonds : return 1;
            case Color::Clubs   : return 2;
            case Color::Hearts  : return 3;
            case Color::Spades  : return 4;
        }
    }
}

class Card {

public:
    Card(Value value, Color color);

    Color getColor() const;

    Value getValue() const;

private:
    Value value;
    Color color;
};

inline bool operator==(const Card& lhs, const Card& rhs) {
    return ((rhs.getValue() == lhs.getValue()) && (rhs.getColor() == lhs.getColor()));
}
inline bool operator!=(const Card& lhs, const Card& rhs) {return !operator==(lhs,rhs);}
inline bool operator< (const Card& lhs, const Card& rhs) {
    if (as_int(lhs.getValue()) < as_int(rhs.getValue())){
        return true;
    }
    else if (as_int(rhs.getValue()) == as_int(lhs.getValue())){
        return as_int(lhs.getColor()) < as_int(rhs.getColor()) ? true : false;
    }
    else {
        return false;
    }
}
inline bool operator> (const Card& lhs, const Card& rhs) {return  operator< (rhs,lhs);}
inline bool operator<=(const Card& lhs, const Card& rhs) {return !operator> (lhs,rhs);}
inline bool operator>=(const Card& lhs, const Card& rhs) {return !operator< (lhs,rhs);}

#endif //POKER_CARD_H

Plain enum (unlike enum class) convert implicitly to integers, so there's no need for the as_int() functions if you declare the enums in ascending order.  You probably want the suits in the order ♣♦♥♠ if you want to re-use your cards for Bridge as well as Poker.

I had to make up the constructor and get methods, since they are missing from the review:

Card(Value value, Color color)
    : value{value},
      color{color}
{}

Color getColor() const { return color; };

Value getValue() const { return value; };

I also created a simple test program that returns non-zero if we break the expectations:

int main()
{
    return
        + (Card{Two, Clubs} > Card{Ace, Spades})
        + (Card{Two, Clubs} > Card{Two, Spades})
        + (Card{Two, Spades} > Card{Ace, Spades});
}

Now, we can remove the as_int():

enum Value { Two, Three, Four, Five, Six, Seven, Eight, Nine, Ten, Jack, Queen, King, Ace};
enum Color { Clubs, Dimonds, Hearts, Spades};

inline bool operator==(const Card& lhs, const Card& rhs)
{
    return rhs.getValue() == lhs.getValue()
        && rhs.getColor() == lhs.getColor();
}

inline bool operator!=(const Card& lhs, const Card& rhs) { return !(lhs==rhs); }

inline bool operator< (const Card& lhs, const Card& rhs)
{
    if (lhs.getValue() < rhs.getValue()) {
        return true;
    }
    else if (rhs.getValue() == lhs.getValue()) {
        return lhs.getColor() < rhs.getColor() ? true : false;
    }
    else {
        return false;
    }
}

inline bool operator> (const Card& lhs, const Card& rhs) {return rhs < lhs;}
inline bool operator<=(const Card& lhs, const Card& rhs) {return !(lhs>rhs);}
inline bool operator>=(const Card& lhs, const Card& rhs) {return !(lhs<rhs);}

Note that this ternary conditional:

        return lhs.getColor() < rhs.getColor() ? true : false;

is exactly equivalent to this:

        return lhs.getColor() < rhs.getColor();

But we can simplify the < much further by delegating to std::tuple:

inline bool operator< (const Card& lhs, const Card& rhs) {
    return std::tuple(lhs.getValue(), lhs.getColor()) < std::tuple(rhs.getValue(), rhs.getColor());
}

In C++20, the entire set of 6 comparisons can be replaced with a single function:

public:
    auto operator<=>(const Card&) const = default;


Basic quiz in jQuery I am using jQuery to implement a basic quiz functionality. How can I improve this code snippet even more? Is there any better way of doing it?

I am using jQuery 1.6.4.

/*jshint -W065 */
$(function () {
    var jQuiz = {
        answers: {
            q1: 'd',
            q2: 'd',
            q3: 'a',
            q4: 'c',
            q5: 'a'
        },
        questionLenght: 5,
        checkAnswers: function () {
            var arr = this.answers;
            var ans = this.userAnswers;
            var resultArr = [];
            for (var p in ans) {
                var x = parseInt(p) + 1;
                var key = 'q' + x;
                var flag = false;
                if (ans[p] == 'q' + x + '-' + arr[key]) {
                    flag = true;
                } else {
                    flag = false;
                }
                resultArr.push(flag);
            }
            return resultArr;
        },
        init: function () {
            $('.btnNext').click(function () {
                if ($('input[type=radio]:checked:visible').length === 0) {

                    return false;
                }
                $(this).parents('.questionContainer').fadeOut(500, function () {
                    $(this).next().fadeIn(500);
                });
                var el = $('#progress');
                el.width(el.width() + 120 + 'px');
            });
            $('.btnPrev').click(function () {
                $(this).parents('.questionContainer').fadeOut(500, function () {
                    $(this).prev().fadeIn(500);
                });
                var el = $('#progress');
                el.width(el.width() - 120 + 'px');
            });
            $('.btnShowResult').click(function () {
                var arr = $('input[type=radio]:checked');
                var ans = jQuiz.userAnswers = [];
                for (var i = 0, ii = arr.length; i < ii; i++) {
                    ans.push(arr[i].getAttribute('id'));
                }
                $('#progress').width(300);
                $('#progressKeeper').hide();
                var results = jQuiz.checkAnswers();
                var resultSet = '';
                var trueCount = 0;
                for (var i = 0, ii = results.length; i < ii; i++) {
                    if (results[i] === true) trueCount++;
                    resultSet += '
 Question ' + (i + 1) + ' is ' + results[i] + '
';
                }
                resultSet += '
Your total score is ' + trueCount * 20 + ' / 100
';
                $('#resultKeeper').html(resultSet).show();
            });

        }
    };
    jQuiz.init();
});

Some suggestions

If you ignore the q your questions object is a list of sequential integers so might as well be an array. This will simplify the logic later on.

answers: [
    'd',
    'd',
    'a',
    'c',
    'a'
]

If you use an array as suggested above, then we can simplify this block quite a lot (also, use strict comparators!):

var resultArr = [];
        for (var p in ans) {
            var x = parseInt(p) + 1;
            var key = 'q' + x;
            var flag = false;
            if (ans[p] == 'q' + x + '-' + arr[key]) {
                flag = true;
            } else {
                flag = false;
            }
            resultArr.push(flag);
        }

    var results = [];
    for ( var i = 0; = < ans.length; i++ ) {
        var flag = (ans[p] === arr[i]);
        result.push(flag);
    }

This is daft unless you want to make certain it's not something other than a boolean;

if (results[i] === true) {}

if (results[i]) {}

It's much nicer to build HTML using jQuery than with Strings. And your for loop seems to have an unnecessary variable.

 var resultSet = $('<div>');
 var trueCount = 0;
 for (var i = 0; i < results.length; i++) {
    if (results[i] === true) trueCount++;
    resultSet.text('Question ' + (i + 1) + ' is ' + results[i] + ');
 }

If you're declaring multiple variables you can separate them with a comma.

var results = jQuiz.checkAnswers(),
  resultSet = $('<div>'),
  trueCount = 0;


Ajax call with Spring MVC Controller I have been woring on AJAX and JQuery site. Here is my simple implemtation. i am looking forward to getting your reviews for any improvement/replacement for code mentioned below.

  Brief explanation: when someone want to submit the form with specified ID, all form fields are assigned to appropriate variables. After that a new JSON document is generated based on the form field variables. Then AJAX call is performed. It directed to URL which is specified in the action attribute of form tag. The JSON is used as a data which need to be processed. Type of the request is POST (it can vary depending on operation, e.g. for update it will has PUT value).

        <input name="search" type="text" maxlength="100" id="search"/>

        <button onclick="searchID()"> Search ID </button>
        <button onclick="getAll()"> Get All </button>

 Id: <input name="search" type="text" maxlength="100" id="userId"/>

 First name: <input name="search" type="text" maxlength="100" id="fName"/>

 Last name: <input name="search" type="text" maxlength="100" id="lName"/>

        <button onclick="save()"> Save </button>

I think you just can get rid of getAll() function as it is just a special case of searchID().

 function searchID() {
     var id = document.getElementById("search").value;
     $("#customers").html("");

     $.getJSON("http://localhost:8080/customer/" + id,
       function(data) {
                 ([].concat(data)).forEach(function(customer) {
                     $('#customers').append("
ID: " + customer.userId + "
");
                     $('#customers').append("
First name: " + customer.fName + "
");
                     $('#customers').append("
Last name: " + customer.lName + "

");
                 });
       });
  }

If the id value is empty then the results it's exactly the same as getAll().

Another important thing to change is the url, if your server is the same, you can just remove the initial part:

$.getJSON("/customer/"...);

This helps to avoid bugs in the future or need of refactoring if you just simply add https.

Cache the jQuery search to avoid the repeat:

 function searchID() {
     var $customers = $("#customers");
     var id = document.getElementById("search").value;
     $customers.html("");

     $.getJSON("http://localhost:8080/customer/" + id,
       function(data) {
                 ([].concat(data)).forEach(function(customer) {
                     $customers.append("
ID: " + customer.userId + "
");
                     $customers.append("
First name: " + customer.fName + "
");
                     $customers.append("
Last name: " + customer.lName + "

");
                 });
       });
  }

Here is an example inside the searchID() function, if you use in all the module, you should save as a module variable.

Another thing is that is not responsability of searchID() to know how to get the id:

 function searchID(id) {
     var $customers = $("#customers");
     $customers.html("");

     $.getJSON("http://localhost:8080/customer/" + id,
       function(data) {
                 ([].concat(data)).forEach(function(customer) {
                     $customers.append("
ID: " + customer.userId + "
");
                     $customers.append("
First name: " + customer.fName + "
");
                     $customers.append("
Last name: " + customer.lName + "

");
                 });
       });
  }

Instead you should add a new function:

function performSearch() {
    var id = document.getElementById("search").value;
    searchID(id);
}

And the same for the draw of the customers on the page:

function displayCustomers($el, customers) {
    $el.html("");

    customers.forEach(function(customer) {
           $el.append("
ID: " + customer.userId + "
");
           $el.append("
First name: " + customer.fName + "
");
           $el.append("
Last name: " + customer.lName + "

");
    });
}

And then:

function performSearch() {
    var id = document.getElementById("search").value;
    searchID(id, function(customersToDisplay) {
        displayCustomers($("#customers"), customersToDisplay);
    });
}

And the new searchID():

 function searchID(id, callback) {
     $.getJSON("/customer/" + id,
       function(data) {
           callback(([].concat(data));
       });
  }

In this way you separate the different aspects:

handle the dom event and launch a command accordingly;
perform an ajax request and handle the harmonization of the response.
use a callback to handle the ajax response
update the DOM


Python novice's calculator This is my first ever project I've coded in Python so I'm sorry if it's a little novice. I'm just looking for beginner feedback on what I could improve next time.

There is one issue with this calculator: let's say I add 1 and 2, I expect to get 3, instead it returns 3.0, but I'm guessing that's because I'm casting to a float.

The reason I'm casting to a float is to accept decimals in the equation. It would be nicer if it displayed the whole number without the .0

while True:
    calculation = input("Calculation: ")

    if (calculation.__contains__("+")):
        print(float(calculation.split('+')[0]) + float(calculation.split('+')[1]))

    elif (calculation.__contains__("-")):
        print(float(calculation.split('-')[0]) - float(calculation.split('-')[1]))

    elif (calculation.__contains__("*")):
        print(float(calculation.split('*')[0]) * float(calculation.split('*')[1]))

    elif (calculation.__contains__("/")):
        print(float(calculation.split('/')[0]) / float(calculation.split('/')[1]))

Pythons magic methods are there to make your life easier, not harder. They allow custom classes to use built-in functionality. They are not there so you have to use long names with built-in types.

The __contains__ method is called under the hood whenever you use the in keyword.

In addition, you do the same thing for all four operations: See if it is present, split by the operator and apply it. This can be simplified using a loop over a dictionary, mapping the string of the operator to the operator itself (using the operator module which gives you functions for most operations).

And so your code can be simplified to:

import operator

OPERATIONS = {"+": operator.add,
              "-": operator.sub,
              "*": operator.mul,
              "/": operator.div}

while True:
    calculation = input("Calculation: ")
    for name, op in OPERATIONS.items():
        if name in calculation:
            a, b = map(float, calculation.split(name))
            print(op(a, b))

And at that point you should ask yourself how to modify this to allow multiple operations or even grouping expressions with parenthesis.

You should also probably add some error handling (what happens if the user tries to enter multiple operations? What happens if they do not enter a number?).