String permutation iterative improvements? I have written some code (below) which iterates over each character of a string and adds that character to every index of previous permutations.

For example, in case of abc: b is added to every index of perm(a) resulting in ab, ba.

Now c is added to every index of perm(c) resulting in cab, abc, acb, cba, bac, bca.

I use a Set, to avoid duplicates.

Please review code and how it could be improved while keeping it iterative. I am particularly looking for improvements to my existing code, rather than alternative solutions(if possible), so that I can better understand the optimization process.

There may be a faster recursive solution, but I'm currently looking to improve my iterative approach.

  //method called from main
Set<String> perm(String input)
{
    Set<String> perm = new HashSet<String>();
    if(input == null || input.length() == 0 || input.length() == 1)
    {
        perm.add(input);
        return perm;
    }
 //convert to char array for iteration
    char[] arr = input.toCharArray();
 //add first char as first perm
    perm.add(String.valueOf(arr[0]));

    //iterate arr and find perm for each char
    for(int i=1; i<arr.length; i++)
    {
        perm = findPerm(String.valueOf(arr[i]), perm);
    }
    System.out.println(perm.size());
    return perm;
}

private Set<String> findPerm(String next, Set<String> perm)
{
    Set<String> perms = new HashSet<String>(perm);
 //iterate each perm to add next char            at all indexes
    for(String each : perm)
    {
        perms.addAll(addAtEachIndex(each, next));
   //remove the older perms
   //for example in case of ABC,  ab and ba should be deleted after making cab abc acb cba bca and bac
        perms.remove(each);
    }
    return perms;
}

private Set<String> addAtEachIndex(String each, String next)
{
    //ab c
    //^a^b^
    //0,0+c+0,1  0,1+c+1,1  0,1+c
//add the next char to each index of current perm
    Set<String> finalPerms = new HashSet<String>();
    finalPerms.add(each+next);
    finalPerms.add(next+each);
    for(int i=0; i<each.length(); i++)
    {
        finalPerms.add(each.substring(0,i+1) + next +each.substring(i+1, each.length()));
    }
    return finalPerms;
}

Descriptive naming

Set<String> perm(String input)

I would call this

Set<String> permute(String input)

When I see perm, my first thought is someone making straight hair curly.  My second thought is that it might be short for permanent.  Well after that I realize that in this context, you are permuting things.  In my opinion, the extra three letters increases readability greatly.

   Set<String> perm = new HashSet<String>();

Ignoring the confusion with naming a variable the same as the method using it, I'd go with

    Set<String> permutations = new HashSet<>();

Now I know what's supposed to be in there.

Unless you are compiling against an old Java version, you don't need to write out String the second time.  The compiler will figure it out.

Avoid casts

    char[] arr = input.toCharArray();

While this often helps when processing something character by character, I don't think it does so here.

    perm.add(String.valueOf(arr[0]));

This could be

    permutations.add(input.substr(0, 1));

So rather than converting from a String to a character array, then getting one character, then converting that character to a String, we just take a single character substr which returns the String that we want.  Since a String is immutable, this can use the original backing array to hold the data.  The other version would likely allocate new memory because it's not obvious that it's just taking a substring.

But even better might be

   permutations.add("");

Then we can simplify the gating test to just check for null.  And we can put the rest of the logic in the loop.

    for (char current : input.toCharArray())
    {
        permutations = permute(current, permutations);
    }

Yes, I did change the name of findPerm, and I changed it to take a character rather than a String.  I didn't like the name findPerm because you aren't finding anything.  You are generating permutations.

If we change the first parameter to be a character rather than a String, we can use it as a character later.

The other version may be slightly more efficient, but at the cost of additional coding complexity.

And now we are processing the String character by character, so we convert it to a character array.  We don't need i, as we can work with the characters directly.

Don't do unnecessary work

You have

    Set<String> perms = new HashSet<String>(perm);

and

        perms.remove(each);

But if you change the first to

    Set<String> permutations = new HashSet<String>();

Then you don't need the second line.  There is no reason to put the partial permutations in the results.

Don't Repeat Yourself (DRY)

        perms.addAll(addAtEachIndex(each, next));

So addAtEachIndex creates a Set just to return it and add to another Set.  Why bother?

        addAtEachIndex(each, next, permutations);

If we pass the Set into the method, we can add to it directly.  So we check for duplicates once, not twice.

Choose your datatype

        finalPerms.add(each.substring(0,i+1) + next +each.substring(i+1, each.length()));

You are doing a lot of String manipulation in this section and it's not really necessary.  Consider what happens if you leave next as a character rather than a String.

    StringBuilder permutation = new StringBuilder(each);
    permutation.append(next);

    permutations.add(permutation.toString());
    for (int i = each.length(); i > 0; i--)
    {
        permutation.setCharAt(i, permutation.charAt(i - 1));
        permutation.setCharAt(i - 1, next);
        permutations.add(permutation.toString());
    }

This also works with a StringBuilder rather than a String.

Now, rather than copying two substrings and a single character String into a new String, we only copy once.  We do three character operations rather than five String operations.  And we can keep reusing the same StringBuilder as a base.

Summary

public static Set<String> permute(String input)
{
    Set<String> permutations = new HashSet<String>();
    if (input == null)
    {
        permutations.add(input);
        return permutations;
    }

    permutations.add("");

    for (char current : input.toCharArray())
    {
        permutations = _permute(current, permutations);
    }

    return permutations;
}

private static Set<String> _permute(char next, Set<String> partials)
{
    Set<String> permutations = new HashSet<String>();

    for (String partial : partials)
    {
        addAtEachIndex(partial, next, permutations);
    }

    return permutations;
}

private static void addAtEachIndex(String partial, char next, Set<String> permutations)
{
    StringBuilder permutation = new StringBuilder(partial);
    permutation.append(next);

    permutations.add(permutation.toString());
    for (int i = partial.length(); i > 0; i--)
    {
        permutation.setCharAt(i, permutation.charAt(i - 1));
        permutation.setCharAt(i - 1, next);
        permutations.add(permutation.toString());
    }
}

I also made all the methods static as they weren't using any object state.

Time complexity

If we call the outer permute once, we call the inner _permute about \$n\$ times, where \$n\$ is the length of the input.

The first time we call the inner _permute, it calls addAtEachIndex once.  The second time, once.  The third time, twice.  So on the \$i\$th time, we call it \$(i-1)!\$ times if there are no duplicate characters in the String.  If there are duplicate characters, we call it fewer times.

The first time that we call the inner _permute, each call to addAtEachIndex iterates 0 times.  The second time, one time.  The third time, twice.  The fourth time, six iterations.  Yep, \$(i - 1)!\$ again.

Perhaps the simplest way to think about this are that there are \$n+1\$ steps to find the permutations.  On the \$i\$th step, we generate \$(i-1)!\$ permutations.  So for a given input length of \$n\$, we generate \$ \sum_{i=0}^{n} i! \$ permutations.  But that's just \$\mathcal{O}(n!)\$ because it is smaller than \$ (n+1)!\$ which is \$\mathcal{O}(n!)\$.

This is as efficient as we can get asymptotically, as there are \$n!\$ results.  And we must generate each one.


Check mongodb status is running or not I have written a program in golang which checks the mongodb is running or not using commands. But I'm not sure that the program is sufficient or there is something that needs to change in the program. Can you review my code so I can be sure that my code does not have any error or doesn't break if there is any problem at all?

Code

package main

import (
    "bytes"
    "fmt"
    "os/exec"
)

/*
*Function check mongodb is running? if not running then it will help to run it by command
*/
func main() {
    status := ExecuteCommand("systemctl is-active mongod")
    if status == true {
    fmt.Println("Connected")
    } else {
        fmt.Println("disconnected")
        status = ExecuteCommand("echo <password> | sudo -S service mongod start")
        if status == true {
            fmt.Println("Connected")
        }
    }
}

/*
* Function to execute the command using golang code.
*
* Params command type string
*
* Used by main func
*
* Returns nil (if no error)
* or      err type error (if error occurs)
*/
func ExecuteCommand(command string) bool {
    cmd := exec.Command("sh", "-c", command)
    var out bytes.Buffer
    var stderr bytes.Buffer
    cmd.Stdout = &out
    cmd.Stderr = &stderr
    err := cmd.Run()
    if err != nil {
        fmt.Println(fmt.Sprint(err) + ": " + stderr.String())
        return false
    }
    return true
}

Golang playground

I think your code works. Another approach would be trying to connect to the database using the MongoDB Go Driver.

import (
"context"
"log"

"go.mongodb.org/mongo-driver/mongo"
"go.mongodb.org/mongo-driver/mongo/options"
"go.mongodb.org/mongo-driver/mongo/readpref")

func GetClient() *mongo.Client {
    clientOptions := options.Client().ApplyURI("mongodb://localhost:27017")
    client, err := mongo.NewClient(clientOptions)
    if err != nil {
        log.Fatal(err)
    }
    err = client.Connect(context.Background())
    if err != nil {
        log.Fatal(err)
    }
    return client
}

If you can't connect (assuming you passing the right information), it means that the database is not running.

But like I said, it's just another approach.


Flatten an array in Python I would like to get some code review for my recursive implementation of python flatten array method.

  Write a piece of functioning code that will flatten an array of
  arbitrarily nested arrays of integers into a flat array of integers.
  e.g. [[1,2,[3]],4] -> [1,2,3,4].

I'm particularly looking for some feedback on the following:

Is usage of TypeError exception justified?
Did I miss some valid edge cases in my tests?
Here is my solution including unit tests:

def flatten(input_array):

    result_array = []
    for element in input_array:
        if isinstance(element, int):
            result_array.append(element)
        elif isinstance(element, list):
            result_array += flatten(element)
    return result_array

it has passed all of the following tests

from io import StringIO
import sys

# custom assert function to handle tests
# input: count {List} - keeps track out how many tests pass and how many total
#        in the form of a two item array i.e., [0, 0]
# input: name {String} - describes the test
# input: test {Function} - performs a set of operations and returns a boolean
#        indicating if test passed
# output: {None}
def expect(count, name, test):
    if (count is None or not isinstance(count, list) or len(count) != 2):
        count = [0, 0]
    else:
        count[1] += 1

    result = 'false'
    error_msg = None
    try:
        if test():
            result = ' true'
            count[0] += 1
    except Exception as err:
        error_msg = str(err)

    print('  ' + (str(count[1]) + ')   ') + result + ' : ' + name)
    if error_msg is not None:
        print('       ' + error_msg + '\n')

# code for capturing print output
#
# directions: capture_print function returns a list of all elements that were
#             printed using print with the function that it is given. Note that
#             the function given to capture_print must be fed using lambda.
class Capturing(list):
    def __enter__(self):
        self._stdout = sys.stdout
        sys.stdout = self._stringio = StringIO()
        return self

    def __exit__(self, *args):
        self.extend(self._stringio.getvalue().splitlines())
        sys.stdout = self._stdout

def capture_print(to_run):
    with Capturing() as output:
        pass
    with Capturing(output) as output:  # note the constructor argument
        to_run()
    return output

