Checking if a date was increased by date-constants At the moment I have a Reminder Date. In the UI I have two fields to create the reminder date. The first field is an int and the second field is a combo box.

public int Frequency {get; set;}
public ReminderComboItems item {get; set;}

public enum ReminderComboItems
{
    Minutes, Hours, Days, Weeks, Months, Years
}

public static DateTime ReminderComboItem(ReminderComboItems item, DateTime activityDate, int frequency)
{
    switch (item)
    {
        case ReminderComboItems.Minutes:
            date = date.AddMinutes(frequency);
            return date;
        case ReminderComboItems.Hours:
            date = date.AddHours(frequency);
            return date;
        case ReminderComboItems.Days:
            date = date.AddDays(frequency);
            return date;
        case ReminderComboItems.Months:
            date = date.AddMonths(frequency);
            return date;
        case ReminderComboItems.Years:
            date = date.AddYears(frequency);
            return date;
        default:
            return date;
    }
}

Using the top function creates the Reminder Date, now I'm trying to do the reverse.

public static ReminderDTO ReverseAppointment(DateTime activityDate, DateTime reminderDate)
{
    for (int i = 0; i < 100; i++)
    {
        var incrementedDate = reminderDate.AddYears(-i);
        if (incrementedDate == activityDate)
            return new reminderDTO{ ReminderComboItem = ReminderComboItems.Years, i};
    }
    for (int i = 0; i < 100; i++)
    {
        var incrementedDate = reminderDate.AddMonths(-i);
        if (incrementedDate == activityDate)
            return new ReminderDTO{ ReminderComboItem = ReminderComboItems.Months, i};
    }

   ...
   iterative loop for weeks, days, hours, minutes
   ...

    //default
    var minutes = activityDate.Subtract(reminderDate).TotalMinutes
    return new ReminderDTO{ ReminderComboItem = ReminderComboItems.Months, minutes};
}

I'm trying to have something that's less iterative, the fact that years/months aren't static(diff month lengths and leap years) it makes it a bit difficult. May I get some thoughts/opinions on this please.

Here's how I would do it.  First make sure the reminderDate is after the activityDate.  Then check if the month, day, and time are the same.  If so you can do the year difference.  If not then you check if just the day and time are the same and do the month difference (along with the year difference times 12).  Otherwise subtract the dates and look at the TimeSpan.  If the minutes are not zero then return the TotalMinutes.  If the hours are not zero return the TotalHours.  If the number of days is divisible by 7 then do the weeks.  Otherwise just return the Days.

public static ReminderDTO ReverseAppointment(
    DateTime activityDate, DateTime reminderDate)
{
    if (reminderDate <= activityDate)
        throw new ArgumentException("reminder cannot be <= activity");

    if (reminderDate.Month == activityDate.Month
        && reminderDate.Day == activityDate.Day
        && reminderDate.TimeOfDay == activityDate.TimeOfDay)
    {
        return new ReminderDTO
                {
                    ReminderComboItem = ReminderComboItems.Years,
                    Frequency = reminderDate.Year - activityDate.Year
                };
    }

    if (reminderDate.Day == activityDate.Day
        && reminderDate.TimeOfDay == activityDate.TimeOfDay)
    {
        return new ReminderDTO
                {
                    ReminderComboItem = ReminderComboItems.Months,
                    Frequency =
                        (reminderDate.Month - activityDate.Month)
                        + (12 * (reminderDate.Year - activityDate.Year))
                };

    }

    var difference = reminderDate - activityDate;

    if (difference.Minutes != 0)
    {
        return new ReminderDTO
                {
                    ReminderComboItem = ReminderComboItems.Minutes,
                    Frequency = (int)difference.TotalMinutes
                };
    }
    if (difference.Hours != 0)
    {
        return new ReminderDTO
                {
                    ReminderComboItem = ReminderComboItems.Hours,
                    Frequency = (int)difference.TotalHours
                };
    }

    if (difference.Days % 7 == 0)
    {
        return new ReminderDTO
        {
            ReminderComboItem = ReminderComboItems.Weeks,
            Frequency = difference.Days / 7
        };
    }

    return new ReminderDTO
            {
                ReminderComboItem = ReminderComboItems.Days,
                Frequency = difference.Days
            };
}

Note that if the dates differ by less than a minute you'll end up with 0 days, but I'm assuming all you're dates will differ by at least one minute based on how they are generated.


Finding the distance between two points in C++ I made a program in C++ where it calculates the distance between two coordinates. Is there anything to improve? Add? Make it more user-friendly?

Code:

 #include <iostream>
 #include <cmath>

using namespace std;

int ch;
double x;
double y;
double a;
double b;
double answer;

double distanceBetweenTwoPoints(double x, double y, double a, double b);

int main(){

        cout << "Enter the points for the coordinates";
        cout << endl;
        cout << "Point x for first coordinates: ";
        cin >> x;
        cout << endl;
        cout << endl;
        cout << "Point y for first coordinate: ";
        cin >> y;
        cout << endl;
        cout << endl;
        cout << "Point x for the second coordinate: ";
        cin >> a;
        cout << endl;
        cout << endl;
        cout << "Point y for the second coordinate: ";
        cin >> b;
        cout << endl;
        cout << endl;
        answer = distanceBetweenTwoPoints(x, y, a, b);
        cout << "The answer is " << answer;
}

double distanceBetweenTwoPoints(double x, double y, double a, double b){
return sqrt(pow(x - a, 2) + pow(y - b, 2));
}

I think @john has made some excellent comments. I agree with them, but you've already gotten them, so I'll try not to repeat the same.

Global variables

Variables defined outside any function, like these:

int ch;
double x;
double y;
double a;
double b;
double answer;

...are generally called "global", because anything, anywhere in the world program can depend on them and/or modify them.

When some parts of the code modify them and other parts depend on them, it can become difficult to figure out how the code will act under any given circumstances, how to get it to to exactly what you really want, and so on.

Usually, you want to restrict each variable to (approximately) the smallest scope necessary for it to do its job. None of these variables is used outside of the main function, so we can define them there to assure that only the code in main can use them:

int main(){
    double x;
    double y;
    double a;
    double b;
    double answer;

        cout << "Enter the points for the coordinates";
        cout << endl;
        cout << "Point x for first coordinates: ";
        cin >> x;
        cout << endl;
        cout << endl;
        cout << "Point y for first coordinate: ";
        cin >> y;
        cout << endl;
        cout << endl;
        cout << "Point x for the second coordinate: ";
        cin >> a;
        cout << endl;
        cout << endl;
        cout << "Point y for the second coordinate: ";
        cin >> b;
        cout << endl;
        cout << endl;
        answer = distanceBetweenTwoPoints(x, y, a, b);
        cout << "The answer is " << answer;
}

Avoid std::endl

Although it's used in a lot of tutorials and such, std::endl is almost never what anybody really wants. Although it won't make any real difference as you've used it here, using std::endl when (for example) you're writing lots of data to a file can easily make your program run 10x slower than it really needs to1.

Use functions

You may not have learned (much) about functions yet, but they can make your life much easier, largely by reducing repetition. For example, quite a bit of the code in main consists of a number of repetitions of the same basic sequence:

Ask user for input
Read the input

You can make your life considerably simpler by moving repeated actions like this into functions:

double get_value(char const *prompt) {
    double value;

    std::cout << "\n" << prompt;
    std::cin >> value;
    return value;
}

Then the code in main to get values can be shortened a bit, to something like this:

cout << "Enter the points for the coordinates\n";
x = get_value("Point x for first coordinate: ");
y = get_value("Point y for first coordinate: ");
a = get_value("Point x for second coordinate: ");
b = get_value("Point y for second coordinate: ");

This not only reduces the amount of typing involved, but when you make a mistake (and we all do) it improves the chance that you've only made it in one place, so when you fix it, it'll be fixed in general.

Mathematics

You should usually think twice about computing a hypotenuse at all. In a lot of cases (e.g., comparing one distance to another) you can add the squares of the X/Y coordinates, but not take the square root of the result. For a lot of common cases like "find which point is closest to the one I clicked", the square of the distance works just as well as the actual distance, but is much faster to compute (sqrt is often relatively slow).

If you do need to compute a hypotenuse, consider using std::hypot instead of re-implementing it yourself. In the worst case, this will just be a thoroughly tested and debugged version of the code you'd write on your own. In a better case, it might use a different algorithm to compute the result. For example, you might have a case where both your \$\Delta x\$ and \$\Delta y\$ are within range and the final answer would be within range, but \$\Delta x^2 + \Delta y^2\$ is out of range. In this case, the simplistic approach will overflow, but some others won't. Depending on your hardware and/or required precision, alternatives may also be faster.

Avoid using namespace std;

This using directive brings a huge number of names into scope, which can lead to all sorts of problems from colliding with names you try to define. Almost nobody really knows all the names it defines, so it's often hard to avoid them.

It's (by far) easier in the long term to bite the bullet and put up with typing things like std::cout when you need to use them.

Note that this warning applies primarily (maybe even exclusively) to the std namespace. Something like using namespace std::chrono; is often perfectly fine.

If you are going to have a using namespace std;, you usually want to restrict it to the scope of a single function:

int main() {
     using namespace std;

}

This can still lead to the same basic problems, but at least you've restricted the scope, so when a problem arises, cleaning up the mess is less work.

1. In case you care about the reason: making an operating system call to write data to a stream usually has a fair amount of overhead, so each call takes a fair amount of time, even to write a small amount of data. To avoid this, the code in the library stores data in a buffer. It only calls the OS to write the data to the external file once in a while, when the buffer gets full.

std::endl writes a new-line to the output stream (which you want) and it flushes the stream's buffer--forces an immediate OS call to write the data in the buffer, even if it's not full (which you almost never want). For interactive input like this, it won't actually hurt much, but as a simple rule of thumb, I'd just avoid using it at all. In the (rare) cases that you really do want to flush a stream's buffer, use std::flush to do it.


Vue.js code to update selection on screen I have the following code snippet:

    WHU
    20 Oct
    BHA

    CHE
    21 Oct
    WAT

and the following VueJS method:

methods: {
            changeTeam(selection) {
                this.team = selection;
            }
         }

So as you can see, I'm calling this function and passing in the value in the table cell (i.e.  tag) to change a selection. This works for me, but is there a way I can capture the contents of the table cell, by means of some other method? Something a little more reusable or would this just be introducing extra overhead?

The table itself is the result of a dynamically generated page, so I'm not overly bothered about the apparent duplication.

One technique would be to apply fundamentals of event delegation by adding a method to handle all clicks. For instance, an event listener could be added to the root element (or one if child its child nodes, like a
)

mounted: function() {
    this.$el.addEventListener('click', this.clickHandler);
}

