Copying properties between two classes I have an ASP.NET MVC 5 app and I map properties between model and viewmodel and vice versa.

I could use automapper to do this but since I don't need mapping rules to convince me to use it as I mostly do simple 1 to 1 mapping the performance overhead that automapper will add is not worthwhile. If I have mapping differences I'll map it manually where I reference it in my controller.

Here is my code

public class Dog
{
    public int Id { get; set; }

    public string Name { get; set; }

    public DateTime? BirthDate { get; set; }

    public int? CreatedBy{ get; set; }

    public DateTime? DateCreated { get; set; }
}

public class DogViewModel
{
    public int Id { get; set; }

    public string Name { get; set; }

    public DateTime? BirthDate { get; set; }

    public static implicit operator DogViewModel(Dog dm)
    {
        var vm= new DogViewModel{
            Id = dm.Id,
            Name = dm.Name,
            BirthDate = dm.BirthDate
       };
        return vm;
    }

   public static implicit operator Dog(DogViewModel vm)
   {
       var dm = new Dog
       {
            Id = vm.Id,
            Name = vm.Name,
            BirthDate = vm.BirthDate
        };
        return dm;
    }
}

Question: Is there a way to use a common mapping method in my ViewModel class  or some other way to so that I don't have to duplicate code for copying properties?

Good for you for resisting the temptations of Automapper.

My answer to your question is to design around it.

I often write my viewmodels generically so I don't have to copy properties.  This does not work in every case but works in more cases than you might think.  Your viewmodel might become something like this:

public class BaseViewModel<T>
{
    public T Model {get; set;}

    public BaseViewModel( T model)
    {
        this.Model = model;
    }
}

public class DogViewModel : BaseViewModel<Dog>
{
    public DogViewModel(Dog dog) : base(dog)
    {

    }

    public void DoSomething()
    {
        // do something with Dog object...
        Model.Weight = 10;
    }

}

I wrote a little about it on my site:
http://www.samwheat.com/Post/The-function-of-ViewModels-in-MVC-web-development


Countdown timer for new year Please help me improve my code. Note that it runs just fine. Especially with the way I use SFML, I need lots of guidance since I've just started learning how to use this library. I'm still learning C++ as well.

main.cpp

#include <SFML/Graphics.hpp>
#include "counter.hpp"

std::string day(int num) { return (num!=1)? "s":""; } //the 's' in the word day(s) in the text displayed

int main()
{
    sf::RenderWindow window(sf::VideoMode(800,300), "New Year Counter");
    window.setVerticalSyncEnabled(true);
    window.setPosition(sf::Vector2i(sf::VideoMode::getDesktopMode().width/2 -400, sf::VideoMode::getDesktopMode().height/2 -150)); //put window in the middle of the screen

    sf::Font font;
    font.loadFromFile("Beyond Wonderland.ttf");
    sf::Text text;
    text.setFont(font);
    text.setCharacterSize(80);
    text.setFillColor(sf::Color::White);
    text.move(25, 10);

    sf::Text counter = text; //initialize with the same attributes as text so that I won't have to write everything again.

    while(window.isOpen())
    {
        sf::Event event;
        while(window.pollEvent(event))
        {
            if(event.type == sf::Event::Closed)
                window.close();
        }

        TimeToNewYear year;
        window.clear(sf::Color::Black);
        text.setString("Until New Year "+year.get_newYear()+"\n"+year.get_daysLeft()+" day"+day(std::stoi(year.get_daysLeft()))+" left\n\t\tand");
        window.draw(text);
        counter.setString(year.get_counter());
        counter.setCharacterSize(120);
        counter.setPosition(360, 140);
        window.draw(counter);
        window.display();
    }
}

counter.hpp

#ifndef COUNTER_HPP
#define COUNTER_HPP
#include <string>
#include <ctime>

class TimeToNewYear
{
private:
    time_t m_nowTime = time(0);
    time_t m_tmny; //for mktime (time new year)
    tm *m_locTime = localtime(&m_nowTime);
    tm m_newYear {};
    int m_timeLeft;
    int m_daysLeft;
    std::string m_counter;

    void addNum(int num) //to display single digits with a 0 ex: 03
    {
        if(num<=9)
            m_counter += "0";
        m_counter += std::to_string(num);
    }
public:
    TimeToNewYear()
    {
        m_newYear.tm_mday =1;
        m_newYear.tm_year = 1+ m_locTime->tm_year; //set m_newYear
        m_tmny = mktime(&m_newYear);
        m_timeLeft = difftime(m_tmny, m_nowTime);
        //calculate days left
        m_daysLeft = m_timeLeft/(24*3600);
        m_timeLeft %= 24*3600;
    }

    std::string get_daysLeft() const { return std::to_string(m_daysLeft); }
    std::string get_newYear()  const { return std::to_string(m_newYear.tm_year +1900); } //tm_year is the years from 1900
    std::string get_counter()
    {
        addNum(m_timeLeft/3600);
        m_counter += ":";
        m_timeLeft %= 3600;
        addNum(m_timeLeft/60);
        m_counter += ":";
        addNum(m_timeLeft%60);
        return m_counter;
    }
};

#endif

First of all congratulations on a good start to C++ and SFML. You should consult good C++ design and programming practices such as https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md and/or https://www.gitbook.com/book/lefticus/cpp-best-practices/details. But I will point out a few things quickly here:

Typically the hpp file would consist of declarations and the implementation would be done in cpp file. This is recommended for significant implementations such as TimeToNewYear() ctor and the get_counter() method.
Just as you are scoping std::string, doing the same for time, tm, etc. as std::time, std::tm is a good idea - improves readability of the code.
You can also pretty much take all of the contents of your ctor and initialize all the variables as TimeToNewYear::TimeToNewYear() : m_nowTime {std::time(0)}, m_locTime {std::localtime(&m_nowTime)}, ... {} etc. But watch out for the order of initialization which depends on the order of member specification!
You can always return a reference to m_counter by having const std::string& TimeToNewYear::get_counter() so that clients cannot modify the contents. Haven't looked but am almost positive sf::Text::setString method takes a const std::string&. Saves time on making temporaries.

I am not an expert on the SFML side, but having used other windowing systems, these are general guidelines:

I would derive from sf::RenderWindow or compose it in your application specific class such as class MyAppWindow () : m_sfWindow {sf::VideoMode(), ...}, ... {} where m_sfWindow is a private member of your class.
This would permit you to have the event loop inside MyAppWindow::display() or some such method. It is likely that you can sf::RenderWindow::draw() inside the event loop in an already displayed sf::RenderWindow, thus avoiding the sf::RenderWindow::display() call.
It is also possible to have a New Year Countdown button, which when clicked, displays the countdown text box. You can get rid of it with a Close button.

Welcome to C++ (and SFML) development! After doing C++ for 20 years, I am still learning, and am in it for the long haul. Good luck!


SQL Negation: Find X when X does not do Y I'm learning The essence of SQL by Rozenshtein indirectly from the SQL Cookbook which has a section on it with modern SQL operators introduced after the book was released.

The exercise:

Given:

create table student
( sno integer,
  sname varchar(10),
  age integer
);
--
/* table of courses */
create table courses
( cno varchar(5),
  title varchar(10),
  credits integer
);
/* table of students and the courses they take */
create table take
( sno integer,
  cno varchar(5)
);

The question is: Find students who do not take CS112

I came up with:

with s1 as (
    select sno from student except select s.sno from student s inner join take t on s.sno = t.sno where t.cno = 'CS112'
)
select s.* from student s inner join s1 on s.sno = s1.sno

Rozenshtein:

select *
from student
where sno not in (select sno
                  from take
                  where cno = 'CS112')

SQL Cookbook style 1:  Group by

select s.sno, s.sname, s.age
from student s left join take t on (s.sno = t.sno)
group by s.sno, s.sname, s.age
having max(case when t.cno = 'CS112' then 1 else 0 end) = 0;

SQL Cookbook style 2: Window function

select distinct sno, sname, age
from (
    select s.sno,
           s.sname,
           s.age,
           max(case when t.cno = 'CS112' then 1 else 0 end) over (partition by s.sno, s.sname, s.age) as takes_CS112
    from student s left join take t on (s.sno = t.sno)
) as x
where takes_CS112 = 0;

I just wanted your opinion on how my query would perform compared to the other queries and if they are in the same ballpark, especially the SQL Cookbook queries. I'm targeting PostgreSQL.

I would personally use the JOIN, because the query here is fairly simple. You are just matching two tables.
A subselect is handy for more complex lookups though.

But each DBMS has its own way of parsing and optimizing queries.
You should run the execution plan on each query and compare results.

Like with Mysql, use the EXPLAIN command to determine which one would perform better. To get representative results you should fill up the tables with sample data. Add a least a few thousand records.
Also add indexes where appropriate. It could be interesting to run the tests with and without indexes.

The presence (or lack) of indexes will affect the query optimizer. Sometimes you have to provide hints, if the query optimizer does not choose the 'best' index.

It should be noted that performance is related to good DB design. These table examples have no primary key, no index etc.


Composing a prototype chain in JavaScript As an exercise, I wrote a simple function that composes a prototype chain from a list of objects by appending to the prototype chain of the first object, the rest of the objects. This is the code (example usage and tests in this gist):

var assign = Object.assign || require('object.assign');

// Non-enumerable properties are omitted. Property attributes are not respected
// and getters and setters aren't copied.
function appendToProto(obj, proto)
{
  if (obj === null)
    throw new Error(
      'Prototype chains are assumed to be delimited by Object.prototype.'
    );
  else if (obj === Object.prototype)
    return proto;
  else
    return assign(
      Object.create(
        appendToProto(Object.getPrototypeOf(obj), proto)
      ),
      obj
    );
}

// Takes a list of objects. The first object will be the farthest away from root
// (Object.prototype).
function composeProtoChain(prototypes)
{
  return prototypes.reduce(appendToProto);
}

As noted above, I can identify three issues with this code:

Non-enumerable properties are omitted from the prototype chain.
Property attributes are not respected and getters and setters aren't copied.
The assumption that Object.prototype is the last object before null in the prototype chain of objects.