def test():
    results = flatten([1, [2, 3, [4]], 5, [[6]]])
    return (len(results) == 6 and
            results[0] == 1 and
            results[1] == 2 and
            results[2] == 3 and
            results[3] == 4 and
            results[4] == 5 and
            results[5] == 6)

expect(test_count, 'should return [1,2,3,4,5,6] output for [1, [2, 3, [4]], 5, [[6]]] input', test)

def test():
    results = flatten([])
    return len(results) == 0

expect(test_count, 'should return [] output for [] input', test)

def test():
    results = flatten([1, [2, 3, [4], []], [], 5, [[], [6]]])
    return (len(results) == 6 and
            results[0] == 1 and
            results[1] == 2 and
            results[2] == 3 and
            results[3] == 4 and
            results[4] == 5 and
            results[5] == 6)

expect(test_count, 'should return [1,2,3,4,5,6] output for [1, [2, 3, [4], []], [], 5, [[], [6]]] input (note the empty arrays)', test)

print('PASSED: ' + str(test_count[0]) + ' / ' + str(test_count[1]) + '\n\n')

Your code looks fine, however to improve it, you should use a proper test system like pytest or unittest. To demonstrate, here is your code when using pytest, and making the test proper (you don't need to test every specific item:

def flatten(input_array):
    result_array = []
    for element in input_array:
        if isinstance(element, int):
            result_array.append(element)
        elif isinstance(element, list):
            result_array += flatten(element)
    return result_array

def test01():
    results = flatten([1, [2, 3, [4]], 5, [[6]]])
    assert results == [1, 2, 3, 4, 5, 6]

def test02():
    results = flatten([1, [2, 3, [4], []], [], 5, [[], [6]]])
    assert results == [1, 2, 3, 4, 5, 6]

And here are the results:

C:\PycharmProjects\codereview\tests>pytest scratch_14.py
======================== test session starts ========================
platform win32 -- Python 3.7.0, pytest-3.6.2, py-1.5.4, pluggy-0.6.0
rootdir: C:\PycharmProjects\codereview\tests, inifile:
plugins: cov-2.5.1, celery-4.2.0
collected 2 items

scratch_14.py ..                                               [100%]

===================== 2 passed in 0.09 seconds ======================

This is much easier to set up, and less code to write to validate if the solution is correct.
You asked: Is usage of TypeError exception justified?
I don't actually see any code referencing a type error. Did you forget to put it in? Or are you referring to the use of isinstance? If so, that code is fine.
Hope this helps!


Reversing a singly-linked List Is this the best way to reverse a singly-linked list? Can it be done with two, or fewer pointers? Any other comments?

public class ReverseLL {
    Node start;
    ReverseLL()
    {
        start=null;
    }
    class Node
    {
        Node next;
        int data;

        Node(int newData)
        {
            next=null;
            data=newData;
        }
        public void setData(int newData)
        {
            data=newData;
        }
        public int getData()
        {
            return data;
        }
        public void setNext(Node n)
        {
            next=n;
        }
        public Node getNext()
        {
            return next;
        }

    }
    public void insert(int newData)
    {
        Node p=new Node(newData);
        if(start==null)
        {
            start=p;
        }
        else
        {
            Node temp=start;
            while(temp.getNext()!=null)
            {
                temp=temp.getNext();
            }
            temp.setNext(p);
        }
    }

    public void reverse()
    {
        Node temp=start;
        Node previous=null;
        Node previous1=null;
        while(temp.getNext()!=null)
        {
            if(temp==start)
            {
                previous=temp;
                temp=temp.getNext();
                previous.setNext(null);
            }
            else
            {
                previous1=temp;
                temp=temp.getNext();
                previous1.setNext(previous);
                previous=previous1;
            }
        }
        temp.setNext(previous);
        start=temp;
    }
    public void display() {
        int count = 0;

        if(start == null) {
            System.out.println("\n List is empty !!");
        } else {
            Node temp = start;

            while(temp.getNext() != null) {
                System.out.println("count("+count+") , node value="+temp.getData());
                count++;
                temp = temp.getNext();
            }
            System.out.println("count("+count+") , node value="+temp.getData());
        }
    }
    public static void main(String args[])
    {
        ReverseLL ll=new ReverseLL();
        ll.insert(1);
        ll.insert(2);
        ll.insert(3);
        ll.insert(4);
        ll.insert(5);
        ll.insert(6);
        ll.insert(7);
        ll.insert(8);
        ll.display();
        System.out.println();
        ll.reverse();
        ll.display();
    }
}

This is a nice, clean implementation of a Linked list... Generally a good job.

You have a bug in your reverse method, a NullPointerException when the list is empty. There is an easy fix, but you should be aware.

I also had a look at your reverse method. I cannot see a way to do it with fewer than 3 variables, while still keeping the logic readable. I am not particularly fond of your implementation... The distinct if/else condition makes the internal logic cumbersome. It makes things easier if you consider the process to be closer to a swap... we want to swap the direction of the pointer between nodes.

So, the logic is, for three nodes A->B->C, we want to make B point to A, but, we have to remember that C comes after B before we reverse the pointer. Then we have to make C point to B, becoming A<-B<-C

But, we have a couple of loose ends (pun is intended)... we have the start pointer which points at A, and A is pointing at B still, So, we need to remove the now-redundant A->B pointer, and also move start to point at C..... All so complicated, but it boils down to a simple loop:

    public void reverse() {
        if (start == null) {
            return;
        }
        Node current = start;
        Node after = start.next;
        while (after != null) {
            Node tmp = after.next; // preserve what will come later.
            after.next = current;  // reverse the pointer
            current = after;       // advance the cursor
            after = tmp;           // the node after is the one preserved earlier.
        }
        start.next = null;         // null-out next on what was the start element
        start = current;           // move the start to what was the end.
    }

This, to me, is much more readable than the conditional logic you had. It does use three pointers in addition to the start.

If you want to, you can probably find a way to do it with one less pointer, but that is by hacking the start pointer and using it as a tracker in the loop (probably instead of current, but the readability, and simplicity will suffer if you do that.

Note also that Java coding convention puts the { open brace at the end of the line containing the conditional block.

Finally, at the risk of adding a little complexity to your code, most general-purpose Linked Lists in 'real' applications have an O (1) mechanism for getting the List size. If you have a custom purpose for the list where the size is not important, you can skip that, but, you should otherwise consider adding a size field so you can avoid doing a full iteration to get the size.

Another Finally, The Java Iterator concept is a very common idiom. It is surprisingly complicated though to get your implementation to match the specification. I strongly recommend that you take it upon yourself to make your List iterable, and to make sure your Iterator implementation conforms to the specification (especially the conditions under which the iterator throws exceptions).

I also extended your main method to do a few more tests than you have:

    public static void main(String args[]) {
        ReverseLL ll=new ReverseLL();
        ll.reverse();
        ll.display();
        System.out.println();

        ll.insert(1);
        ll.reverse();
        ll.display();
        System.out.println();

        ll.insert(2);

        ll.reverse();
        ll.display();
        System.out.println();

        ll.reverse();
        ll.display();
        System.out.println();

        ll.insert(3);
        ll.insert(4);
        ll.insert(5);
        ll.insert(6);
        ll.insert(7);
        ll.insert(8);
        ll.display();
        System.out.println();

        ll.reverse();
        ll.display();
        System.out.println();
    }


Implementing widgets within a main window I am learning PySide and am trying to implement widgets within a main window. My goal right now is very simple: I want to place a QLabel, with independently defined event handling, inside a main window that has its own event handling:

# -*- coding: utf-8 -*-
from PySide import QtGui, QtCore

class MainWindow(QtGui.QWidget):
    '''Main window lets user know when window was clicked on non-widget space'''
    def __init__(self):
        QtGui.QWidget.__init__(self)
        self.initUI()
    def initUI(self):
        self.myLabel=TalkingQLabel("Don't mess with Breakfast!", self)
        self.myLabel.setGeometry(50, 50, 200, 120)
        self.setGeometry(300,200,300,250)
        self.show()
    def mousePressEvent(self, event):
        print "Well, you pressed in main window!"

class TalkingQLabel(QtGui.QLabel):
    '''QLab el that indicates when it was pressed'''
    def __init__(self, txt, parent):
        QtGui.QLabel.__init__(self, txt, parent)
        self.initUI()
    def initUI(self):
        self.setAlignment(QtCore.Qt.AlignCenter)
        self.setStyleSheet("QLabel { color: rgb(255, 255, 0); font-size: 15px; background-color: rgb(0,0,0)}")
    def mousePressEvent(self, event):
        print "This time you pressed in a label"

def main():
    import sys
    qtApp=QtGui.QApplication(sys.argv)
    myTalker=MainWindow()
    sys.exit(qtApp.exec_())

if __name__=="__main__":
    main()

Was it good practice to have two separate classes like this for Qt programming, or should I have defined TalkingQLabel within MainWindow because they are in a parent-child relationship, and always will be in this application? Is there a standard within the Qt community I should know about, especially in cases when one widget is a child of another?

Related pages:

https://stackoverflow.com/questions/330/should-i-use-nested-classes-in-this-case
Nested class or two separate classes?
https://stackoverflow.com/questions/1581931/should-i-refactor-static-nested-classes-in-java-into-separate-classes
https://softwareengineering.stackexchange.com/questions/171790/are-nested-classes-under-rated

While there is no hard and fast rule, it seems to make code more readable and modular (i.e., Pythonic) to have a separate class for something that is effectively a new Qt widget. This is especially true if there is any chance that new widget might be something you want to break out and use in another project.

See the example labelledwidgets.py from Chapter 11 of Summerfield's book.


Demonstration of inheritance with some shapes I had a job interview take home test where I was asked a series of questions on object inheritance. In addition to answering the questions, I created the following application to demonstrate inheritance among different objects.

The first piece of code are different shape classes. Some inherit from an IShape interface.

The second piece of code is a console application that uses these classes in different inheritance scenarios. The console.write tells the use case.

I would like feedback if my implementation looks correct.

Class definitions

using System;
using OOP.Interfaces;

namespace OOP.Models
{
    public class Circle : IShape
    {
        public void Draw()
        {
            Console.WriteLine("Circle object's draw method\n");
        }
    }

    public class Rectangle : IShape
    {
        public void Draw()
        {
            Console.WriteLine("Rectangle object draw method\n");
        }
    }

    public class Square : Rectangle
    {
        public void Draw()
        {
            Console.WriteLine("Square object draw method\n");
        }
    }

}

Console App

using System;
using OOP.Interfaces;
using OOP.Models;

namespace OOP_Calling_Client
{
  public class Program
  {
    static void Main(string[] args)
    {
        Console.WriteLine("This is a demo console app that shows examples of OOP inheritance through a series of shape classes and interfaces.");
        Console.WriteLine("*********************************\n");

        DrawCircle();
        DrawRectangle();
        DrawSquare();
        AssignDifferentTypesToSameInstanceVariable();
        Console.Read();
    }

    private static void DrawCircle()
    {
        IShape circle = new Circle();
        circle.Draw();
        Console.WriteLine("Object of type circle stored in a variable of type IShape\n");
        Console.WriteLine("*********************************\n");
    }

    private static void DrawRectangle()
    {
        Console.WriteLine("Object of type rectangle stored in a variable of type rectangle\n");
        Rectangle rectangle = new Rectangle();
        rectangle.Draw();
        Console.WriteLine("*********************************\n");
    }

    private static void DrawSquare()
    {
        Console.WriteLine("Object of type square stored in a variable of type rectangle. Rectangle is derived from square\n");
        Rectangle rectangle = new Square();
        rectangle.Draw();
        Console.WriteLine("*********************************\n");
    }

    private static void AssignDifferentTypesToSameInstanceVariable()
    {
        Console.WriteLine("Instance variable holds a type of circle, and then a type of rectangle. The draw methods change\n");
        IShape shape = new Circle();
        Console.WriteLine("The object variable is of type IShape,\nand is holding an instance of a Circle.\n");
        shape.Draw();
        Console.WriteLine("The object variable is of type IShape,\nbut is now holding an instance of a Rectangle.\n");
        shape = new Rectangle();
        shape.Draw();
    }
  }
}

Shadowing

There is some shadowing going on between Rectangle and Square.

Rectangle implements IShape, and Square inherits from Rectangle.

As it stands, Square is hiding Rectangles Draw method. This is evident from the following compiler warning:

  Warning   1   'ConsoleApplication1.Square.Draw()' hides inherited member
  'ConsoleApplication1.Rectangle.Draw()'. Use the new keyword if hiding
  was
  intended.

This can pose a problem in the following scenario:

  Square s = new Square();
  s.Draw(); //Square method called

  Rectangle r = new Square();
  r.Draw(); //Rectangle method called!

As you see, this breaks inheritance and will only call the method on Square if the variable is a Square.

Instead, you might want to make the method virtual on Rectangle:

public class Rectangle : IShape
{
    public virtual void Draw()
    {
        Console.WriteLine("Rectangle object draw method\n");
    }
}

public class Square : Rectangle
{
    public override void Draw()
    {
        Console.WriteLine("Square object draw method\n");
    }
}

This will ensure that the correct method is always called.

Line breaks

There is no need to add \n when calling Console.WriteLine. It will add a line break automatically.

Moreover, \n is not the correct method of adding a line break in .NET. Use Environment.NewLine instead.

Misc.

The order of the final print in your example is wrong. It prints before actually assigning shape = new Rectangle().


Conditionally set strings if files exist How can I reduce the lines I use with this code block?

if ((test.contains("1.jpg") || test.contains("1.png")) && (maxP == false)){
        page++;
        add = 1;
        if (QFile().exists(basepath + spage + ".jpg")){
            p_left = basepath + spage + ".jpg";
        }
        else if(QFile().exists(basepath + spage+ ".png")){
            p_left = basepath + spage + ".png";
        }
        if (QFile().exists(basepath + QString::number(page + 1) + ".jpg")){
            p_right = basepath + inc + ".jpg";
        }
        else if(QFile().exists(basepath + QString::number(page + 1) + ".png")){
            p_right = basepath + inc + ".png";
        }
    }
    else if ((test.contains("1.jpg") || test.contains("1.png")) && (maxP == true)){
        gpage = max;
        add = 1;
        if (QFile().exists(basepath + smax + ".jpg")){
            p_left = basepath + smax + ".jpg";
        }
        else if(QFile().exists(basepath + smax + ".png")){
            p_left = basepath + smax + ".png";
        }
        if (QFile().exists(basepath + QString::number(page + 1) + ".jpg")){
            p_right = basepath + sinc + ".jpg";
        }
        else if(QFile().exists(basepath + QString::number(page + 1) + ".png")){
            p_right = basepath + sinc + ".png";
        }
    }

I see a number of things that may help you improve your code.

Use named constants

There are a number of constant strings which are used repeatedly.  If they're used more than once, it's probably a sign that you should use named constants instead.

constexpr static char[] JPG_EXT{".jpg"};
constexpr static char[] PNG_EXT{".png"};

Consolidate tests in compound if statements

Right now the code is basically this:

if ((test.contains("1.jpg") || test.contains("1.png")) && (maxP == false)){
    // do things
}
else if ((test.contains("1.jpg") || test.contains("1.png")) && (maxP == true)){
    // do other things
}

This can be simplified like this:

if ((test.contains("1.jpg") || test.contains("1.png")) {
    if (maxP) {
        // do other things
    } else {  // maxP must be false
        // do things
    }
}

Consolidate code where only the data changes

The code currently has repeated patterns like this:

if (QFile().exists(basepath + smax + ".jpg")){
    p_left = basepath + smax + ".jpg";
}

It then repeats, but with slightly different data.  That suggests further consolidation:

if ((test.contains("1.jpg") || test.contains("1.png")) {
    QString leftbase, rightbase;
    if (maxP) {
        gpage = max;
        leftbase = basepath + smax;
        rightbase = basepath + sinc;
    } else {
        page++;
        leftbase = basepath + spage;
        rightbase = basepath + inc;
    }
    add = 1;
    if (QFile(leftbase + JPG_EXT).exists()) {
        p_left = leftbase + JPG_EXT;
    } else if (QFile(leftbase + PNG_EXT).exists()) {
        p_left = leftbase + PNG_EXT;
    }
    if(QFile(basepath + QString::number(page + 1) + JPG_EXT).exists()){
        p_right = rightbase + JPG_EXT;
    } else if(QFile(basepath + QString::number(page + 1) + PNG_EXT).exists(){
        p_right = rightbase + PNG_EXT;
    }
}

Use a function for repeated actions

If you find yourself writing similar code multiple times, it might be a sign that you could use a function.  In this code, I'd write this:

void setIfJpgOrPngExists(const QString& querybase, const QString& answerbase,
        QString &target)
{
    if (QFile(querybase + JPG_EXT).exists()) {
        target = answerbase + JPG_EXT;
    } else if (QFile(querybase + PNG_EXT).exists()) {
        target = answerbase + PNG_EXT;
    }
}

if ((test.contains("1.jpg") || test.contains("1.png")) {
    QString leftbase, rightbase;
    if (maxP) {
        gpage = max;
        leftbase = basepath + smax;
        rightbase = basepath + sinc;
    } else {
        page++;
        leftbase = basepath + spage;
        rightbase = basepath + inc;
    }
    add = 1;
    setIfJpgOrPngExists(leftbase, leftbase, p_left);
    setIfJpgOrPngExists(basepath + QString::number(page + 1), rightbase, p_right);
}


Performance of mask (1,2,4,8,16) convertor in python My mask translator is performing poorly in scenario where I have 30 masks and 10,000 ownerships. This is a bit field and similar to bit masks such as enum.IntFlag.

I though about creating a list with all combinations of masked values and names (1 - alex, 2 - ben, 3 - alex/ben, 4 - clark, 5,... sum from 1 to 30 = 465) and then doing a simple lookup rather than evaluating the function for each ownership item. What do you think about this approach?

def findbits(num,object):
  for i in range(128):
      if num & 1 << i:
          y = "{0}".format(i + 1)
          output.append([names[int(y)-1], object])

output = []
map = [['1','alex'],['2','ben'],['4','clark'],['8','dean'],['16','eric']]
mask = [row[0] for row in map] #['1', '2', '4', '8', '16']
names = [row[1] for row in map] #['alex', 'ben', 'clark', 'dean', 'eric']
ownership = [['radio','3'],['telly','31']]

for item in ownership:
  findbits(int(item[1]),item[0])

print(output)
#[['alex', 'radio'], ['ben', 'radio'], ['alex', 'telly'], ['ben', 'telly'], ['clark', 'telly'], ['dean', 'telly'], ['eric', 'telly']]

In a nutshell: I have to find which names own which objects, that is to decompose masked number for each object (e.g. radio: 31 = 16 + 8 + 4 + 2 + 1) and look up values (names) for each sub-number (16 - eric, 8 - dean, ...).

And a large scale example:

import random
import string

def get_ownerships(nbr):
    for i in range(nbr):
        objects = ''.join(random.choice(string.ascii_letters) for m in range(4))
        ownership.append([objects,mask[i]])
    return ownership

def findbits(num,object):
  for i in range(int(num**0.5)+1): #roundup to nearest integer; this is crucia for performance. Shall be dependent on num,
  #precisely the position of the highest subnumer. If num = 17, then 17 = 16 + 1 where 16 is 5th element (1 - 2 - 4 - 8 - 16),
  #hence 5 elements = roundup(17^0.5). Could be improved though
      if num & 1 << i:
          y = "{0}".format(i + 1)
          output.append([names[int(y)-1], object])

ownership = []
mask = [random.randrange(1, 32767) for _ in range(1000)]
get_ownerships(1000)
output = []
map = [['1','al'],['2','be'],['4','cl'],['8','de'],['16','er'],['32','fa'],['64','ga'],['128','ha'],['256','ia'],['512','ja'],['1024','ka'],['2048','la'],['4096','ma'],['8192','na'],['16384','oa']]
mask = [row[0] for row in map] #['1', '2', '4', '8', '16']
names = [row[1] for row in map] #['alex', 'ben', 'clark', 'dean', 'eric']

for item in ownership:
  findbits(int(item[1]),item[0])

Code block 1

Please don't use variable names like object, i or y.
Standard indentation in Python is 4 spaces, not a mix between 2 and 4.
Mutating values out of a functions scope isn't best practice. This is because it leads to confusing code.
The following snippet is not needed. You can just use names[i].

y = "{0}".format(i + 1)
names[int(y)-1]

Rather than findbits I would make a function find_names. Which will return the names that can use the object.
Your bit mask, num & 1 << i, is good. But you can change it to use while num: to allow you to not waste any loops. Since the input you've provided us only goes to 1 << 4, there's not much point in looping all the way to 128. And so we can bitshift the other way.

def find_names(number, names):
    index = 0
    while number:
        if number & 1:
            yield names[index][1]
        number >>= 1
        index += 1

def main(map, ownership):
    output = []
    for obj, number in ownership:
        for name in find_names(int(number), map):
            output.append([name, obj])
    return output

if __name__ == '__main__':
    print(main(
        [['1','alex'],['2','ben'],['4','clark'],['8','dean'],['16','eric']],
        [['radio','3'],['telly','31']],
    ))

Both my solution and your solution are similar, however my solution bitshifts number downwards, where yours shifts i upwards. This means that you're constantly making 1 << i larger and comparing with number. This means on the first iteration 1 << i is \$1\$, in binary form, where on the third iteration it's \$100\$, in binary form.

Mine however works the opposite way checking the least significant bit, with number & 1. And so on the first iteration number would be something like \$10001001\$. Where on the third iteration it would be \$100010\$. This is as >> discards the least significant bit.

I find the following table to highlight the increase and decrease in size of the numbers to help. It's apparent that yours grows to match the bitlength of the original number, where mine shrinks until it reaches zero. The red highlights the bit that we're comparing with on each iteration.

$$
\begin{array}{l l r l}
\text{iteration} & \text{matching bit}   & \text{Yours - 1 << i} & \text{Mine - number} \\
0                & 1000100\color{red}{1} &        \color{red}{1} & 1000100\color{red}{1}\\
1                & 100010\color{red}{0}1 &       \color{red}{1}0 & 100010\color{red}{0} \\
2                & 10001\color{red}{0}01 &      \color{red}{1}00 & 10001\color{red}{0}  \\
3                & 1000\color{red}{1}001 &     \color{red}{1}000 & 1000\color{red}{1}   \\
4                & 100\color{red}{0}1001 &    \color{red}{1}0000 & 100\color{red}{0}    \\
5                & 10\color{red}{0}01001 &   \color{red}{1}00000 & 10\color{red}{0}     \\
6                & 1\color{red}{0}001001 &  \color{red}{1}000000 & 1\color{red}{0}      \\
7                & \color{red}{1}0001001 & \color{red}{1}0000000 & \color{red}{1}       \\
\end{array}
$$

Code block 2

We can transfer a lot of the changes from the above code block to the second.
I would split the objects comprehension over multiple lines, as I find it helps with readability.
The variable m is never used and so you can use the name _ to denote that it is never used.
I would move the mask iterator into get_ownerships and just generate one in the for. This allows you to generate both the name and the mask with ease.

import random
import string

def generate_ownerships(amount, limit):
    for _ in range(amount):
        object_name = ''.join(
            random.choice(string.ascii_letters)
            for _ in range(4)
        )
        mask = random.randrange(1, limit)
        yield [object_name, mask]

def find_names(number, names):
    index = 0
    while number:
        if number & 1:
            yield names[index][1]
        number >>= 1
        index += 1

def main(map, ownership):
    output = []
    for obj, number in ownership:
        for name in find_names(int(number), map):
            output.append([name, obj])
    return output

if __name__ == '__main__':
    main(
        [['1','al'],['2','be'],['4','cl'],['8','de'],['16','er'],['32','fa'],['64','ga'],['128','ha'],['256','ia'],['512','ja'],['1024','ka'],['2048','la'],['4096','ma'],['8192','na'],['16384','oa']],
        generate_ownerships(1000, 32767),
    )

Further comments

The first value in names is never used. As only names[index][1] is used. This means you can change your input so that you only specify the names.

If you would still like to specify the value in any order, [['2', 'be'], ['1', 'al']], then you can normalize the names. This requires ordering the names by the first value.
Since this is tagged performance you may be able to get a performance increase if you use an LRU cache from functools. This will store the \$n\$ most recently used values to a function so you don't need to regenerate the output.

This would require a small change to find_names to return an iterable rather than an iterator, and to only take hashables. For best performance it would only take number.

It should be noted that this may not lead to a performance gain. The amount of performance gain you get may also change depending on the size of \$n\$ and so you should test the performance on your data with different values.

If you have both space and time constraints then you can utilize the generalized birthday problem to find a rough number to limit the cache to. As it's 50% chance for hits at a fraction the size of the maximum input is a good way to save on both. If this is needed then I will leave it to you to implement.

import functools

def normalize_names(names):
    return [name for _, name in sorted(names, key=lambda i: int(i[0]))]

@functools.lru_cache(maxsize=None)
def find_names(number):
    output = ()
    index = 0
    while number:
        if number & 1:
            output += (index,)
        number >>= 1
        index += 1
    return output

def main(map, ownership):
    output = []
    for obj, number in ownership:
        for index in find_names(int(number)):
            output.append([map[index], obj])
    return output

if __name__ == '__main__':
    print(main(
        normalize_names(
            [['2','ben'],['1','alex'],['4','clark'],['8','dean'],['16','eric']],
        ),
        [['radio','3'],['telly','31']],
    ))


Am I multiplying two matrices together correctly? I just wanted to know if I'm doing this right.  Note that all the matrices are int[16]s.

for (int i= 1; i <= 3; i++){
    for (int j= 1; j <= 3; j++){
        int sum = 0;
        for (int h = 1; h <= 3; h++){
           sum = sum + position[i*4+h]*projection[h*4+j];
        }

        modelview[i*4+j] = sum;
    }
}

No. Let's first look at it as two dimensional arrays, so we know what we're talking about:

int i[4][4];
int j[4][4];
int k[4][4];

for (int x = 0; x < 4; x++) { // row number of output
    for (int y = 0; y < 4; y++) { // column number of output
        k[x][y] = 0;
        for (int z = 0; z < 4; z++) { // four elements are added for this output
            k[x][y] += i[x][z] * j[z][y];
        }
    }
}

Important difference!!! We start at 0 for the indices!

Now I'll convert it to int[16]s for you assuming that i[1] is the second element of the first row:

int i[16];
int j[16];
int k[16];

for (int x = 0; x < 4; x++) { // row number of output
    for (int y = 0; y < 4; y++) { // column number of output
        k[4*x+y] = 0;
        for (int z = 0; z < 4; z++) { // four elements are added for this output
            k[4*x+y] += i[4*x+z] * j[4*z+y];
        }
    }
}

This is equivalent to your code except for the fact that you start at 1 instead of zero.


Brute-force password cracker I am trying to write a program to find a zip file's password. I know the password consists of only lower-case letters and it's length does not exceed 6 characters. I wanted to check the passwords of length 1 first, then length 2 and so on.

So I used breadth first search, but then I realized that BFS consumes so much memory, and std::queue makes things even worse. So I had to switch to depth first search. BUT... depth first search does not check the shortest passwords first. So that's a problem. How can I improve the memory management in my bfs function, Or change my dfs function so that it would check the shortest passwords first (I'm not sure if the latter is even possible)

Also my estimation is that the bfs function would be using about 2 Gigabytes of memory. How can I fix this?

#include <iostream>
#include <queue>

std::string key = "banana";

void checkPassword(std::string s) {
  if (s == key) { //example checker function
    std::cout << "Password is : " << s << std::endl;
  }
}

void bfs(std::string s = "") {
  std::queue <std::string> q;
  q.push(s);
  while(q.size()) {
    std::string u = q.front();
    checkPassword(u);
    q.pop();
    if (u.size() < 6) {
      for (int i = 'a'; i <= 'z'; i++) {
        u += i;
        q.push(u);
        u.pop_back();
      }
    }
  }
}

void dfs(std::string s = "") {
  checkPassword(s);
  if (s.size() < 6) {
    for (int i = 'a'; i <= 'z'; i++) {
      s.push_back(i);
      dfs(s);
      s.pop_back();
    }
  }
}

int main() {
  dfs();
  std::cout << "Finished Time : " << clock() / (double) CLOCKS_PER_SEC;
}

There are some things I've seen that may help you improve your code.

Pass references where practical

The dfs() routine doesn't really need to create a duplicate string.  Instead, it could simply reuse a passed string.  We do that by changing the prototype to this:

void dfs(std::string &s)

And then call it like this:

int main() {
    std::string s;
    dfs(s);
    std::cout << "Finished Time : " << clock() / (double) CLOCKS_PER_SEC;
}

On my machine, the original code takes 6.3 seconds, but when modified like this, takes 4.7 instead, making a very easy improvement.

Prefer iterations to recursion

Recursive functions are often a good way to approach a programming task, but there is a tradeoff in terms of memory and time.  Specifically, one can often reduce or eliminate the computational cost of a function call and reduce or eliminate the memory overhead as well by converting from a recursive to an interative function.  In this case, I'd advise altering the code so that the prototype is something like this:

bool brute(const std::string &alphabet, size_t len)

It's then a simple matter of calling the function with increasing sizes until either the password is found or you run out of combinations.  For generating the combinations, I'd suggest using std::next_permutation something like this SO question shows.

Look at similar questions here

Another nice source of advice is other similar questions.  For example, this question is very similar and the answers are generally applicable to your code.


Python program to find Armstrong numbers in a certain range I have written a Python program to check for Armstrong numbers in a certain range.

An Armstrong number of three digits is an integer such that the sum of the cubes of its digits is equal to the number itself. For example, 371 is an Armstrong number since 3**3 + 7**3 + 1**3 = 371.

Here is my code:

lower = int(input("Enter lower range: "))
upper = int(input("Enter upper range: "))

for num in range(lower, upper + 1):

   order = len(str(num))

   sum = 0

   temp = num
   while temp > 0:
       digit = temp % 10
       sum += digit ** order
       temp //= 10

   if num == sum:
       print(num)

Here is an example output:

Enter lower range: 200
Enter upper range: 5000

370
371
407
1634

So, I would like to know whether I could make this program shorter and more efficient.

Any help would be highly appreciated.

Names

Using sum as a variable name is not adviced as it hides the sum builtin. I'd suggest sum_pow as an alternative.

Also, temp does not convey much information. I'd use remaining even though I am not fully convinced.

Extracting digits from a number

You've used divisions and modulo to compute the different digits for a number. You can use divmod which is a pretty unknown builtin which returns the result for both operations in one go.

That would give:

   remaining = num
   while remaining:
       remaining, digit = divmod(remaining, 10)
       sum_pow += digit ** order

Also, a better alternative would be to avoid doing this ourselves: extracting the different digits is pretty much what str does for us. Also, we already call it anyway, we could make the most out of it and reuse the result.

   num_str = str(num)
   order = len(num_str)
   sum_pow = 0
   for digit in num_str:
       sum_pow += int(digit) ** order

More builtins

Disclaimer: next comment may be a bit overwhelming for a beginner. Don't worry, just take your time and read documentation online if need be.

We've already delegated most of the hard work to Python builtins but we can go further. The summing part could be handled by the sum builtin along with generator expressions.

for num in range(lower, upper + 1):
   num_str = str(num)
   order = len(num_str)
   sum_pow = sum(int(digit) ** order for digit in num_str)
   if num == sum_pow:
       print(num)

Going further

A few other things could be improved from an organisation point of view:

split the code in functions
use the if name == "main" check

Going even further, we could write unit tests for the logic.


Shell script to count chess game outcomes I came across this blog post by Adam Drake from around a year ago which is now making the rounds again.

I made some improvements to his code, but wish to see if there are additional tweaks that could be made to make it run even faster.

The task is to extract chess game results from PGN files.  The files contain sequences of games, where each has a header which contains a "Result" line like this:

[Result "1-0"]
[Result "0-1"]
[Result "1/2-1/2"]

These three results indicate a white win, a black win, and a draw, respectively.  The task is to simply collect and report a summary of these results.

Here is my solution to be reviewed:

find . -type f -name '*.pgn' -print0 |
 xargs -0 mawk -F '[-"]' '/Result/ { ++a[$2]; }
   END { print a["1"]+a["0"]+a["1/2"], a["1"], a["0"], a["1/2"] }'

I was skeptical of using find over just listing the files in the reference data set, but my timings indicate that this is actually faster than a shell wildcard (Bash 4.3.11(1)-release).

tripleee@xvbvntv:ChessData$ time find . -type f a-name '*.pgn' | wc -l
3025

real    0m0.014s
user    0m0.008s
sys     0m0.011s

tripleee@xvbvntv:ChessData$ time printf '%s\n' */*.pgn | wc -l
3025

real    0m0.037s
user    0m0.032s
sys     0m0.010s

The optimization I originally had in mind was to close the data file after reading the Result line, but as it turns out, the reference data set files contain multiple games, and thus multiple results (and the game portion is a lot smaller than I thought it would be).

tripleee@xvbvntv:ChessData$ time find . -type f -name '*.pgn' -print0 |
> xargs -0 mawk -F '[-"]' '/Result/ { ++a[$2]; }
>   END { print a["1"]+a["0"]+a["1/2"], a["1"], a["0"], a["1/2"] }'
6829065 2602614 1974505 2251946

real    0m50.232s
user    0m19.820s
sys     0m2.542s

This is as far as I got.  (An earlier version, based on the blog post, attempted parallel processing, but removing that was the biggest performance improvement I made.) I don't think switching from Awk to a "bigger" language would buy me any serious benefits -- one of the strengths of Awk is that it's quick to write, parse, and execute. (Compiled code would probably be a tad faster, but I don't think I want to go there; let's try to keep a realistic cost/benefit ratio.)  Are there any additional improvements to be made, to make it go even faster?

Here is the data set referenced in the blog, which I used to obtain my results.

(Another, slightly smaller data set is available from here.)

Sadly, the Hadoop experiment which is linked from Adam's blog is now a 404.  He quotes the original author as clocking 26 minutes to process the (smaller?) data set on seven c1.medium instances.  His own code took 12 seconds, but I was unable to reproduce that -- with this data set, it took 2 minutes and 30 seconds on my computer, so I have improved over that by some 60%.

I don't see the reason for chaining with -print0 | xargs -0.
It's better and simpler to use -exec:

find . -type f -name '*.pgn' -exec mawk -F '[-"]' '...' {} +

I don't see a way to make the AWK code faster,
but:

Some of the double-quoting is unnecessary
I would add a space around operators for somewhat better readability
A semicolon can be dropped

Like this:

/Result/ { ++a[$2] }
   END { print a[1] + a[0] + a["1/2"], a[1], a[0], a["1/2"] }


Passing an object to the view in CodeIgniter I'm trying to use an object and persist it into the data base.

I created a class in the library's folder.

<?php
if (!defined('BASEPATH')) exit('No direct script access allowed');

class Solicitud {

    private $categoria;
    private $consecutivo;
    private $id;
    private $mensaje;
    private $solicitante;

    function __construct(){
        $this->categoria = 1;
        $this->consecutivo = 1;
        $this->mensaje = "Mensaje por defecto";
    }

    public function escalarSolicitud( User $actual, User $nuevo){
        $this->setSolicitante($nuevo);
    }

    # Getters and Setters

    public function setCategoria( $categoria ){
        $this->categoria = $categoria;
    }

    public function getCategoria(){
        return $this->categoria;
    }

Here is the Controller:

<?php

if (!defined('BASEPATH')) exit('No direct script access allowed');

class Administrador extends Ci_Controller {

    function __construct(){
        parent::__construct();
    }

    public function crearSolicitud(){

        $session_data = $this->session->userdata('session_user');
        $usuario      = $session_data['usuario'];
        $categoria    = $this->input->post('category');
        $mensaje      = $this->input->post('description');

        $solicitud = new solicitud;

        $solicitud->setSolicitante($user);
        $solicitud->setCategoria($categoria);
        $solicitud->setMensaje($mensaje);

        $this->helpdesk_model->createSupport( $solicitud );
    }

And in the Model how can I use the object properties:

public function createSupport( $soporte ){

    $this->db->trans_start();

        $data = array(
            'id' => null,
            'id_user' => $soporte[usuario], ???
            'id_category' => $soporte[categoria], ???
            'message' => $soporte[mensaje] ???
        );

        $this->db->insert('supports', $data);

    $this->db->trans_complete();

    return $this->db->trans_status();

}

I need to know if I'm in the right direction or if the use of the 'Solicitud' class is not necessary.  If I'm right, how should I call in the model the object attributes?

I would like to add something to @Dukeatcoding comment in the question.  What is the point of passing the whole object? Thinking on it as a part of the main controller, you should only pass the data as an array to the model:

$data = array(
    'id_user'     => $usuario,
    'id_category' => $soporte->getCategoria
    'message'     => $soporte->getMensaje
)
$this->helpdesk_model->createSupport( $solicitud );

Although, as you are getting all these properties so often, what about a general getter in your soporte class?

public function getValue( $var ){
    return $this->{$var};
}

And you'd have something like:

$data = array(
    'id_user'     => $soporte->getVar( 'usuario'),
    'id_category' => $soporte->getVar( 'categoria'),
    'message'     => $soporte->getVar( 'mensaje' )
)

From your code, I would take care of the validation of your values, as it looks like you don't validate if they're numeric, if they exists on your DDBB, etc. Maybe you're doing it, because I suppose the code you posted is just a excerpt (due to some method are missed in Solicitud, as setSolicitante($nuevo) ), but I prefer to write it, just in case.

Also, I won't assign to a var the values you take directly, just call them an take the value:

//Instead of:
$session_data = $this->session->userdata('session_user');
$usuario      = $session_data['usuario'];
// Better
$usuario      = $this->session->userdata('session_user');

Off topic, you could check Doctrine, http://www.doctrine-project.org/projects/orm.html, is an ORM that maybe could help you, and it has its own CI recipe:
http://docs.doctrine-project.org/en/2.0.x/cookbook/integrating-with-codeigniter.html

Have fun!


Function to get rows from database I have a function that returns rows from my database, and it works fine, but I was told that it was poorly written, and there is a lot of unnecessary code in it. I'm wondering if there is a simpler, easier way to rewrite this function.

function gd($query) {

    global $connect;
    $fetch = mysqli_query($connect, $query);
    $count = mysqli_num_rows($fetch);

    while ($row=mysqli_fetch_array($fetch,MYSQLI_NUM)) {

        $count --;
        $arrayCount = count($myArray);

        $tempArrayCount = 0;
        while($tempArrayCount < $arrayCount){
            $array[$count][$tempArrayCount]= $row[$tempArrayCount];
            $tempArrayCount++;
        } //end of while($tempArrayCount < $arrayCount)

    } //end of while ($row=mysqli_fetch_array($fetch,MYSQLI_NUM))

    return $array;

} //end of function gd($query)

Your code will work, of course, but you go to a lot of trouble to grind the result set into an array when that capability is already available. Mysqli would provide the fetch_all() function if you had the mysqlnd drivers available; fortunately, PDO has provided it since PHP 5.1.

You will need to create a PDO connection with

$dsn = "mysql:host=" . $hostname . ";dbname=" . $dbname . ";";
$dbh = new PDO($dsn, $dbuser, $dbpass);

then your function would be

function gd($query)
{
    global $dbh;
    $result = $dbh->query($query);
    $array = $result->fetchAll(PDO::FETCH_NUM);  // fetchAll() does the same as most of your original code
    return $array;
}


Simple Caesar cipher cryptanalysis in Ruby I really like Ruby and want to get better at it. Any input is appreciated.

Description

This program prompts the user for a string which is ciphertext resulting from a Caesar cipher given (English) plaintext and a key on the range [0, 25]. The program then uses a table of character frequencies of the English language to perform simple cryptanalysis to determine the top 5 most probable shift keys, and displays the resulting plaintext when these keys are used to decrypt the ciphertext.

It should also be noted that the only input expected is letters and whitespace, but I think it works with most punctuation.

Code:

class Cryptanalysis
  def initialize
    @alphabet = ('A'..'Z').to_a.join

    @english_frequency = { :A => 0.080, :B => 0.015, :C => 0.030,
                           :D => 0.040, :E => 0.130, :F => 0.020,
                           :G => 0.015, :H => 0.060, :I => 0.065,
                           :J => 0.005, :K => 0.005, :L => 0.035,
                           :M => 0.030, :N => 0.070, :O => 0.080,
                           :P => 0.020, :Q => 0.002, :R => 0.065,
                           :S => 0.060, :T => 0.090, :U => 0.030,
                           :V => 0.010, :W => 0.015, :X => 0.005,
                           :Y => 0.020, :Z => 0.002 }
  end

  def decrypt_caesar(ciphertext, shift)
    i = shift % @alphabet.size
    decrypt = @alphabet
    encrypt = @alphabet[i..-1] + @alphabet[0...i]

    ciphertext.tr(encrypt, decrypt)
  end

  def analyze(ciphertext)
    chars = ciphertext.gsub(/[^A-Z]/i, '').split(//)
    ciphertext_frequency = character_frequency(chars)

    correlation_of_frequency = Hash.new

    alphabet = @alphabet.split(//)
    (0..25).each { |i|
      sum = 0.0
      alphabet.each { |c|
        e = alphabet.index(c)
        sum += ciphertext_frequency[c.to_sym] * @english_frequency[alphabet[(26 + e - i) % 26].to_sym]
      }
      correlation_of_frequency[i] = sum
    }

    correlation_of_frequency.sort_by { |_, v| -v }.take(5).to_h
  end

  def character_frequency(chars)
    frequency = Hash.new
    inc = 1.0 / chars.size

    @alphabet.each_char { |c| frequency[c.to_sym] = 0.0  }
    chars.each          { |c| frequency[c.to_sym] += inc }

    frequency
  end
end

crypt = Cryptanalysis.new

puts 'Enter a string to analyze'
text = gets.chomp.upcase
result = crypt.analyze(text)

puts 'Top 5 possible shifts: '
choice = 1

result.each do |k, v|
  puts '%10s) Shift amount: %2s Correlation: %%%.4f Result: %s' % [choice, k, v, crypt.decrypt_caesar(text, k)]
  choice += 1
end

Nice work.  You can get some easy improvements by:

Taking advantage of ruby's functional methods to eliminate temp variables and generally make the code more declarative
Use constants where appropriate
Make your code reflect your style by using static method on a module rather than a class -- you're not actually using the class for anything in the original code.  Alternatively, you can pass in a save the ciphertext in the class constructor, and then do something like crypt_instance.analyze.
Avoid "magic numbers" like 25 or 26.

Rewrite:

module Cryptanalysis
  ALPHABET = ('A'..'Z').to_a

  ENGLISH_FREQUENCY = { :A => 0.080, :B => 0.015, :C => 0.030,
                        :D => 0.040, :E => 0.130, :F => 0.020,
                        :G => 0.015, :H => 0.060, :I => 0.065,
                        :J => 0.005, :K => 0.005, :L => 0.035,
                        :M => 0.030, :N => 0.070, :O => 0.080,
                        :P => 0.020, :Q => 0.002, :R => 0.065,
                        :S => 0.060, :T => 0.090, :U => 0.030,
                        :V => 0.010, :W => 0.015, :X => 0.005,
                        :Y => 0.020, :Z => 0.002 }

  def self.decrypt_caesar(ciphertext, shift)
    decrypt = ALPHABET
    encrypt = ALPHABET.rotate(shift)
    ciphertext.tr(encrypt, decrypt)
  end

  def self.analyze(ciphertext)
    freqs = frequencies(clean(ciphertext))

    (0..ALPHABET.size)
    .map{|i| [i, score(freqs, i)]}
    .sort_by { |x| -x[1] }
    .take(5)
    .to_h
  end

  def self.clean(text)
    text.upcase.gsub(/[^A-Z]/i, '')
  end

  def self.score(freqs, shift)
    ENGLISH_FREQUENCY
    .values
    .rotate(shift)
    .zip(freqs.values)
    .map{|x| x.first * x.last}
    .reduce(:+)
  end

  def self.frequencies(text)
    ENGLISH_FREQUENCY
    .keys
    .map {|k| [k.to_sym, text.count(k.to_s) / text.size.to_f]}
    .to_h
  end
end

text = "the quick brown fox jumps over the lazy dog"
puts 'Top 5 possible shifts: '
puts Cryptanalysis.analyze(text)


Project Euler 378: Count ordered triplets whose triangular numbers have decreasing number of factors Project Euler problem 378 says:

  Let \$T(n)\$ be the \$n\$th triangle number, so \$T(n) = {n (n+1) \over 2}\$.

  Let \$\mathit{dT}(n)\$ be the number of divisors of \$T(n)\$.
  E.g.: \$T(7) = 28\$ and \$\mathit{dT}(7) = 6\$.

  Let \$\mathit{Tr}(n)\$ be the number of triples \$(i, j, k)\$ such that \$1 ≤ i < j < k ≤ n\$ and \$\mathit{dT}(i) > \mathit{dT}(j) > \mathit{dT}(k)\$.
  \$\mathit{Tr}(20) = 14\$, \$\mathit{Tr}(100) = 5772\$ and \$\mathit{Tr}(1000) = 11174776\$.

  Find \$\mathit{Tr}(60\,000\,000)\$.
  Give the last 18 digits of your answer.

This is my attempt at solving it:

def t(n):
    tri_num = (n * (n+1))//2
    return tri_num

#finding nth triangle numbers

def dt(n):
    count = 0
    for i in range(1,t(n)+1):
        if t(n)%i == 0:
            count += 1
    return count

#finding nth triangle numbers' total number of dividers

def factors(n):
    factors = []
    for i in range(1,t(n)+1):
        if t(n)%i == 0:
            factors.append(i)
    return factors
#finding nth triangle number's dividers

def tr(n):
    triplesnum = 0
    triples = [(i, j, k) for i in range(n) for j in range(n) for k in range(n) if 1 <= i < j < k <= n and dt(i) > dt(j) > dt(k)]
    for i in triples:
        triplesnum += 1
    return triplesnum
while True:
    n = int(input("N =???"))
    print("triples number",tr(n))

#solution

I am new to python; I only know at most the loops,list comprehension, functions, and some class.  I am sure this problem can be solved more simply.

Although I see this code as a success considering my knowledge, it works too slowly for me after 3-digit values of n.

Is there an improvement I can make with my current knowledge?  If not, what should I be learning about next?

Note: I know some of the code is not necessary, but I used them as a testing material in the further functions.

I am new to python; I only know at most the loops,list comprehension,
  functions, and some class. I am sure this problem can be solved more
  simply.

  ...

  Is there an improvement I can make with my current knowledge? If not,
  what should I be learning about next?

Loops and arrays are all you really need to solve almost any Project Euler challenge. The only exceptions are the ones which also require reading an input file. The point of PE is not to push your knowledge of language features, but to push your mathematical skills and knowledge of data structures and algorithms.

I would advise you to solve at least 40 of the first 50 challenges before you start looking at challenges beyond 100, because the early challenges introduce key techniques which you will use many times in later challenges. Trying to solve #378 before #12 is simply doing things in the wrong order.

The most important of the key techniques is dynamic programming. So many PE problems can only be solved in a reasonable time by exploiting shared structure. In this challenge, for example, you use the "number of divisors" function \$d(n)\$. The structure of this function can be summed up as $$\begin{align}
d(p^a) &= a+1&\qquad \textrm{when }p\textrm{ is prime} \\
d(mn) &=d(m)d(n)&\qquad \textrm{when }\gcd(m,n)=1
\end{align}$$
Exploiting that structure by adapting the sieve of Eratosphenes will allow you to calculate an array containing dt(n) for every \$1 \le n \le 60000000\$ in less than a minute.

Similarly, exploiting the structure of \$1 ≤ i < j < k ≤ n\$ and \$\mathit{dT}(i) > \mathit{dT}(j) > \mathit{dT}(k)\$ will allow you to reduce the current \$O(n^3)\$ runtime of that accumulation phase to \$O(nw)\$ where \$w = \max \{dT(i) : 1 \le i \le n\}\$. That's enough to solve the problem in about 40 minutes, using only basic data structures.

To solve the problem in under two minutes you will need an advanced data structure, but that's more than enough spoiler. Solve the problem at all and you'll get access to the forum thread where people explain their solutions.


PDO wrapper class for database I have my PDO Wrapper class and would love to get some more tips/opinions about what you think. This is my first class and I basically put my time into making it as logical and standardized as possible. I know I need to add comments.

<?php
/*
    COPYRIGHT © SIMPLEBB.NET
    CREATED BY: MikeM.
    CREATED IN: 2014
    COPYRIGHT © SIMPLEBB.NET
*/
class Database{
    private $db;

    public function __construct($config){
        $driver = strtolower($config['database']['driver']);
        $host = $config['database']['host'];
        $database = $config['database']['name'];
        $username = $config['database']['username'];
        $password = $config['database']['password'];

        $this->connect($driver, $host, $database, $username, $password);
    }

    private function connect($driver, $host, $database, $username, $password){
        try{
            $this->db = new PDO($driver.":host=".$host.";dbname=".$database, $username, $password);
            $this->db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
            $this->db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        }catch(PDOException $ex){
            $errorCode = $ex->getCode();
            $errorMessage = $ex->getMessage();
            $errorFile = $ex->getFile();
            $errorLine = $ex->getLine();
            include(dirname(dirname(__FILE__))."/error.php");
        }
    }

    public function close(){
        $this->db = null;
    }

    public function nonQuery($query){
        try{
            $run = $this->db->prepare($query);
            if(func_get_args() > 0){
                $args = func_get_args();
                array_shift($args);
                return $run->execute($args);
            }else{
                return $run->execute();
            }
        }catch(PDOException $ex){
            $errorCode = $ex->getCode();
            $errorMessage = $ex->getMessage();
            $errorFile = $ex->getFile();
            $errorLine = $ex->getLine();
            include(dirname(dirname(__FILE__))."/error.php");

        }
    }

    function query($query){
        try{
            $run = $this->db->prepare($query);
            if(func_get_args() > 0){
                $args = func_get_args();
                array_shift($args);
                $run->execute($args);
            }else{
                $run->execute();
            }
            return $run; // using: ->fetchall(PDO::FETCH_ASSOC); needs: print_r($db->query("SELECT * FROM `test2`"));
        }catch(PDOException $ex){
            $errorCode = $ex->getCode();
            $errorMessage = $ex->getMessage();
            $errorFile = $ex->getFile();
            $errorLine = $ex->getLine();
            include(dirname(dirname(__FILE__))."/error.php");
        }
    }
}
$db = new Database($config);
/*
    COPYRIGHT © SIMPLEBB.NET
    CREATED BY: MikeM.
    CREATED IN: 2014
    COPYRIGHT © SIMPLEBB.NET
*/
?>

I'd recommend to do:

$db = new Database($config);

only when you really need the database class, you don't want un-closed connections to be open. You can let this the same but make sure you manually do the connection when you need it!

Therefore this is globalized error logging. In some cases you do not want that so I'd recommend for such cases to destroy these try catches and actually do that when you call the function.

This review will consist of 2 parts:

  The design/concept
  The code itself

1: Code should solve problems

Every piece of code you write and will ever write should solve a problem. If it is not a solution to a problem you have. Throw it away. Eventually it will break, and then kittens will die!

To make sure we don't kill to many kittens, we have to write good, solid and simple code. Code that everyone understands, even 'me'-in-5-days-with-a-hangover. Some very smart geeks have even made a couple of rules: SOLID, KISS ...

Let's look at your class. It's name is Database. That tells me that your class represents a Database. In orde to function, it will probably need a DatabaseConnection, and maybe even a LoggerInterface (psr-3). But thats just a guess. It will probably help me in creating transactions, flushing the data, error-retrieval, ...

I am so wrong

The Database object needs 1 parameter. It's called $config, sadly, the documentation is simply missing. So I start analysing the code. Apparantly it needs $driver, $host, $database, $username, $password. So why not ask for it? Really. What is more user friendly:

$dbConnection = new Database(array('config'=>array('username'=>'',...)));

or

$dbConnection = new Database('username','password',...);

If you need something, ask for it.

So what problem are you trying to solve?

One problem could be the creation of DatabaseConnections. This could be solved using a DatabaseConnection Factory. Using the factory pattern that is.

Or maybe, you are trying to create some kind of DBAL? A class that uses a DatabaseConnection and performs queries.

Or maybe you wanted to create an error-logger/catcher (looking at all those try/catch)?

Different problems need differen solutions

Every class should solve 1 problem. At max.

2: Code should be stupid

If code doesnt read like normal language or poetry, it is bad code. If code does read like normal language, read again to check if it makes sence. an example:

class Cabin
{
    public function open() {
        //the cabin is now open
    }
}

Looks perfectly normal.

$myCabin = new Cabin();
$myCabin->open();
//we can now put stuff in the cabin

Read well, everyone knows whats going on. Now read it again. The actor is the acinb, the action is open.You open something, so what is the cabin opening?
Somehow, we wrote code where the Cabin is opening itself, magically. Better would be:

$cabinHandler->openCabin($cabin);
if ( $myCabin->isOpen() ) {
    //we can no put stuff in the cabin
}

A more real-world example:

$user->resetPassword();
$user->setResetPasswordMail();

Looks normal, right? But why does the User know how to send emails? Better:

$email = new Email($resetPasswordEmailContent)
$mailer->sendEmail($email, $user);

Now back to your Database class. Let's just say you went for the DBAL solution. We start with the __construct. What is the exact minimum requirement? a connection. We use pdo:

//name is not perfect
class Database
{
    public function __construct(PDO $dbh)
    {
        $this->dbh = $pdo;
    }
}

We also want to abstract away the preparing, and statement handling etc etc

/**
 * Query the database connection and return the result
 * @param  {String} $sql            [description]
 * @param  {Array} $values=array() [description]
 * @return {Array}                 [description]
 */
public function query($sql, $values=array())
{
    try {
        $sth = $this->dbh->prepare($sql);
        $sth->execute($values);
    } catch (PDOException $e) {
        $this->logger->critical($e->getMessage, array($sql, $values));
        throw new DatabaseException('An error occured performing a query');
    }

    return $sth->fetchAll();
}

So instead of failing miserably on an exception. We log our PDO exception (We use PDO so we can know about it). And then throw e new more generic error message. The code that then called us can choose 2 things on an error: 1) ignore or 2) catch it and try and fix the problem.

Eventually, if there is no caller up the chain that actually knows what to do with this Excpetion. The DatabaseException will be caught by your error_handler. This one will simply show a nice error page with some generic error message. For instance the google-monkeys.

Now that we have established how we should code. Let's look at how the SOLID rules could help us.

S : Single responsibility principle

Every piece of code should have one single responsibility. This goes for Objects and for methods. The class DatabaseHandler should only handle the Database - or act as an abstraction. Every method should only do one thing: perform a query, open a transaction, close a transaction. It shouldn't perform a query AND handle the exceptions AND render the error-view (include html stuff).

O : open/closed principle

Every module - read piece of code - should be open for extension but closed for modification. This means that we should be able to change the behaviour without changing the source code itself. Your Database constructs the dsn for the PDO object. But what if you want to use sqlite instead of mySQL? Your Database class does  not handle this case. So for changing the behaviour, we need to change your SourceCode.

  :(

That is my sad face

L : Liskov substitution principle

Not applicable here since you are not extending classes. But it is a nice principle to look into when you start building very large aplications.

I : Interface segregation principle

Only ask what you need. And not only for variables, but for methods aswell. In your construct you are asking for a $config array. By just looking at it I know there is more then just the $config['database'] in there. Why? The database should care less about other config stuff. It shouldn't get them. It shouldn't even know that you are passing config around in an array. The same applies when using other objects.

Your problem with the SQLite db can forinstance be solved with Typehinting for an interface instead of a class (PDO in your case). You are only using the prepare($query) method. So why ask for an entire PDO object? What if we want to change PDO with something different - you never know, PDO could be the biggest kitten killer out there. So create an interface and ask that:

interface DatabaseConnection
{
    /**
     * @param  {String} $query The query with placeholders
     * @return {StatementHandler}
     */
    public function prepare($query);
}

and then typehint for this DatabaseConnection:

class Database
{
    public function __construct(DatabaseConnection $db) {
        $this->db = $db;
    }
}

Now everyone who is going to use your code knows what a DatabaseConnection should look like. A very easy example is the following one:

class PdoDatabaseConnection extends PDO implements DatabaseConnection {}

And now everyone is happy. Even the guy who is writing the dreadfull MysqlDatabaseConnection with all it's mysql_query glory

D : Dependency inversion principle

Your connect() function is a good example of how NOT to do it. What happens when you construct your database object is the following:

1) get some parameters from an array into a local variable (why? i dont know)
2) pass those variables to the connect function
3) try to connect to the database with the config/credentials provided
4) if you fail, handle the error and show an error screen.

That's a lot. To much. Your abstract 'Database' class that should have abstracted the hard PDO object, know completly depends on that PDO object. It even knows how to create it. While all the class needs it some object where he can call ->prepare() and then get a Statement back that he can ->execute() and ->fetchAll().

Everytime you find yourself writing new inside a class. And the class doesn't end witht he word Factory. Stop what you are doing and ask for an interface instead.

There are ofcourse exceptions. For instance PDO. The DatabaseConnection interface will be overkill in most real world examples.

Know we have established the SOLID rules, there still remains DRY: Don't Repeat Yourself. Your Database class has 5 methods, 70 lines of code. The 3 biggest methods have 23 lines of combined unique lnes, and the same amount of duplicated code (try catch stuff). That is WET code. And we all know what happens when you leave things wet for to long, fungus, smell, dead kittens.

  Note: Everything I wrote here is meant to help you in your journey in the land of code. If you feel offended, I apollogise. That was not what I meant to do. If you don't aggree, good for you. At the end of the day this is just another review. Happy coding!


Template integer range, version 2 This question is a follow-up. The original question was itself a follow-up to an older question by @LokiAstari.

  The idea is to provide a compile-time integer range.

This version incorporates almost everything from the answers and comments to the previous iteration of the review (except the handling of corner cases such as INT_MAX, which is kind of hard). Along with the fixes comes a new feature: the possibility to specify the « step » of the range. Here is the new implementation:

#include <cstddef>
#include <utility>

namespace detail
{
    template<
        typename Integer,
        typename SequenceBase,
        Integer Begin,
        Integer Step,
        bool IsIncreasing
    >
    struct integer_range_impl;

    template<
        typename Integer,
        Integer... N,
        Integer Begin,
        Integer Step
    >
    struct integer_range_impl<Integer, std::integer_sequence<Integer, N...>, Begin, Step, true>
    {
        using type = std::integer_sequence<Integer, (N * Step + Begin)...>;
    };

    template<
        typename Integer,
        Integer... N,
        Integer Begin,
        Integer Step
    >
    struct integer_range_impl<Integer, std::integer_sequence<Integer, N...>, Begin, Step, false>
    {
        using type = std::integer_sequence<Integer, (Begin - N * Step)...>;
    };
}

template<
    typename Integer,
    Integer Begin,
    Integer End,
    Integer Step
>
using make_integer_range = typename detail::integer_range_impl<
    Integer,
    std::make_integer_sequence<
        Integer,
        ((Begin < End ? End - Begin : Begin - End) - 1) / Step + 1
    >,
    Begin,
    Step,
    (Begin < End)
>::type;

template<std::size_t Begin, std::size_t End, std::size_t Step>
using make_index_range = make_integer_range<std::size_t, Begin, End, Step>;

Here is an example of how this template integer range can be used:

#include <array>
#include <iostream>
#include <iterator>
#include <numeric>

template<typename T, std::size_t N, std::size_t... Ind>
auto print(const std::array<T, N>& arr, std::index_sequence<Ind...>)
    -> void
{
    int dummy[] = {
        (std::cout << std::get<Ind>(arr) << ' ', 0)...
    };
    (void) dummy;
}

int main()
{
    std::array<int, 20u> arr;
    std::iota(std::begin(arr), std::end(arr), 0);

    // prints 15 12 9 6 3
    print(arr, make_index_range<15u, 0u, 3u>{});
}

I accept any kind of review, be it about style, correctness or possible improvements to the utility in general :)