Then have the method accept the click event argument:

clickHandler(clickEvent){

That method could check if the target of the event was a table cell by checking the tagName attribute (for TD). To restrict to only certain table cells, add a class attribute - e.g. team. If that class is found in the target element's classList, set the team property to the innerHTML of the target.

clickHandler(clickEvent){
    const target = clickEvent.target;
    if (target.tagName == 'TD' && target.classList.contains('team')) {
        this.team = target.innerHTML;
    }
}

See a demonstration in the sample below. Notice how there is now a clean separation of the event handling logic (JS) and the markup for display (HTML).

new Vue({
  el: '#app',
  data: {
    message: 'Hello Vue.js!',
    team: ''
  },
  mounted: function() {
    this.$el.addEventListener('click', this.clickHandler);
  },
  methods: {
    clickHandler(clickEvent) {
      const target = clickEvent.target;
      if (target.tagName == 'TD' && target.classList.contains('team')) {
        this.team = target.innerHTML;
      }
    }
  }
})

Selected Team: {{ team }}

      WHU
      20 Oct
      BHA

      CHE
      21 Oct
      WAT


Print the name of day How can I improve this code? It takes a number in the range of [1,7] and prints its corresponding day.

#include "stdafx.h"
#include <iostream>     // std::cout
#include <vector>       // std::vector
#include <string>

using std::cin;
using std::cout;
using std::vector;
using std::string;

int main()
{
    vector<string> name_day = { "Saturday" ,"Sunday" , "Monday" , "Tuesday" , "Wednesday" , "Thursday" , "Friday" };
    int n ;

    cout << "This program takes a number in the range of" << "\n" << "[1 , 7] and prints its corresponding day." << "\n" << "............................................................." << "\n";

    while (true)
    {
        bool flag = true;

        cout << "Input a number : ";
        cin >> n;

        if (!cin) //If input is wrong.
        {
            // reset failbit
            cin.clear();
            cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        }

        //If the condition is not satisfied .

        else
        {
            for (int i = 0; i <= 6; i++)
            {
                if (n-1 == i)
                {
                    cout << name_day[i] << "\n" << "-----------------------" << "\n";;
                    flag = false;
                }

            }//end of for
        }

        if (flag) { cout << "Bad Entery !." << "\n" << "-----------------------" << "\n"; }

    }//end of while

    system("pause");
}

Here are some things that may help you improve your program.

Isolate platform-specific code

If you must have stdafx.h, consider wrapping it so that the code is portable:

#ifdef WINDOWS
#include "stdafx.h"
#endif

Make sure you have all required #includes

The code uses std::numeric_limits but doesn't #include <limits>.  A program may compile without the explicit #include because some other header might happen to include it, but that is not guaranteed by the standard.  To compile reliably, the documented #include files should be used.

Don't use system("pause")

There are two reasons not to use system("cls") or system("pause").  The first is that it is not portable to other operating systems which you may or may not care about now.  The second is that it's a security hole, which you absolutely must care about.  Specifically, if some program is defined and named PAUSE or pause, your program will execute that program instead of what you intend, and that other program could be anything.  First, isolate these into a seperate functions pause() and then modify your code to call those functions instead of system.  Then rewrite the contents of those functions to do what you want using C++.  For example:

void pause() {
    getchar();
}

Use const where practical

The name_day vector is not and should not be modified, so it should be declared const.  Better yet, it could be declared static const.

Use better naming

The vector named name_day is well named because it is descriptive of the contents.  However, flag is not a good name because it doesn't suggest the meaning of the variable in the context of the program.

Use string concatenation

The main routine includes this very long line:

std::cout << "This program takes a number in the range of" << "\n" << "[1 , 7] and prints its corresponding day." << "\n" << "............................................................." << "\n";

Each of those is a separate call to operator<< but they don't need to be.  Another way to write that would be like this:

std::cout << "This program takes a number in the range of\n"
             "[1 , 7] and prints its corresponding day.\n"
             ".............................................................\n";

This reduces the entire menu to a single call to operator<< because consecutive strings in C++ (and in C, for that matter) are automatically concatenated into a single string by the compiler.  It is also more readable.

Fix the spelling error

The error string currently says "Bad Entery !."  With corrected punctuation and spelling, that becomes "Bad entry!".  However, it would be even better if the error message suggested what was actually wrong and gave some suggestion to the user about what to do about it.

Use indexing rather than a loop

The current code has this peculiar bit of code:

for (int i = 0; i <= 6; i++)
{
    if (n-1 == i)
    {
        std::cout << name_day[i] << "\n" << "-----------------------" << "\n";;
        flag = false;
    }

}//end of for

Instead of a loop to look up the day, why not simply use an index?  Here's one way to do it:

while (true) {
    std::cout << "Input a number : ";
    std::cin >> n;
    if (std::cin && n >= 1 && n <= 7) {
        std::cout << name_day[n-1] << "\n----------------\n";
    } else {
        std::cout << "Bad entry: The number must be in the range 1-7 inclusive\n";
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
    }
}

Note that this will flush the input stream on any error which differs from the behavior of the current code.

Consider the user of the code

There is currently no graceful way to end the program.  It would be nicer if, for instance, a value of 0 or maybe -1 were used as a signal to end the program.

Think carefully about using using

There is nothing inherently wrong with these lines:

using std::vector;
using std::string;

However, since each is only used once, I'd probably omit them and simply use the full namespace in the code:

static const std::vector<std::string> name_day{
    "Saturday" ,"Sunday" , "Monday" , "Tuesday" , "Wednesday" , "Thursday" , "Friday" };

Consider using constexpr

Because it is a vector of strings, one can't make name_day a constexpr value, but it could be simply by making a minor change:

static constexpr const char* name_day[]{
    "Saturday" ,"Sunday" , "Monday" , "Tuesday" , "Wednesday" , "Thursday" , "Friday" };

This also means that the #include <vector> and #include <string> are no longer needed.


Two methods for loading jobs I am wondering which method code is more "Pythonic":

def load_job(self, identifier, job_name):
    i = 5
    ex = None
    while i:
        try:
            for ep in iter_entry_points(group='core.account', name=identifier):
                self.modules[identifier] = ep
            # load_entry_point(ep.dist, 'core.account', ep.name)
            # The following is the same
            loaded = ep.load()
            job = getattr(loaded, job_name)
            return job
        except ImportError as e:
            ex = e
            i -= 1
            time.sleep(2)
            continue

    raise ex

Or

def load_job(self, identifier, job_name):

    i = 5
    job = None
    ex = None
    while i:
        try:
            for ep in iter_entry_points(group='core.account', name=identifier):
                self.modules[identifier] = ep
                # load_entry_point(ep.dist, 'core.account', ep.name)
                # The following is the same
                loaded = ep.load()
                self.info("Loaded %s", ep.module_name)
                job = getattr(loaded, job_name)

            break
        except ImportError as er:
            ex = er
            i -= 1
            time.sleep(2)

    if job:
        return job
    else:
        raise ex

While the later is a bit longer, I feel it's easier to read. My main concern is whether I should return or break from within the while loop.

There are three things wrong with the while loop.

You are looping over a fixed number of times. The only other way to leave the loop is a return statement. This is exactly what a for loop is for. A while loop is used when the number of times the loop needs to execute is not known ahead of time. You should use the right tool for the job.
Don't use the truth value of a value when you can very easily explicitly state what you want the break condition to be. There are two advantages to using i > 0 is better than what you have. First, it is much easier for the reader. What you have forces the reader to think about what i is, how it might be equivalent to False, and how the loop has to manipulate i to reach that condition. Secondly, you can accidentally cause an infinite loop if you are not properly manipulating i. If you accidentally double decrement i when it is equal to 1, then while clause will evaluate to True when it checks -1. This will not happen when you are explicit about when you want the looping to continue.
In the first example, the last line of the loop is continue. This serves no purpose. There is no code that would be executed otherwise if the continue was omitted. Since it is the last line, the next thing that would happen would be to evaluate the while clause. That is the same thing a continue will trigger.

Edit:

With respect to the actual question, I think returning is fine. The seconds example is an attempt to follow the single return pattern. However, the first implementation still only has one return statement. The only other way to exit the method is by having an exception raised. The try block is the main body of the method. Everything else is the retry functionality.


Rock Paper Scissors with score count This is my Rock Paper Scissors code for Python 3 with score count. I want to make it more readable and concise if possible.

import random

def player_choice(user_choice):  # Register and shows the players choice
    if user_choice == 1:
        print("The player chose rock")
    elif user_choice == 2:
        print("The player chose paper")
    else:
        print("The player chose scissors")

def computer_choice(cpu_choice):  # Register and shows the cpus choice
    if cpu_choice == 1:
        print("The computer chose rock")
    elif cpu_choice == 2:
        print("The computer chose paper")
    else:
        print("The computer chose scissors")

def result(user_choice, cpu_choice, player_score, cpu_score):  # Define the game result
    if user_choice == cpu_choice:
        return [player_score + 0.5, cpu_score + 0.5]
    elif user_choice == 1 and cpu_choice == 3:
        return [player_score + 1, cpu_score]
    elif user_choice == 2 and cpu_choice == 1:
        return [player_score + 1, cpu_score]
    elif user_choice == 3 and cpu_choice == 2:
        return [player_score + 1, cpu_score]
    else:
        return [player_score, cpu_score + 1]

def print_score(p_score, c_score):  # Identifies the result and print the total score
    print("Score:""\nPlayer:", p_score, "\nComputer:", c_score)

def validation_input():  # Validates the input
    while True:
        try:
            user_input = int(input("Put your choice:"))
            if user_input not in range(1, 4):
                print("We only accept commands between 1 and 3, according to the table, type again")
                continue
            if type(user_input) == int:
                break
        except ValueError:
            print("We only accept exact numbers")
            continue
    return user_input

print('''1 - Rock
2 - Paper
3 - Scissors''')  # Printing the instructions
human_score = 0
computer_score = 0
while True:  # The condition is not important since the loop will stop on line 68 if the user wishes so
    user = validation_input()
    player_choice(user)
    ai = random.randint(1, 3)
    computer_choice(ai)
    human_score, computer_score = result(user, ai, human_score, computer_score)  # Accumulate the score
    print_score(human_score, computer_score)
    command = int(input("Type 0 to stop the program, type any another number to keep playing"))
    if command == 0:
        break

The result function can be much shorter:

def result(user_choice, cpu_choice):
    game_result = (3 + user_choice - cpu_choice) % 3
    user_score = (0.5, 1.0, 0.0)[game_result]
    return (user_score, 1.0 - user_score)

I changed the following things:

Since the result of a single game does not depend on the previous results, it is easier to only compute a single result.
The result of a game can be computed by taking the difference of the choices.
The 3 + prevents the difference from getting negative. This is done because different programming languages choose to produce different results for the expression -2 % 3. Adding 3 prevents that.
game_result is now either 0, 1 or 2
This can be used in a small lookup table to produce the correct user_score.
Since user_score + cpu_score == 1.0 for a single game, there's no need to do the complicated calculation twice.
Returning a tuple (1.0, 0.0) instead of a list [1.0, 0.0] is more appropriate, since lists can change their size. This return value has always size 2, since there are only 2 players.

The code is a little trickier to understand, but it works. And it is more concise.

def choice_to_string(choice):
    return ('rock', 'paper', 'scissors')[choice - 1]

With that function, you can write:

print("{0} chose {1}".format('The player', choice_to_string(2))

The rest of the code is fine, except for the variable names. You are using way too many synonyms.

computer, cpu, ai all mean the same. Choose one of them and only use that.
human, user, player all mean the same. Choose one of them and only use that.

Writing technical documents and code is very different from writing a novel. In a novel, using different terms for the same thing is generally accepted, in code it is not.

Instead of asking the user alternatingly to type "1 to 3" and "0 or anything else" is too much of a burden. You can combine these into "type 1 to 3 to play, or 0 to quit". This allows a game to be played with 2 key strokes instead of 4.


Bash script which downloads the latest linux stable kernel I made this script when I started downloading the latest stable linux kernel's source from https://www.kernel.org to compile it myself, now I have improved it a bit and decided to create a public repo for it on github (https://github.com/Tom1380/latestkernel).

This is the code (mind you this is the first time I made bash scripts):

lk (stands for latest kernel):

#!/usr/bin/env bash

cd ~

if [ ! -d "linux-stable" ]; then
    mkdir linux-stable
fi

if [ ! -f "/etc/latest_kernel" ]; then
    if  [[ $EUID > 0 ]] ; then
        echo "/etc/latest_kernel does not exist and you are not running the script as root, " \
              "please fix that either by manually running 'sudo create_etc_lk', " \
              "or rerun the script as root to automatically fix it."
        exit
    else
        touch /etc/latest_kernel
        echo "$(uname -r)" > /etc/latest_kernel
    fi
fi

wget=$(wget --output-document - --quiet https://www.kernel.org/ | grep -A 1 "latest_link")

wget=${wget##*.tar.xz\">}

wget=${wget%}

latest_acknowledged=$(</etc/latest_kernel)

if [ "$wget" == "$latest_acknowledged" ]; then
    echo "You have the latest stable kernel downloaded: $wget"
#    Uncomment (take out the #) the first 4 lines below if you want to be prompted when you are using a different kernel than
#    the latest one while already having installed it
#    runningkernel=$(uname -r)
#    if [ $runningkernel != $latest_acknowledged ]; then
#        echo "But you are running $runningkernel"
#    fi
    exit
fi

echo "Updated kernel available: $wget, you have $latest_acknowledged."

if  [[ $EUID > 0 ]] ; then
    echo "If you wish to download the latest kernel, rerun the script as root."
    exit
fi

# Since there is only a '>', /etc/latest_kernel will be overwritten entirely.

echo "$wget" > /etc/latest_kernel

echo "Writing latest kernel available in /etc/latest_kernel."

echo "Preparing to parse link to latest kernel for wget."

wget=$(wget --output-document - --quiet https://www.kernel.org/ | grep -A 1 "latest_link")

wget=${wget##**}

echo "Done parsing."

cd ~/linux-stable

echo "Changed cwd to ~/linux-stable to download kernel source."

echo "Downloading, this may take up to 10 minutes."

wget $wget

echo "Finished downloading..."

echo "Uncompressing the kernel's source."

tar xvfJ linux-$(</etc/latest_kernel).tar.xz

echo "Done uncompressing the kernel's source."

rm linux-$(</etc/latest_kernel).tar.xz

echo "Done removing the old archive, end of the script."

create_etc_lk:

#!/usr/bin/env bash

if [[ $EUID > 0 ]] ; then
    echo "You need root privileges for this script."
    exit
fi

touch /etc/latest_kernel && echo "$(uname -r)" > /etc/latest_kernel

install:

#!/usr/bin/env bash

if [[ $EUID > 0 ]] ; then
    echo "You need root privileges for this script."
    exit
fi

cp lk /usr/bin/lk
cp create_etc_lk /usr/bin/create_etc_lk
cp purgelatestkernel /usr/bin/purgelatestkernel

purgelatestkernel:

#!/usr/bin/env bash

if [[ $EUID > 0 ]] ; then
    echo "You need root privileges for this script."
    exit
fi

cp lk /usr/bin/lk
cp create_etc_lk /usr/bin/create_etc_lk
cp purgelatestkernel /usr/bin/purgelatestkernel

PS: Feel free to clone the repo if you think lk might be helpful to you

Good stuff:

Yay for putting your code on github.  I see a lot of folks afraid to wait until everything is perfect to do that.  But having the backup and history of git is too handy to wait like that.  So I love seeing somebody doing that from day 1 of a project.
Yay for use the double square bracket form of if and checking for common mistakes.
Yay for using env.  As you can see from my github I'm not so great at this one, but it is a good idea.

Suggestions:

Move common code into a library.  Your EUID check is the most obvious culprit.  To create a shell library you should create a file of functions and then source it into each of your commands.  So to translate your check into a function is easy:

function validate_root {
    if [[ $EUID > 0 ]] ; then
            echo "You need root privileges for this script."
            exit
    fi
}

Then use the library later like so:

source lib.sh

validate_root # be root or exit

Move common data into the library.  The most common item is /etc/latest-kernel... what if you want to move it somewhere else?  If it is in a variable in one place it will be a lot easier to do.  There's no reason not to throw those into the common shell library file too.
It looks like you made a cut and paste error on purgelatestkernel but since I can check out the github version I can see what you meant there.  Generally you're not supposed to edit code during code review put I think you'd be ok to fix that part up since you're not altering the code you meant to review.
Avoid duplicate code by looping in the install:

for file in lk create_etc_lk purgelatestkernel
do
        echo copying $file to /usr/bin
        cp $file /usr/bin
done

This also makes it easy to add other things like a validation of permissions or check for return values without insane amounts of error-prone cut and paste.
Instead of having the option for a prompt be something that requires a source code alteration why not make it conditional based on a variable that possibly could be altered with a command line option?


Search for a filename with keyword I have a directory with lot of files and folders in it. The function which I wrote works, but I want to make it more efficient and faster. I was thinking of multiprocessing or threading.

The code which I wrote is as follows:

LIST.delete(0,END) # this is a list view
usePATH = '/nfzcae/nvh_mdp/Messdatenbank_Powertrain'
fileLevels = [] # code of interest is below

for path, dirs, f in os.walk(usePATH):
    for d in dirs:
        for f in glob.iglob(os.path.join(path, d,'*COMPARE.mat')):
            if 'COMPARE.mat' in f:                                          # if 'COMPARE.mat' in f and not 'MIN' in f and not 'MAX' in f / if 'COMPARE.mat' in f )   # if 'COMPARE.mat' in f and not 'MIN' in f and not 'MAX' in f
                fileLevels.append(f.split('/'))                             # Split path string at all '/'
                LIST.insert(END,f)                                          # Insert path with name string into the listbox
                LIST.update_idletasks()                                     # Update listbox after each element added
                nr = LIST.size()                                            # Get current number of files in listbox
                VAR.set(nr)                                                 # Set number of files as a variable for label
                LIST.see(END)                                               # See last element added
                FILE_LIST.append(f)
            else:
                pass                                                        # Do nothing
LIST.insert(END,'Search finished')

It is actually made for a GUI Button. I want to make this code faster. I know that threading does not work for nested for loops, so I am stuck in figuring out how to use the multithreading module for this problem. Any ideas? I am having an idea on these lines:

get all the Sub directories names in a list
parallel pool now using the list and write a function which checks the Sub Folders for the file Name containing the keyword

Will this work??

P.S: The Folder has lot of subdirectories (more than 1000)

os.walk gives you the filenames already. There is no need to use glob to get them from the OS again. The looping could be done like this:

for path, dirs, filenames in os.walk(usePATH):
    for f in filenames:
        if f.endswith('COMPARE.mat'):
            ...

Updating a GUI can be slow. While I'm not familiar with what you are using, I would try if eliminating update_idletasks from the inner loop speeds it up.


Simple DBLayer class review I have created a simple DBLayer class. But I am not sure whether this class have any bug/issue using in production.

public class DBLayer
{
    private static string connectionString = ConfigurationManager.AppSettings["ConnectionString"];

    public static DataTable GetDataTable(string strSQL, CommandType type)
    {
        var objCmd = new SqlCommand(strSQL);
        objCmd.CommandType = type;
        return GetDataTable(objCmd);
    }

    public static DataTable GetDataTable(SqlCommand objCmd)
    {
        SqlConnection objConn = null;
        try
        {
            objConn = new SqlConnection(connectionString);
            objCmd.Connection = objConn;
            var objAdapter = new SqlDataAdapter(objCmd);
            var ds = new DataSet();
            objAdapter.Fill(ds);
            return ds.Tables[0];
        }
        finally
        {
            objConn.Close();
        }
    }

    public static int ExecuteNonQuery(SqlCommand objCmd)
    {
        int r = -1;
        SqlConnection objConn = null;
        try
        {
            objConn = new SqlConnection(connectionString);
            objCmd.Connection = objConn;
            objConn.Open();
            r = objCmd.ExecuteNonQuery();
        }
        finally
        {
            objConn.Close();
        }

        return r;
    }
}

Please review this code?

Looks good. You may also want some other things like getScalar (A single object, or string, as in 1 cell), getDataRow, just the top row, getDataRowCollection, i frequently only use datatable to get the rows, and forget about the columns, so I can iterate over it like so....

DataRowCollection drc = getDataRowCollection("SELECT * FROM tblTest");
for(DataRow r in drc){
    Console.WriteLine(r["name"]);
}

So you could have NonQuery, you may want to test these before using them....

    public DataTable getDataTable(SqlCommand objCmd)
    {
        SqlConnection objConn = null;
        try
        {
            objConn = new SqlConnection(connectionString);
            objCmd.Connection = objConn;
            var objAdapter = new SqlDataAdapter(objCmd);
            var ds = new DataSet();
            objAdapter.Fill(ds);
            return ds.Tables[0];
        }
        finally
        {
            objConn.Close();
        }
    }

    public DataRowCollection getDataRowCollection(SqlCommand objCmd)
    {
        return getDataTable(objCmd).Rows;
    }

    public DataRow getDataRow(SqlCommand objCmd){
        return getDataRowCollection(objCmd)[0];
    }

    public string getScalar(SqlCommand objCmd)
    {
        SqlConnection objConn = null;
        try
        {
            objConn = new SqlConnection(connectionString);
            objCmd.Connection = objConn;
            return objCmd.ExecuteScalar().ToString();
        }
        finally
        {
            objConn.Close();
        }
    }

    public int NonQuery(SqlCommand objCmd)
    {
        SqlConnection objConn = null;
        try
        {
            objConn = new SqlConnection(connectionString);
            objCmd.Connection = objConn;
            return objCmd.ExecuteNonQuery();
        }
        finally
        {
            objConn.Close();
        }
    }

I would recommend writing quite a few variations of the methods, and implement enums for each connection string so you don't mis-type anywhere and have them set in concrete.

Take a look at my getScalar variations....

    public static string getScalar(string query)
    {
        return getScalar(query, defaultConnection);
    }

    public static string getScalar(string query, ConnectionString cs)
    {
        return getScalar(query, new { }, cs);
    }

    public static string getScalar(string query, object param, ConnectionString cs)
    {
        return getScalar(query, param, cs, defaultTimeout);
    }
    public static string Query(string query, object param, int to)
    {
        return getScalar(query, param, defaultConnection, to);
    }

    public static string getScalar(string query, object param, ConnectionString cs, int to)
    {
        return getScalar(ConnectionStringToDb(cs), query, param, to);
    }

    public static string getScalar(MySqlConnection db, string query)
    {
        return getScalar(db, query, defaultTimeout);
    }

    public static string getScalar(MySqlConnection db, string query, int to)
    {
        return getScalar(db, query, null, to);
    }

    public static string getScalar(MySqlConnection db, string query, object param, int to)
    {
        string result = "";
        if (db.State != ConnectionState.Open)
            OpenConnection(db);
        MySqlParameter[] p = ObjectToParameters(param);
        using (db)
        {
            try
            {
                MySqlCommand command = new MySqlCommand();
                command.Connection = db;
                command.CommandText = query;
                command.CommandTimeout = to;
                foreach (MySqlParameter pm in p)
                {
                    command.Parameters.Add(pm);
                }
                result = command.ExecuteScalar().ToString();
            }
            catch { }
        }
        return result;
    }

Also if you class works as expected it should be fine... Looks okay. I use mine day to day, but I use MySql so I'm not to sure on the differences between the two classes, should be relatively the same.


TSP Brute Force Optimization in Python I am currently working on a Python 3.4.3 project which includes solving the Traveling Salesman Problem using different algorithms. I have just written a brute force algorithm and I would love some feedback. Now I understand that the number of operations increases by the factorial of the route length so brute forcing is not considered feasible, but for my project I need a standard brute force.

My overarching question is: How can I make this as fast and efficient as possible?

import itertools

def distance(p1, p2):
        #calculates distance from two points
        d = (((p2[0] - p1[0])**2) + ((p2[1] - p1[1])**2))**.5
        return int(d)

def calCosts(routes, nodes):
    travelCosts = []

    for route in routes:
        travelCost = 0

        #Sums up the travel cost
        for i in range(1,len(route)):
            #takes an element of route, uses it to find the corresponding coords and calculates the distance
            travelCost += distance(nodes[str(route[i-1])], nodes[str(route[i])])

        travelCosts.append(travelCost)

    #pulls out the smallest travel cost
    smallestCost = min(travelCosts)
    shortest = (routes[travelCosts.index(smallestCost)], smallestCost)

    #returns tuple of the route and its cost
    return shortest

def genRoutes(routeLength):
    #lang hold all the 'alphabet' of nodes
    lang = [ x for x in range(2,routeLength+1) ]

    #uses built-in itertools to generate permutations
    routes = list(map(list, itertools.permutations(lang)))
    #inserts the home city, must be the first city in every route
    for x in routes:
        x.insert(0,1)
    return routes

def main(nodes=None, instanceSize=5):
    #nodes and instanceSize are passed into main() using another program
    #I just gave them default values for this example

    #The Node lookup table.
    Nodes = {
        '1': (565.0, 575.0),
        '2': (25.0, 185.0),
        '3': (345.0, 750.0),
        '4': (945.0, 685.0),
        '5': (845.0, 655.0),
        '6': (880.0, 660.0),
        '7': (25.0, 230.0),
        '8': (525.0, 1000.0),
        '9': (580.0, 1175.0),
        '10': (650.0, 1130.0),
        '11': (1605.0, 620.0),
        '12': (1220.0, 580.0),
        '13': (1465.0, 200.0),
        '14': (1530.0, 5.0),
        '15': (845.0, 680.0)
    }

    routes = genRoutes(instanceSize)
    shortest = calCosts(routes, Nodes)

    print("Shortest Route: ", shortest[0])
    print("Travel Cost: ", shortest[1])

if __name__ == '__main__':
    main()

Lets have a little style review, before some code refactoring, and finish off with some performance comparison.

Style and code review

I suggest you read up on PEP8, which is the official style guide for Python.

Not using snake_case for variable and function names - You are using mostly camelCase, and in some cases one letter variables
Good use of singular vs plural – I like combinations like for route in routes. They make sense
Good use of if __name__ ... - This is good construct
Many temporary variables – You have a little too many temporary variables, which are only used once. These can often be skipped
Avoid using indexes in loops – In general, it is better to avoid using indexes in loops, rather than looping on the elements themself. Especially bad is it when you use str(i) to get the index, when the Nodes list should have used pure integers directly as keys.
Avoid building large lists in memory – Your code builds the routes list in memory. When calling this for a route length of 9, your original code uses approx 6.7MB to store the routes. My optimised routine below, uses 0.02MB because it uses generators instead...
Avoid recalculating distances – Python can memoize the output from a function for each given input by using decorators. This means that you can calculate the distance between two points once, and the next time you need it the memoize function picks your previous calculation of the result.
Use docstrings instead of ordinary comment to document functions – If you use docstrings, some of the editors will provide interactive help when you use your functions, and it is according the style guide.

Performance comparison

I made some variants of the code, both of your original, the one by SuperBiasedMan (with some correction as his code currently has a bug), some versions using only generators, itertools, sum and min, and finally a version using for loops instead of list comprehension and a slightly alternate optimisation to find the minimum.

I used the following version of memoize, which most likely can be further optimised:

def memoize(f):
    """ Memoization decorator for functions taking one or more arguments. """
    class memodict(dict):
        def __init__(self, f):
            self.f = f
        def __call__(self, *args):
            return self[args]
        def __missing__(self, key):
            ret = self[key] = self.f(*key)
            return ret
    return memodict(f)

@memoize
def distance(p1, p2):
    """Calculates distance between two points, memoizes result"""
    d = (((p2[0] - p1[0])**2) + ((p2[1] - p1[1])**2)) **.5
    return int(d)

One version taking it to the extreme regarding list comprehension and generators are this one:

def route_generator(route_length):
    """Generate all possible routes of length route_length, starting at 1."""
    for route in permutations(xrange(2, route_length+1)):
        yield (1, ) + route

def main_holroy_v2(instance_size=INSTANCE_SIZE):
    print min((sum(distance(NODES[start], NODES[stop])
                   for start, stop in pairwise(route)), route)
                       for route in route_generator(instance_size))

I made the Nodes into a global dict with integers as key, named NODES. After some testing I found that the extreme variant was slower than expected, so I fumbled around and found this to be the fastest in my environment:

def find_shortest_route3(nodes, route_length):
    """Find shortest route of length route_length from nodes."""

    minimum_distance = None
    for route in permutations(xrange(2, route_length+1)):
        current_distance = 0

        prev = nodes[1]
        for next in route:
            current_distance += distance(prev, nodes[next])
            prev = nodes[next]
            if minimum_distance and current_distance > minimum_distance:
                break
        else:
            if not minimum_distance or current_distance < minimum_distance:
                minimum_distance = current_distance
                minimum_route = route

    return minimum_distance, minimum_route

def main_minimum3(instance_size=INSTANCE_SIZE):
    distance.clear()

    cost, route = find_shortest_route3(NODES, instance_size)
    print('Shortest route: {}'.format((1, ) + route))
    print('Travel cost   : {}'.format(cost))

Some comments on this code:

It turns out that list comprehensions are slightly slower than plain for loops in this case
Instead of completing the sum for a route, I break out of the inner loop if the sum has crossed an earlier minimum. The current route is then not interesting any more
The actual route returned  does not include the starting point. This can easily be added, as done in the print statement. This eliminates the need for either adding in the starting point to all routes, or to remove all routes not starting at the correct point. In short, it is more efficient

If the inner for loop completes ordinarily (no breaks), then it goes into the else block of the for loop. A slightly strange construct, but for this purpose ideal, as we then can compare the minimum distance versus the current route distance
The distance.clear() is used to reset the memoisation between test run done using timeit in IPython. Memory profiling was done using a module name memory_profiler.

Here are some of the timing results I got:

In [325]: %timeit main_org_str(9)    # Your original code
    1 loops, best of 3: 529 ms per loop
In [326]: %timeit main_org(9)        # Using int as keys
    1 loops, best of 3: 304 ms per loop
In [327]: %timeit main_holroy_v2(9)
    1 loops, best of 3: 420 ms per loop
In [328]: %timeit main_superbiasedman(9)
    1 loops, best of 3: 362 ms per loop
In [330]: %timeit main_minimum3(9)
   10 loops, best of 3: 146 ms per loop
In [330]: %timeit main_minimum3(9)    # After removing `**0.5`
   10 loops, best of 3: 116 ms per loop

As can be seen here your original version took around 0.5 seconds, but only changing into using ints as keys and memoisation, shaved off 0.2 seconds. My extreme version and version by SuperBiasedMan, actually uses longer time than your version when optimised. But still the fastest version is the minimum3 version, clocking in at 146 milliseconds. (The last number, 116 ms, comes if you remove the **0.5 in distance(). Taking the square root is somewhat expensive.)

So there you have it, your code is not too bad timewize, but it used way too much memory. When coding according to the most pythonic ways, using generators, list comprehensions and sum and min you lower the memory consumption, changes the readability, but don't always gain that much in reduced time.

Of the alternatives I tested, the optimal combination for a brute force approach runs in approximately 30% of the time, and uses almost none memory. Some other timings for my optimal method: (10: 1.26 seconds, 11: 11.7 seconds, 12: 1 min 54 seconds). So better algorithms should be applied if wanting to calculate the shortest route for routes longer than 11/12 nodes.


Index Match implementation The code below takes header variables, retrieves the column index, and then, using an Index/Match function, returns data from a matching account number.

The reason it is written this way is because I wanted to make the code "reusable". By being able to change the hard-coded header names, I can update the Macro based on our client.

The code itself is unreasonably slow. It takes 35 secs to pull 4 accounts.

I'm looking to make the code more efficient.
Is there a different approach to making "reusable" code, which would be easier to read and look neater.

   Sub RetrieveData()

Dim Headers(1 To 21, 1 To 2)

 Headers(1, 1) = "StockNbr"
    Headers(2, 1) = "Customer Last Name"
    Headers(3, 1) = "Customer First Name"
    Headers(4, 1) = "Date Sold"
    Headers(5, 1) = "Amount Financed"
    Headers(6, 1) = "Finance Charges"
    Headers(7, 1) = ""
    Headers(8, 1) = "APR Rate"
    Headers(9, 1) = ""
    Headers(10, 1) = "Payment Amount"
    Headers(11, 1) = "Payment Schedule"
    Headers(12, 1) = "Contract Term (Month)"
    Headers(13, 1) = "Year"
    Headers(14, 1) = "Make"
    Headers(15, 1) = "Model"
    Headers(16, 1) = "VIN"
    Headers(17, 1) = "Odometer"
    Headers(18, 1) = "Principal Balance"
    Headers(19, 1) = "Cash Down"
    Headers(20, 1) = ""
    Headers(21, 1) = ""

    Dim FundingSheet As Worksheet
    Dim AccountNumber As Variant
    Dim AccountRange As Range

    Dim i As Integer
    Dim x As Integer
    Set AccountRange = Selection
    Debug.Print AccountRange.Address

    'B/c there is no naming convention, many different static data names
    Set FundingSheet = Sheets("StaticFunding")
    i = 1

    'looking for the column index and attaching to second dimension
    For i = LBound(Headers) To UBound(Headers)
        If Headers(i, 1) = "" Then
            Headers(i, 2) = ""
        Else
            Headers(i, 2) = Application.Match(Headers(i, 1), FundingSheet.Rows(3), 0)
        End If

    Next i

    'retrieving information using Index Match

    For Each Cell In AccountRange
    AccountNumber = Cell.Value
    x = 2
    i = 1

    For i = LBound(Headers) To UBound(Headers)

        If Headers(x, 2) = "" Then
            x = x + 1
        Else
            Cell.Offset(0, x).Value = Application.index(FundingSheet.Columns(Headers(x, 2)), Application.Match(CStr(AccountNumber), FundingSheet.Columns(Headers(1, 2)), 0))
            x = x + 1
        End If

        If x = 22 Then Exit For
    Next i

    Next Cell

    End Sub

If you want to make the code "easier to read and look neater", the first thing that I would do is to run it through an indenter. This is currently haphazard at best, and makes it difficult to follow.

The second thing I would do for ease of readabilty and maintenance is to extract the top portion of the Sub where you build the Headers array and calculate the column indexes into a function. This Sub is doing 2 things, and everything above 'retrieving information using Index Match can be treated as an atomic procedure.

I'd replace the string literals "" with the constant vbNullString - it's more readable and doesn't require a memory allocation. While you're at it, I'd get rid of all of the other magic numbers that you're using.  For example, FundingSheet.Rows(3) would be much more readable as FundingSheet.Rows(HEADER_ROW).

I'm not sure I understand why you declare AccountNumber As Variant, and then repeatedly cast it to a String with CStr(AccountNumber). If you're using it as a String, declare it as a String. Declare it as a String and then just cast it once when you assign it:

AccountNumber = CStr(Cell.Value)

Avoid 1 based indexing like Dim Headers(1 To 21, 1 To 2) unless there is a good reason to do so. The default array base is zero, you aren't setting the Option Base, and there is nothing in the procedure where having a base of 1 is relevant. All it does is add cognative load when you've scrolled down to the bottom of the Sub and trying to process the indexing.

Regarding the comment 'B/c there is no naming convention, many different static data names, you should consider passing the sheet name to the Sub as a parameter instead of hard coding it (presumably in a copied and pasted version of the procedure).  It isn't clear from your description if the same applies to the hard coded column headers, but those can also be passed as a parameter if they are not fixed (and would be another reason to extract the top portion as a function).

Performance

This code is slow because you are writing VBA code as if it were intended to be a function on a worksheet. If you need it to function independently (for example, in a UDF), that might be appropriate, but it is never going to be performant. Take a look at the top of the Sub. You're basically trying to build a lookup table, but then promptly discarding (or ignoring) the captured information later in the code and relying on Application.Index and Application.Match to get your column definitions in the correct order. This section (line break added for clarity)...

For i = LBound(Headers) To UBound(Headers)

    If Headers(x, 2) = "" Then
        x = x + 1
    Else
        Cell.Offset(0, x).Value = Application.Index(FundingSheet.Columns(Headers(x, 2)), _
            Application.Match(CStr(AccountNumber), FundingSheet.Columns(Headers(1, 2)), 0))
        x = x + 1
    End If

    If x = 22 Then Exit For
Next i

...is repeating the row lookup for every single column. That is incredibly inefficient. It might make sense if this was a UDF (and would be inefficient as a user function too), because each cell in a column would need to find the appropriate row. It's silly in this context though, because the Application.Match is going to return exactly the same thing for each of the 22 times that you call it.

For each account, you need to do two things; First, find the row that contains the account data, Second, copy the data based on the column lookup. I'd consider using a Scripting.Dictionary for the column lookup - you're doing a fairly simply column mapping between source column and destination column, so once you have the mapping built you can do lookups on that instead of repeatedly using Application.Index. Find the row target row once before you loop through the Headers array, and then just map the columns to the correct destinations. That would be a huge first step in improving the performance.

That said...

You're using Excel as if it was database. The real solution would be to use an actual database as a back-end and use Excel to simply present the data. If you can't do that for some logistical reason (IT resistance, for example), then stop treating Excel as a spreadsheet when you retrieve data from it. Open an ADO connection, and just query for your lookup results. ADO doesn't care about the column order, and you can just request columns by name and write them where they need to go. This is going to be way faster than using the Excel lookups (the driver is optimized for this), and you can request multiple results in one operation.

If Excel is your "database", then take advantage of the database tools that are available.


Warshall's algorithm for transitive closure I was going through this code for implementing Warshall's algorithm.
I think the time complexity for this simple problem is huge because there are too many loops running here. The time complexity for this code should be \$O(n^3)\$.

Is there a way to optimize this code so that the time complexity can be reduced a bit?

#include<stdio.h>
#include<unistd.h>
#include<math.h>
int maximum(int,int);
void warshal(int p[10][10],int n)
{
int i,j,k;

for(i=1;i<=n;i++)
 for(j=1;j<=n;j++)
   for(k=1;k<=n;k++)
     p[i][j]=maximum(p[i][j],p[i][k]&&p[k][j]);
}
int maximum(int a,int b)
{                                                       ;
if(a>b)
return(a);
else
return(b);
}
void main()
{
int p[10][10]={0},n,e,u,v,i,j;

 printf("\n Enter the number of vertices:");
 scanf("%d",&n);

 printf("\n input values now\n");
 for(i=1;i<=n;i++)
  for(j=1;j<=n;j++)
   scanf("%d",&p[i][j]);

   printf("\n Matrix of input data: \n");
 for(i=1;i<=n;i++)
 {
   for(j=1;j<=n;j++)
 printf("%d\t",p[i][j]);
 printf("\n");
 }
 warshal(p,n);
 printf("\n Transitive closure: \n");
 for(i=1;i<=n;i++)
 {
 for(j=1;j<=n;j++)
 printf("%d\t",p[i][j]);
 printf("\n");
 }
  }

Arrays are 0-indexed

In C, arrays are 0-indexed. Not 1-indexed. So you're skipping the first element and running off the back in these loops. You want:

for(i=0;i<n;i++)
  for(j=0;j<n;j++)
    for(k=0;k<n;k++)

Use braces

Nested logic is crying for braces to make it easer to read:

for(i=0;i<n;i++) {
  for(j=0;j<n;j++) {
    for(k=0;k<n;k++) {
    }
  }
}

That'll also future proof anything else you add into these loops. What if you added logging? You'd have to go back and add braces then anyway. It's a good habit to get into. Always braces.

maximum()

There's actually no reason for this function. The logic you want is:

R(k)[i, j] = R(k-1)[i, j] or (R(k-1)[i,k] and R(k-1)[k,j])

We can just do that directly using bitwise math:

for(i=0;i<n;i++) {
  for(j=0;j<n;j++) {
    for(k=0;k<n;k++) {
      p[i][j] = p[i][j] | (p[i][k] & p[k][j]);
    }
  }
}


Listing all possible names from a phone number In the program we have to input a number and then calculate all the possible name we can form according to the following rule:

2: A,B,C     5: J,K,L    8: T,U,V
3: D,E,F     6: M,N,O    9: W,X,Y
4: G,H,I     7: P,R,S

Just like we did on a keypad phone. Q and Z have been excluded (for simplicity, I believe)

For example 4734 produces

GPDG GPDH GPDI GPEG GPEH GPEI GPFG GPFH GPFI GRDG GRDH GRDI
GREG GREH GREI GRFG GRFH GRFI GSDG GSDH GSDI GSEG GSEH GSEI
GSFG GSFH GSFI HPDG HPDH HPDI HPEG HPEH HPEI HPFG HPFH HPFI
HRDG HRDH HRDI HREG HREH HREI HRFG HRFH HRFI HSDG HSDH HSDI
HSEG HSEH HSEI HSFG HSFH HSFI IPDG IPDH IPDI IPEG IPEH IPEI
IPFG IPFH IPFI IRDG IRDH IRDI IREG IREH IREI IRFG IRFH IRFI
ISDG ISDH ISDI ISEG ISEH ISEI ISFG ISFH ISFI

The digits in the inputted number can vary from 1 through 12.

I tried to do this using recursion.

import java.util.*;
import java.io.*;
public class namenum{
        public static void main(String[] args) throws Exception{

        Scanner sc = new Scanner(new File("namenum.in"));

        PrintWriter pw  = new PrintWriter(new File("namenum.out"));
        String n = sc.next();
        ArrayList<String> ans = new ArrayList<>();
        List<Character> arr2 = new ArrayList<>();
        arr2.add('A');
        arr2.add('B');
        arr2.add('C');

        List<Character> arr3 = new ArrayList<>();
                arr3.add('D');
        arr3.add('E');
        arr3.add('F');

        List<Character> arr4 = new ArrayList<>();
        arr4.add('G');
        arr4.add('H');
        arr4.add('I');

        List<Character> arr5 = new ArrayList<>();
        arr5.add('J');
        arr5.add('K');
        arr5.add('L');

        List<Character> arr6 = new ArrayList<>();
        arr6.add('M');
        arr6.add('N');
        arr6.add('O');
        List<Character> arr7 = new ArrayList<>();
        arr7.add('P');
        arr7.add('Q');
        arr7.add('R');

        List<Character> arr8 = new ArrayList<>();
        arr8.add('T');
        arr8.add('U');
        arr8.add('V');

        List<Character> arr9 = new ArrayList<>();
        arr9.add('W');
        arr9.add('X');
        arr9.add('Y');

        List<List<Character>> Lists = new ArrayList<>();
        Lists.add(arr2);
        Lists.add(arr3);
        Lists.add(arr4);
        Lists.add(arr5);
        Lists.add(arr6);
        Lists.add(arr7);
        Lists.add(arr8);
        Lists.add(arr9);

        List<String> result = new ArrayList<String>();
        genPermutations(Lists, result, 0, "");
        boolean ansFound = false;
        for(int i = 0; i< result.size(); i++){
            String name = result.get(i);
            if(isInDict(name)){
                pw.println(name);
                ansFound = true;
            }

        }
        if (!ansFound){
            pw.println("NONE");
        }
        pw.close();

    }
    static boolean isInDict(String name) throws Exception{
        boolean ans = false;
        Scanner dict = new Scanner(new File("dict.txt"));
        while(dict.hasNext()){
            if(dict.next() == name){
                ans = true;
                break;
            }
        }
        return ans;

    }

    static void genPermutations(List<List<Character>> Lists, List<String> result, int depth, String current){
        if(depth == Lists.size()){
            result.add(current);
            return ;
        }
        for(int i = 0; i<Lists.get(depth).size();i++){
            genPermutations(Lists, result, depth+1,current+Lists.get(depth).get(i));
        }
    }
    }

But it uses more time (2.128 seconds to be precise) than the accepted 1 second and I get a time limit exceeded verdict.

The question can be found here USACO

What are the ways in which I can simplify the program?

Without the link, I can only make certain assumptions, so I am going to give a couple of global comments:

When checking if something is contained in a dictionary, you each time read the file again. Second, you can linearly through it in order to determine if a name is contained in it or not. This is very, very slow. Is this because you have some memory constraints? Because else, it would be much faster to read it at the start, and use a HashSet to check for existence.
Should you be able to read the whole file in memory, perhaps you could even create a mapping between the names and the key combinations that make up this name, by precalculating this. I think that would be a rather optimal solution?
genPermutations does not work. You pass a depth, which is basically a key index, and generate all permutations from that key until the final key. If you nevertheless keep and fix the genPermutations method, you have another point which can be improved. As I commented earlier, if you would look at the generated results, you could notice a lot of duplication. Take your 4734 example, which among other things generates GPDG, HPDGand IPDG. The three strings are equal except for the first letter. This means you are calculating a lot of the same things. This could be cached.

Some minor remarks:

Class names should be uppercase (or better, CamelCase), e.g. NameNum
Variables should start with a lowercase, so List<List<Character>> lists = new ArrayList<>();
Do not use reference equality with Strings, instead use the equals method. You might get unexpected results otherwise.


Learning factory design pattern I'm making the very common example of Factory design pattern which creates a factory of cars and return an instance of a car. I found a example here https://refactoring.guru/design-patterns/factory-method/php/example and decided to make my own factory. My question is why do I need a Creator for each car?
<?php

interface Car
{
    public function drive();
}

<?php
require_once('Car.php');

class Audi implements Car
{
    public function drive()
    {
        return "driving an Audi";
    }
}

<?php
require_once('Car.php');

class Mercedes implements Car
{

    public function drive()
    {
       return "driving an Mercedes";
    }
}

<?php

abstract class CarFactory
{
    abstract public function factoryMethod(): Car;

    public function someOperation(): string
    {
        // Call the factory method to create a Product object.
        $product = $this->factoryMethod();
        // Now, use the product.
        $result = "I want to drive something..... " .
            $product->drive();

        return $result;
    }
}

<?php
require_once('CarFactory.php');
require_once('Car.php');
require_once('Audi.php');

class AudiCreator extends CarFactory
{

    public function factoryMethod(): Car
    {
        return new Audi();
    }
}

<?php
require_once('CarFactory.php');
require_once('Car.php');
require_once('Mercedes.php');

class MercedesCreator extends CarFactory
{

    public function factoryMethod(): Car
    {
       return new Mercedes();
    }
}

<?php

require_once('car/AudiCreator.php');
require_once('car/MercedesCreator.php');

function clientCode(CarFactory $carFactory)
{
    // ...
    echo "Client: "
        . $carFactory->someOperation();
    // ...
}

echo "The client .
";
clientCode(new AudiCreator());
echo "\r\n";
echo  "
";
echo  "
";

clientCode(new MercedesCreator());

I think you're thinking of this backwards. The point isn't that you need to define special subclasses to use the factory method. Rather, when you need to define special subclasses containing their own complex functionality, then you use the factory method.
From the Gang of Four:

Use the Factory Method pattern when a class can't anticipate the class of objects it must create, [or it] wants its subclasses to specify the classes it creates [...].

I think part of the problem is that your own example is a bit confusing. Interfaces are more for defining things different objects can do. You'll often hear people describe an interface as a sort of contract, because any object that implements an interface is required to implement every function of that interface. A Car would be more logically defined as a class because an Audi and a BMW are a type of car. On the other hand, it might make sense to define a Drivable interface, implemented by an abstract Vehicle class.
interface Drivable
{
    public function drive();
}

You could then define abstract base classes for each of the generic base object classes that implement this interface.
abstract class Vehicle implements Drivable
{
    abstract public function drive();
}

Once you do that, then you can implement functionality for each object.
abstract class Car extends Vehicle
{
    abstract public function drive();
}

class Audi extends Car
{
    public function drive()
    {
        //...
    }
}

abstract class Aircraft extends Vehicle
{
    abstract public function drive();
}

class Airbus extends Aircraft
{
    public function drive()
    {
        // ...
    }
}

Keeping with the literal nature of your example, think about how different an aircraft factory and a car factory are. They're both factories that make vehicles, sure, (which is why their respective factories could -and probably would- be derived from the same base abstract VehicleFactory class) but the process of building an aircraft is so different from that of building a car that you have to define special factory subclasses for each of them.
Getting more specific, let's focus on why and when you would use a factory. Suppose you're writing a game where a player can purchase a car. How do you know which car to instantiate until the user picks it?
A simple script might do something like this.
$car = null;

switch ($choice) {
    case 'BMW': {
        $car = new BMW();
    } break;

    case 'Audi': {
        $car = new Audi();
    } break;

    ...
}

$car->drive();

This is all the factory pattern is. The difference is that an actual factory class gives you several benefits that this simple script doesn't.
First of all, consider a CarFactory class that replicates the above script's functionality.
class CarFactory
{
    public static function create($car) : Car
    {
        switch ($car)
        {
            case 'BMW': return new BMW();
            case 'Audi': return new Audi();
            // ...
        }
    }
}

$car = CarFactory::create($choice);
$car->drive();

Right off the bat, you don't need null as the sentinel value for "the user hasn't picked a car yet." The car doesn't exist yet, so the car variable doesn't exist yet.
Second, a script can't inherit or be derived from anything; it just is. In contrast, if you wanted your player to also be able to purchase an Aircraft, or any kind of transport derived from the Vehicle class, you can define a base VehicleFactory class from which you can implement your CarFactory and AircraftFactory classes.
interface Build
{
    public static function build($choice);
}

abstract class VehicleFactory implements Build
{
    abstract public static function build($choice) : Vehicle;
}

class AircraftFactory extends VehicleFactory
{
    public static function build($aircraft) : Aircraft
    {
        switch ($aircraft)
        {
            case 'Airbus': return new Airbus();
            case 'Boeing': return new Boeing();
            // ...
        }
    }
}

class CarFactory extends VehicleFactory
{
    public static function build($car) : Car
    {
        switch ($car)
        {
            case 'BMW': return new BMW();
            case 'Audi': return new Audi();
            // ...
        }
    }
}

$aircraft = AircraftFactory::build($choice);
$aircraft->drive();

$car = CarFactory::build($choice);
$car->drive();

Hopefully this gives you an idea of why the factory pattern is useful. The upshot is that your base factory class defines the interface by which users will request an object be created, and the factory subclasses take care of the actual definition for how it happens. In addition, defining each factory's return type in terms of a base object (i.e..., the CarFactory returns a Car) allows us to choose the actual subclass of that object that actually gets created at runtime (users can pick a BMW or an Audi, both of which are Cars, all of which are Vehicles, all of which are Drivable).


Decide the start of the circular gas station path Suppose there is a circle. There are n petrol pumps on that circle. You are given two sets of data.

The amount of petrol that petrol pump will give.
Distance from that petrol pump to the next petrol pump.

Calculate the first point from where a truck will be able to complete the circle (The truck will stop at each petrol pump and it has infinite capacity). Expected time complexity is O(n). Assume for 1 litre petrol, the truck can go 1 unit of distance.
For example, let there be 4 petrol pumps with amount of petrol and distance to next petrol pump value pairs as {4, 6}, {6, 5}, {7, 3} and {4, 5}. The first point from where truck can make a circular tour is 2nd petrol pump. Output should be “start = 1″ (index of 2nd petrol pump).

Looking for code-review, best-practices and optimizations.
public final class PetrolPumpCircularTour {

    private PetrolPumpCircularTour() {};

    public static int startingStations(List<PetrolPumpData> gasStations) {

        if (gasStations.size() == 0) {
            throw new IllegalArgumentException("There should be atleast a single gas stats");
        }

        int front = 0;
        int rear = 0; // is actually 0, after a circle. but 1 is just a game we play for circular queue.

        int statationsCovered = 0;
        int gasSavedSoFar = 0;

        while (statationsCovered < gasStations.size()) {
            int currentPetrol = gasStations.get(rear).getLitres() + gasSavedSoFar;
            int currentDistance = gasStations.get(rear).getDistance();
            gasSavedSoFar = currentPetrol - currentDistance;
            statationsCovered = statationsCovered + 1;

            if (gasSavedSoFar < 0) {
                rear = (rear + 1) % gasStations.size();
                front =  (front + 1) % gasStations.size();
                gasSavedSoFar = 0;
                statationsCovered = 0;
                if (rear == 0) {
                    return -1;
                }
            } else {
                rear = (rear + 1) % gasStations.size();
            }
        }

        return front;
    }
}

public class PetrolPumpCircularTourTest {

    @Test
    public void test1( ) {
        List<PetrolPumpData> list1 = new ArrayList<PetrolPumpData>();
        list1.add(new PetrolPumpData(4, 6));
        list1.add(new PetrolPumpData(6, 5));
        list1.add(new PetrolPumpData(7, 3));
        list1.add(new PetrolPumpData(4, 5));

        assertEquals(1, PetrolPumpCircularTour.startingStations(list1));
    }

    @Test
    public void test2() {
        List<PetrolPumpData> list2 = new ArrayList<PetrolPumpData>();
        list2.add(new PetrolPumpData(40, 6));
        list2.add(new PetrolPumpData(6, 5));
        list2.add(new PetrolPumpData(7, 3));
        list2.add(new PetrolPumpData(4, 5));

        assertEquals(0, PetrolPumpCircularTour.startingStations(list2));
    }

    @Test
    public void test3() {
        List<PetrolPumpData> list3 = new ArrayList<PetrolPumpData>();
        list3.add(new PetrolPumpData(4, 6));
        list3.add(new PetrolPumpData(3, 5));
        list3.add(new PetrolPumpData(70, 3));
        list3.add(new PetrolPumpData(4, 5));

        assertEquals(2, PetrolPumpCircularTour.startingStations(list3));
    }

    @Test
    public void test4( ) {
        List<PetrolPumpData> list4 = new ArrayList<PetrolPumpData>();
        list4.add(new PetrolPumpData(4, 6));
        list4.add(new PetrolPumpData(3, 5));
        list4.add(new PetrolPumpData(2, 3));
        list4.add(new PetrolPumpData(40, 5));

        assertEquals(3, PetrolPumpCircularTour.startingStations(list4));
    }
}

Can you prove the algorithm works? It really looks suspicious. Besides, running it against a

{4,6}, {6,5}, {1,3}, {7,3}, {4,5}

test case returns 2. (Hint: the correct algorithm is much simpler).


async.waterfall call Does anyone have any recommendations for how I could make this async.waterfall call neater? Ideally I'd be able to do something like:

async.waterfall([
  serializeUser(user_id),
  generateAccessToken(user_id),
  generateRefreshToken(user_id)
], function(error, results) {
  //my user, access token, and refresh token are all in results
});

But as far as I know, I can't just aggregate all of my results at the end without explicitly doing so throughout my functions array, nor can I just pass in variables like I did there, as each subsequent function call depends on the one prior to it. The code below is functioning just fine and does exactly what it needs to, I simply want to make it cleaner.

    var fb_id = req.body.profile.fbID;

    async.waterfall([
      function(cb) {
        return User.serializeUser(fb_id, cb);
      },
      function(user, cb) {
        return auth.generateAccessToken(user._id, function(error, access_token) {
          return cb(null, user, access_token);
        });
      },
      function(user, access_token, cb) {
        return auth.generateRefreshToken(user._id, function(error, refresh_token) {
          if (error) {
            return cb(error, null);
          }
          else if (refresh_token) {
            return cb(null, {user: user, at: access_token, rt: refresh_token});
          }
          //no error but no token either- no clue what happened, return a code of 500 and say its a server error
          else {
            return cb(null, "An unknown error has occured.", 500);
          }
        });
      }
    ], function(error, results){
      if (err) {
        console.log("Waterfall error: ", error, "\n");
        res.send("Error: ", error);
      }
      else if (results) {
        console.log("Waterfall results: ", obj, "\n");
        res.status(200).json({ user: results.user,  access_token: results.at.access_token, refresh_token: results.rt.tokenString})
      }
      else {
        res.status(500).send("An unknown error has occured.");
      }
    })
  }]

Keep your variable naming consistent. Some of your code uses camelCase variable names, others uses snake_case. In JavaScript, we use camelCase by default.
The first argument in a Node-style callback should be reserved for errors.

You do this fairly well, but at one branch inside generateRefreshToken you pass a string error and error code to the callback as success arguments rather than as an Error. Speaking of which, use Error objects rather than strings for errors.

You can create your own derivations of the Error type if you want to be able to determine more specifically what Error you've received. This is generally not necessary unless you want your error to be recoverable.

You should set up your express application such that any Error that bubbles up to the top (That is not an authentication error) is presented to the user as a generic HTTP 500 error with no specific information encoded in it. The easiest way to do this is to throw Errors inside middleware when things go wrong and then have another middleware catch this error and present it to the user. I'd recommend boom for this.

Consider using Promises for async flow.

You can of course continue to use callbacks in this manner with the async library but Promises are (in my opinion) a lot simpler to use and compose. When you are interacting with a library that primarily uses callbacks you can still use Promises at the edges of that API, but I'd recommend using Promises everywhere internally.

The main benefit is that the code is more readable, but also Promises make it much harder for you to forget to handle an error (it's very easy to forget that in callbacks), make code more composable and are also first class citizens in ES6 as well as receiving syntactic sugar in ES7.

Extract the logic inside your async.waterfall code to separate functions

You already alluded to this in your answer, but you can very easily extract this code out so it reads a lot easier.

Rule of silence

Don't console.log unless it's for output to user actions. If you really want to do this, use debug from npm, which enables you to flag all debug actions and have them disabled unless you pass an environment variable.

Errors should be written to console.error as this can be piped to a separate error log file.

Here's my revised version of your code in ES5:

var async = require('async')

function serializeUser(callback) {
  return User.serializeUser(fbId, cb)
}

function generateAccessToken(user, callback) {
  return auth.generateAccessToken(user._id, function (error, accessToken) {
    return callback(null, user, accessToken)
  })
}

function generateRefreshToken(user, accessToken, callback) {
  return auth.generateRefreshToken(user._id, function (error, refreshToken) {
    if (error) {
      return cb(error, null)
    } else if (refreshToken) {
      return cb(null, { user: user, at: accessToken, rt: refreshToken })
    } else {
      const err = new Error('no refresh token was generated')
      return cb(err)
    }
  })
}

function requestHandler(req, res, next) {
  var fb_id = req.body.profile.fbID
  async.waterfall([
    serializeUser,
    generateAccessToken,
    generateRefreshToken
  ], function (error, results) {
    if (error) {
      // Let another middleware handle this (like express-boom).
      throw error
    }

    res.status(200)
      .json({
        user: results.user,
        access_token: results.at.access_token,
        refresh_token: results.rt.tokenString
      })
  })
}

I haven't included Promises here due to your answer about being new to Node - also, they don't really work extremely well with express.


Shader class in OpenGL Im am currently wrapping all OpenGL ressources (which are simply a GLuint) into classes that own and manage the deletion of them, to make it possible to use OpenGL in an object oriented fassion. Since these classes manage a ressource that get's destroyed in their destructor i know the rule of five is something to think about. This is how my Shader classes look like:

Shader.h :

#pragma once

#include <glad/glad.h>
#include <string>

class Shader
{
public:

    Shader(GLuint type, const std::string& shaderPath);
    virtual ~Shader();

    Shader(const Shader& s) = delete;
    Shader& operator = (const Shader& s) = delete;

    Shader(Shader&&) = default;
    Shader& operator = (Shader&&) = default;

private:

    /*
        The Shader objects are passed to the constructor
        of ShaderProgram that compiles the glShaderProgram -> needs id
    */
    friend class ShaderProgram; //

    GLuint id = 0;
};

class FragmentShader : public Shader
{
public:

    FragmentShader(const std::string& shaderPath) : Shader(GL_FRAGMENT_SHADER, shaderPath) {};
};

class VertexShader : public Shader
{
public:

    VertexShader(const std::string& shaderPath) : Shader(GL_VERTEX_SHADER, shaderPath) {};
};

Shader.cpp :

Shader::Shader(GLuint type, const std::string& shaderPath)
{
    std::ifstream fstram;
    std::stringstream sstream;

    fstram.exceptions (std::ifstream::failbit | std::ifstream::badbit);

    fstram.open(shaderPath);
    sstream << fstram.rdbuf();
    fstram.close();

    id = glCreateShader(type);

    auto data = sstream.str();
    const char* dataPtr = data.c_str();

    glShaderSource(id, 1, &dataPtr, NULL);
    glCompileShader(id);

    int result = 0;
    glGetShaderiv(id, GL_COMPILE_STATUS, &result);

    if(result == 0)
    {
        char infolog[1024];
        glGetShaderInfoLog(id, 1024, NULL, infolog);

        throw std::runtime_error(infolog);
    }
}

Shader::~Shader()
{
    glDeleteShader(id);
}

Use them like this:

try
{
    VertexShader vs("Path to file");
    FragmentShader fs("Path to file");
}
catch(const std::exception& e)
{
    std::cerr << e.what() << std::endl;
}

Because i am planning to implement classes for the ofther OpenGL ressources like buffers, textures, etc  in a similar fassion i would like some feedback. Any insight on mistakes that i made or how to improve my code are very appreciated. And do i need to declare move assignment and move constructor in the derived shader classes too?

Is it really necessary to split vertex and fragment shaders into their own type? It causes the type to go from 4 bytes to 16 bytes on a typical 64 bit processor, just for it to store a vtable ptr for the destructor that's never going to do anything different.

They could privately inherit and not have a virtual destructor, but I would rather they just be a single class and let incorrectly assigning shaders to a program be a runtime error. There's already a potential failure point when creating the shader program, and the gl error should alert the client to what went wrong, so it's not introducing a new point of failure.

You can't use the default move functions because they won't zero-out the id, and glDeleteShader will be called twice on the same id.

Consider using a factory function to create shaders from file instead of doing file IO in the constructor. It will make it easier to test, and it's also not obvious that this is a blocking function that shouldn't be called on UI threads.

It also makes it more portable to possibly use in a context that doesn't want to use fstreams for doing file IO, for example if you had zipped shaders or hosted them on a cloud it would make more sense to pass in a byte array which is what  glShaderSource will expect.

If you're going to use a path constructor, consider std::filesystem::path.


Use a standard stream, and restore its settings afterwards I sometimes find myself changing stream settings (e.g. to output hexadecimal, or to change fill character), and needing to change those settings back afterwards, to avoid perturbing subsequent output.  This gets tedious, so I wrote a short class to act as a scope guard, restoring the settings when it is destructed.

Now we can "borrow" an input or output stream, and give it back in the condition we found it.

#include <ios>
#include <iomanip>

// Members are all public and mutable, so if we really don't want
// to restore any particular part of the state, we can override.
template<class CharT, class Traits = typename std::char_traits<CharT>>
struct save_stream_state
{
    std::basic_ios<CharT,Traits>& stream;
    std::ios_base::fmtflags flags;
    std::locale locale;
    std::streamsize precision;
    std::streamsize width;
    CharT fill;

    save_stream_state(std::basic_ios<CharT,Traits>& stream)
        : stream{stream},
          flags{stream.flags()},
          locale{stream.getloc()},
          precision{stream.precision()},
          width{stream.width()},
          fill{stream.fill()}
    {}

    // deleting copy construction also prevents move
    save_stream_state(const save_stream_state&) = delete;
    void operator=(const save_stream_state&) = delete;

    ~save_stream_state()
    {
        stream.flags(flags);
        stream.imbue(locale);
        stream.precision(precision);
        stream.width(width);
        stream.fill(fill);
    }
};

It's not strictly necessary to delete operator=, as the compiler won't generate one for a class with a non-static reference member, but I think it's better to be explicit there.

Here's a simple test program that shows how it's used.  If you are using an old standards version, the template type deduction won't work - you'll need to specify the guard type in full (save_stream_state<char> and save_stream_state<wchar_t>).

#include <iostream>
int main()
{
    auto test = []() {
        std::cout << std::setw(15) << "Foo" << ' '
        << true << ' ' << 123456 << '\n';
    };
    {
        test();
        const save_stream_state guard{std::cout};
        std::cout << std::setfill('_') << std::left << std::uppercase
                  << std::boolalpha << std::hex << std::showbase;
        test();
    } // stream restored here
    test();

    std::cout << std::endl;

    // Now with wide-character stream:
    auto wtest = []() {
        std::wclog << std::setw(15) << L"Foo" << L' '
        << true << L' ' << 123456 << L'\n';
    };
    {
        wtest();
        // AAA style initialization - and guard multiple streams
        auto const guard = { save_stream_state{std::wclog},
                             save_stream_state{std::wcin} };
        std::wclog << std::setfill(L'_') << std::left << std::uppercase
                   << std::boolalpha << std::hex << std::showbase;
        wtest();
    } // stream restored here
    wtest();
}

I agree with everything what @JiveDadson said in the comment. I believe there are some ways to improve current code.

 ## Syntax

To use the guard, people will need to introduce a scope. My desired syntax would be:

stream << session << settings << data;

I'll present solution with the syntax above later.

My solution seem to solve only a subset of problems which original code does. Thus my point about syntax is not valid. Example can be multiple statement prints, in which case my solution would do unnecessary work.

Naming

Would it be better to use stream_state_guard? The code uses name guard in the tests. It is funny that tests reflect what was actually intended to be written. I found that it is the case in my code too.

Alternative solution

To achieve the syntax above, I've used some of the new features of C++17. My thoughts are a mess more than usual, so watch out for potential nitpicks.

First of all, session needs to be object of some empty type, just like nullptr is to nullptr_t or nullopt is to nullopt_t.

struct session_t {};
static inline constexpr session_t session;

inline is used to deal with linker.

Now, the class itself.

#include <ios>
#include <iomanip>
#include <ostream>

class session_stream
{
    std::ostream& stream;
    std::ios_base::fmtflags flags;
    std::locale locale;
    std::streamsize precision;
    std::streamsize width;
    char fill;

    session_stream(std::ostream& stream)
        : stream{stream},
          flags{stream.flags()},
          locale{stream.getloc()},
          precision{stream.precision()},
          width{stream.width()},
          fill{stream.fill()}
    {}

public:
    friend session_stream operator<<(std::ostream& os, session_t);

    // deleting copy construction also prevents move
    session_stream(const session_stream&) = delete;
    void operator=(const session_stream&) = delete;

    template <typename T>
    std::ostream& operator<<(T&& value)
    {
        stream << std::forward<T>(value);
        return stream;
    }

    ~session_stream()
    {
        stream.flags(flags);
        stream.imbue(locale);
        stream.precision(precision);
        stream.width(width);
        stream.fill(fill);
    }
};

The code is just adjusted version from the question. Interesting things to note:

Constructor and copy semantics are private

Wrong usage is impossible through conventional means. People will need to be either too smart or cause undefined behavior.
Create an object only by using stream << session

This might look fishy. Especially this line:

return session_stream{os};

Since C++17 this is guaranteed copy elision, thus it works even if copy constructor is unavailable. I don't think that guarantees impossibility of prolonging lifetime of session_stream, but it will guard against unintentional wrong usages.
Uses its own operator<< only on the first call

I wasn't sure if overload resolution and ADL won't intervene, so I decided to return ostream& on operator<<.

Rewritten tests:

#include <iostream>

int main()
{
    auto test = []() {
        std::cout << std::setw(15) << "Foo" << ' '
                  << true << ' ' << 123456 << '\n';
    };
    {
        test();
        std::cout << session << std::setfill('_') << std::left << std::uppercase
                  << std::boolalpha << std::hex << std::showbase << std::setw(15)
                  << "Foo" << ' ' << true << ' ' << 123456 << '\n';
        test();
    } // stream restored here
    test();
    std::cout << std::endl;

}

Live on Wandbox.

And just for reference: sessions can be nested.


Setting properties inside of a class Instead of using multiple setter functions within a class like

public function setAction()
public function setMethod()

etc

I've been using a method from a base class that all other classes have access to.

public function set_properties($properties) {

    foreach ($properties as $key => $value) {
    $this->$key = $value;
    }
}

So I can just use it like so

$Class->set_properties(array('action' => 'page.php', 'method' => 'get'));

Is this a bad practice that I'm getting into or is this an okay way of setting properties?

Having individual setters has a couple of advantages:

A dedicated setter allows you to put dedicated validations for these properties. You could obviously put them in the bulk setter as well, but that will make the bulk setter large and unwieldy very quickly. You want methods to do one dedicated thing instead. That keeps them maintainable.

Dedicated setters clearly communicate which properties can be set. With a bulk setter, it needs documentation or a look at the source code to learn what I can legally set. I usually don't want to look at your class code. I just want to look at the public API and understand how I can use that class.

That's not to say bulk setters are bad per se. If you know folks are likely to set two or more values after another because they naturally go together, you could allow for a setFooAndBar($foo, $bar) method. But as you can see, that's still much more explicit than a generic set(array $data). Also one could argue that if you need to set arguments that naturally go together, you rather want them to be in an object of their own and pass that object then.

On a side note: Your bulk setter will create properties on the fly for those keys that are not properties in the class. While you could consider that a feature, it makes debugging harder because a developer has to know the object's current state at runtime to know what properties it has.

On another side note: a base class is usually a code smell. It tends to amass unrelated functionality and turn in a God object quickly. It also hampers reuse because all your classes will depend on it then.


Scraping IMDB and exporting to JSON or/and CSV This code is scraping the data from IMDB and displays it in JSON or CSV format.

I plan to extend it beyond the top rated movies. How can I improve the quality of my code? I am using Python 3.7.3.

from bs4 import BeautifulSoup
import requests
import csv
import json
import codecs

films = []

def getFilms250():
    sourceTop250 = requests.get(
        'https://www.imdb.com/chart/top?ref_=nv_mv_250').text
    soup = BeautifulSoup(sourceTop250, 'lxml')
    counter = 0
    for title_container, rating_container in zip(soup.find_all('td', class_='titleColumn'),soup.findAll('td', class_='ratingColumn imdbRating')):
        title = title_container.a.text
        counter += 1
        year = title_container.span.text
        rating = rating_container.strong.text
        film_data = [counter, title, year.strip('()'), rating]
        films.append(film_data)
    return films

def csvFormat(films, userDelimiter):
    with open('new_film_base.csv', 'w', newline='') as file:
        header = ['Position', 'Title', 'Year', 'imdbRanking']
        film_writer = csv.writer(file, delimiter=userDelimiter)
        film_writer.writerow(header)
        film_writer.writerows(films)
    file.close

def jsonFormat(films):
    with open('new_film_base.json', 'w', encoding='utf8') as file:
        arr = []
        for i in films:
            x = {
                "position": i[0],
                "title": i[1],
                "year": i[2],
                "imbdRanking": i[3]
            }
            arr.append(x)
        movies = {
            "filmsRequested": arr
        }
        json.dump(movies, file, indent=True, ensure_ascii=False)
    file.close

def handleUserFilm(prompt):
    #While user enters a proper input a function will get list of wanted films
    while True:
        try:
            userMessageFilm = input((prompt))
        except ValueError:
            print('Input unkown')
        if userMessageFilm == 'films250':
            getFilms250()
            break
        elif userMessageFilm != 'films250':
            print('No such data')
            continue
    return userMessageFilm

def handleUserFormat(prompt):
    while True:
        try:
            userMessageFormat = input((prompt))
        except ValueError:
            print('Input unkown')
            continue
        if userMessageFormat != 'csv' and userMessageFormat != 'json':
            print('Format unkown')
            continue
        elif userMessageFormat == 'json':
            jsonFormat(films)
            break
        elif userMessageFormat == 'csv':
            while True:
                delimiter = input(
                    'Enter the Delimiter for csv format(";" recommended): ')
                if len(delimiter) > 4:
                    print("delimiter can't be that long")
                    continue
                else:
                    csvFormat(films, delimiter)
                    break

        return userMessageFormat

filmsUser = handleUserFilm('To get Top250 type - films250: ')
formats = handleUserFormat('To get csv format type - csv\nTo get json format type - json ')

Python uses snake_case, not camelCase for variable and function names. csvFormat for example, should be csv_format.

In two places you do this weird thing:

with open(. . .) as file:
    . . .
file.close

with already closes file, so there's no need to manually close it.
file.close doesn't do anything, and a good IDE would warn you of that. You need to add () to call close: file.close().

Speaking of weird things, you have this in a couple places as well:

try:
    userMessageFormat = input((prompt))
except ValueError:
    print('Input unkown')

input, as far as I know, will never throw. If you had a call to int in there, or if you were using Python 2 (maybe?), this may make sense. I don't think this try does anything, and I don't think 'Input unkown' will ever be printed.

if userMessageFormat != 'csv' and userMessageFormat != 'json':
    . . .
elif userMessageFormat == 'json':
    . . .
elif userMessageFormat == 'csv':

This is written in kind of a "fragile" way. If you ever add more formats, you'll need to make two changes: 1. Add a new elif userMessageFormat == check, and 2. Add to the first condition to validate the format ahead of time. If you forget to update the latter, none of the other cases will be True, and return userMessageFormat will cause a NameError since userMessageFormat will have never been defined.

Just make use of an else case:

if userMessageFormat == 'json':
    . . .
elif userMessageFormat == 'csv':
    . . .
else:
    print('Format unkown')

This is what else is for: to handle when none of the other checks were true.

for i in films:
    x = {
        "position": i[0],
        "title": i[1],
        "year": i[2],
        "imbdRanking": i[3]
    }

This could make use of destructuring to avoid needing to index i:

for position, title, year, rank in films:
    x = {
        "position": position,
        "title": title,
        "year": year,
        "imbdRanking": rank
    }

If the sub-collections in films contain more than 4 elements each, you'll need to change the for line to:

for position, title, year, rank, *_ in films:

The *_ catches all the remaining elements. Without this, you'll get a ValueError.

if userMessageFilm == 'films250':
    . . .
elif userMessageFilm != 'films250':
    . . .

This is redundant. If the first check is false, the second must be true. This can just be:

if userMessageFilm == 'films250':
    . . .
else:
    . . .