I don't think there is a solution for the first issue, but workarounds could be implemented for the second and third ones. I preferred to keep things simple here, so I avoided doing that.
(Exceeding the maximum recursion depth in appendToProto shouldn't be a problem.)

I have two questions:

Is throwing an error in appendToProto the right way to enforce the assumption from the third issue?
Are there any other caveats that I haven't considered?

I'll admit to being new to this concept, and can't think of a real-world application for this method. Regardless, here's my thoughts.

Issues #1 and #2 are directly results of your use of Object.Assign which seems an acceptable limitation. Issue #3 is stating a fact of how Object prototypes are set up. The prototype of Object.prototype is by definition null, so #3 is technically a given.

However, your questions make your meaning of #3 more clear.

  Is throwing an error in appendToProto the right way to enforce the assumption from the third issue?

I wouldn't think so, as it limits the usefulness of your function. I would simply skip null values:

if (obj === null || obj === Object.prototype)
    return proto;

This has the benefit of treating null input essentially as if it were not provided, which would seem a more expected result. You could still have an error thrown, but I don't see the value.

If you choose to have an error, at least make the issue more obvious. "Prototype chains are assumed to be delimited by Object.prototype." is confusing and unclear what the issue is to me. I would go for something like: "Cannot appendToProto of null."

In general the error message should indicate what is wrong with the input instead of the codes philosophy regarding what its input should be.


Interactive logistic map The purpose of this project is to generate an interactive logistic map. The user can click on the produced picture to magnify the picture at that point.
Here is the logistic map equation:

Here is my program:
public class LogisticMap {
    public static double returnLogisticEquation(double x, double r) {
        return r * x * (1 - x);
    }

    public static double ignoreFirstIterations(int numberOfIterations, double x, double r) {
        for (int i = 0; i < numberOfIterations; i++) {
            x = returnLogisticEquation(x, r);
        }
        return x;
    }

    public static void drawLogisticMap(int numberOfHorizontalPoints, int numberOfVerticalPoints, double leftOfRInterval, double rightOfRInterval, double x) {
        StdDraw.setYscale(0, 1);
        StdDraw.enableDoubleBuffering();
        double subintervalLength = (rightOfRInterval - leftOfRInterval) / numberOfHorizontalPoints;
        int numberOfIterations = 1000;
        for (double r = leftOfRInterval; r < rightOfRInterval; r += subintervalLength) {
            x = ignoreFirstIterations(numberOfIterations, x, r);
            for (int p = 0; p < numberOfVerticalPoints; p++) {
                x = returnLogisticEquation(x, r);
                StdDraw.point(r, x);
            }
        }
        StdDraw.show();
    }

    public static void interactiveLogisticMap(int numberOfHorizontalPoints, int numberOfVerticalPoints, double leftOfRInterval, double rightOfRInterval, double x) {
        StdDraw.setXscale(leftOfRInterval, rightOfRInterval);
        drawLogisticMap(numberOfHorizontalPoints, numberOfVerticalPoints, leftOfRInterval, rightOfRInterval, x);

        double r = 0;
        double y = 0;
        double scale = 0.05;

        while (true) {
            if (StdDraw.isMousePressed()) {
                r = StdDraw.mouseX();
                y = StdDraw.mouseY();

                double newLeftOfRInterval = r - scale;
                double newRightOfRInterval = r + scale;
                double belowRInterval = y - scale;
                double aboveRInterval = y + scale;

                StdDraw.setXscale(newLeftOfRInterval, newRightOfRInterval);
                StdDraw.setYscale(belowRInterval, aboveRInterval);
                StdDraw.clear();
                drawLogisticMap(numberOfHorizontalPoints, numberOfVerticalPoints, newLeftOfRInterval, newRightOfRInterval, x);

                scale /= 10;
            }
        }
    }

    public static void main(String[] args) {
        StdDraw.setCanvasSize(850, 850);

        int numberOfHorizontalPoints = 800;
        int numberOfVerticalPoints = 1000;
        double leftOfRInterval = 2.4;
        double rightOfRInterval = 4;
        double x = Math.random();

        interactiveLogisticMap(numberOfHorizontalPoints, numberOfVerticalPoints, leftOfRInterval, rightOfRInterval, x);
    }
}

StdDraw is a simple API written by the authors of the book Computer Science An Interdisciplinary Approach. I checked my program and it works. Here is one instance of it.
Output (actually a succession of outputs):

Is there any way that I can improve my program?
Thanks for your attention.

public static double returnLogisticEquation(double x, double r) {

The name is not quite correct, as it does return the result of equation, not the equation itself, so more like calculateLogisticEquation.

    public static double returnLogisticEquation(double x, double r) {
        return r * x * (1 - x);
    }

I'm torn on this one. Normally, I say that you are only allowed to use single-letter variables when dealing with dimensions (yes, that disallows "i", "j", "k" too). However, when implementing mathematical functions there might be a reason to use them. However, using the long names might still be an improvement:
    public static double returnLogisticEquation(double populationRatio, double reproductionate) {
        return reproductionRate * populationRatio * (1 - populationRatio);
    }

for (int i = 0; i < numberOfIterations; i++) {

That's what I meant:
for (int counter = 0; counter < numberOfIterations; counter++) {

public static double ignoreFirstIterations(int numberOfIterations, double x, double r) {

I'm not sure about the name here, maybe forward(int numberOfIterations, ...) would be a better name, as normally, I'd not assume a function that ignores something to return something.

    public static double ignoreFirstIterations(int numberOfIterations, double x, double r) {
        for (int i = 0; i < numberOfIterations; i++) {
            x = returnLogisticEquation(x, r);
        }
        return x;
    }

Assigning to a parameter is bad style. Even though you can treat it as just another declaration, it might lead to confusion down the line. For example if you have an object as parameter and the following line:
thisIsAParameter.setValue("value");

If you have the habit of assigning parameters, you now need to check whether there was a different object assigned to the parameter before that function call. Ideally, parameters would be final, but that is quite noisy, though. Threat them as final.

int numberOfIterations = 1000;

It's not the number of iterations, it's the number of ignored iterations.

r += subintervalLength

For the record: += and other shorthand operators are not short for a = a + b, but for a = (TYPE_A)(a+b), which might yield unexpected results. For example, intA = intA + 5.5d will yield an error that a double is truncated to an int. intA += 5.5d will yield no such error.

    public static void drawLogisticMap(int numberOfHorizontalPoints, int numberOfVerticalPoints, double leftOfRInterval, double rightOfRInterval, double x) {
        StdDraw.setYscale(0, 1);
        StdDraw.enableDoubleBuffering();
        double subintervalLength = (rightOfRInterval - leftOfRInterval) / numberOfHorizontalPoints;
        int numberOfIterations = 1000;
        for (double r = leftOfRInterval; r < rightOfRInterval; r += subintervalLength) {
            x = ignoreFirstIterations(numberOfIterations, x, r);
            for (int p = 0; p < numberOfVerticalPoints; p++) {
                x = returnLogisticEquation(x, r);
                StdDraw.point(r, x);
            }
        }
        StdDraw.show();
    }

A few more empty lines to group logic would be nice (like you've done in other functions).
    public static void drawLogisticMap(int numberOfHorizontalPoints, int numberOfVerticalPoints, double leftOfRInterval, double rightOfRInterval, double x) {
        StdDraw.setYscale(0, 1);
        StdDraw.enableDoubleBuffering();

        double subintervalLength = (rightOfRInterval - leftOfRInterval) / numberOfHorizontalPoints;
        int numberOfIterations = 1000;

        for (double r = leftOfRInterval; r < rightOfRInterval; r += subintervalLength) {
            x = ignoreFirstIterations(numberOfIterations, x, r);

            for (int p = 0; p < numberOfVerticalPoints; p++) {
                x = returnLogisticEquation(x, r);

                StdDraw.point(r, x);
            }
        }

        StdDraw.show();
    }

while (true) {

If you can include a dynamic break condition, do so (like listening for ESC).

        double r = 0;
        double y = 0;

Declare variables where they are used, to limit the scope in which they exist to limit possible errors.
                double r = StdDraw.mouseX();
                double y = StdDraw.mouseY();

double scale = 0.05;

Seems more like this should either be a parameter, or a class-level constant.

I haven't tested your code or verified the mathematical correctness, but looks good so far.


Python password generator This is considered as my second python project, I made the password generator where user has to input length of password minimum 5.

I made the code that it will guarantee at least one lowers case, one upper case and one special character is included in the password.
please review and comment.

import random

def letters():
    return list('abcdefghijklmnopqrstuvwxyz')

def upper_case():
    uppercase = []
    for x in letters():
        uppercase.append(x.upper())
    return uppercase

def special_char():
    return list('!@#$%^&*()\'\"')

def all_char():
    return letters() + upper_case() + special_char()

def create_password(password, n):
    for x in range(n):
        password += random.choice(all_char())
    return password

def check_password(password):
    nletter, nupper, nspecial = [0]*3
    for x in password:
        if x in letters():
            nletter += 1
        if x in upper_case():
            nupper += 1
        if x in special_char():
            nspecial += 1
    if nletter > 0 and nupper > 0 and nspecial > 0:
        return True
    else:
        return False

def main():
    n = 0
    while n < 5:
        n = int(input('Enter your password length (minimum 5): '))
    password = ''
    while not check_password(password):
        password = ''
        password = create_password(password, n)
        check_password(password)

    print('Generated password : {}'.format(password))

while True:
    main()

To build on what else has been posted, validating your inputs is necessary and you have a lot of functions and loops that can be removed.

String module

First, I would like to introduce you to the string module that is part of the python standard library. This module has string constants that you can import and use:

>>> import string
>>> string.ascii_lowercase
'abcdefghijklmnopqrstuvwxyz'
>>> string.ascii_uppercase
'ABCDEFGHIJKLMNOPQRSTUVWXYZ'
>>> string.ascii_letters
'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'
>>> string.punctuation
'!"#$%&\'()*+,-./:;<=>?@[\\]^_`{|}~'

Using these would allow you to get rid of your first three functions (letter, upper_case, and special_char). Similarly to what Joe said, it would be better to make your character space a variable to avoid creating extra objects, and remove the all_char function at the same time.

create_password function

This function is clear and works okay, but can also be improved. As you are doing the same operation in each iteration of the loop (random.choice) this is and ideal scenario to use a list comprehension.

# Before
def create_password(password, n):
    for x in range(n):
        password += random.choice(all_char())
    return password

# After
def create_password(n):
    return ''.join([random.choice(all_char()) for _ in range(n)])

This works by creating a list where a random character is selected from all_char() and then joining them all together. The underscore is a 'throwaway' variable here, see here.

Alternatively, as we are already using the random module we could use random.choices (but only in python3.6 and above) as this will return a list of k size:

def create_password(n):
    return ''.join(random.choices(all_char(), k=n))

check_password function

In general, simple statements are better than complex statements. Therefore, I would change the first line of this function to be:

nletter, nupper, nspecial = 0, 0, 0

As this is clearer.

The for loop works, and could be improved by using the string module constants, though they would need to be made into lists.

for x in password:
    if x in list(string.ascii_lowercase):
        nletter += 1
    if x in list(string.ascii_uppercase):
        nupper += 1
    if x in list(string.punctuation):
        nspecial += 1

Another way to test if two strings overlap is to use sets; but this is getting more complex.

Altogether

import random
import string

def create_password(n, chars):
    return ''.join(random.choices(chars, k=n))

def check_password(password):
    nletter, nupper, nspecial = 0, 0, 0
    for x in password:
        if x in list(string.ascii_lowercase):
            nletter += 1
        if x in list(string.ascii_uppercase):
            nupper += 1
        if x in list(string.punctuation):
            nspecial += 1
    if nletter > 0 and nupper > 0 and nspecial > 0:
        return True
    else:
        return False

def main():
    characters = string.ascii_letters + string.punctuation
    n = 0
    while n < 5:
        try:
            n = int(input('Enter your password length (minimum 5): '))
        except ValueError:
            print('Not a number')

    password = ''
    while not check_password(password):
        password = create_password(n, characters)

    print('Generated password : {}'.format(password))

run = True
while run:
    try:
        main()
    except KeyboardInterrupt:
        run = False
        print('Ctrl+c caught exiting')


Python class inheritance - using super() and __str__ I have been trying out some stuff using inheritance with classes in Python 3.4. In the code below, I use the super() call to return the __str__ from the parent class. Is it correct coding practice to save the information returned by the parent class method in a variable, concatenate the __str__ from the child class to that of the parent class, and return like I did twice below?

Also, what would be best coding practice to convert such output to a string? Would it be better to

Do this also in the parent class.
Also in the child class.
When printing - in this case - Director or Manager?

class Employee:
   def __init__(self, name, salary, position):
      self._name = name
      self._salary = salary
      self._position = position

   def __str__(self):
      return str(self._name) + ' ' + str(self._salary) + ' ' + str(self._position)

class Director(Employee):
   def __init__(self, name, salary, position, mngteam):
      super().__init__(name, salary, position)
      self.managementteam = mngteam

   def __str__(self):
      st = super().__str__() # IS THIS GOOD CODING PRACTICE?
      st += ' ' + str(self.managementteam)
      return st

class Manager(Employee):
   def __init__(self, name, salary, position, supervises):
      super().__init__(name, salary, position)
      self.supervises = supervises

   def __str__(self):
      st = super().__str__() # IS THIS GOOD CODING PRACTICE?
      st += ' ' + str(self.supervises)
      return st

d = Director('DirMax', 100000, 'Director', 5)
print(d)
m = Manager('ManSam', 50000, 'Manager', 10)
print(m)

In general, calling the parent class implementation, keeping the result and then processing it is perfectly acceptable OOP practice. However, there is a problem with your string handling; concatenation (+) is not the most efficient way of doing things (e.g. a + b + c becomes (a + b) + c, creating intermediate strings that aren't needed). Instead, when implementing __str__ and __repr__, you can use str.format, for example:

class Employee:

   ...

   def __str__(self):
      return '{0._name!s} {0._salary!s} {0._position!s}'.format(self)

class Director(Employee):

   ...

   def __str__(self):
      return '{0} {1.managementteam}'.format(super().__str__(), self)

You aren't entirely compliant with the style guide; for example, managementteam/mngteam should really be management_team. Additionally, you have an odd mix of private-by-convention (leading underscore, e.g. _salary) and public (no leading underscore, e.g. supervises) instance attributes - why? Should these be protected? Should the instance be immutable? Some docstrings would be nice, too.

I would be inclined to add some validation of inputs - for example, if salary should always be a positive integer, you could add checking for that. One way to do that is in __init__, but a more thorough approach is with a property:

class Employee:

    def __init__(self, ..., salary, ...):
        ...
        self.salary = salary  # note assignment to salary not _salary
        ...

    ...

    @property
    def salary(self):
        return self._salary

    @salary.setter
    def salary(self, new_salary):
        if new_salary < 0:
            raise ValueError('salary must be positive')
        self._salary = new_salary


Script for checking password strength I have a jQuery script that checks for the strength of a password. The code looks like this (with more more of those "blocks"):

if(passwordLC && lessThan12Password)
{
    $('.passwordStrenght').animate({width:'75px',height:'5px'},200).show();
    $('.passwordStrenght').css('background-color',red);
    $('#password').css('border-color',green);
    $('.passwordStrenghtInfo').text(weak).show();
    passwordErrors = true;
}else

if(passwordUC && lessThan12Password)
{
    $('.passwordStrenght').animate({width:'75px',height:'5px'},200).show();
    $('.passwordStrenght').css('background-color',red);
    $('#password').css('border-color',green);
    $('.passwordStrenghtInfo').text(weak).show();
    passwordErrors = true;
}else

if(passwordN && lessThan12Password)
{
    $('.passwordStrenght').animate({width:'75px',height:'5px'},200).show();
    $('.passwordStrenght').css('background-color',red);
    $('#password').css('border-color',green);
    $('.passwordStrenghtInfo').text(weak).show();
    passwordErrors = true;
}else

And it goes on and on...

Is there a way to reduce all that code to something simpler and efficient, or am I stuck with those horrible "blocks" of repeated code?

The solution depends on your exact requirements and the context.
In your case, you could simplify the if-condition like below. If all fit into one if, you can write your code block there only once. If you need it and can't unify your condition, make it a function.

Here, I'm using a combined approach. I put the code from your if into a function, jQuery objects are cached and don't have to be recreated (but are still protected from outside modification because they only exist inside the scope of that function). And I combined all the conditions I could see in your example.

var indicateBadPassword = (function() {
        var $passwordStrength = $('.passwordStrength'),
            $password = $('#password'),
            $passwordStrengthInfo = $('.passwordStrenghtInfo');
        return function() {
            $passwordStrength.
                animate({width:'75px',height:'5px'},200).
                show().
                css('background-color',red);
            $password.css('border-color',green);
            $passwordStrengthInfo.text(weak).show();
            passwordErrors = true;

        }
    })();
if (lessThan12Password && (passwordLC || passwordUC || passwordN)) {
    indicateBadPassword();
}


Simple class exercise - min, max, and average of vector items This program calculates the min and the max value and the average of the items of a vector. It's an exercise from a book so I can't change the header file.

Is this code proper?

sales.h

#include <vector>

class Sales
{
    private:
        std::vector<double> sales;
        double average;
        double maximum;
        double minimum;
        double calcAvg();

    public:
        Sales(const std::vector<double> ar);
        void setSales();
        void showSales() const;
};

sales.cpp

#include "sales.h"
#include <iostream>
#include <algorithm>

Sales::Sales(const std::vector<double> ar)
{
     sales = ar;
     minimum = *std::min_element(sales.begin(), sales.end());
     maximum = *std::max_element(sales.begin(), sales.end());
     average = calcAvg();
}

void Sales::setSales()
{
    std::vector<double> temp;
    std::cout << "Enter values separated by a space: ";
    for (int i = 0; i < 4; i++)
    {
        int item;
        std::cin >> item;
        temp.push_back(item);
    }
    *this = Sales(temp);
}

void Sales::showSales() const
{
    for(std::size_t i = 0; i < sales.size(); i++)
        std::cout << "Item " << i <<" = " << sales.at(i) << std::endl;
    std::cout << "Average: " << average << std::endl;
    std::cout << "Min, max: " << minimum << " , " << maximum << std::endl;
}

double Sales::calcAvg()
{
    double sum;
    for(std::size_t i = 0; i < sales.size(); i++)
       sum += sales.at(i);
    return sum/sales.size();
}

program.cpp

#include "sales.h"
#include <vector>

int main()
{
    std::vector<double> ar;
    ar.push_back(4);
    ar.push_back(5);
    ar.push_back(23);

    Sales s1(ar);
    s1.showSales();

    Sales s2;
    s2.setSales();
    s2.showSales();
    return 0;
}

First, of all, this class has a setSales member much like the setgolf member of the class you posted in your last question.  I think it suffers from all the same issues that I've mentioned; there's not really anything I can add, so I'll leave those comments to represent this, too.

Now to the main issue, which I'd say is the calcAvg member.  It should in any case be const; finding and caching the average of a vector doesn't change external state, and were you to ever print the vector (without first modifying it) you'd come to the same result.

As things are, calcAvg doesn't need access to most of the members of Sales.  It looks to me like you should just write a free average function template that takes an std::vector<T> const& and returns the average (of type T).

Also, about the caching: whether you should do this or not depends on how the class is used.  I wouldn't expect this class to be used as such, but you know better.  I would delay it until the thing is printed; in that case, you'd have to make your minimum, average and maximum all mutable, and probably also of type std::atomic<double>.  This isn't necessarily better: look at the usage and choose.

Finally, as a minor detail, there's no need to use at when you can easily prove that the index is in-bounds anyway, and you can use std::accumulate for calculating the average.


Implementing Traversable for Coproduct Given the following definitions of Functor and Coproduct from fp-course:

class Functor f where
  (<$>) ::
    (a -> b)
    -> f a
    -> f b

class Functor f => Applicative f where
  pure ::
    a -> f a
  (<*>) ::
    f (a -> b)
    -> f a
    -> f b

class Functor t => Traversable t where
  traverse ::
    Applicative f =>
    (a -> f b)
    -> t a
    -> f (t b)

data Coproduct f g a =
  InL (f a)
  | InR (g a)

Is there a cleaner or more succinct way to implement the following?

instance (Traversable f, Traversable g) =>
  Traversable (Coproduct f g) where
  traverse ::
    Applicative h =>
    (a -> h b)
    -> Coproduct f g a
    -> h (Coproduct f g b)
  traverse f (InL fa) = InL <$> traverse f fa
  traverse f (InR ga) = InR <$> traverse f ga

Here is how Edward Kmett implemented traverse in the Data.Functor.Coproduct package.

instance (Traversable f, Traversable g) => Traversable (Coproduct f g) where
  traverse f = coproduct
    (fmap (Coproduct . Left) . traverse f)
    (fmap (Coproduct . Right) . traverse f)

While he writes in a points free style and uses Either under the hood instead of your bespoke Coproduct instance, the result is still the same.  Apply the supplied function to either the left or right branch and wrap the result back in the constructor.

I do not see a better way to than the implementation given unless η-conversion to points-free is appealing for some reason.


Given a list of words, remove the shorter word of every pair This is not a "please do my homework" question. I solved a problem two ways and would like to know which solution is better; better being defined by better readability, lower complexity and lower memory consumption. I'd also like to note that this is a not a homework assignment but an optional no-credit practice problem.

My CS class offers practice problems through their site. One specific question is named "removeShorterStrings." The prompt being:

  Write a method removeShorterStrings that takes an ArrayList of Strings as a parameter and that removes from each successive pair of values the shorter string in the pair. For example, suppose that an ArrayList called list contains the following values: {"four", "score", "and", "seven", "years", "ago"} In the first pair, "four" and "score", the shorter string is "four". In the second pair, "and" and "seven", the shorter string is "and". In the third pair, "years" and "ago", the shorter string is "ago". Therefore, the call: removeShorterStrings(list); should remove these shorter strings, leaving the list as follows: "score", "seven", "years". If there is a tie (both strings have the same length), your method should remove the first string in the pair. If there is an odd number of strings in the list, the final value should be kept in the list.

I have come up with the following solutions:

The "ask for permission" solution:

void removeShorterStrings(ArrayList<String> list) {
    if (list.size() != 0) {
        int isOdd = (list.size() >> 0) & 1;
        for (int i = 0; i <= (list.size() >> 1) - isOdd; i++) {
            String first = list.get(i);
            String second = list.get(i + 1);
            if (first.length() <= second.length()) {
                list.remove(i);
            } else {
                list.remove(i + 1);
            }
        }
    }
}

and the "ask for forgiveness" solution:

void removeShorterStrings(ArrayList<String> list) {
    for (int i = 0; i <= (list.size() >> 1); i++) {
        try {
            String first = list.get(i);
            String second = list.get(i + 1);
            if (first.length() <= second.length()) {
                list.remove(i);
            } else {
                list.remove(i + 1);
            }
        } catch (IndexOutOfBoundsException e) {
            break;
        }
    }
}

I come from a JavaScript/python background, so I'm unsure of what is considered "good practice" in java.

TL;DR: Which solution is better in terms of better readability, lower complexity and lower memory consumption?

Asking for forgiveness would be the best policy for dealing with errors that may be beyond your control, such as when opening a file for reading.  Checking beforehand is useless, because too many things could go wrong: the file might not exist, you might not have the right permissions, the disk could fail altogether, etc.  Furthermore, even if you did check beforehand, circumstances could change during the brief period between your check and the real operation.  Therefore, the only reasonable approach would be to just do it, and deal with any errors that come up.

When working with a list of words, however, the outcome is entirely deterministic.  Since it is entirely possible to write your code such that it never triggers an error, that's what you should aim for.  (Well, maybe the program is multithreaded, and some other thread is manipulating the ArrayList behind your back, but that would be your program's fault, and such errors are also preventable with proper engineering.)

Therefore, your first approach is better.  However, the list.size() != 0 check is a superfluous check, and list.size() >> 0 is a pointless shift.

Personally, I find the indexing confusing, because the list is being modified as you traverse it, and the termination condition (list.size() >> 1) - isOdd is a moving target.  Consider working backwards, so that each pair you consider is always in "virgin" territory.

public static void removeShorterStrings(ArrayList<String> list) {
    for (int i = (list.size() & ~1) - 2; i >= 0; i -= 2) {
        String first = list.get(i);
        String second = list.get(i + 1);
        if (first.length() <= second.length()) {
            list.remove(i);
        } else {
            list.remove(i + 1);
        }
    }
}


Dealing with resource closure when rethrowing an exception I would like a review and recommendations for improvement of the code below.  I want to allow an exception to be handled by the caller, so I am rethrowing it.  But I have resources to close.  So I have the resource closing code duplicated in both the try and the catch blocks.  Is there a better way to do this?

const string HTTPopotamus::GET(void) {
  HINTERNET  hSession = NULL,
      hConnect = NULL,
      hRequest = NULL;
    try {
        hSession = this->open();
        hConnect = this->connect(hSession);
        hRequest = this->openRequest(hConnect);
        this->sendRequest(hRequest);
        this->receiveResponse(hRequest);
        string strData = this->readData(hRequest);
        /* Close any open resources. */
        if( hRequest ) WinHttpCloseHandle( hRequest );
        if( hConnect ) WinHttpCloseHandle( hConnect );
        if( hSession ) WinHttpCloseHandle( hSession );
        return strData;
    } catch (const ExceptionCyclOps& e) {
        /* Close any open resources. */
        if( hRequest ) WinHttpCloseHandle( hRequest );
        if( hConnect ) WinHttpCloseHandle( hConnect );
        if( hSession ) WinHttpCloseHandle( hSession );
        throw;
    }

}

It seems that your code would benefit dramatically from use of RAII. To illustrate, let's assume the existence of an HTTPConnection class that opens an appropriate connection in its constructor and closes it at destruction. The code would look more like the following:

const string HTTPopotamus::GET(void) {
    HTTPConnection conn(/*connection parameters here*/);

    conn.sendRequest(/*request parameters here*/);
    conn.waitForResponse(HTTP_REQUEST_TIMEOUT);

    return this->readData(hRequest);
}

This approach is more elegant and clear and explains itself with no need for comments. Any exception thrown, such as exceptions that represent a failed connection or a timeout event would be propagated to the caller while the HTTPConnection object would still be destroyed properly.


Improving my PHP autoloader I'd like to improve my router.

This is the current code for my autoloading action within my MVC application:

spl_autoload_register(function ($className) {
if (file_exists(ROOT . DS . 'library'  . DS . 'intranet'  . DS  . 'classes' . DS .strtolower($className) . '.php')){
    require_once(ROOT . DS . 'library'  . DS . 'intranet'  . DS  . 'classes' . DS .strtolower($className) . '.php');
} else if (file_exists(ROOT . DS . 'application' . DS . 'controller' . DS . strtolower($className) . '.php')) {
    require_once(ROOT . DS . 'application' . DS . 'controller' . DS . strtolower($className) . '.php');
} else if (file_exists(ROOT . DS . 'application' . DS . 'model' . DS . strtolower($className) . '.php')) {
    require_once(ROOT . DS . 'application' . DS . 'model' . DS . strtolower($className) . '.php');
} else if (file_exists(ROOT . DS . 'application' . DS . 'view' . DS . strtolower($className) . '.php')) {
    require_once(ROOT . DS . 'application' . DS . 'view' . DS . strtolower($className) . '.php');
} else {
throw new exception("$className" class failed to load: file not found");
});

It is looking for the class file in different folder, requiring it. If that fails, an exception is thrown.

It doesn't seem flexible, and I doubt that it'll play well with other libraries and autoloaders that are introduced as the project goes.

How could I improve my existing autoloader?

What you've written is more or less the standard implementation of spl_autoload().

This is an equivalent approach:

$paths = array(
    get_include_path(),
    ROOT . DS . 'library'  . DS . 'intranet'  . DS  . 'classes',
    ROOT . DS . 'application' . DS . 'controller',
    ROOT . DS . 'application' . DS . 'model',
    ROOT . DS . 'application' . DS . 'view',
);

// help system to find your classes
set_include_path(join(PATH_SEPARATOR, $paths));

// use standard auto loader
spl_autoload_register();

Using namespaces in your application would help a lot as well; for instance, in each of your controller classes you simply add:

namespace controller;

When you take a similar approach for models and views as well, you reduce above code to this:

$paths = array(
    get_include_path(),
    ROOT . DS . 'library'  . DS . 'intranet'  . DS  . 'classes',
    ROOT . DS . 'application',
);

set_include_path(join(PATH_SEPARATOR, $paths));

// use standard auto loader
spl_autoload_register();

// ...
$x = new controller\something();

Namespaces are converted to directories by spl_autoload() so you only need to set the application directory to make it work.


LinkedList Queue implementation I'm working on a project which simulates (very simple/watered down) a network. The basis of this is to learn how to implement a Queue. I understand there are many ways to go about this, but I ultimately went with a LinkedList mainly because of it's nature (FIFO) which makes it easy to utilize in a Queue. I would appreciate any feedback on my implementation, and would also greatly appreciate with any critiques on my Queue's toString(), I know it is an unconventional way of doing it but I thought it would by a nice chance to work on recursion.

The data class in the this project is the Packet. It represents a packet and code is as follows. I've omitted the getters and setters for brevity:

private static int packetCount;         // Determined at the time of the objects creation [incremental]
private int id;                         // Assigned by packetCount
private int packetSize;                 // Size of the packet being sent
private int timeArrive;                 // Creation "time stamp"
private int timeToDest;                 // Number of simulation units it took for the packet to arrive to destination

/**
 * Default constructor
 */
public Packet() {
    id = 0;
    packetSize = 0;
    timeArrive = 0;
    timeToDest = 0;

}

public Packet(int id, int packetSize, int timeArrive, int timeToDest) {
    this.id = id;
    this.packetSize = packetSize;
    this.timeArrive = timeArrive;
    this.timeToDest = timeToDest;
}
@Override
public String toString() {
    return "Packet #" + id + " => arrive at simulation unit: " + timeArrive + " => packet size: " + packetSize;
    }
}

Here is my Node class:

public class Node {
private Packet data;
private Node next;
private Node prev;

public Node() {
    next = null;
    data = null;
    prev = null;
}

public Node(Packet data) {
    this.data = data;
    next = null;
    prev = null;
}

public Packet getData() {
    return data;
}

public Node getNext() {
    return next;
}

public Node getPrev() {
    return prev;
}

public void setData(Packet data) {
    this.data = data;
}

public void setNext(Node next) {
    this.next = next;
}

public void setPrev(Node prev) {
    this.prev = prev;
}

}

And my Queue class which is actually a router within this simulation:

/**
 * Represents a router on the network [queue]
 */
public class Router{

private Node head;
private Node tail;
int size;

public Router() {
    head = null;
    tail = null;
    size = 0;
}

public boolean isEmpty() {
    return head == null;
}

public int size() {
    return size;
}

public void enqueue(Packet packet) {
    Node node = new Node(packet);

    if(head == null) {
        head = node;
        tail = node;
    }
    tail.setNext(node);
    node.setPrev(tail);
    tail = node;

    size++;
}

public Packet dequeue(){
    if(isEmpty()) {
        try {
            throw new Exception("Underflow");
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

    Node ptr = head;
    head = ptr.getNext();

    if(head == null) {
        tail = null;
    }

    size--;

    return ptr.getData();
}

public Packet peek() {
    if(isEmpty()) {
        try {
            throw new Exception("Underflow");
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

    return head.getData();
}

public String recToString(StringBuilder strBlder, Node cursor) {

    if(cursor !=head) {
        strBlder.append(cursor.getData().toString() + "\n");
        recToString(strBlder, cursor.getPrev());
    }

    strBlder.append(cursor.getData().toString());
    return strBlder.toString();
}

@Override
public String toString() {
    StringBuilder s = new StringBuilder();
    return recToString(s, tail);
}
}

Packet

private static int packetCount; // Determined at the time of the objects creation [incremental]

I would remove this field. Personally I feel it's odd for a Packet to need to know the number of packets. Apart from that you would need to synchronize access to this field, should your class ever be used in a multi-threading environment.

Node

make data final. A node should never change it's data once created. Remove the setter as well.
Remove the default constructor public Node(). It is unused and does not initialize the (now final) field data.
rename the field data to packet. A clear naming will help other developers and your future self remember what is stored here.

Router

size should be private too

public void enqueue(Packet packet) {
    Node node = new Node(packet);

    if (head == null) {
        head = node;
        tail = node;
    }
    tail.setNext(node);
    node.setPrev(tail);
    tail = node;

    size++;
}

if head == nullyou set head = tail = node. You now have three references to the same Packet object. After that you set next and prev on this object to itself.

public Packet dequeue() {
    Node ptr = head;
    head = ptr.getNext();

    if (head == null) {
        tail = null;
    }

    size--;

    return ptr.getData();
}

If now dequeue is called, head is assigned to ptr (which is the Packet pointing forward and backward to itself. head is assigned ptr.next which means, it is still the same object. head == null will never be true. For every subsequent call to dequeue you will get that Packet.

If another Packet ist enqueued the next call to dequeue will return the first Packet once more and then return the new Packet, removing it from your list.

To fix that, change enqueue to

public void enqueue(Packet packet) {
    Node node = new Node(packet);

    if (head == null) {
        head = node;
        tail = node;
    } else {
        tail.setNext(node);
        node.setPrev(tail);
        tail = node;
    }

    size++;
}

Are you sure, that the output created by recToString is what you want/need?

Calling

router.enqueue(new Packet(11, 11, 11, 11));
router.enqueue(new Packet(22, 22, 22, 22));
router.enqueue(new Packet(33, 33, 33, 33));
System.out.println(router.toString());

yields the output:

Packet #33 => arrive at simulation unit: 33 => packet size: 33
Packet #22 => arrive at simulation unit: 22 => packet size: 22
Packet #11 => arrive at simulation unit: 11 => packet size: 11Packet #22 => arrive at simulation unit: 22 => packet size: 22Packet #33 => arrive at simulation unit: 33 => packet size: 33

Since you implement FIFO I think, the first (head) element should be printed first.

Remove recToString and replace toString:

@Override
public String toString() {
    StringBuilder result = new StringBuilder();

    Node current = head;
    while (current != null) {
        result.append(current.getData().toString()).append("\n");

        current = current.getNext();
    }

    return result.toString();
}

It now starts at head and walks forward through the list till it hits the end.

With recToString removed the only call to Node#getPrev() got removed, too. A FIFO-structure does not need to be double linked. Let's remove it.

Node:

public class Node {
    private Packet data;
    private Node next;

    public Node(Packet data) {
        this.data = data;
        next = null;
    }

    public Packet getData() {
        return data;
    }

    public Node getNext() {
        return next;
    }

    public void setNext(Node next) {
        this.next = next;
    }
}

Router#enqueue()

public void enqueue(Packet packet) {
    Node node = new Node(packet);

    if (head == null) {
        head = node;
        tail = node;
    } else {
        tail.setNext(node);
        tail = node;
    }

    size++;
}

Edit:
Forgot my own comment.

Exception handling

Create your own Exception:

public class QueueEmptyException extends RuntimeException {
    public QueueEmptyException() {
    }

    public QueueEmptyException(final String message) {
        super(message);
    }

    public QueueEmptyException(final String message, final Throwable cause) {
        super(message, cause);
    }

    public QueueEmptyException(final Throwable cause) {
        super(cause);
    }

    public QueueEmptyException(final String message, final Throwable cause, final boolean enableSuppression, final boolean writableStackTrace) {
        super(message, cause, enableSuppression, writableStackTrace);
    }
}

Create a method that throws the Exception, if the queue is empty:

private void assertNotEmpty() {
    if (isEmpty()) {
        throw new QueueEmptyException("Queue is emtpy");
    }
}

Change peek

public Packet peek() {
    assertNotEmpty();

    return head.getData();
}

and dequeue

public Packet dequeue() {
    assertNotEmpty();

    [...]
}


List of Happy Numbers in scala Definition of Happy numbers taken from Wikipedia.

  A happy number is defined by the following process: Starting with any positive integer, replace the number by the sum of the squares of its digits in base-ten, and repeat the process until the number either equals 1 (where it will stay), or it loops endlessly in a cycle that does not include 1. Those numbers for which this process ends in 1 are happy numbers, while those that do not end in 1 are unhappy numbers (or sad numbers)

  Example: 19 is happy, as the associated sequence is

  1*1 + 9*9 = 82,
  8*8 + 2*2 = 68,
  6*6 + 8*8 = 100,
  1*1 + 0*0 + 0*0 = 1.

import scala.collection.mutable.Set

object HappyNumber extends App {

  def findSquareSum(n: Int): Int =
    n.toString.foldLeft(0) { (product, num) => product + num.asDigit * num.asDigit }

  val visited = Set[Int]()

  def isHappyNumber(n: Int): Boolean = {
    n match {
      case 1 => true
      case _  =>
        if (visited contains n) false
        else {
          visited += n
          if (isHappyNumber(findSquareSum(n))) { visited -= n; true} else false
        }
    }
  }

  (1 to 247) foreach { num => if(isHappyNumber(num)) println(num) }
}

Just a nitpick about naming: the "find" in findSquareSum is meaningless, and perhaps even misleading, since it's not searching for anything.  Just squareSum would be fine.  Even better, call it something like sumSquaresOfDigits to be explicit about what the function does.


Add existing models to a relation using nested attributes I needed to add existing models to a has-many relation using nested attributes so I overwrote thumbnails_attributes=:

class Gallery < ActiveRecord::Base
  has_many :thumbnails, dependent: :nullify

  accepts_nested_attributes_for :thumbnails

  def thumbnails_attributes=(thumbnails_attributes)
    ids = thumbnails_attributes.map { |t| t['id'] }
    (ids - thumbnail_ids).each do |id|
      thumbnail = Thumbnail.find id
      thumbnails << thumbnail
    end
    super(thumbnails_attributes)
  end
end

class Thumbnail < ActiveRecord::Base
  belongs_to :gallery
end

Given the following data (as fixture):

thumbnails.yml

one:
  gallery: one

two:
  gallery:

galleries.yml

one:
  name: MyString

I can now run:

galleries(:one).update(thumbnails_attributes: ['id' => thumbnails(:two).id])

and galleries(:one).thumbnails will return both thumbnails.

While it works pretty well, I wanted to ask for reviews and ways to improve/clean this code.

The only issue I have is with include? (which is used by assert_includes in my tests):

galleries(:one).update(thumbnails_attributes: ['id' => thumbnails(:two).id])
galleries(:one).thumbnails.include? thumbnails(:two) # false
galleries(:one).thumbnails                           # [#<Thumbnail one>, #<Thumbnail two>]
galleries(:one).thumbnails.include? thumbnails(:two) # true

It looks like I have to reload the relation for include? to work, but I haven't found a fix nor a way to properly test it.

galleries(:one).thumbnails.to_a.include? thumbnails(:two) works, include? doesn't appear to load the association.

Couple of things to improve here:

Firstly - do not use fixtures, use factories. There is a neat library allowing you to do this with ease called FactoryGirl (for rails you will need FactoryGirlRails). Having this in place you can define factories like:

FactoryGirl.define do
  factory :thumbnail do
    name 'Thumbnail name if you have such a field'
    other_field :other_value
  end
end

And then use it as:

thumbnail = FactoryGirl.create :thumbnail

One advantage of factories is that it does not load all the data into the database for each test, but rather allows you tell it what is needed instead.

There are quite a lot of options you can add to your factories, like dynamic field values (useful when you have uniqueness validations), building parameters, building associated objects etc. All are well documented on gem github wiki.

Secondly, accepts_nested_attributes_for already provides you with a method to assign objects using their ids:

@model.children_ids << [1,2,3]

or even modify the whole association in one go

@model.children_ids = [1,2,3]

This is usually more useful in terms of web forms. If you have a form where client can select multiple objects from a list from db, all you need to do is to name those fields children_ids and rails will take care of persisting the association.


Count number of cycles in permutation Write a function that a receives file name and an integer N as arguments, that represent a binary file which contains N integers, which constitute a permutation. Return the number of cycles in the permutation.

Code

int countCyclesHelper(int* array, int N) {
  int i, first, current, temp, counter = 0;
  for (i = 0; i < N; i++) {
    first = i;
    current = first;
    while (1) {
      if (array[current] == -1) {
        break;
      }
      temp = array[current] - 1;
      array[current] = -1;
      current = temp;
      if (current == first) {
        counter++;
        break;
      }
    }
  }
  return counter;
}

int countCycles(char *name, int N) {
  char *buffer = (char*)malloc(N);
  int *array = (int*)malloc(N);
  int i, numberOfCycles;
  FILE *fp = fopen(name, "rb");
  fread(buffer, sizeof(buffer), 1, fp);
  for (i = 0; i < N; i++) {
    array[i] = buffer[i] - '0';
  }
  numberOfCycles = countCyclesHelper(array, N);
  free(buffer);
  free(array);
  fclose(fp);
  return numberOfCycles;
}

Example

Input (test.bin):

54321

Output:

3

Any review is appreciated.

Unless you use a very old compiler, I suggest you to declare (and initialize) variables as late as possible and limit their scope. This prevents potential mistakes like

return current; /* whoops, meant counter! */

in countCyclesHelper.

Next, unless you got invalid input, only array[first] may be -1, otherwise one of your nodes in your (implicit) graph has two incoming edges and the original file didn't contain a valid permutation.

If we take both suggestions into account, we end up with the following countCyclesHelper:

int countCyclesHelper(int* array, int N) {
  int total_cycles = 0;

  for (int i = 0; i < N; i++) {
    const int first = i;
    int current = first;

    if (array[current] == -1) {
      /* already counted this cycle */
      continue;
    }

    while (1) {
      const int temp = array[current] - 1;
      array[current] = -1;
      current = temp;
      if (current == first) {
        counter++;
        break;
      }
    }
  }
  return total_cycles;
}

While I kept first, it's possible to replace it with i.

As you can see, I've added const at various places. If we don't intend to change a value it's usually a good idea to prevent it with const. This holds for example for your name in countCycles. We don't want to accidentally change the file name:

int countCycles(const char *name, int N)

Next, fread(buffer, sizeof(buffer), …) is—unfortunately—wrong, since buffer is a pointer. You end up with fread(buffer, sizeof(char*), …). Therefore, your program contained undefined behaviour, unless N <= sizeof(char*).

That being said, countCycles is an overkill, since you handle only single digit permutation indices. For any  N >= 10the content ofarraywon't be a permutation. With this in mind, we can simplifycountCycles` a lot:

enum { MAXIMUM_DIGITS = 9 };

int countCycles(const char *name, int N) {
  if(N <= 0 || N > MAXIMUM_DIGITS) {
    return -1;
  }

  char buffer[MAXIMUM_DIGITS];

  FILE *fp = fopen(name, "rb");
  if(fread(buffer, N, 1, fp) != N) {
    fclose(fp);
    return -1;
  }
  fclose(fp);

  int array[MAXIMUM_DIGITS];

  for (int i = 0; i < N; i++) {
    array[i] = buffer[i] - '0';
  }
  return countCyclesHelper(array, N);
}

If MAXIMUM_DIGITS gets larger at some point, you probably need to switch to malloc again, but simple arrays with a static size work well enough for your use case.


Animations when navigation items are clicked I'm a beginner in jQuery and am wondering how I could write the following code shorter, since I'm repeating a lot.

Working example

//-----------Fullscreen overlay menu click events -----//

$('#overlay-menu').bind("click", function() {
    $(".overlay").addClass('overlay-open');
    $("#overlay-menu").fadeOut(800);
});

$(".overlay-close").bind("click", function() {
    $(".overlay").removeClass('overlay-open');
    $("#overlay-menu").fadeIn(800);
});

//------------ Scroll function-------//

function scrollToAnchor(linkID) {
    var linkName = $("a[name='"+ linkID +"']");
    $('html, body').animate({scrollTop: linkName.offset().top}, 'slow');
}

//------------- the repeating part --------------------------------//

$("#homelink").bind("click", function() {
    scrollToAnchor('home_anchor');
    $(".overlay").removeClass('overlay-open');
    $("#overlay-menu").fadeIn(800);
});

$("#overlink").bind("click", function() {
    scrollToAnchor('over_anchor');
    $(".overlay").removeClass('overlay-open');
    $("#overlay-menu").fadeIn(800);
});

$("#daglink").bind("click", function() {
    scrollToAnchor('dag_anchor');
    $(".overlay").removeClass('overlay-open');
    $("#overlay-menu").fadeIn(800);
});

$("#werkzaamhedenlink").bind("click", function() {
    scrollToAnchor('werkzaamheden_anchor');
    $(".overlay").removeClass('overlay-open');
    $("#overlay-menu").fadeIn(800);
});

$("#contactlink").bind("click", function() {
    scrollToAnchor('contact_anchor');
    $(".overlay").removeClass('overlay-open');
    $("#overlay-menu").fadeIn(800);
});

//------------One page item (there are 5)---------------//

over

//------------Fullscreen menu overlay ------------------//

                <button type="button" class="overlay-close">Close</button>

home

over

een dag bij

werkzaamheden

contact

Consider what you would do if JavaScript were unavailable.  You would probably have more semantic markup, with a menu like this:

home

over

een dag bij

werkzaamheden

contact

and page items like this:

over

such that clicking on a menu item would scroll to the relevant page item.

Then, it's just a matter of using jQuery to intercept that click and replace the behaviour with a more polished animation.

function scrollTo(selector) {
    var destination = $(selector);
    $('html, body').animate({scrollTop: destination.offset().top}, 'slow');
}

$("nav > ul > li > a").bind("click", function() {
    scrollTo($(this).attr('href'));
    $(".overlay").removeClass('overlay-open');
    $("#overlay-menu").fadeIn(800);
});


Sorting two related lists I have code that sorts two lists related with each other

"""
    list1 reperesents customer
    list2 reperesents amount of products they have brought
"""

list1=['a','c','e','i','o','u']
list2=[12,23,45,10,7,2]
list3=[]
for i in range(0,len(list1)):
        list3.append([list1[i],list2[i]])
print list3
"""
    [['a', 12], ['c', 23], ['e', 45], ['i', 10], ['o', 7], ['u', 2]]
"""
list3=sorted(list3,key=lambda x: list3[list3.index(x)][1])

print(list3)
"""
    [['u', 2], ['o', 7], ['i', 10], ['a', 12], ['c', 23], ['e', 45]]
"""
for i in range(0.len(list3)):
    list1[i]=list3[i][0]
    list2[i]=list3[i][1]

print list1
print list2

List1 is the customers' names
List2 is the number of products each customer bought

So:

'a' bought 12 products
'c' bought 23 products

And so on.

Is there a better way to do this?

You should probably just change how the data is stored. You should have one list which contains all of a customers data. For instance, a dictionary that follows this template:

{"name": "a", "purchased": 12}

If you had a list of those, it would be trivial to sort them, using the list sorting functions.

customer_data = [
                    {"name": "a", "purchased": 12},
                    {"name": "c", "purchased": 23},
                    {"name": "o", "purchased": 7},
                ]
customer_data.sort(key=lambda customer: customer["purchased"])
print customer_data
# [{'name': '0', 'purchased': 7}, {'name': 'a', 'purchased': 12}, {'name': 'c', 'purchased': 23}]


Parsing 2D matrix: "[[1,2][3,4]]" I feel I am over-complicating things, and that there should be a succinct way to achieve this - perhaps using concepts I have not learnt - or some prelude function I have missed out on.

How can I improve this?

data Mat2x2 = MkMat2x2 Integer Integer Integer Integer
  deriving (Show, Eq)

-- modified versions of preludes readParanthesis
readBrackets :: ReadS a -> ReadS a
readBrackets g = mandatory
                 where mandatory r = do
                              ("[", s) <- lex r
                              (x,t)    <- g s
                              ("]", u) <- lex t
                              return (x,u)

readInt :: String -> Maybe (Integer, String)
readInt s = listToMaybe ((reads :: String -> [(Integer, String)]) s)

-- greedy consumption of comma separated values and returns them as list and remaining string, ex:
-- parseCsv readInt "2,3,5,2"
-- ([2,3,5,2],"")
parseCsv :: (String -> Maybe (a, String)) -> String -> ([a], String)
parseCsv f s = case (f s) of
                        Nothing          -> ([], s)
                        Just (x,(',':r)) -> (x : (nextXs r), (nextR r))
                        Just (x,r)       -> ([x], r)
                      where nextXs ns = fst (parseCsv f ns)
                            nextR  ns = snd (parseCsv f ns)

-- reads an arbitrary length vector "[1,2,3,...N]" -> Just [1,2,3...,N]
readVec :: String -> Maybe ([Integer], String)
readVec s = listToMaybe (readBrackets ((:[]) . parseCsv readInt) s) -- note: (:[]) is operator making a list of one element

readVec2 :: String -> Maybe ((Integer, Integer), String)
readVec2 s = case readVec s of
                Just ((a:[b]), r)  -> Just ((a,b), r)
                Just (_, _)        -> Nothing
                Nothing            -> Nothing

read2Vec2 :: String -> Maybe (((Integer, Integer),(Integer,Integer)), String)
read2Vec2 s = case readVec2 s of
                    Nothing          -> Nothing
                    Just ((a,b), r1) -> case readVec2 r1 of
                                            Nothing          -> Nothing
                                            Just ((c,d), r2) -> Just (((a,b),(c,d)), r2)

readMat2x2 :: String -> Maybe (Mat2x2, String)
readMat2x2 s = case listToMaybe (readBrackets (maybeToList . read2Vec2) s) of
                    Nothing                 -> Nothing
                    Just (((a,b),(c,d)), r) -> Just ((MkMat2x2 a b c d), r)

Many improvements are possible.

First, you shouldn't use ReadS and other rudumentary Prelude parsing function for anything but exercises. Use parsing libraries from Hackage instead. parsec is a good library to start with, if you are not sure of which one to use.

Second, if it is an exersise, you can avoid long chains of Nothing matches by using Maybe monad. I reimplemented read2Vec2 for example:

read2Vec2 :: String -> Maybe (((Integer, Integer),(Integer,Integer)), String)
read2Vec2 s = do
    ((a,b), r1) <- readVec2 s
    ((c,d), r2) <- readVec2 r1
    Just (((a,b),(c,d)), r2)

Third, you can avoid passing unparsed input around by keeping it in the State monad. At this point our code will do what parsec does :) except parsec does it better.

If you want to stay with your current approach then readBrackets can be shortened:

readBrackets :: ReadS a -> ReadS a
readBrackets g r = do
    ("[", s) <- lex r
    (x,t)    <- g s
    ("]", u) <- lex t
    return (x,u)

Here is full parser using parsec:

{-# LANGUAGE NoMonomorphismRestriction #-}
import Text.ParserCombinators.Parsec
import Control.Applicative

data Mat2x2 = MkMat2x2 Integer Integer Integer Integer
  deriving (Show, Eq)

parseMat2x2 = parse mat2x2 "(unknown)"

nat = read <$> many1 digit

mat2x2 = do
    string "[["
    d1 <- nat
    char ','
    d2 <- nat
    string "]["
    d3 <- nat
    char ','
    d4 <- nat
    string "]]"
    return $ MkMat2x2 d1 d2 d3 d4

main = print $ parseMat2x2 "[[2,2][3,4]]"

The parser can be made less strict if necessary. For example, current parser doesn't accept any whitespace. And for a better performance you could try parsec with a lexer stage (current code avoids lexer altogether).

For ultimate performance you should use alex and happy tools, which are haskell's lex and yacc respectively.


Recursive uniform cost search that needs to be optimized I have this uniform cost search that I created to solve Project Euler Questions 18 and 67. It takes the numbers in the txt file, places them into a two dimensional list, and then traverses them in a uniform cost search (that I hoped was a kind of implementation of an a* search). It looks like this:

f = open('triangle.txt', 'r')
actualRows = [[n for n in p.split(" ")] for p in f.read().split('\n')]
actualRows.pop()
actualRows = [[int(n) for n in p] for p in actualRows]
rows = [[100 - n for n in p] for p in actualRows]

def astar(pq):
    m = min(pq,key=lambda w: w[3])
    if len(m[2]) + 1 is len(actualRows):
        return m
    pq.remove(m)
    toAdd = list(m[2])
    toAdd.append(m[0])
    pq.append((actualRows[m[4]+1][m[5]], rows[m[4]+1][m[5]], toAdd, m[3] + m[1], m[4]+1, m[5]))
    pq.append((actualRows[m[4]+1][m[5]+1], rows[m[4]+1][m[5]+1], toAdd, m[3] + m[1], m[4]+1, m[5]+1))
    return astar(pq)

# Each tuple is: (actualScore, minScore, previous, totalScore, y, x)
priorityQueue = [(actualRows[0][0], rows[0][0], [], 0, 0, 0)]
a = astar(priorityQueue)
print a
print str(sum(a[2]) + a[0])

I'm not asking for you to tell me how to solve the Problem, I just want to optimize this search so that it doesn't crash going past the 17th row of numbers. How would I optimize this? Or how would I write a proper uniform cost search?

Unpack your tuple to improve readability. There's already a comment

# Each tuple is: (actualScore, minScore, previous, totalScore, y, x)

Put those names in code like this so you can use eg. actualScore instead of m[0]:

(actualScore, minScore, previous, totalScore, y, x) = m

You are using recursion to implement a simple loop. I get RuntimeError: maximum recursion depth exceeded even from problem 18. That's because you recurse from every node on every path until you find a solution. Just use a while loop instead.
Use the heapq module to implement a priority queue to avoid the linear search done by min.
Compare numbers using ==. Only use is for object identity.

To learn about the A* algorithm, see the highly readable tutorial at Amit’s A* Pages.
You'll notice that your approach is different. In particular, you don't compute a heuristic. On the other hand I'm not sure if a useful heuristic exists for this problem, but in any case your approach is more like Dijkstra'a algorithm.
You should keep track of already visited nodes to avoid exploring the same multiple times.
When you add nodes to the priority queue, you should already compute their scores. Now you add both children with the score of the parent. When you eventually pop these nodes from the queue, you get the left child even though you need the better one first. Try the small example at the top of the problem 18 page: program answers 19 instead of 23, because at the bottom row the 5 pops from the queue before the 9.


NODE JS Unlink and delete MySQL data Here is my revised code from my other review question Node JS delete multiple MySQL linked records and delete physical file
Just would like to hear some reviews if I have the right logic.
router.delete('/:id', (req, res)=>{

  function deleteCommentRecords(){
    db.beginTransaction(function(err) {
      if(err) return res.status(500).end(err.message);
      //Delete comment section records
      db.query("DELETE FROM commentSchema WHERE PostID = ?", req.params.id, (err, result)=>{
        if(err){
          db.rollback(()=>{
            return res.status(500).end(err.message);
          });
        }
        db.commit((err)=>{
          if(err){
            db.rollback(()=>{
              return res.status(500).end(err.message);
            });
          }
          console.log('Transaction Completed Successfully.');
        });
      });
    });
  }

  function deletePostSchemaReords(){
    // Delete PostSchema records
    db.query("delete from postschema where id = ?", req.params.id, (err, result)=>{
      if(err) {
        db.rollback(()=>{
          return res.status(500).end(err.message);
        });
      }
      db.commit((err)=>{
        if(err){
          db.rollback(()=>{
            return res.status(500).end(err.message);
          });
        }
        console.log('Transaction Completed Successfully.');
      });
    });
  }

  function loadData(){

    // Get filenames from Comments
    db.query("SELECT image FROM commentschema WHERE postID = ?", req.params.id, (error, comments_image_output)=>{
      if(error) return res.status(500).end(err.message);

      // If there is an image
      if(comments_image_output.length > 0){
        // Foreach image, delete one by one
        comments_image_output.forEach(function(row){
          try {
            console.log(row.image);
            fs.unlinkSync(uploadDir + row.image);
            console.log('Successfully deleted');
            // Query to remove commentSchema records
            deleteCommentRecords();
          } catch (err) {
          // handle the error
          }
        });
        // Redirect back to posts
        res.redirect(303, '/admin/posts');
      }
        deleteCommentRecords();
    })

    // Get filename from PostSchema
    db.query("SELECT filename FROM PostSchema WHERE id = ?", req.params.id, (err,post_image_output)=>{
      if(err) return res.status(500).end(err.message);
      if(post_image_output.length > 0){
        // Foreach image, delete one by one
        post_image_output.forEach(function(row){
          try {
            console.log(row.filename);
            fs.unlinkSync(uploadDir + row.filename);
            console.log('Successfully deleted files');
            deletePostSchemaReords();
          } catch (err) {
          // handle the error
          }
        });
      }
        deletePostSchemaReords();
    });
  }
  loadData();
});

There are some problems with this implementation:

if both the query in loadData fail, res.status(500).end(err.message) is run twice so you will get an error RESPONSE ALREADY CLOSED that could lead to server crash, and this must be avoided
you are not using a linter on your code. I can say that because you have the error parameter in the callback, but in the code, you wrote ..end(err.message), so adopt a linter to see these error before they happen in production that would cause a crash of your application since err would be undefined
every request adds in the memory heap the functions loadData, deletePostSchemaReords and deleteCommentRecords causing pressure on the garbage collector and slowing down your endpoints and this can be voided
a lot of code replicated that must be avoided to have a nice and maintainable endpoint
fs.unlinkSync kills the performance in an API endpoint
deleteCommentRecords() is called for every comments_image_output but this would execute comments_image_output.length times the same query, this is a functional error
in deleteCommentRecords() a transaction begins and the immediately committed so it is not adding any performance gain: a transaction works best when there are multiple query to execute across multiple tables
the loadData function is deleting rows from DB, so the name is misbehaviour
in deletePostSchemaReords there is only a query without the transaction so the rollback is ineffective
the response object should be managed by one entity otherwise there is too much coupling between general functions (like delete an array of files) and the HTTP protocol

Here how I would proceed with the refactor.

I used the callback style (instead of async/await) since you are not using promises
I assume there is a db and a uploadDir global objects - since are not in the code example

router.delete('/:id', (req, res) => {
  loadData(req.params.id, (err, files) => {
    if (err) { return res.status(500).end(err.message) }

    // if both query are successful delete from the database
    deleteAll(req.params.id, (err) => {
      if (err) { return res.status(500).end(err.message) }

      res.redirect(303, '/admin/posts') // response to the client first

      // the database deletion is OK, now delete the files quitely
      const deleteFileArray = files.comment.concat(files.post)
      deleteFiles(deleteFileArray, (err) => {
        if (err) {
          console.log('ops failed to delete files, but who cares?')
        }
      })
    })
  })
})

function loadData (postId, callback) {
  let runs = 0
  const output = { comment: null, post: null, error: null }

  db.query('SELECT image FROM commentschema WHERE postID = ?', postId, processQueryResult.bind(null, 'comment', 'image'))
  db.query('SELECT filename FROM PostSchema WHERE id = ?', postId, processQueryResult.bind(null, 'post', 'filename'))

  // this function will be executed twice an manage only one callback call
  function processQueryResult (responseType, columnName, error, images) {
    if (error) {
      output.error = error
    } else {
      output[responseType] = images.map(row => uploadDir + row[columnName])
    }

    if (++runs === 2) { // call the callback with the sum of the files to delete
      callback(output.error, output)
    }
  }
}

function deleteAll (postId, callback) {
  // Delete PostSchema records
  db.beginTransaction(function (err) {
    if (err) return callback(err)

    // Delete comment section records
    db.query('DELETE FROM commentSchema WHERE PostID = ?', postId, (err) => {
      if (err) { return db.rollback(callback.bind(null, err)) }

      db.query('DELETE FROM postschema where id = ?', postId, (err) => {
        if (err) { return db.rollback(callback.bind(null, err)) }

        db.commit((err) => {
          if (err) { return db.rollback(callback.bind(null, err)) }
          console.log('Transaction Completed Successfully.')
          callback()
        })
      })
    })
  })
}

function deleteFiles (files, callback) {
  let i = files.length
  files.map(function (filepath) {
    fs.unlink(filepath, function (err) {
      if (err) {
        callback(err)
      } else if (--i <= 0) {
        callback(null)
      }
    })
  })
}
```


Phonetic Alphabet Trainer This is a little program I put together to help people learn the phonetic alphabet. Feel free to query the code.

import random

phonetic = {    'a' : 'alpha',    'b' : 'bravo',    'c' : 'charlie',    'd' : 'delta',    'e' : 'echo',    'f' : 'foxtrot',    'g' : 'golf',    'h' : 'hotel',    'i' : 'india',    'j' : 'juilett',    'k' : 'kilo',    'l' : 'lima',    'm' : 'mike',    'n' : 'november',    'o' : 'oscar',    'p' : 'papa',    'q' : 'quebec',    'r' : 'romeo',    's' : 'sierra',    't' : 'tango',    'u' : 'uniform',    'v' : 'victor',    'w' : 'whiskey',    'x' : 'x-ray',    'y' : 'yankee',    'z' : 'zulu'    }

print('Welcome to the IkeaFish Phonetic Trainer. \n Answer no or n to stop the program.')

again = 'yes'
while again != 'no' or 'n':

    #Picks either 0 or 1. If equal to 1 tests the letters, If equal to 0, tests the words.
    if random.randint(0,1) == 1:

        question = random.choice(list(phonetic.keys()))
        answer = input('What does this letter {} stand for: '.format(question))

        if answer == phonetic[question]:
            print('Correct')
        else:
            print('Wrong answer', question, 'stands for', phonetic[question])

    else:
        letter = random.choice(list(phonetic.keys()))
        #Grabbing a random key
        question = phonetic[letter]
        answer = input('What does this word {} stand for: '.format(question))

        if answer == letter:
            print('Correct')
        else:
            print('Wrong answer', question, 'stands for', phonetic[letter])

Your line

while again != 'no' or 'n'

is incorrect: again != 'no' or 'n' is evaluated to (again != 'no') or 'n' which is True as 'n' evaluates to True in any case.
You have to use again != 'no' and again != 'n' here
    or again not in ['no', 'n'].

Anyway, I replaced it with while True and added a break statement. This allows to exit from the loop exactly when we need it. (Otherwise we get unnecessary message that our answer is not correct.) Also your again variable never changed, so your loop in any case is infinite.

I also did several improvements that you can see in the code, and updated the spelling to match the NATO phonetic spellings.

import random

phonetic = {'a': 'alfa', 'b': 'bravo', 'c': 'charlie', 'd' : 'delta',
            'e': 'echo', 'f': 'foxtrot', 'g': 'golf', 'h': 'hotel',
            'i': 'india', 'j': 'juliett', 'k': 'kilo',
            'l': 'lima', 'm': 'mike', 'n': 'november', 'o': 'oscar',
            'p': 'papa', 'q': 'quebec', 'r': 'romeo',
            's': 'sierra', 't': 'tango', 'u': 'uniform', 'v': 'victor',
            'w' : 'whiskey', 'x': 'x-ray', 'y': 'yankee', 'z': 'zulu'}
# made this a bit more pep8-compliant

print("Welcome to the IkeaFish Phonetic Trainer.\nAnswer 'no' or 'stop' to stop the program.")
# n can be valid answer, so it is not good to use it a stop-word

while True:

    letter, word = random.choice(list(phonetic.items()))
    # we can pick both elements at once

    if random.choice([True, False]):
        answer = input('What does this letter {} stand for: '.format(letter))
        correct = word
    else:
        answer = input('What does this word {} stand for: '.format(word))
        correct = letter

    answer = answer.lower()
    # make answers case-insensetive

    if answer in ['no', 'stop']:
        break

    if answer == correct:
        print('Correct')
    else:
        print('Wrong answer', letter, 'stands for', word)


Python Password Brute-Forcer I'm working on a password brute-forcer that takes in a password from the user and brute-forces solutions until it finds a match. I was wondering if there is any way to improve performance or readability?

import itertools
import time

Alphabet = ("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXiuYZ1234567890-_.")

Password = input("What is your password?\n")

start = time.time()

counter = 1

CharLength = 1

for CharLength in range(25):

    passwords = (itertools.product(Alphabet, repeat = CharLength))

    print("\n")

    print("Currently working on passwords with ", CharLength, " characters.")
    print("We are currently at ", (counter / (time.time() - start)), "attempts per second.")
    print("It has been ", time.time() - start, " seconds.")
    print("We have tried ", counter, " possible passwords.")

    for i in passwords:

        counter += 1

        i = str(i)

        i = i.replace("[", "")
        i = i.replace("]", "")
        i = i.replace("'", "")
        i = i.replace(" ", "")
        i = i.replace(",", "")
        i = i.replace("(", "")
        i = i.replace(")", "")

        if i == Password:

            end = time.time()

            timetaken = end - start

            print("\nPassword found in ", timetaken, " seconds and ", counter, "attempts.")

            print("That is ", counter / timetaken, " attempts per second.")

            print("\nThe password is \"%s\"." % i)

            input("\nPress enter when you have finished.")

            exit()

Thanks.

Some recommendations just looking at the style of the code:

It would benefit from being run through pycodestyle, flake8 and/or similar tools to be more idiomatic. This would make the code easier to read for anyone familiar with Python.
Timing code should not be part of your program. External tools like time can handle that.
Use argparse rather than input to make the program scriptable. The script should not stop anywhere to ask for input.
The Alphabet and 25 in this code are good candidates for configuration or parameters.
You can remove all of a list of characters from a string in a single command.


Finding the number in a list that differs in parity Given any string with a series of numbers find out which one of the numbers differs from the others in evenness and return its position.

Examples:

"2 4 6 7 8 10" returns 4
"3 5 7 9 10" returns 5

I found 2 different ways to do so, but I'm wondering what is the best approach in matter of time complexity.

Solution 1

def num_test(numbers)
  (i = numbers.split.map(&:to_i)).index(i.partition(&:odd?).find(&:one?).first)+1
end

Solution 2

def num_test(numbers)
  numbers.index(numbers.split.map(&:to_i).partition(&:odd?).find(&:one?).first.to_s)+1
end

Also, any other solution is welcome.

Well, one way we can do it is by counting the first three elements. If more than one is odd, it means the sequence is odd, and we should look for an even number. If one or less is odd, the sequence is even, and we should look for an odd number.

a1 = "2 4 6 7 8 10"
a2 = "3 5 7 9 10"

def index_of_outlier(string)
  array = string.split(' ').map(&:to_i)
  count_of_odd = (array[0..2]).count { |i| i.odd? }
  if count_of_odd > 1
    array.index { |i| i.even? }
  else
    array.index { |i| i.odd? }
  end
end

puts index_of_outlier(a1) #=> 3
puts index_of_outlier(a2) #=> 4

This looks messy, but it gets the job done quickly. The lower bound is \$3 + 1\$ iterations, and the upper bound is \$3 + n - 1\$. This makes it \$O(n)\$.

You need three iterations to determine the nature of the sequence and up to \$n - 1\$ more to find the solution.

I'm not sure how partition works under the hood, but I assume it traverses the array at least once. This means my solution will perform better if the target index is lower than \$n - 5\$.

If you want the char position in the string and not the array index, just add one to the result (as you are doing in your own solutions).


Concatenate disjoint Excel ranges to use as a single range in a formula I need to vertically concatenate two disjoint ranges in VBA to be passed to an Excel function (in this case the LINEST function). I hate working with arrays in VBA but I managed to get it working using the following code:

Option Base 1

Function VERTCAT(R1 As Range, R2 As Range) As Variant

    Dim ncols As Integer
    ncols = R1.Columns.Count
    Dim nrows1 As Integer
    nrows1 = R1.Rows.Count
    Dim nrows2 As Integer
    nrows2 = R2.Rows.Count

    Dim arr1 As Variant
    arr1 = R1
    Dim arr2 As Variant
    arr2 = R2

    Dim arr3 As Variant
    ReDim arr3(nrows1 + nrows2, ncols)

    For Col = 1 To ncols
        For Row = 1 To nrows1
            arr3(Row, Col) = arr1(Row, Col)
        Next Row
        For Row = 1 To nrows2
            arr3(Row + nrows1, Col) = arr2(Row, Col)
        Next Row
    Next Col

    VERTCAT = arr3

End Function

I'm not worried about adding basic error checking, like making certain that the two arrays have the same number of columns or adding features. I am hoping that someone can show me what the idiomatic VBA way is to have written this function? Could I have avoided some of the helper variables (like arr3 for example?)? Could I have avoided the loops?

To answer your question, no. There is no "idiomatic" way to combine multiple arrays together without creating a bigger array (or extending one of the originals) and manually copying in the array contents.

Anyway, while I'm here,

Miscellaneous Points

I don't know if you just left it out, but if it's not there, Option Explicit should be at the top of every single code module everywhere. IF you don't have it enabled, go to Tools --> Options --> Require Variable Declaration.

Your function is implicitly public because you didn't scope it. Explicitly scope everything. Assuming you want to use it in other modules, it should be Public Function VERTCAT() as Variant.

Integer is outdated. Unless you're in a position where 4 bytes of memory has serious performance impacts, use Long instead. Integer will fail if you give it a number greater than about 33,000, that's a number that could reasonably occur when dealing with big spreadsheets. Long will handle numbers up to 2.1 Billion and is also (marginally) faster.

Variable Naming

First off, VERTCAT violates several good practices.

Standard VBA convention for function/procedure names is PascalCase. VERTCAT is also distinctly unclear and ambiguous. Names should always be clear, concicse and most importantly, unambiguous.

If I see VERTCAT I'm going to think "What does this do? Vertically... catenate?". Okay, but catenate what? Strings? Lists? Ranges? Arrays? VerticallyConcatenateRanges really isn't very long, and is absolutely clear on what it takes as parameters, and what it does to them. Depending on your style, you might consider ArrayFromConcatenatedRanges to be even better.

If in doubt, always err on the side of more verbose.

VBA conventions you should be aware of:

Function/Procedure names are PascalCase.

Local variable names are camelCase so nCols instead of ncols and col instead of Col.

N.B. if Col is capitalised because it's a module-level variable, that's an even worse offence (unnecessarily broad scope) than bad naming.

Module/Global variable names are PascalCase so if I see ReferenceVariable in the middle of a procedure, I know to look for it outside of that procedure's scope.

And now for your variables

R1 and R2 are terrible variable names. Variables should always sound like what they are. I suggest topRange,bottomRange. Suddenly, it's clear that they are ranges, and that the first one is the one that's going to end up on top of the final concatenation.

Similarly, ncols,nrows1,nrows2 are not great. It should be nCols, nRows1, nRows2 anyway, and then numColumns,rowsInTopRange,rowsInBottomRange still sound like counts of rows/columns, but it's much clearer what they are/should be. Similarly, arr1 and arr2 could do with the same treatment.

This is your code, with better naming, and it is so much easier for someone to read/follow:

Option Explicit
Option Base 1

Public Function VerticallyConcatenateRanges(topRange As Range, bottomRange As Range) As Variant

    Dim row As Long, col As Long

    Dim numColumns As Long
        numColumns = topRange.Columns.Count

    Dim rowsInTopRange As Long
        rowsInTopRange = topRange.Rows.Count

    Dim rowsInBottomRange As Long
        rowsInBottomRange = bottomRange.Rows.Count

    Dim totalRows As Long
        totalRows = rowsInTopRange + rowsInBottomRange

    Dim arrTopRange As Variant
        arrTopRange = topRange

    Dim arrBottomRange As Variant
        arrBottomRange = bottomRange

    Dim concatenatedArray As Variant
    ReDim concatenatedArray(1 To totalRows, 1 To numColumns)

    Dim ix As Long

        For col = 1 To numColumns

            For row = 1 To rowsInTopRange
                concatenatedArray(row, col) = arrTopRange(row, col)
            Next row

            For row = 1 To rowsInBottomRange
                ix = row + rowsInTopRange
                concatenatedArray(ix, col) = arrBottomRange(row, col)
            Next row

        Next col

        VerticallyConcatenateRanges = concatenatedArray

End Function

And, crucially, if you took any of those lines out of context and gave them to someone, that person would be able to figure out very easily what the variables were and what the code was doing.


Dephi: faster way to convert a "dirty" string to a number How can I make this code faster? the string can contain characters such as ", .?#" and possibly others.

Const Nums = ['0'..'9'];

function CleanNumber(s: String): Int64;
Var z: Cardinal;
begin
  for z := length(s) downto 1 do
   if not (s[z] in Nums) then Delete(s,z,1);
  if s = '' then
    Result := 0 else
    Result := StrToInt64(s);
end;

Don't over use set. It is faster when the items are discrete, but slower when the items are continuous. Although, the difference is very small in your case.

The problem here is that you use delete repeatedly. Each call moves (copies) a potentially large chuck of characters. This is inefficient because many characters are moved multiple times. For example, 5 in string 1 2 3 4 5 is moved 4 times.

Given that string in Delphi is mutable, it is better to copy individual characters:

function CleanNumber(s: String): Int64;
Var
  z, l: Cardinal;
begin
  l := 1;
  for z := 1 to length(s) do
    if (s[z] >= '0') and (s[z] <= '9') then
    begin
      s[l] := s[z];
      inc(l);
    end;
  SetLength(s, l - 1);

  if s = '' then
    Result := 0 else
    Result := StrToInt64(s);
end;

It can be further improved:

function CleanNumber(s: String): Int64;
Var
  z, l: Cardinal;
begin
  l := 1;
  while (l <= length(s)) and (s[l] >= '0') and (s[l] <= '9') do
    inc(l);       // Scan for the first non-numeric char

  for z := l + 1 to length(s) do  // Start from l + 1
    if (s[z] >= '0') and (s[z] <= '9') then
    begin
      s[l] := s[z];
      inc(l);
    end;

  if l = 1 then Result := 0 else
  begin
    SetLength(s, l - 1);
    Result := StrToInt64(s);
  end;
end;

Note: code not tested.


Generic cached value class mimicking Lazy Before I wrote this I searched and found a number of solutions that make use of a caching provider to handle a set of items. I felt that was too cumbersome of an approach and set out to create a class that could cache a single value, similar to the way Lazy<T> manages the initialization of a single value.

I'm having trouble determining the thread-safety of this implementation, specifically where lock statements should be used. Can any experts on working with multi-threaded applications spot where I may have gone wrong? Or is this approach sound?

public class Cached<T> {
    private readonly TimeSpan _duration;
    private DateTime _expiration;
    private Lazy<T> _value;
    private readonly Func<T> _valueFactory;

    public T Value {
        get {
            // Do I need locking while checking the expiration?
            if (DateTime.Now >= _expiration)
                RefreshValue();

            return _value.Value;
        }
    }

    public Cached(Func<T> valueFactory, TimeSpan duration) {
        _duration = duration;
        _valueFactory = valueFactory;

        RefreshValue();
    }

    private void RefreshValue() {
        // Is this sufficient while updating the expiration and value?
        lock (this) {
            _value = new Lazy<T>(() => _valueFactory());
            _expiration = DateTime.Now.Add(_duration);
        }
    }
}

I'm using Lazy<T> for the value getter because it's possible that the cached value will be (1) expensive to load and (2) not immediately needed.

First off, never lock on this. You never know who else is locking on that instance. Conversely, clients don't know that the cache is locking on itself either. For example, this seemingly innocent code would lead to a deadlock:

//Thread A
lock(cache)
lock(someObject)
{
    //do something
}

//Thread B
lock(someObject)
{
    //here, the cache will lock on itself.
    //the locks are accidentally taken in reverse order -> deadlock!
    var x = cache.Value;
}

That's why it's good practice to lock on a private readonly object.
Moving on...

Your current implementation allows the value to be refreshed twice in a row. This is a problem if the resource is expensive to create (and resources worth caching usually are).

Two threads might check whether DateTime.Now >= _expiration, then both go into the RefreshValue method and execute the critical region in quick sucession. To solve this you can either:

move the lock up to the place where the expiration date is being checked.
move the expiration check inside the RefreshValue method.

I would go with option 2, and add a double-checked lock to avoid hitting the lock every single time:

private readonly object _refreshLock = new object();

private void RefreshValueIfNeeded()
{
    if (DateTime.Now >= _expiration)
    {
        lock (_refreshLock)
        {
            if (DateTime.Now >= _expiration)
            {
                _value = new Lazy<T>(_valueFactory);
                _expiration = DateTime.Now.Add(_duration);
            }
        }
    }
}

public T Value
{
    get
    {
        RefreshValueIfNeeded();
        return _value.Value;
    }
}

One more thing: you don't need to wrap the value factory into another delegate, you can just pass it directly to Lazy<T>'s constructor:

//change this
new Lazy<T>(() => _valueFactory())
//to
new Lazy<T>(_valueFactory)


"FIZZ BANG BUZZ!" 3,7,11 efficiency I wrote a fizz buzz variation method in Python which prints "fizz", "bang", and "buzz" for 3, 7, and 11 respectively.

def FizzBang():
    string = ''
    for n in range(1,101):
        msg = ""
        if not n % 3:
            msg += "Fizz"
        if not n % 7:
            msg += "Bang"
        if not n % 11:
            msg += "Buzz"
        print msg or str(n)

What can I do to increase the speed of this program? I know the modulus is quite expensive. Also maybe concatenating strings might not be needed as well.

Indeed, addition is faster than modulo.  This implementation runs about 33% faster than the original when counting up to 100.  However, I expect that it wouldn't scale as well to larger limits due to its O(n) memory usage.

def fizz_bang(limit=100):
    limit += 1
    strings = [''] * limit

    fbb = ((3, 'Fizz'), (7, 'Bang'), (11, 'Buzz'))
    for stride, noise in fbb:
        for n in range(0, limit, stride):
            strings[n] += noise
    for n in range(1, limit):
        print strings[n] or n

Also, according to PEP 8 conventions, FizzBang should be named fizz_bang instead.