My main objection to your code is:

Principle of Least Surprise

I was pretty surprised that:

make_index_range<15u, 0u, 3u>

is the sequence

std::index_sequence<15, 12, 9, 6, 3>

given that it sure looks like we're going from 15 to 0 by 3s... which intuitively should produce std::index_sequence<>. You already do correctly provide two type aliases, where make_index_range is an alias for make_integer_range... so if somebody wants a negative range, they should have to do:

make_integer_range<int, 15, 0, -3>.

That change will also simplify your implementation, which leads into...

Simplify, Simplify, Simplify

Once you drop the increasing/decreasing check, the code becomes simpler. We are always going from Begin to End by Step.

First, let's determine the size of our sequence. For signed integers, we can just divide by Step and floor at 0. For unsigned integers, we have to ensure that End is at least as big as Begin:

template <class T, T Begin, T End, T Step>
struct sequence_size
: std::conditional_t<
        std::is_signed<T>::value,
        std::integral_constant<T, std::max<T>(0, (End - Begin)/Step)>,
        std::integral_constant<T, (End >= Begin) ? (End - Begin)/Step : 0>
        >
{ };

And then, with a default template parameter, our detail::make_integer_range can just be a simple pack expansion. No need for separate cases for increasing/decreasing sequences, since that would be handled by simply having a negative Step for signed integral types:

namespace detail {
    template <class T, T Begin, T End, T Step,
        typename = std::make_integer_sequence<T, sequence_size<T, Begin, End, Step>::value>
        >
    struct make_integer_range;

    template <class T, T Begin, T End, T Step, T... Idx>
    struct make_integer_range<T, Begin, End, Step, std::integer_sequence<T, Idx...>>
    {
        using type = std::integer_sequence<T, Idx * Step + Begin...>;
    };
}

template <class T, T Begin, T End, T Step>
using make_integer_range = typename detail::make_integer_range<T, Begin, End, Step>::type;

template <std::size_t Begin, std::size_t End, std::size_t Step>
using make_index_range = make_integer_range<std::size_t, Begin, End, Step>;

Add Default Step

Using 1 as a default is pretty standard, so let's just do that:

template <class T, T Begin, T End, T Step=1>
using make_integer_range = typename detail::make_integer_range<T, Begin, End, Step>::type;

template <std::size_t Begin, std::size_t End, std::size_t Step=1>
using make_index_range = make_integer_range<std::size_t, Begin, End, Step>;

Add Default Start

You could also provide a default beginning by having your aliases take a pack of ints. Something like:

template <class T, T... Vals>
using make_integer_range = typename detail::make_integer_range_variadic<T, Vals...>::type;

template <std::size_t... Vals>
using make_index_range = make_integer_range<std::size_t, Vals...>;

With:

template <class T, T... Vals>
struct make_integer_range_variadic
: make_integer_range<T, Vals...>
{ };

template <class T, T End>
struct make_integer_range_variadic<T, End>
: make_integer_range<T, 0, End, 1>
{ };

template <class T, T Begin, T End>
struct make_integer_range_variadic<T, Begin, End>
: make_integer_range<T, Begin, End, 1>
{ };

This would allow:

make_index_range<5>       ==> <0, 1, 2, 3, 4>
make_index_range<2, 5>    ==> <2, 3, 4>
make_index_range<2, 5, 2> ==> <2, 4>

YMMV on whether or not you want to allow this, but just throwing this out as an option in case you do.


Number guessing game for beginners I am teaching a python class to some high school seniors. I was thinking of doing an "oracle" game where the computer would come up with a number, and the player would guess it. These are brand new coders, and I'll teach them what they need to know to get there, but is this a good first project? I have my code here as my implementation of this game. I want to make sure that I don't teach them anything bad or give them bad habits.

My software requirements are:

Must generate a random (or psuedo-random) number
Must import a library
Must use a loop
Must take user input
Must tell the user to guess higher or lower

# imports
import random
import time
import sys
import os

# set global variables
MIN_NUMBER = 1
MAX_NUMBER = 10

# important functions
def get_random(min, max):
    print "Coming up with a number."
    time.sleep(2)
    random_number = random.randint(min, max)
    print "Got it!"
    return random_number

def to_int(x):
    try:
        x = int(x)
        return x
    except ValueError:
        print "That is not a whole number."
        return False

def how_close(guess, right_answer):
    return right_answer - guess

def hi_low_done(delta):
    if delta < 0:
        print "Guess lower."
        return False
    elif delta > 0:
        print "Guess higher."
        return False
    else:
        print "You got it!"
        return True

# main Program

#clear screen
os.system('cls')  # on windows
print "Welcome. I am the Oracle."
print "I will think of a number between %s and %s. Guess it and you will win!" % (MIN_NUMBER, MAX_NUMBER)

while True:
    user_answer = raw_input("Ready? <y/n> ")
    if user_answer.lower() == 'y' or user_answer.lower() == 'yes':
        print "Great! Let's go."
        break;
    elif user_answer.lower() == 'n' or user_answer.lower() == 'no':
        print "Okay. Consult the Oracle when you are ready."
        sys.exit()

oracles_number = get_random(MIN_NUMBER, MAX_NUMBER)

number_of_guesses = 0
while True:
    number_of_guesses = number_of_guesses + 1
    user_answer = raw_input("\nGuess> ")
    user_answer = to_int(user_answer)
    # if user answer is outside min max... else...
    if user_answer:
        delta = how_close(user_answer, oracles_number)
        done = hi_low_done(delta)
        if done:
            break;

print "It took you %s guesses to guess my number." % (number_of_guesses)

I guess my concerns with my code are:

Is this simple enough? It isn't the first thing they will code, but the first project they will do. (maybe group project?)
Did I make any blatant errors or "In python, you should do this instead" errors? Python isn't my first language so I'm not sure.

I have created a gist file with my most up-to-date version of this.

There is a serious problem at:

def get_random(min, max):
    print "Coming up with a number."
    time.sleep(2)
    random_number = random.randint(min, max)
    print "Got it!"
    return random_number

This function mixes user interaction (printing and sleeping) and logic (returning a random number). This is basic separation of intents, and should be taught to beginners from the start.

The same can be seen at:

def hi_low_done(delta):
    if delta < 0:
        print "Guess lower."
        return False
    elif delta > 0:
        print "Guess higher."
        return False
    else:
        print "You got it!"
        return True

This function should only return a message, not return a message and print.

About to_int

def to_int(x):
    try:
        x = int(x)
        return x
    except ValueError:
        print "That is not a whole number."
        return False

While it is possible to return different types from a function in Python, it is a symptom of bad design. I reccomend writing a is_int method to return True or False if the argument is an int and then converting to int with int.

Further refactoring

Asking a user for yes or no is a common task and should be its own function, also using startswith is more flexible and allows 'yeah' and 'yup' for example.

Also you should write a function to generate the message. You then print it and break if guess and real number are equal.

Use Python 3

Python 3 is continually improved while Python 2 is deprecated, it makes more sense to teach 3 than 2 (The two are similar just use parens around print).

Avoid needless comments

Comments should tell why not what, if you write

# imports

You just add noise as it is obvious that the following lines are imports.

Define a main function and call it

It is a good habit to get into and allows the importing of scripts.

Like:

def number_guessing_game(minimum, maximum):
     # All your currently top level code

if __name__ == "__main__":
    number_guessing_game()


Regex to find addresses and phone numbers I am trying to optimize my Java code where I am parsing an address field.

Address fields have the format:

full_address;phone; full_address;phone; full_address;phone;

where full_address = addresstype^street^city^state^zip
and where street = street1;street2;street3;street4;

So my string is

final String string = "Billing^Tata;3001 Garden Parkway^^NJ^;100-00-0009;Home^Goggle;3341 Main Parkway^^NY^;;";

My object location stores each of the above attributes.

//regular expression to match the address type
Pattern newPattern = Pattern.compile("(([^\\^]*)\\^([^\\^]*)\\^([^\\^]*)\\^([^\\^]*)\\^([^;]*);([^;]*);)");
Matcher newMatcher = newPattern.matcher(addressLongText);
List<Location> discreteListOfLocations = new ArrayList<Location>();
MatchResult result = null;
while (newMatcher.find())
{
    result = newMatcher.toMatchResult();
    Location location = new Location();
    location.setAddressTypeCdValue(result.group(2));
    String[] str_arr = result.group(3).split(";");
    if (str_arr.length > 0)
    {
        location.setStreetAddress1(str_arr[0]);
    }
    if (str_arr.length > 1)
    {
        location.setStreetAddress2(str_arr[1]);
    }
    if (str_arr.length > 2)
    {
        location.setStreetAddress3(str_arr[2]);
    }
    if (str_arr.length > 3)
    {
        location.setStreetAddress4(str_arr[3]);
    }
    location.setCity(result.group(4));
    location.setState(result.group(5));
    location.setZip(result.group(6));
    discreteListOfLocations.add(location);
}

I am a bit confused how to optimize the regex so that it is easier for someone else to understand what my regex is doing. Any idea or suggestion will be helpful.

Not sure about Java string catenation.
Below is your regex formatted and commented (by RegexFormat 5)

This puts it in expanded mode. The good thing is anybody can read it in
your source code for later reference.

Below is 2 versions. One a c++ normal catenation where newline \n are
added. Two a single quoted string where the newline is natural.

The nice thing about doing this in your code is you can always print it out
for debug purposes. It prints as a nice format.

"(?x)                                                               \n"
"   ( [^\\^]* )            # (1), Address type                      \n"
"   \\^                                                             \n"
"   ( [^\\^]* )            # (2), street1;street2;street3;street4;  \n"
"   \\^                                                             \n"
"   ( [^\\^]* )            # (3), City                              \n"
"   \\^                                                             \n"
"   ( [^\\^]* )            # (4), State                             \n"
"   \\^                                                             \n"
"   ( [^;]* )             # (5), Zip                                \n"
"   ;                                                               \n"
"   ( [^;]* )             # (6), Phone                              \n"
"   ;                                                               \n"

======================================

"(?x)
   ( [^\\^]* )            # (1), Address type
   \\^
   ( [^\\^]* )            # (2), street1;street2;street3;street4;
   \\^
   ( [^\\^]* )            # (3), City
   \\^
   ( [^\\^]* )            # (4), State
   \\^
   ( [^;]* )             # (5), Zip
   ;
   ( [^;]* )             # (6), Phone
   ;
"


Merging three CSV files with a primary key This is my first time coding in my life. I don't really know much about coding. I googled whatever I need and combined them all together. It works great, but want to know if there is any improvement needed. Is there a better language to write in?

import os, csv, re, time
from datetime import date, timedelta

SendIDs =[]
BouncedEAes=[]
SubKeys=[]
MerchantIDs=[]
EventDates=[]
BounceCategories=[]
BounceReasones=[]
#SendTimes=[] #To find the most recent date that a welcome email was sent.
BounceDate=0

f = open("SendJobs.csv") #To get Send IDs and dates for Welcome emails
for row in csv.reader(f):
    if "Welcome" in row[7]:
        SendIDs.append(row[1])
        # SendTimes.append(
        #   time.strftime("%Y%m%d",
        #       time.strptime(row[5],"%m/%d/%Y  %H:%M:%S %p")))
f.close()

# if not os.path.exists('SendIDs.csv'):
#   open('SendIDs.csv', 'w').close()

# f = open("SendIDs.csv")
# for row in csv.reader(f):
#   SendIDs.append(row[0])

# UniqSendIDs = {}.fromkeys(SendIDs).keys()
# f.close()

# f = open("SendIDs.csv","w")
# with f as output:
#   writer = csv.writer(output, lineterminator='\n')
#   for item in UniqSendIDs:
#       writer.writerow([item])
# f.close()

f = open('Bounces.csv')
for row in csv.reader(f):
    for item in SendIDs: #OR UniqSendIDs
        if item == row[1]:
            SubKeys.append(row[2])
            BouncedEAes.append(row[3])
            BounceCategories.append(row[8])
            BounceReasones.append(row[10])
            #EventDate: Only need bounce date, NO time required.
            BounceDate = time.strptime(row[6],"%m/%d/%Y  %H:%M:%S %p")
            BounceDate = time.strftime("%m/%d/%Y", BounceDate)
            EventDates.append(BounceDate)
f.close()

f = open('Attributes.csv')
for row in csv.reader(f):
    for item in BouncedEAes:
        if item == row[2]:
            MerchantIDs.append(row[4])
f.close()

SubKeys.insert(0,"SubscriberKey")
BouncedEAes.insert(0,"EmailAddress")
MerchantIDs.insert(0,"Merchant_Number")
EventDates.insert(0,"EventDate")
BounceCategories.insert(0,"BounceCategory")
BounceReasones.insert(0,"BounceReason")

new_data=[[SubKeys[i],BouncedEAes[i], MerchantIDs[i], EventDates[i], BounceCategories[i], BounceReasones[i]] for i in range(len(BouncedEAes))]

# if len(SendTimes)==0:
#   yesterday = date.today() - timedelta(1)
#   SendTimes.append("NoWelcomeEmailsSent_"+yesterday.strftime("%Y%m%d"))

# if len(BouncedEAes)==1:
#   SendTimes.append("NoBouncedEmails_"+SendTimes[-1])

# FILENAME FORMAT: Welcome-Cadence-#2-RESULT-YYYYMMDD
# YYYYMMDD: One day before the day the file was received.

yesterday = date.today() - timedelta(1)

f=open("Welcome-"+yesterday.strftime('%Y%m%d')+".csv","wb")
output=csv.writer(f)

for row in new_data:
    output.writerow(row)
f.close()

"""
ERROR MESSAGES
"""

#new_data=[[SubKeys[i],BouncedEAes[i], MerchantIDs[i], EventDates[i], BounceCategories[i], BounceReasones[i]] for i in range(len(BouncedEAes))]
#IndexError: list index out of range

"""
Check Attributes.csv
"""

if it's really your first time coding, then that's great: you actually managed to use some parts of the Python standard library that people being exposed for the first time to the language tend to needlessly reinvent, such as the csv module :)

Python version

The first big question: you used Python, but why did you use the version 2.7? This version (minus the bugfixes) is already 5 years old and its maintenance should have been dropped this year (it has been extended to 2020 since it's still widely used). But if you start from scratch, you might as well start with a fresh new version, such as Python 3.4. Generally speaking, don't use Python 2.7 unless you're forced to (there are still reasons); try to adapt to the most recent version instead.

Commented out code

Generally speaking, you should never leave commented-out pieces of code in your code. They will only hinder readability. I must admit that you probably don't use source control source control software if it's your first time programming, but you might want to look at software like Git in the future so that every version of your code is saved somewhere and you don't have to keep outdated remnants of code in your source.

Useless stuff in general

More generally, remove from your code what is unneeded, such as unused variables or unused includes. In your case, you could get rid of the re, os and time imports: you don't use re, you only use os in commented-out code that shouldn't belong, and what you use from time is actually also available in datetime.

The with statement

One thing that is easy to forget is closing files. You didn't forget to close them, which is great, but Python provides a superior alternative to closing files in the form of the with statement. Here are your first lines of code rewritten with it:

with open("Welcome-"+yesterday.strftime('%Y%m%d')+".csv","wb") as f:
    output=csv.writer(f)

    for row in new_data:
        output.writerow(row)

As you can see, I didn't manually call f.close(), this is automatically done when we leave the with block. Actually, with isn't limited to closing files, it can do many other things, but it depends on the type you use it with.

Use the PEP 8

Python has a style guide which is rather good and people tend to follow it. Therefore, you could try to rewrite your code following that style guide so that it looks more like what people want to read when they read Python code. For example, I will rewrite once again the previous piece of code, following the PEP8:

with open("Welcome-" + yesterday.strftime('%Y%m%d') + ".csv", "wb") as f:
    output = csv.writer(f)

    for row in new_data:
        output.writerow(row)

Basically, it's still the same. I only added spaces. But even those can matter when you read code :)