Building a simple php Mail class I am relatively new to OOP. I have an application that requires to send some emails from the server for various use cases. I have moved my code into a Mail class to try to abstract this out. Coming from years as a functional programmer, it is becoming challenging to move into the OOP space. This class is for very generous purposes, and I do not expect to handle attachments, bcc, or any other extra mailer usages. While there are nice and handy Mail packages I can find and implement within a few minutes, I thought it would be best to create this on my own.

My class is as follows:

<?php
/**
 * Mail Class
 */

namespace Utility;

class Mailer
{
    public $sendTo;
    public $sendFrom;
    public $subject;
    public $message;
    public $headers;
    public $error = [];

    public function __construct($sendTo, $sendFrom = null, $subject, $message)
    {
        $this->sendTo = $sendTo;
        $this->sendFrom = ($sendFrom) ? $sendFrom : 'webmaster@shelter.pet';
        $this->subject = $subject;
        $this->message = $message;
        //$this->headers = $this->setHeader();
    }

    public function setTo($email, $name) {
        return $this->sendTo = $email;
    }

    public function getTo() {
        return $this->sendTo;
    }

    public function setFrom($email, $name)  {
        return $this->sendFrom = $email;
    }

    public function setSubject($subject) {
        return $this->subject = $subject;
    }

    public function getSubject() {
        return $this->subject;
    }

    public function setMessage($message) {
        $this->message = $message;
        //@TODO:string repace, etc

        return $this;
    }

    public function getMessage() {
        return $this->message;
    }

    /*public function setHeader($header, $email, $name = null) {
        $address = $this->formatHeader((string) $email, (string) $name);
        $this->headers[] = sprintf('%s: %s', (string) $header, $address);
        return $this;
    }*/

    public function setHeader() {
        $headers = 'From: '.$this->getFrom() . "\r\n" .
            'Reply-To: '.$this->getFrom() . "\r\n" .
            'X-Mailer: PHP/' . phpversion();

        $this->headers = $headers;
        return $this;
    }

    public function getFrom() {
        return $this->sendFrom;
    }

    public function send() {
        $to = $this->sendTo;
        $from = $this->sendFrom;
        $subject = $this->subject;
        $message = $this->message;
        //$headers = $this->headers;
        $headers = 'From: '.$this->getFrom() . "\r\n" .
            'Reply-To: '.$this->getFrom() . "\r\n" .
            'X-Mailer: PHP/' . phpversion();

        return mail($to, $subject, $message, $headers);
    }
}

And to implement the class I do the following in my code:

//alert admin of failed login
                $adminFailedAlert = new Mailer('email@domain.com', null, 'User Failed Login', 'A user has unsuccessfully logged in at '.Utility::getDateTime().'. You may want to look into this.' );
                $adminFailedAlert->send();

This Class works fine for me. The only issue I am having currently is creating dynamic header string - currently I just manually place them into the send() method - which is why you see the commented out header references. I have not implemented the $errors method so will work on that. I guess at this point, am I on the right path? I am also confused why I should have my getter and setter methods when I am not even using it when I implement the Class. I am also trying to overcome to obstacle of knowing when to use private or public, etc method declarations.

Other, similar libraries don't require you to pass everything to the constructor. What if you want to send an email to more than one person? You should be able to call $mailer->setTo() more than once.

For now, I would suggest getting rid of the constructor entirely, and calling the setters manually as needed. After all, if you're planning on doing the entire operation in one call then you might as well write a function instead of a class.

If you're going to implement setter methods then you should make your properties private or protected so they can't be changed manually by whoever is using your class. Usually, the setters are implemented so you can validate input before setting them, for example,

public method setTo($email){
    if(filter_var($email, FILTER_VALIDATE_EMAIL)) $this->toEmail = $email;
    else $this->lastError = "Invalid to email: {$email}.";
}

Now you can prevent the user from manually setting $mail->toEmail by marking it as private, thus, forcing the input to be validated by your setter.

You're not using your toName parameter and you're only allowing a single receiver. Check out the manual and you'll notice that you can have multiple recievers and you can specify a name for each one in the following format:

  User <user@example.com>, Another User <anotheruser@example.com>

Therefore, you should have an array for your toNames and another array for your toEmails.

public method setTo($email, $name=""){
    if(filter_var($email, FILTER_VALIDATE_EMAIL)){
        $this->toEmail[] = $email;
        $this->toName[] = $name;
    }else $this->lastError = "Invalid to email: {$email}.";
}

Then, in your send function...

$to = "";
foreach($this->toEmail as $k=>$v){
    if(!empty($this->toName[$k])) $to .= "{$this->toName[$k]}<$v>,";
    else $to .= "$v,";
}
$to = rtrim($to, ","); // trim off trailing comma

If you're not sending an HTML email you should wordwrap your $message parameter at 70 chars. (You can create a variable to determine if the email will contain HTML).

if(!$this->isHTML) $message = wordwrap($message, 70, "\r\n");

If it is HTML you have to let the client know by adding these headers:

$headers .= 'MIME-Version: 1.0'."\r\n".
    'Content-type: text/html; charset=iso-8859-1';

You can also carbon copy someone with this header:

'Cc: birthdayarchive@example.com'

Anyway, you're on the right track, but make sure you read the manual to see what other cool stuff you could be doing.


Polymorphic animals speak their name and make a noise I'm new to GNU Smalltalk. I'd like to port a script I've written in Ruby, Scala, CoffeeScript, and several others. It's the one I use to try to learn the classic OOP concepts of abstract classes, abstract methods, overriding, dispatch, etc. It goes like this:

Define an abstract class Animal with a constructor to set the animal's name, a speak method to return a string N says S where N is the name and S is the sound the animal makes.
Define three concrete subclasses Horse, Cow, and Sheep, each with their own sound-returning string method.
Create a horse and sheep and a cow, and make them speak. For the horse and cow, create a variable. For the sheep, call the speak method on an "anonymous" object.

My GNU Smalltalk solution is:

Object subclass: Animal [
    | name |
    Animal class >> new: n [^super new init: n]
    init: n [name := n]
    speak [^name, ' says ', self sound]
]

Animal subclass: Horse [
    sound [^'neigh']
]

Animal subclass: Sheep [
    sound [^'baaaa']
]

Animal subclass: Cow [
    sound [^'moooo']
]

h := Horse new: 'CJ'.
h speak displayNl.
c := Cow new: 'Bessie'.
c speak displayNl.
(Sheep new: 'Little Lamb') speak displayNl.

I get the expected output but I am unhappy with the solution. I'd appreciate input on:

The whole new and init thing. Is there a way to make it cleaner?
I noticed I did not have to override new or init in my subclasses; did the subclasses really inherit new? It is a classmethod and I don't know if it is inherited or not.
Are we supposed to give an implementation of new? I had heard this was not a good idea. I know from Ruby that you just define initialize and use the built-in new to automatically invoke the initialize instance method.
Is this GNU Smalltalk syntax I got from the GNU tutorial proper or is it an extension of some sort? Should this be written in a proper, "purer" Smalltalk style? I've seen some where there is an instanceVariables message.
I know Smalltalk has something called subclassResponsibility. Is it wise to declare the sound method in the Animal class (as an abstract method)?

Item 1
Instead of Animal new: n use Animal named: aString implemented on the class side of Animal as:

named: aString
    ^self new name: aString

Then in the instance side of the class define

name: aString
    name := aString

Item 2
Yes, new is inherited by the subclass. The same happens with all class methods; they are inherited by subclasses.

Item 3
You don't have to implement new, unless you want to do something special in your class. In most dialects (I haven't checked GNU Smalltalk) new will call the instance method initialize, which by default does nothing. If you need to initialize you instances, then implement initialize as

initialize
     super initialize.
     "your specific code"

Item 4
Can't help on this one but yes, the usual message for creating a subclass includes a keyword for declaring named instance variables.

Item 5
You have two options here. (1) You can implement sound in Animal as self subclassResponsibility. (2) You can implement it with a default value ^oops. The first alternative will indicate the programmer extending the hierarchy what method they must to provide in the new subclass. The second will just provide a default sound for the programmer to find out and refine.

---EDIT---

A more realistic implementation (meaning, one not intended to test OOP concepts in Smalltalk) would get rid of the name instance variable of Animal. This is possible because the class name already provides the animal's name. In other words, the instance doesn't need to "remember" its name because its class already knows it. Here is the instance side code

name
    ^self class name asLowercase

Note that instances of the abstract class Animal will now answer the string 'animal' as their name.

With this simplification the instance creation method (class side) named: would no longer be required. However, we could tweak its implementation so it answers an instance of the appropriate subclass:

named: aString
    | subclass |
    subclass := Animal withAllSubclasses
        detect: [:class | class name asLowercase = aString] ifNone: [self].
    ^subclass new

With these changes we no longer need the name instance variable and also will be able to create, say an instance of Horse evaluating: Animal named: 'horse', etc.


how to properly expose an ES class function residing inside a module? I have a little class for loading a config file:

// index.js:

class Config {
    constructor(path) {
        this.path = path || this.defaultPath;
        this.config = fs.readFileSync(this.path, { encoding: 'utf8' });
        this.config = JSON.parse(this.config);
    }
    get() {
        return this.config;
    }
}
Config.prototype.defaultPath = './config/config.json';

module.exports = {
    Config: Config,
};

And I test it like so:

// test.js:

const index = require('../index');

let config;

describe('config ', () => {
    before(() => {
        config = new index.Config().get();
    });
});

It works, but I'm wary of including a capitalized property in module.exports (mostly because I haven't seen this done before). Is there a best practice for achieving this in other ways? Should I just make the config class into its own .js file?

I'm not aware of any best practice here, so this answer is my own hunch.

I would stick with using an uppercase 'C' for the exported member name.

In JavaScript, the usual protocol is to use an uppercase name for a function that should be new from, rather than called directly e.g.

function Config()

to show that the function should be used with a new call, to treat it as a constructor. e.g.

var config = new Config();

As you are exposing a class from your module, in other words a function that can be new'd up from, then I would stick with using a capital C.


To-do list - HTML/CSS/JS I created my first "program": a to-do list. I would appreciate it if somebody could look at the code and give me some hints on how to improve it.

var list = document.createElement("ol");
list.id = "list";
list.className = "list";

var addTask = document.getElementById("inputBtn");
addTask.addEventListener('click', function (){

 // checking whether the field is empty
 var task = document.getElementById("task").value;
 if(task=="") alert("Are you sure you do not have any task to do?");
 else
 {
  // adding an element
  addElement (list, task);

  //clear the input field
  document.getElementById("task").value ="";
 }
}, false);

function addElement(ol, text)
{
 var element = document.createElement("li");
 element.id = "element";
 element.className = "element";
 element.appendChild(document.createTextNode(text));
 ol.appendChild(element);
 document.getElementById("board").appendChild(ol);

 //adding a remove button
 addRemoveBtn(element);

 //adding a function which change the decoration of the text
 addCrossStyle(element);
}

function addRemoveBtn(li)
{
 //creating a remove button
 var removeTask = document.createElement("input");
 removeTask.setAttribute("type", "button");
 removeTask.setAttribute("value", "Remove");
 removeTask.setAttribute("class", "remove");

 //remove task function
 removeTask.addEventListener('click', function (){
  li.parentNode.removeChild(li);
 }, false);
 li.appendChild(removeTask);
}

function addCrossStyle(li)
{
 var check = false;

  li.addEventListener('click', function (){
   if (check==false)
   {
     li.style.textDecoration = "line-through";
     li.style.textDecorationColor = "red";
     check = true;
   }
   else
   {
     li.style.textDecoration = "none";
     check = false;
   }
  }, false);
}
body
{
 margin: 0;
 background-color: #26282E;
 color: #b3b3ff;
    font-family: 'Arial', sans-serif;
    font-size: 28px;
}

h1
{
 font-size: 64px;
 font-weight: 400;
 text-align: center;
 letter-spacing: 5px;
 margin-bottom: 0px;
 margin-top: 40px;
}

.addtask
{
 margin-top: 20px;
 text-align: center;
}

input
 {
   margin-top: 10px;
   border: solid;
   padding: 10px;
   font-size: 20px;
   background-color: #b3b3ff;
}

input::placeholder
{
   color: white;
   text-align: center;
}

.inputBtn
{
 background-color: #8080ff;
 margin-top: 20px;
 border: 3px black solid;
    color: white;
    padding: 15px 32px;
    text-decoration: none;
    display: inline-block;
 font-size: 28px;
}

.inputBtn:hover
{
 cursor:pointer;
 background-color: #6b6b99;
}

.list
{
 width: 900px;
 margin: auto;
 background-color: #b3b3ff;
 margin-top: 10px;
 color: white;
}

.remove
{
 background-color: #b30000;
 border: 3px black solid;
 color: white;
    padding: 5px 15px;
 font-size: 12px;
 position: absolute;
 margin-right: 5px;
 right: 0;
 top: 0;
 opacity: 0.7;
}
.remove:hover
{
 cursor: pointer;
 opacity: 1;
 background-color: red;
}

.element
{
 position: relative;
 padding: 15px 32px;
 font-size: 22px;
 word-wrap: break-word;
 padding-bottom: 10px;
 border-bottom: solid #8080ff;
 cursor: pointer;
}

.element:hover
{
 background: #8080ff;
}
<!DOCTYPE html>

 <meta charset="utf-8">
 <title>To Do List</title>
 <meta name="author" content="Szymon Wolny">

 <meta http-equiv="X-Ua-Compatible" content="IE=edge,chrome=1">

 <link rel="stylesheet" href="todo.css">

 To Do List

    <form>

     <input type ="text" name="task" placeholder="New task..." id="task" size="45" >

    </form>
    <button type="button" id="inputBtn" class="inputBtn">Add a new task to the List </button>

so just 2 things here:

if(task=="") alert("Are you sure you do not have any task to do?");
such comparison is a bit risky in JS. In case of checking for empty values it's better to do if(!condition) {} else {} or at least if(task ==="") (strong type checking). Not saying it's wrong in this case, just a rule of a thumb. Read more about issues with JS comparison / type casting, you'll find out what I mean.
Creating UI in code is fine of course, and it's nice to understand what's going on under the hood, but for readability of your UI you mind find it better to define everything up-front in HTML and than later just show / hide parts of it or add remove classes, depending on your application state. This is not a general guideline, because it only makes sense for small-medium apps, for larger, more complex, you probably will use some framework anyway (angular or such).
Other than that, fine simple app (assuming it works, didn't run it to be honest) ;).


Improving Point Cloud Slice Parameter's Adjusting loop This part of the function is adjusting the sliceDepthSubtractor and startDepthSlice parameters that are used in order to take a geometrical slice (h) of a point cloud object (a[i]), that it will be used for determining if the object is a circle or not.

If the slice it is containing too few points in order to make a circle determination, the slice is retaken with different parameters before throwing an exception.

sliceDepthSubtractor and startDepthSlice are local parameters that are filled with data from XML file parsing class (The XML file is read at the application initialization inside of a dedicated class). Initial values are 1 (sliceDepthSubtractor) and 0.98 (startDepthSlice).

The list of point cloud object (a) is given as a parameter to this method.

As I hate nested loops, I would like to improve this code.

double sliceDepthSubtractor =  xlmClass.sliceDepthSubtractor;
double startDepthSlice      =  xlmClass.startDepthSlice;
bool adjustSliceDepthSubtractor = true;
while (adjustSliceDepthSubtractor)
{
   adjustSliceDepthSubtractor = false;
   bool adjustingStartDepthSlice = true;
   while (adjustingStartDepthSlice)
   {
     adjustingStartDepthSlice = false;
     for (int i = 0; i < Count; i++)
     {
        PointCloud h = TakeSlice(a[i],sliceDepthSubtractor,
                     startDepthSlice);

       if ((h.PointCount < 100) && (startDepthSlice > 0.10))
       {
         startDepthSlice  -= 0.1;
         adjustingStartDepthSlice  = true;
         break;
       }
       if ((h.PointCount < 100) && (sliceDepthSubtractor < 3))
       {
         sliceDepthSubtractor += 1;
         adjustSliceDepthSubtractor = true;
         break;
       }
       if (h.PointCount < 100)
       {
         throw new ComputationException("Insufficient Points");
       }
       LmsSolver(h, a[i]);
     }
   }
   if (adjustSliceDepthSubtractor) {continue;}
   Save(a);
}

You are using the same pattern in both your loops - enable some flag before the loop, and disable that flag immediately as you enter the loop. So basically you want to always enter the loop and always exit loop unless something will enable the flag.

bool flag = true;
while (flag)
{
    flag = false;
    // ...
    // some condition can set flag to true

    if (flag)
      continue;
}

Same can be achieved without using a flag - use infinite loop and break it only when your exit condition happen

while (true)
{
    // ...
    if (sliceDepthSubstractorAdjusted)
       break;
}

Now, what about nested loop? I stick to single indentation level rule of thumb. To avoid arrow anti-pattern. You can move loop logic to separate method and return outer exit loop condition from that method. Thus you can keep loop even without a body:

while (!TryAdjustSliceDepthSubstructor()) {}

Pretty descriptive - repeat adjustment until it done. But you should pass different values to that method - a, sliceDepthSubtractor, startDepthSlice. And this code will not look so good with all these parameters flying around. The reason is that we keep data and logic separated. So simply put logic (points adjustment with some parameters) and data (parameters) into the class.

public class Butcher// Of course, you should change the name
{
     private double sliceDepthSubtractor;
     private double startDepthSlice;

     public Butcher(double sliceDepthSubtractor, double startDepthSlice)
     {
         // initialize fields here
     }

     public void AdjustForSlicing(List<PointCloud> points) { ... }

     // TakeSlice
     // lmsSolver
}

Now you can initialize this class and your code with outer loop will look like

var points // List<PointCloud>
var butcher = new Butcher(xlm.sliceDepthSubtractor, xlm.startDepthSlice);
butcher.AdjustForSlicing(points);
Save(points); // use PascalCase names for methods

Looks clean. Let's look inside

public void AdjustForSlicing(List<PointCloud> points)
{
    while(!AllSlicesContainEnoughPoints(points)) // surprise - single loop here
    {
        if (CanAdjustStartDepthSlice)
        {
            startDepthSlice -= 0.1;
            continue;
        }

        if (CanAdjustDepthSubstractor)
        {
            // NOTE: Consider to reset startDepthSlice here
            sliceDepthSubtractor += 1;
            continue;
        }

        throw new ComputationException("Insufficient Points");
    }
}

private bool AllSlicesContainEnoughPoints(List<PointCloud> points)
{
    const int minCountOfPointsInSlice = 100;

    foreach(var point in points)
    {
        var slice = TakeSlice(point, sliceDepthSubtractor, startDepthSlice);

        if (slice.PointCount < minCountOfPointsInSlice)
            return false;

        lmsSolver(slice, point);
    }

    return true;
}

And a couple of helper methods. Consider replacing magic numbers with constants

private bool CanAdjustStartDepthSlice => startDepthSlice > 0.1;
private bool CanAdjustDepthSubstractor => sliceDepthSubtractor < 3;

Now you can read this code as a specification.

Note: Probably we should not modify initial values of startDepthSlice and  sliceDepthSubtractor when we are adjusting points in order to be able to call butcher serveral times. But I'll leave this exercise for you :)


Finding the weight of the heaviest path in a binary tree I'm fairly new to Java, arriving in the "future" from C and returning to type safety from Python. I've programmed an algorithm to return the weight of the heaviest path from the root of a binary tree to one of its leaves.

Each Node in the tree has a weight (its data).  The weight of the path is the sum of all of the Nodes weights from root to leave inclusive. This algorithm finds the heaviest.

I'm looking for your suggestions to improve this code in the following areas:

Terminology - is there a name for this problem? have I used
conventional names for things?
Correctness - are there any bugs?
Java conventions and idioms - can I make better use of the standard
libraries?
Runtime performance.
Repetition - is there any way to avoid writing the field names 4 times each?

package binaryTree;

class Node {
    Integer data;
    Node left;
    Node right;

    public Node(Integer data, Node left, Node right) {
        this.data = data;
        this.left = left;
        this.right = right;
    }

    public Node(Integer data) {
        this(data, null, null);
    }

}

public class BinaryTree {

    /**
     * Finds the heaviest path in the tree from the given @root Node to a leaf Node.
     * @param root
     * @return integer representing the sum weight of the heaviest path.
     */
    public static int getHeaviestPathWeight(Node root) {
        if (root == null)
            return 0;
        else
            return root.data + Math.max(getHeaviestPathWeight(root.left), getHeaviestPathWeight(root.right));
    }

}

The algorithm looks sound.  It visits every node once, and cannot be more efficient than that.

I find it odd that you split the code into two classes, one of which is non-public.  Since the Node class is non-public, no code in any other package will be able to construct a Node.  The algorithm is therefore "unusable".

There are two ways to model a tree: a "lazy" way (exposing the nodes, and a tree is just referred to by its root node) and a "polished" way (with the tree being an opaque data structure that hides and manages its nodes).  Here, you have an odd hybrid.

If you want to have a BinaryTree class that contains nothing but a static method, then you should suppress the implicit no-argument default BinaryTree() constructor by making it private:

public class BinaryTree {
    // Suppress the default constructor
    private BinaryTree() {}

    public static int getHeaviestPathWeight(…) {
        …
    }
}

Package names are conventionally all lowercase.

Whenever possible, use int instead of Integer.  Using Integer objects is less efficient and usually results in more complicated source code and bytecode.  One reason why you might need an Integer instead of an int would be to store a null — and that sounds more like a liability than an advantage in this case.

To language purists, "data" is plural, and "datum" is singular.  However, I would suggest a different name altogether, such as weight.

Simple solution

package binarytree;

public class BinaryTree {
    private final int weight;
    private final BinaryTree left, right;

    public BinaryTree(int weight, BinaryTree left, BinaryTree right) {
        this.weight = weight;
        this.left = left;
        this.right = right;
    }

    public BinaryTree(int weight) {
        this(weight, null, null);
    }

    /**
     * JavaDoc here
     */
    public int getHeaviestPathWeight() {
        return this.weight + Math.max(
            this.left  == null ? 0 : this.left.getHeaviestPathWeight(),
            this.right == null ? 0 : this.right.getHeaviestPathWeight()
        );
    }
}

Alternate solution

package binarytree;

public class Node {
    public final int datum;
    public final Node left, right;

    public Node(int datum, Node left, Node right) {
        this.datum = datum;
        this.left = left;
        this.right = right;
    }

    public Node(int datum) {
        this(datum, null, null);
    }
}

public class HeaviestPath {
    private HeaviestPath() {}

    /**
     * JavaDoc here
     */
    public static int weight(Node root) {
        if (root == null) {
            return 0;
        } else {
            return root.datum + Math.max(weight(root.left), weight(root.right));
        }
    }
}


Regex to find variants of "Google" From the HackerRank question definition:

  The word google can be spelled in many different ways.

  E.g. google, g00gle, g0oGle, g<>0gl3, googl3, GooGIe etc...

  Because

  g = G

  o = O = 0 = () = [] = <>

  l = L = I

  e = E = 3

  That's the problem here to solve.

  And the match has to be only this single word, nothing more, nothing
  less.

  E.g.

  "g00gle" = True, "g00gle " = False, "g google" = False, "GGOOGLE" =
  False, "hey google" = False

What I'm looking in review: How can I make my code more pythonic?

#!/usr/bin/env python3
# -*- coding: utf-8 -*-

import re

g = ["g", "G"]
o = ["o", "O", "0", "()", "[]", "<>"]
l = ["l", "L", "I"]
e = ["e", "E", "3"]

regex = (g, o, o, g, l, e)
regex = ((re.escape(y) for y in x) for x in regex)
regex = ("(?:{})".format("|".join(x)) for x in regex)
regex = "^{}$".format("".join(regex))

print(bool(re.match(regex, input())))

Honestly there's not much code to comment on, it's also a programming challenge, so there's not much incentive to make it any better than it is. I too would have done it your way.

The only objective criticism I have is I'd just add a function regex_options to build the non-capturing group.

Other than that I'd apply this to the creation of g, o, l and e. As I think it's a little cleaner. But you may want to perform it before "".join, and after regex = (g, o, o, g, l, e).

import re

def regex_options(options):
    options = (re.escape(o) for o in options)
    return "(?:{})".format("|".join(options))

g = regex_options(["g", "G"])
o = regex_options(["o", "O", "0", "()", "[]", "<>"])
l = regex_options(["l", "L", "I"])
e = regex_options(["e", "E", "3"])

regex = "^{}$".format("".join((g, o, o, g, l, e)))

print(bool(re.match(regex, input())))

If this were professional code, I'd suggest using an if __name__ == '__main__': block, and possibly a main function.


Finding index of first substring I wrote this function to find the index of the first substring. I was wondering if you can help me find some flaws or possibly help increase performance?

Example:

  str = "happy" substr = "app"

  index = 1

My code:

public static int subStringIndex(String str, String substr) {
    int substrlen = substr.length();
    int strlen = str.length();
    int j = 0;
    int index = -1;

    if (substrlen < 1) {
        return index;
    }
    else {
        for (int i = 0; i < strlen; i++) {              // iterate through main string
            if (str.charAt(i) == substr.charAt(j)) {    // check substring
                index = i - j;                              // remember index
                j++;                                    // iterate
                if (j == substrlen) {                   // when to stop
                    return index;
                }
            }
            else {
                j = 0;
                index = -1;
            }
        }
    }
    return index;
}

Just checking that you intend to be reinventing-the-wheel, you can do:

System.out.println("happy".indexOf("app"));

You did know that, right?

Or, if you want to reformat the 'signature' to match yours, it is:

public static int subStringIndex(String str, String substr) {
    return str.indexOf(substr);
}

There are a number of helper methods on String which will help:

String.indexOf(substr) - return the index of the first occurrence of substr
String.indexOf(substr, start) - return the index of the first occurrence of substr on or after the start position.
String.lastIndexOf(substr) - return the index of the last occurrence of substr
String.lastIndexOf(substr, start) - return the index of the last occurrence of substr starting before the start position.
 -


QuickSort in Java with Lomuto Partition or Hoare Partition I have developed a quick sort in java, that can work with both Lomuto Partition Strategy, or Hoare Partition Strategy.

Both of them sort the array correctly.

I would like to have it code reviewed to ensure I was able to correctly implement each one of those partitioning strategies.

Is there any reason to use one vs the other?

Here it is:

public class QuickSort {

    public static void main(String... args) {
        int array[] = {2, 3, 4, 1, 11, 5, 7, 0, 10, -1, 99, 23};
        quickSort(array);
        System.out.println(Arrays.toString(array));
    }

    public static void quickSort(int array[]) {
        quickSort(array, 0, array.length - 1);
    }

    private static void quickSort(int array[], int l, int r) {
        if (l >= r) {
            return;
        }
        int p = lomutoPartition(array, l, r);//hoarePartition(array, l, r);//lomutoPartition(array, l, r);
        quickSort(array, l, p - 1);
        quickSort(array, p + 1, r);
    }

    private static int lomutoPartition(int array[], int l, int r) {
        //pivot chosen
        int pivot = array[r];
        int i = l - 1;
        for (int j = l; j < r; j++) {
            if (array[j] < pivot) {
                i++;
                swap(array, i, j);
            }
        }
        swap(array, i + 1, r);
        return i + 1;
    }

    private static int hoarePartition(int array[], int l, int r) {
        int pivot = array[r];
        int left = l;
        int right = r;
        while (true) {
            while (array[left] < pivot) {
                left++;
            }
            while (array[right] > pivot) {
                right--;
            }
            if (left >= right) {
                return left;
            }
            swap(array, left, right);
        }
    }

    private static void swap(int array[], int a, int b) {
        int temp = array[a];
        array[a] = array[b];
        array[b] = temp;
    }

}

public static void main(String... args) {
    int array[] = {2, 3, 4, 1, 11, 5, 7, 0, 10, -1, 99, 23};
    quickSort(array);
    System.out.println(Arrays.toString(array));
}

I like that you're testing your code, but you have to visually verify your program's correctness. It would be better to use a testing framework and automate that task.

private static void quickSort(int array[], int l, int r) {

One letter variable names are hard to read.
It would be better to use left and right here.
Alternatively, lhs and rhs are common abbreviations for "left hand side" and "right hand side".
I don't care for them, but they're common enough that I let them slide through code reviews at work.

   int p = lomutoPartition(array, l, r);//hoarePartition(array, l, r);//lomutoPartition(array, l, r);

There's dead code here...
It happened because you're physically changing the code in order to switch between your two different strategies.
It would be better to actually implement the strategy pattern.

You could create a PartitioningStrategy like this.

interface PartitioningStrategy {
    int partition(int numbers[], int left, int right);
}

And then have two implementations.

public class HoarePartitioningStrategy implements PartitioningStrategy {
    public int partition(int numbers[], int left, int right) {
        //...
    }
}

public class LomutoPartitioningStrategy implements PartitioningStrategy {
    public int partition(int numbers[], int left, int right) {
        //...
    }
}

Then you can decide which strategy to use in your main.

public static void main(String... args) {
    int array[] = {2, 3, 4, 1, 11, 5, 7, 0, 10, -1, 99, 23};
    quickSort(array, new LomutoPartitioningStrategy());
    System.out.println(Arrays.toString(array));
}

Nice and clean. Keeps your code open for extension, but closed for modification.


Simple login form with vanilla HTML markup This is just a simple login form with a very vanilla HTML markup, but  I just wanted to get some professional feedback. I know about jslint and try to adhere to there principals where possible.

<script>
//event listeners
$('#sign_in_button').on('click', sign_in);
$('#create_new_account').on('click', create);
$('#create_signup_form').on('click', modal);

function modal() {
    $('#error_box2').text('').hide();
}

function login_success() {
    linkLocation = 'home.html';
    $("body").fadeOut(3000, redirect_page);
}

function redirect_page() {
    window.location = linkLocation;
}

function login_error() {
    var msg = 'Error logging in';
    error(msg);
}

function get_form_input(id,validator) {
    var check ='',
        type =$(id).attr('type');

    if(type != 'checkbox' ) {
        check = $(id).val();
    } else if (type == 'checkbox') {
        check = $(id).attr('checked');
    }

    if(validator(check)){
        return check;
    } else {
        return false;
    }
    return false;
}

///validation functions
function validate_username(id) {
    var username_regex = /^[A-Za-z](?=[A-Za-z0-9_.]{3,20}$)[a-zA-Z0-9_]*\.?[a-zA-Z0-9_]*$/;
    var msg;
    if (!username_regex.test(id)) {
        if ((id.length) > 21){
            msg = "under 20 characters.";
        } else if((id.length) < 4) {
            msg = " more than 4 characters.";
        }else {
            msg = "You can use 4-20 characters, including underscores and one dot.";
        }
        return error(msg);
    }
    return username_regex.test(id);
}

function validate_password(pass) {
    var msg;
    var password_regex = /^.{8,20}$/;

    if (password_regex.test(pass)) {
        return true;
    } else if (pass.length <8) {
        msg = "Passwords should be 8 characters or more.";
        return error(msg);
    } else if (pass.length >21) {
        msg= "twenty character max";
        return error(msg);
    }
    else if (!password_regex.test(password)) {
        msg= "Passwords can be  8-20 characters";
        return error(msg);
    }
}

function validate_email(email) {
      var email_regex = /^([a-zA-Z0-9_\.\-\+])+\@(([a-zA-Z0-9\-])+\.)+([a-zA-Z0-9]{2,4})+$/;
      var msg;
      if (!email_regex.test(email)) {
          msg = "Not a valid email.";
          return error(msg);
      }
     return email;
}

function error(msg) {
    $('#error_box1').slideDown();
    $('#error_box1').text(msg);

    return false;
}

function create_success() {
    login_success();
}

function create_error() {
    var msg = 'Error logging in';
    error(msg);
}

function sign_in() {
    var send = {
        'password' : get_form_input('#password', validate_password),
        'username' : get_form_input('#username', validate_username),
    };
    var url = "http://";

    if (send.username && send.password) {
    console.log(send);
        $.when($.post(url,send)).then(login_success, login_error);
    } else {
        return;
    }

}

function create() {
    var send = {
        'password' : get_form_input('#new_password', validate_password),
        'username' : get_form_input('#new_username', validate_username),
        'email'    : get_form_input('#new_email', validate_email)
    };

    var url = "http://";

    if (send.username && send.password && send.email) {
        console.log(send);
        $.when($.post(url,send)).then(create_success, create_error);
    } else {
        //return error('Error logging in. Try again soon.');
    }
}

</script>

What is the non-quick fix for the name space issue?
Why couldn't a heavy, more complicated application not function effectively with procedural code?

It's in the HTML (script tag). Link it. Keep 'em separated whenever possible.
Not a big deal but when declaring vars you can do them with one var statement, e.g. var a = 0, b = 3;
Add 'use strict;' to look super cool (but I've seen at least one thing that will break)
Fix stuff like linkLocation = 'home.html'; (no var statement - would be global - illegal in strict mode)
window is global in browsers. window.location can just be called as location
If this is to be seen by interviewers, be consistent with your style. For instance, you  mix ' and " for strings. Most prefer ' to differentiate from HTML attribute quotes which are typically double. Mixing both looks like it's not all your code or you're a little sloppy.
When it's just a JavaScript object and not JSON, it doesn't need quotes on the property names. e.g. send = { 'password':...
If it's an interview situation, don't pretend any of the regEx is yours unless you can explain it.
You're pooping all over the global namespace - quick fix would be:

(function(){

//wrap your code here

})()

Don't know what's wrong with code formatting but that puts your functions in scope with anonymous function that automatically fires so it should keep your stuff from being overwritten or overwriting other stuff in the global namespace.

Otherwise it looks like good, clean, straightforward, procedural code. Nothing wrong with that when it's all that's called for. If this were a heavy, complicated, application, I'd be wondering where the objects were at.


Guess My Number Game in Java Basically, what this program does is that this generates a random number, and asks the user to guess it with multiple chances. So, if the user is close, it says that the user is close, and if the user is not close, then it says that the user is too high/low and if the user guesses it right, it congratulates the user.

So, you must now be wondering what I expect from this code review. Well I expect these stuff:

I expect you to point out flaws
I expect you to identify spots which are inefficient and can be optimised.
I expect you to comment on the use of concepts and how I have not used them right.

Main.java:

package com.GuessMyNumber;

import java.util.Scanner;

public class Main {

    public static void main(String[] args) {
        GuessMyNumber guessMyNumber = new GuessMyNumber();
        Scanner scanner = new Scanner(System.in);
        int input;

        guessMyNumber.generateNumber(1, 75);

        System.out.println("Hey there! Let's play Guess my number! Start guessing my number once you're ready.");

        while (!guessMyNumber.numberFound) {
            input = Integer.parseInt(scanner.next());
            System.out.println(guessMyNumber.validateInput(input));
        }
    }
}

GuessMyNumber.java:

package com.GuessMyNumber;

import java.util.Random;

public class GuessMyNumber {
    int randomNumber;
    String response;
    boolean numberFound = false;

    public GuessMyNumber()
    {

    }

    public int generateNumber(int min, int max)
    {
        Random random = new Random();

        this.randomNumber = random.nextInt((max - min) + 1) + min;

        return this.randomNumber;
    }

    public String validateInput(int guessedNumber)
    {
        if(guessedNumber < this.randomNumber && (this.randomNumber - guessedNumber) < 5) {
            this.response = "You're close!";
        }
        if(guessedNumber < this.randomNumber && (this.randomNumber - guessedNumber) > 5) {
            this.response = "You're too low!";
        }
        if(guessedNumber > this.randomNumber && (guessedNumber - this.randomNumber) < 5) {
            this.response = "You're close!";
        }
        if(guessedNumber > this.randomNumber && (guessedNumber - this.randomNumber) > 5) {
            this.response = "You're too high!";
        }
        if(guessedNumber == this.randomNumber) {
            this.response = "Congratulations! You guessed my number!";
            this.numberFound = true;
        }

        return this.response;
    }
}

Robustness

The randomNumber field of GuessMyNumber should not be public.
For example a bad user could do this:

GuessMyNumber guessMyNumber = new GuessMyNumber();
guessMyNumber.generateNumber(1, 75);
guessMyNumber.randomNumber = 25;
guessMyNumber.validateInput(25);  // -> yeay, I win!

That would be devastating. Internal implementation details like this should be hidden, make them private. The same goes for numberFound.

It would be better to remove the generateNumber method,
and initialize the number once and for all in the constructor.
That is, randomNumber can be final, which will make your class more robust,
less prone to errors.
If you want to play multiple games,
you can create a new instance of GuessMyNumber.

Finally, the response variable really doesn't need to be an instance variable,
it should be a local variable in the validateInput method.
In fact, you don't even need to store the message in a local variable,
you can simply return directly.

Simplify

The conditions in validateInput have many duplicated elements.
It would be better to cache the difference of the guess and the target number in a variable and reuse it.

Also some of the conditions are mutually exclusive,
so you should chain them using else if statements.

Unit testing

To make sure that the validation logic works well and correctly handles all corner cases (too high, too low, close, perfect match),
it would be good to unit test it.

Coding style

The convention in Java is to put opening braces on the same line as the statement,
so instead of writing like this:

public String validateInput(int guessedNumber)
{

I recommend to adopt this writing style:

public String validateInput(int guessedNumber) {

Suggested implementation

Putting the above suggestions together (and then some more):

import org.junit.Test;
import java.util.Random;
import static org.junit.Assert.assertEquals;

class GuessMyNumber {

    public static final int CLOSE_LIMIT = 5;
    public static final String MSG_TOO_HIGH = "You're too high!";
    public static final String MSG_TOO_LOW = "You're too low!";
    public static final String MSG_CLOSE = "You're close!";
    public static final String MSG_WIN = "Congratulations! You guessed my number!";

    private final int randomNumber;
    private boolean numberFound = false;

    public GuessMyNumber(int min, int max) {
        Random random = new Random();
        this.randomNumber = random.nextInt((max - min) + 1) + min;
    }

    public String validateInput(int guessedNumber) {
        int diff = guessedNumber - randomNumber;
        if (diff == 0) {
            this.numberFound = true;
            return MSG_WIN;
        }
        int absoluteDiff = Math.abs(diff);
        if (absoluteDiff <= 5) {
            return MSG_CLOSE;
        }
        if (diff < 0) {
            return MSG_TOO_LOW;
        }
        return MSG_TOO_HIGH;
    }
}

public class GuessMyNumberTest {
    private static final int NUMBER = 55;

    private final GuessMyNumber game = new GuessMyNumber(NUMBER, NUMBER);

    @Test
    public void testTooHigh() {
        assertEquals(GuessMyNumber.MSG_TOO_HIGH, game.validateInput(NUMBER + GuessMyNumber.CLOSE_LIMIT + 1));
    }

    @Test
    public void testTooLow() {
        assertEquals(GuessMyNumber.MSG_TOO_LOW, game.validateInput(NUMBER - GuessMyNumber.CLOSE_LIMIT - 1));
    }

    @Test
    public void testCloseHigh() {
        assertEquals(GuessMyNumber.MSG_CLOSE, game.validateInput(NUMBER + GuessMyNumber.CLOSE_LIMIT));
    }

    @Test
    public void testCloseLow() {
        assertEquals(GuessMyNumber.MSG_CLOSE, game.validateInput(NUMBER - GuessMyNumber.CLOSE_LIMIT));
    }

    @Test
    public void testWin() {
        assertEquals(GuessMyNumber.MSG_WIN, game.validateInput(NUMBER));
    }
}


Insertion sort of a linked list This is supposed to be efficient code, but it's taking much longer than what a normal insertion sort would take. I can't identify what's the problem with this insertion sort. Is there an implementation problem?

// function to sort a singly linked list using insertion sort
    void insertionSort(element_t **head_ref)
{
    // Initialize sorted linked list
    element_t *sorted = NULL;

    // Traverse the given linked list and insert every
    // node to sorted
    element_t *current = *head_ref;
    while (current != NULL)
    {
        // Store next for next iteration
        element_t *next = current->next;

        // insert current in sorted linked list
        sortedInsert(&sorted, current);

        // Update current
        current = next;
    }

    // Update head_ref to point to sorted linked list
    *head_ref = sorted;
}

/* function to insert a new_node in a list. Note that this
  function expects a pointer to head_ref as this can modify the
  head of the input linked list (similar to push())*/
void sortedInsert(element_t** head_ref, element_t* new_node)
{
    element_t* current;
    /* Special case for the head end */
    if (*head_ref == NULL || (*head_ref)->val >= new_node->val)
    {
        new_node->next = *head_ref;
        *head_ref = new_node;
    }
    else
    {
        /* Locate the node before the point of insertion */
        current = *head_ref;
        while (current->next!=NULL &&
               current->next->val < new_node->val)
        {
            current = current->next;
        }
        new_node->next = current->next;

current->next = new_node;
    }
}

Code performs as expected.

Insertion sort has an expected O(n2) performance and OP's code has about 0.25*n2 compares per length n of a linked list.

By adding a counti to sortedInsert() and providing various length lists with random data to insertionSort(), the below graph was determined.

void sortedInsert(element_t** head_ref, element_t* new_node) {
  element_t* current;
  if (*head_ref == NULL || (*head_ref)->val >= new_node->val) {

    counti++;  // *****

    new_node->next = *head_ref;
    *head_ref = new_node;
  } else {
    current = *head_ref;

    counti++;  // *****

    while (current->next != NULL && current->next->val < new_node->val) {

      counti++;  // *****

      current = current->next;
    }
    new_node->next = current->next;
    current->next = new_node;
  }
}

No functional implementation problem found.

Lots of review-able issues (format, lack of supporting code, etc.), but OP did not ask for that.


Command Pattern: Does my implementation make the point? Asking for a review of my implementation of a command pattern. In a Editor, words can be entered and the entering of words can be undone.

General purpose Interface

public interface Command {
    void execute();
}

The concrete write word command

// Command
public class WriteCommand implements Command {

    private Editor editor;
    private String word;

    public WriteCommand(String w, Editor e) {
        this.word = w;
        this.editor = e;
    }

    public String getWord() {
        return this.word;
    }

    public void execute() {
        this.editor.addToWordList(this.getWord());
    }

}

The undo command

public class UndoCommand implements Command {

    private Editor editor;

    public UndoCommand(Editor e) {
        this.editor = e;
    }

    @Override
    public void execute() {
        editor.removeFromWordList();
    }
}

The editor (i.e. receiver, i believe):

// Receiver
public class Editor {

    private List<String> wordList;

    public Editor() {
        wordList = new ArrayList<>();
    }

    public void addToWordList(String w) {
        this.wordList.add(w);
    }

    public void removeFromWordList() {
        this.wordList.remove(this.wordList.size()-1);
    }

    List<String> getWordList() {
        return this.wordList;
    }

}

The invoker keeping the command queue:

public class Invoker {
    private List<Command> undoList;
    private List<Command> redoList;

    public Invoker() {
        undoList = new ArrayList<>();
        redoList = new ArrayList<>();

    }

    public void executeCommand(Command c) {
        undoList.add(c);
        c.execute();
    }
}

The client responsible for creating the commands:

public class Client {
    public static void main(String[] args) {

        Editor e = new Editor();

        Invoker invoker = new Invoker();

        invoker.executeCommand(new WriteCommand("Hello", e));
        invoker.executeCommand(new WriteCommand("World", e));
        System.out.println(e.getWordList());

        invoker.executeCommand(new UndoCommand(e));
        System.out.println(e.getWordList());

    }
}

If this was a school exercise

It's a basic command pattern without any safety checks against programming errors. :) It would be a good idea to check that a word can not be removed from an empty list.

If this was a job interview assignment

While the code shows that you know how to code a command pattern it doesn't tell me if you understand why one would want to use it. Having WriteCommand and UndoCommand objects going around in isolation makes very little sense. And domain-wise, writing to a text editor with command objects is probably not something that I would do.

Instead put the command pattern into actual use by, for example, integrating it to the editor in the form of an undo/redo stack. Appending strings to the editor automatically create undo commands into the stack and performing the undo command executes the last command and adds a corresponding redo command into the redo stack.


Logical shift with signed numbers I am going through the Coursera course  The Hardware/Software Interface and because it has already ended, I would not be able to get any feedback on the code.

The assignment is:

  LogicalShift: shift x to the right by n, using a logical shift. We can assume that 0 ≤ n ≤ 31.

I am wondering if shifting by n and then back by 1 is a good solution.

int logicalShift(int x, int n) {
  int ba = 1<<31; // set MSB to 1
    int a = x & ba; // MSB will be 1 if negative or 0 if positive number

    int numShifted = x>>n; //shift the number
    int msbShifted = (a >> n) << 1; //shift the MSB by n -1

    //if negative ^ the leading 1's to create a logical shift
    return msbShifted ^ numShifted;
}

The C standard does not guarantee that an int is 32 bits.  Even though the problem guarantees that 0 ≤ n ≤ 31, your code fails if run on a machine with a 64-bit int.  One way to resolve that problem is to be explicit about the size of your inputs:

#include <stdint.h>

int32_t logicalShift(int32_t x, int n) {
    …
}

The C standard says (C99 §6.5.7):

  The result of E1 >> E2 is E1 right-shifted E2 bit positions. If E1 has an unsigned type or if E1 has a signed type and a nonnegative value, the value of the result is the integral part of the quotient of \$\dfrac{\mathrm{E1}}{2 ^ \mathrm{E2}}\$. If E1 has a signed type and a negative value, the resulting value is implementation-defined.

Therefore, your best bet is to treat x as an unsigned type:

int logicalShift(int x, int n) {
    return (unsigned int)x >> n;
}


A universal email crawler I've written a script in python using requests module in combination with selenium along with regex to parse email address (if any exists) from any website. I tried to create it in such a way so that it can traverse javascript enabled sites as well. My crawler is supposed to track any website link (given in it's list storage) then find contact us or about us etc keywords from that page and parsing the matching link it will go to the target page and using regular expression it will finally parse the email address from that page. It scrapes the email address along with the link address where it parses the email from. I tried with several links and most of the cases it succeeds. I know it's very hard to create a full-fledged one but I tried and it is not despairing at all. Any suggestion to improve this crawler will be vastly appreciated.

Here is what I have written:

from selenium import webdriver
import requests; import re
from lxml.html import fromstring
from urllib.parse import urljoin

main_url = [
'http://advanta.in/',
'http://warnaengineeringworks.com/',
'http://unifrostindia.com/',
'http://www.superrefrigerations.com/',
'http://www.westernequipments.com/',
]

def search_item(driver,url):
    driver.get(url)
    tree = fromstring(driver.page_source)
    for item in tree.xpath("//a/@href"):
        if "contact us" in item.lower() or "about us" in item.lower() or "contact" in item.lower() or "about" in item.lower():
            get_item(urljoin(url,item))
            break

def get_item(link):
    response = requests.get(link).text
    try:
        email = re.findall(r'[a-zA-Z0-9_.+-]+@[a-zA-Z0-9-]+\.[a-zA-Z0-9-.]+',response)[0]
    except: pass
    if email:
        print(link,email)

if __name__ == '__main__':
    driver = webdriver.Chrome()
    try:
        for link in main_url:
            search_item(driver,link)
    finally:
        driver.quit()

There are several ways we can go to make the code cleaner and faster. Grouping by the scope.

search_item() function

link would probably be a better variable name than a less descriptive item
move the substrings you are looking for into a constant - defined as a list of keywords
then, you can use any() instead of multiple or conditions
fromstring() returns a root node of the tree and not the tree itself - would probably be more logical to name the variable root instead of tree
do not re-do item.lower() - do it once

You may get something like this in the end:

KEYWORDS = ["contact us", "about us", "contact", "about"]

def search_item(driver,url):
    driver.get(url)
    root = fromstring(driver.page_source)

    for link in root.xpath("//a/@href"):
        link = link.lower()
        if any(keyword in link for keyword in KEYWORDS):
            get_item(urljoin(url, link))
            break

get_item() function

you should pre-compile your regular expression
use .search() instead of .findall(), since you are looking for a single match
this would then allow you to avoid handling a bare exception and switch to LBYL approach instead of the EAFP

You may get something like:

EMAIL_PATTERN = re.compile(r'[a-zA-Z0-9_.+-]+@[a-zA-Z0-9-]+\.[a-zA-Z0-9-.]+')

def get_item(link):
    response = requests.get(link).text

    match = EMAIL_PATTERN.search(response)
    if match:
        print(link, match.group(0))

Few other general comments

not sure if "contact us" and "about us" (with a space) would ever appear in an href value..
re-group your imports as per PEP8 recommendations and avoid having multiple different imports on the same line
main_url should probably named be something like start_urls (you need a multiple of "url" since you have a list of them). And, you need to indent the items since you are defining your list on multiple lines:

start_urls = [
    'http://advanta.in/',
    'http://warnaengineeringworks.com/',
    'http://unifrostindia.com/',
    'http://www.superrefrigerations.com/',
    'http://www.westernequipments.com/',
]

consider adding documentation strings to your functions


Comparison to infinity when checking if a point is above a line I have a function that is being called in a tight loop. I've profiled my code and this is where my largest bottleneck is. The function is fairly simple: it checks if a point in (x,y) form is above a line in (slope, intercept) form.

The problem is, it also has to deal with the case where the slope is positive infinity, and the intercept gives the x-intercept. In this case, the function should return True if the point is to the right of the line.

Here's the function written out how it was when I noticed the bottleneck:

def point_over(point, line):
    """Return true if the point is above the line.
    """
    slope, intercept = line
    if slope != float("inf"):
        return point[0] * slope + intercept < point[1]
    else:
        return point[0] > intercept

On my machine, this is how fast it runs:

>>> timeit("point_over((2.412412,3.123213), (-1.1234,9.1234))",
           "from __main__ import point_over", number = 1000000)
1.116534522825532

Since this function is being called a lot, I've inlined it to avoid the function call overhead. The inlined version is fairly simple:

point[0] * line[0] + line[1] < point[1] if line[0] != float('inf') else point[0] > line[1]

And performs similarly in timeit:

>>> timeit("point[0] * line[0] + line[1] < point[1] if line[0] != float('inf') else point[0] > line[1]",
           "point, line = ((2.412412,3.123213), (-1.1234,9.1234))", number = 1000000)
0.9410096389594945

However, this one line of code is still hogging the largest proportion of execution time in my code, even after being inlined.

Why does the comparison to float('inf') take longer than a number of calculations as well as a comparison? Is there any way to make this faster?

As an example of what I'm claiming, here are the speeds of two parts of my ternary statement separated and timed:

>>> timeit("line[0] != float('inf')",
           "point, line = ((2.412412,3.123213), (-1.1234,9.1234))", number = 1000000)
0.528602410095175
>>> timeit("point[0] * line[0] + line[1] < point[1]",
           "point, line = ((2.412412,3.123213), (-1.1234,9.1234))", number = 1000000)
0.48756339706397966

My primary question is why it takes so long to do the comparison to infinity. Secondary is any tips on how to speed these up. Ideally there exists some magical (even if incredibly hacky) way to half the execution time of this line, without dropping down to C. Otherwise, tell me I need to drop down to C.

It would be faster to use the math.isinf() function:

>>> from math import e, pi, isinf
>>> s = [0, float('inf'), e, pi]
>>> map(isinf, s)
[False, True, False, False]

The reason your current check is taking so long is due to how much work it has to do:

>>> from dis import dis

>>> def my_isinf(f):
        return f == float('Inf')

>>> dis(my_isinf)
  2           0 LOAD_FAST                0 (f)
              3 LOAD_GLOBAL              0 (float)
              6 LOAD_CONST               1 ('Inf')
              9 CALL_FUNCTION            1
             12 COMPARE_OP               2 (==)
             15 RETURN_VALUE

There is a global load which involves a dictionary lookup.  There is a two-arguments function call.  The Float constructor has to parse the string and then allocate (and refcount) a new float object in memory.  Then the comparison step dispatches its work to float.__eq__ which returns a boolean value which is then the fed to the if-statement.


Saving and uploading images in a database I am working on a small method that saves uploaded images and records the images in a database.

public function setEventImages($event_id){
            foreach($_FILES['event-images']['tmp_name'] as $tmp_name){
                $imgName = $this->imgPath . $this->random_name . $_FILES['event-images']['name'];
                move_uploaded_file($tmp_name, $imgName);
                $stmt = $this->dbh->prepare("INSERT INTO adrenaline_junkies_uk_event_images (event_image_name, event_id) VALUES (?,?)");
                $stmt->bindParam(1, $imgName, PARAM_STR);
                $stmt->bindValue(2, $event_id, PARAM_INT);
                $stmt->execute();
            }
        }

How can this be made more efficient?

Yes, there certainly is. Prepared statements can be reused, over and over again. The way your code works is: it's creating the same prepared statement over and over again, and binds the new parameters right after doing so, why not move the prepare call outside of the loop?

$stmt = $this->dbh->prepare("INSERT INTO adrenaline_junkies_uk_event_images (event_image_name, event_id) VALUES (?,?)");
foreach($_FILES['event-images']['tmp_name'] as $tmp_name)
{
    $imgName = $this->imgPath . $this->random_name . $_FILES['event-images']['name'];
    move_uploaded_file($tmp_name, $imgName);
    $stmt->bindParam(1, $imgName, PARAM_STR);
    $stmt->bindValue(2, $event_id, PARAM_INT);
    $stmt->execute();
    //done for now, to re-use a statement, close its cursor first?
    //though this is done implicitly, best do it ASAP
    //especially when using SELECT + PDOStatement::fetch in a loop
    $stmt->closeCursor();
}

For a start, that saves you the trouble of that prepare call in the loop, increasing readability and, if all goes well, it'll also improve performance a bit.

Now, I do have some other niggles with your code, like this statement:

$imgName = $this->imgPath . $this->random_name . $_FILES['event-images']['name'];

Now, I don't know how your $_FILES array is structured, but check some of the contributed notes on the man page there are quite a few snippets that deal with $_FILES arrays recursively.

Another thing, and this is really a detail, is that you don't seem to be using transactions. When you're in the process of inserting images, one by one, and you reach a point where your script fails (either through timeout or another reason), you might end up with partially inserted data. That's not great. Transactions are an easy way to avoid this:

try
{
    $this->dbh->beginTransaction();//start transaction here
    $stmt = $this->dbh->prepare("INSERT INTO adrenaline_junkies_uk_event_images (event_image_name, event_id) VALUES (?,?)");
    foreach($_FILES['event-images']['tmp_name'] as $tmp_name)
    {
        $imgName = $this->imgPath . $this->random_name . $_FILES['event-images']['name'];
        move_uploaded_file($tmp_name, $imgName);
        $stmt->bindParam(1, $imgName, PARAM_STR);
        $stmt->bindValue(2, $event_id, PARAM_INT);
        $stmt->execute();
    }
    $this->dbh->commit();//ok, all was well, commit inserted data
}
catch (PDOException $e)
{
    $this->dbh->rollBack();//undo inserts, because not all of them were successful.
    throw $e;//rethrow, let caller deal with the exception
}

Now this is looking better, I do believe. But like I said in the comments, bindParam uses references:

public bool PDOStatement::bindParam ( mixed $parameter , mixed &$variable [, int $data_type = PDO::PARAM_STR [, int $length [, mixed $driver_options ]]] )
                                                               ^^ & => reference

, which (in a loop) can lead to unexpected behaviour from time to time, so I'd rather pass my values to PDOStatement::execute, if I were you:

    foreach($_FILES['event-images']['tmp_name'] as $tmp_name)
    {
        $imgName = $this->imgPath . $this->random_name . $_FILES['event-images']['name'];
        move_uploaded_file($tmp_name, $imgName);
        $stmt->execute(array($imgName, $event_id));
    }

But for completeness' sake, here's the same thing using bindParam outside of the loop:

$stmt = $this->dbh->prepare("INSERT INTO adrenaline_junkies_uk_event_images (event_image_name, event_id) VALUES (?,?)");
$imgName = null;
$stmt->bindParam(1, $imgName, PARAM_STR);
//$event_id might fail, because this var is argument
$stmt->bindParam(2, $event_id, PARAM_INT);
//if it fails, try:
$eventId = &$event_id;
$stmt->bindParam(2, $eventId, PARAM_INT);
foreach($_FILES['event-images']['tmp_name'] as $tmp_name)
{
    $imgName = $this->imgPath . $this->random_name . $_FILES['event-images']['name'];
    move_uploaded_file($tmp_name, $imgName);
    $stmt->execute();
}

Lastly: You're using both bindParam and bindValue as though they both do the same thing. Now at first glance, they do, but be weary of references. read this, to see why references in loops are risky


Base class for implementing IComparable If I create a class that implements IComparable<T>, I must implement CompareTo<T>. It is also recommended that I implement IEquatable<T> and the non-generic IComparable. If I do all that, I am required or encouraged to:

Override GetHashCode()
Implement CompareTo(Object)
Override Equals(Object)
Implement Operator ==(T, T)
Implement Operator !=(T, T)
Implement Operator >(T, T)
Implement Operator <(T, T)
Implement Operator >=(T, T)
Implement Operator <=(T, T)

That's 9 additional methods, most of which depend on the logic that compares two instances of the class. Rather than having to implement all those methods in any class that implements IComparable<T>, I decided to create a base class that implements IComparable<T> and the other recommended interfaces (similar to the way Microsoft provides Comparer as a base class for implementations of IComparer<T>)

It doesn't make sense to compare instances of two different classes that each inherit from the base class. preventing that was the main reason for making the class generic (although it makes coding a derived class a little more complicate).

I would like to ask for a review of the code for the base class. Am I missing something? Can it be simplified? Is this a bad idea?

Here is the base class

public abstract class Comparable<T> : IComparable, IComparable<T>, IEquatable<T> where T: Comparable<T> {
    public abstract override int GetHashCode();

    public abstract int CompareTo(T other);

    public int CompareTo(object obj) {
        T other = obj as T;
        if (other == null && obj != null) {
            throw new ArgumentException($"Objects of type {typeof(T).Name} can only be compared to objects of the same type", nameof(obj));
        }
        return CompareTo(other);
    }

    public override bool Equals(object obj) {
        return CompareTo(obj) == 0;
    }

    new public bool Equals(T other) {
        return CompareTo(other) == 0;
    }

    private static int Compare(Comparable<T> comp1, Comparable<T> comp2) {
        if (comp1 == null) {
            return ((comp2 == null) ? 0 : -1);
        }
        return comp1.CompareTo(comp2);
    }

    public static bool operator == (Comparable<T> comp1, Comparable<T> comp2) {
        return Compare(comp1, comp2) == 0;
    }

    public static bool operator != (Comparable<T> comp1, Comparable<T> comp2) {
        return Compare(comp1, comp2) != 0;
    }

    public static bool operator > (Comparable<T> comp1, Comparable<T> comp2) {
        return Compare(comp1, comp2) > 0;
    }

    public static bool operator < (Comparable<T> comp1, Comparable<T> comp2) {
        return Compare(comp1, comp2) < 0;
    }

    public static bool operator >= (Comparable<T> comp1, Comparable<T> comp2) {
        return Compare(comp1, comp2) >= 0;
    }

    public static bool operator <= (Comparable<T> comp1, Comparable<T> comp2) {
        return Compare(comp1, comp2) <= 0;
    }
}

Below is a minimal implementation of the base class.

public class SeasonCompare : Comparable<SeasonCompare> {
    public int Number {get; set;}

    public override int GetHashCode() {
        return Number;
    }

    public override int CompareTo(SeasonCompare other) {
        if (other == null) {
            return 1;
        }
        return Number.CompareTo(other.Number);
    }
}

It is also recommended that I implement ... the non-generic IComparable.

I don't see that recommendation in the current doc for IComparable. I would recommend against it: having the non-generic method turns compile-time errors into runtime errors, which are more expensive to find and fix.

      private static int Compare(Comparable<T> comp1, Comparable<T> comp2) {
          if (comp1 == null) {
              return ((comp2 == null) ? 0 : -1);
          }
          return comp1.CompareTo(comp2);
      }

For consistency, this requires that subclasses guarantee that CompareTo(null) returns a positive value, but that requirement isn't documented.

Perhaps a better solution would be:

        private static int Compare(Comparable<T> comp1, Comparable<T> comp2) {
            if (comp1 == null) {
                return comp2 == null ? 0 : 0.CompareTo(comp2.CompareTo(comp1));
            }
            return comp1.CompareTo(comp2);
        }

That way the only requirement is that CompareTo(null) be consistent.

There may be a more elegant way of inverting the sense of a comparison, but that's the easiest one I can think of which doesn't fail on corner cases.


Haskell marking procedure for non-unique lists I'm writing a function for generating solutions for a backtracking search problem. To that end, I need to mark an item from a list by removing it from that list, and placing it in a second list.

So I have a pair of lists:

non-marked items
marked items

and my method generates all distinct list pairs of possible markings. Because the list may contain duplicates, I'm selecting the marked item via the index.

Example:

mark 0 ([1,2,2],[]) == ([2,2],[1])
selections ([1,2,2],[]) == [([2,2],[1]),([1,2],[2])]

Code so far:

mark :: Int -> ([a], [a]) -> ([a], [a])
mark i (src,tgt) = (src',tgt')
    where
        src' = let (ys,zs) = splitAt i src in ys ++ (tail zs)
        tgt' = tgt ++ [e]
        e = src !! i

selections :: Eq a => ([a],[a]) -> [([a],[a])]
selections pair@(left,_) = nub [ mark i pair | i <- [0..((length left)-1)] ]

I'm not happy with the implementation: it seems crude, looks ugly, and I think it's obvious that someone with a background in imperative languages wrote this function.

Can this be solved more elegantly, with Array or other list mechanisms, e.g. a fold?

Found a deceptively simple solution.

selections (left,right) = nub [(delete o left,right ++ [o]) | o <- left]

List comprehensions are great.


Calling functions with different variable types I need to call the function that will get the value of a datatable cell with the ColIndex, either a text (column name) or an integer (columnindex).

To avoid writing two functions that, I wrote this code:

Public Function DTableCellValue(ByRef RowData As DataRow, _
                                    ByVal ColIndex As Object, _
                                    Optional ByVal DefaultVal As String = "") As Object
        Try
            Dim ret As Object = RowData.Item(ColIndex)

            If IsDBNull(ret) Then
                Return DefaultVal
            Else
                Return ret
            End If
        Catch ex As Exception
            Throw
        End Try
    End Function

Can you tell me if it uses bad coding practices or is potentially problematic?

If you use this method quit often by passing an Integer you will get performance problems because of the boxing and unboxing of the struct.

My advice would be to write two methods one using an Integer and the other using a String as argument.

Based on the naming guidelines you should name methods by using a verb or a verb phrase. DTableCellValue looks by its name more as a property than a method.

Based on the same guidelines method arguments should be named using camelCase casing. Although VB.NET isnt case sensitive I would like to suggest to follow these guidelines.

The try..catch is in its current form senseless because if an exception is thrown you are just rethrowing it.

A construct like

If condition Then
    Return Value
Else
    Return otherValue
End If

can be simplified by removing the redundant else like so

If condition Then
    Return Value
End If
Return otherValue

because if the condition is true the else won't be reached.

Implementing the mentioned points (leaving aside the method name) leads to two methods like so

Public Function DTableCellValue(ByRef rowData As DataRow, _
                                    ByVal colIndex As Integer, _
                                    Optional ByVal defaultVal As String = "") As Object
    Dim ret As Object = rowData.Item(colIndex)

    If IsDBNull(ret) Then
        Return defaultVal
    End If

    Return ret

End Function

Public Function DTableCellValue(ByRef rowData As DataRow, _
                                    ByVal colIndex As String, _
                                    Optional ByVal defaultVal As String = "") As Object
    Dim ret As Object = rowData.Item(colIndex)

    If IsDBNull(ret) Then
        Return defaultVal
    End If

    Return ret

End Function


Search for longest substring in alphabetical order in a given string I would like some help with a review of a program that I wrote to identify the longest continuous sub-string that is in alphabetical order within a string.  This is a JavaScript adaptation of an exercise originally written in Python.  I am new to JavaScript.  I would like any help identifying ways that this program could be written more efficiently, specifically if there are techniques or conventions in JavaScript that could improve this program.

One thing that was different between Python and JavaScript was that I had to split the string into an array in order to perform the comparison of characters.  I saw another example where this was the technique to assess the strings in JavaScript.

Is there a better way in JavaScript to assess strings when looking for components other than creating an array?

The second part of the program that I would like help with is the way that the search executes immediately, instead of waiting for the call to the function with the button.  I left the label in the code to show that the alphabetical sub-string in the place holder language is immediately identified, instead of waiting for a call to the function with the button.

What should I have done to make the function wait for a new search string?

// test string azcbobobegghakl, should yield beggh
function findSubStr() {
  document.getElementById("main").style.visibility = "visible";
  var string = document.getElementById("enterString").value;
  var lastSub = [],
    longestSub = [];
  var Arr = string.split('');
  for (var i = 0; i < Arr.length; i++) {
    if (Arr[i] >= lastSub.slice(-1)) {
      lastSub = lastSub + Arr[i];
      if (lastSub.length > longestSub.length) {
        longestSub = lastSub;
      }
    } else {
      lastSub = Arr[i];
    }
  }
  document.getElementById("main").innerHTML = longestSub;
}

console.log(findSubStr());
#start {
  top: 25%;
  left: 40%;
  position: absolute;
}
#main {
  top: 50%;
  left: 45%;
  font-size: 1.5em;
  position: absolute;
  visibility: hidden;
}
#enterString {
  color: gray;
}

  <input id="enterString" value="Please enter string">
  <button id="search" onclick="findSubStr()">Sub-String Search</button>

A pen on CodePen.

You don't have to split a string into an Array, you can work on a String directly.

Note that I have removed you inline onclick handler. https://en.wikipedia.org/wiki/Unobtrusive_JavaScript

How your ES3 code probably should have looked

var main = document.getElementById('main');
main.style.visibility = 'visible';
var enterString = document.getElementById('enterString');
// test string azcbobobegghakl, should yield beggh
function findSubStr() {
  var string = enterString.value;
  var lastSub = '';
  var longestSub = '';
  for (var index = 0; index < string.length; index += 1) {
    var character = string.charAt(index);
    if (character >= lastSub.slice(-1)) {
      lastSub += character;
      if (lastSub.length > longestSub.length) {
        longestSub = lastSub;
      }
    } else {
      lastSub = character;
    }
  }
  if (main.firstChild) {
    main.firstChild.nodeValue = longestSub;
  } else {
    main.appendChild(document.createTextNode(longestSub));
  }
  return false;
}

document.getElementById('search').onclick = findSubStr;
#start {
  top: 25%;
  left: 40%;
  position: absolute;
}
#main {
  top: 50%;
  left: 45%;
  font-size: 1.5em;
  position: absolute;
  visibility: hidden;
}
#enterString {
  color: gray;
}

  <input id="enterString" placeholder="Please enter string">
  <button id="search">Sub-String Search</button>

How it could have looked in ES5

var forEach = Function.prototype.call.bind(Array.prototype.forEach);
var main = document.getElementById('main');
main.style.visibility = 'visible';
var enterString = document.getElementById('enterString');
// test string azcbobobegghakl, should yield beggh
function findSubStr() {
  var lastSub = '';
  var longestSub = '';
  forEach(enterString.value, function(character) {
    if (character >= lastSub.slice(-1)) {
      lastSub += character;
      if (lastSub.length > longestSub.length) {
        longestSub = lastSub;
      }
    } else {
      lastSub = character;
    }
  });
  main.textContent = longestSub;
}

document.getElementById('search').addEventListener('click', findSubStr, false);
#start {
  top: 25%;
  left: 40%;
  position: absolute;
}
#main {
  top: 50%;
  left: 45%;
  font-size: 1.5em;
  position: absolute;
  visibility: hidden;
}
#enterString {
  color: gray;
}

  <input id="enterString" placeholder="Please enter string">
  <button id="search">Sub-String Search</button>

How it could have looked in ES6

const main = document.getElementById('main');
main.style.visibility = 'visible';
const enterString = document.getElementById('enterString');
// test string azcbobobegghakl, should yield beggh
function findSubStr() {
  let lastSub = '';
  let longestSub = '';
  for (let character of enterString.value) {
    if (character >= lastSub.slice(-1)) {
      lastSub += character;
      if (lastSub.length > longestSub.length) {
        longestSub = lastSub;
      }
    } else {
      lastSub = character;
    }
  }
  main.textContent = longestSub;
}

document.getElementById('search').addEventListener('click', findSubStr, false);
#start {
  top: 25%;
  left: 40%;
  position: absolute;
}
#main {
  top: 50%;
  left: 45%;
  font-size: 1.5em;
  position: absolute;
  visibility: hidden;
}
#enterString {
  color: gray;
}

  <input id="enterString" placeholder="Please enter string">
  <button id="search">Sub-String Search</button>

There are surely other ways that you could have tackled this, but essentially there is nothing wrong with your approach.

If you have any particular questions about the changes/code posted, then just comment and I will expand in the areas that most need it.


Count characters, words, and lines in files (wc in rust) I wrote a barebones version of wc in rust. wc is a program that counts the number of characters, words, and lines in a file and outputs those values to the command line. Here is an example of the output:
   9   25  246 Cargo.toml
  52  163 1284 src/main.rs
  61  188 1530 total

My version currently lacks the proper output alignment, and it doesn't print the total (it also lacks the command line options, and it panics when fed a directory). But I would like to get some feedback before I go any further.
use std::env;
use std::fs::read_to_string;

struct InputFile {
    words: u32,
    lines: u32,
    characters: u32,
    name: String,
}

impl InputFile {
    fn new(name: &String) -> Self {
        let content = read_to_string(name).unwrap();
        let (mut characters, mut words, mut lines) = (0, 0, 0);
        let mut spaced: bool = false;
        for c in content.chars() {
            if c as u8 != 0 {
                characters += 1;
            }
            if c != ' ' && c != '\n' {
                spaced = false
            }
            if c == '\n' {
                lines += 1;
                if !spaced {
                    words += 1;
                    spaced = true;
                }
            }
            if c == ' ' && !spaced {
                words += 1;
                spaced = true;
            }
        }
        Self { lines, words, characters, name: name.to_string() }
    }
}

impl std::fmt::Display for InputFile {
    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
        write!(f, "{} {} {} {}",
            self.lines, self.words, self.characters, self.name
        )
    }
}

fn main() {
    let files: Vec<String> = env::args().collect();
    for f in &files[1..] {
        println!("{}", InputFile::new(f));
    }
}

InputFile doesn't appear to be a file — a better name might be Statistics.
usize is conventionally used for indexes and sizes instead of u32.
Don't take an argument by &String.  Since the algorithm works for not only files but other streams of characters as well, consider taking a BufRead argument.
Reading the whole file into memory isn't efficient — an alternative is to use the utf8-chars crate to identify characters.
c as u8 != 0 is just c != '\0'.
It took me a while to figure out what spaced does — in_word might be a better name.
char::is_whitespace checks for all kinds of whitespace.
In main, Iterator::skip allows you to skip one argument without allocating a Vec.

Here's my version:
use {
    anyhow::Result,
    parse_display::Display,
    std::{
        env,
        fs::File,
        io::{BufRead, BufReader},
    },
    utf8_chars::BufReadCharsExt,
};

#[derive(Clone, Debug, Display)]
#[display("{characters} {words} {lines}")]
struct Stats {
    characters: usize,
    words: usize,
    lines: usize,
}

impl Stats {
    fn new<R: BufRead>(mut reader: R) -> Result<Self> {
        let mut stats = Stats {
            characters: 0,
            words: 0,
            lines: 0,
        };
        let mut in_word = false;

        for c in reader.chars_raw() {
            let c = c?;

            if c != '\0' {
                stats.characters += 1;
            }

            if !c.is_whitespace() {
                in_word = true;
            } else if in_word {
                stats.words += 1;
                in_word = false;
            }

            if c == '\n' {
                stats.lines += 1;
            }
        }

        Ok(stats)
    }
}

fn main() -> Result<()> {
    for path in env::args().skip(1) {
        let file = BufReader::new(File::open(&path)?);
        let stats = Stats::new(file)?;
        println!("{} {}", stats, path);
    }

    Ok(())
}


Creating every possible combination until a code word is found I understand my main limitation is my computer's ability to compare millions of combinations but I wondered if there is any way to improve the speed at which the comparisons take place.

My code is as follows:

code = open("C:\\Users\\User\\Desktop\\code.txt").read()
dictionary = open("C:\\Users\\User\\Desktop\\dictionary.txt").read()
precomputed = open("C:\\Users\\User\\Desktop\\precompute.txt").read()
code_found = False
from timeit import default_timer as timer
import itertools
printable = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789' #"!"#$%&\'()*+,-./:;<=>?@[\]^_`{|}~'

This sets out the file locations including the file where the 'password' is (named code).

Precomputed is a list of all combinations Printable can make saved to a .txt file:

def bruteforce_by_precomputed():
    code_found = False
    start=timer()
    for attempt in precompute.splitlines():
        if attempt == code:
            print("the word is", attempt)
            code_found = True
            break
        #if attempt != code:
            #print(attempt, "not word")
    if code_found != True:
        print("word not in dictionary")
    end = timer()
    taken = (end-start)
    print("time taken =  :", taken)

This section is comparing each precomputed value within the .txt file to check if it matches the code:

def dictionary_attack():
    for attempt in dictionary.splitlines():
        if attempt == code:
            print("the word is", attempt)
            code_found = True
            break
        #if attempt != code:
            #print(attempt, "not word")
    if code_found != True:
        print("word not in dictionary")

This section uses a .txt file with every English word for a match:

def infinately_increasing_brute():
    import itertools
    lengthoforce = 0
    while attempt != code:
        lengthoforce += 1
        for brute_attempt in itertools.product(printable, repeat = lengthoforce):
            attempt = ''.join(brute_attempt)
            if attempt == code:
                print("succesful brute, your word is: ", attempt)
                break

This section uses .product from itertools to find every combination of Printable and if not found increase the length of combination by 1:

def custom_brute():
    lengthoforce = int(input("what length to force?"))
    start = timer()
    for brute_attempt in itertools.product(printable, repeat = lengthoforce):
        attempt = ''.join(brute_attempt)
        if attempt == code:
            print("")
            print("succesful brute, your word is: ", attempt)
            print("")
            end = timer()
            print(end-start, "seconds were taken to find", attempt)
            print("")
            break

This section simply allows the user to enter the length of the combinations to compare.

I have another section of code which creates all possible combinations and saves them to a .txt file. Instead of generating possibilities, it just reads and compares, but I would like to avoid this and generate combinations and compare.

You should learn about separating concerns. Currently your functions have two concerns, finding the right word and timing how long it takes. You can take out this part by writing a decorator, that allows timing any function you decorate it with.

Next, your bruteforce_by_precomputed and your dictionary_attack are really the same function, just with a different dictionary behind it. You could just let the function accept it as an argument. And let it accept the word do decrypt as well, while we are at it. Normally I would also move the reading of the file here, but since you might want to decrypt more than one word, it could be a good idea to read the files only once.

I would add a maximum length for the infinitely_increasing_brute and just call it brute_force. No point in waiting for the heat death of the universe just to crack that one password...

You should close the files you open. One easy way to do that is to use with.

You should put your main running code under a if __name__ == "__main__ guard. This allows you to easily import the functions defined here in another script, without executing the encryption itself.

You should have a look at the string module, it has many pre-defined sets of characters, including string.ascii_lowercase, string.punctuation and string.printable.

It is not exactly clear if your code file contains only a single word. I modified it so that it can contain many words, separated by newlines.

Putting this together, your code becomes:

import itertools
from time import clock
from string import ascii_lowercase, ascii_uppercase, digits, punctuation

printable = ascii_lowercase + ascii_uppercase + digits

def timeit(func):
    def wrapper(*args, **kwargs):
        starting_time = clock()
        result = func(*args, **kwargs)
        ending_time = clock()
        print('Duration: {}'.format(ending_time - starting_time))
        return result
    return wrapper

@timeit
def dictionary_attack(encrypted, dictionary):
    for attempt in dictionary:
        if attempt == encrypted:
            return attempt

@timeit
def increasing_brute(encrypted, max_len=10):
    for length in range(1, max_len + 1):
        for brute_attempt in itertools.product(printable, repeat=length):
            attempt = ''.join(brute_attempt)
            if attempt == encrypted:
                return attempt

@timeit
def custom_brute(encrypted, length):
    for brute_attempt in itertools.product(printable, repeat=length):
        attempt = ''.join(brute_attempt)
        if attempt == code:
            return attempt

if __name__ == "__main__":
    with open("C:\\Users\\User\\Desktop\\dictionary.txt") as dictionary:
        dictionary_words = dictionary.read().splitlines()
    with open("C:\\Users\\User\\Desktop\\precompute.txt") as precomputed:
        precomputed_words = precomputed.read().splitlines()
    with open("C:\\Users\\User\\Desktop\\code.txt") as code:
        for encrypted in code:
            word = dictionary_attack(encrypted, precomputed_words)
            if word is not None:
                print "The word is", word
                continue
            word = dictionary_attack(encrypted, dictionary_words)
            if word is not None:
                print "The word is", word
                continue
            word = custom_brute(encrypted, int(input("what length to force?")))
            if word is not None:
                print "The word is", word
                continue
            word = increasing_brute(encrpyted)
            if word is not None:
                print "The word is", word
                continue

This could be improved further by making the attack functions generators that yield attempts and a single attack function that performs the comparison using a sequence of functions:

def dictionary_attack(encrypted, dictionary):
    for attempt in dictionary:
        yield attempt

def increasing_brute(encrypted, max_len=10):
    for length in range(1, max_len + 1):
        for brute_attempt in itertools.product(printable, repeat=length):
            yield ''.join(brute_attempt)

def custom_brute(encrypted, *args):
    length = int(input("what length to force?"))
    for brute_attempt in itertools.product(printable, repeat=length):
        yield ''.join(brute_attempt)

@timeit
def attack(encrypted, attack_funcs, attack_args):
    for func, args in zip(attack_funcs, attack_args):
        for attempt in func(encrypted, *args):
            if attempt == encrypted:
                return attempt

if __name__ == "__main__":
    with open("C:\\Users\\User\\Desktop\\dictionary.txt") as dictionary:
        dictionary_words = dictionary.read().splitlines()
    with open("C:\\Users\\User\\Desktop\\precompute.txt") as precomputed:
        precomputed_words = precomputed.read().splitlines()

    attack_funcs = dictionary_attack, dictionary_attack, custom_brute, increasing_brute
    attack_args = [precomputed_words], [dictionary_words], [10], []
    with open("C:\\Users\\User\\Desktop\\code.txt") as code:
        for encrypted in code:
            word = attack(encrypted, attack_funcs, attack_args)


Setting QProgressBar values based on selection I have a function which writes a report based on technologies selected by the user via QCheckBoxes. Depending on the number of technologies chosen, the QProgressBar will go through set values until the function reaches completion.

For example:

1 tech: 0 -> 99 -> 100
2 tech: 0 -> 50 -> 99 -> 100
3 tech: 0 -> 33 -> 66 -> 99 -> 100
4 tech: 0 -> 25 -> 50 -> 75 -> 99 -> 100

Regardless of the number of technologies chosen, once the last one has been processed, the progressbar would be set to 99 to do some textual clean up before reaching 100.

At the moment, I have two such technologies (PV and wind turbine) where I manually set the progress values.

How could I achieve the above in a more systematic fashion?

My current code:

def report(self):
    # Define progress bar
    progressMessageBar = iface.messageBar().createMessage(str('Generating report...'))
    progress = QProgressBar()
    progress.setMaximum(100)
    progress.setAlignment(Qt.AlignLeft | Qt.AlignVCenter)
    progressMessageBar.layout().addWidget(progress)
    iface.messageBar().pushWidget(progressMessageBar, iface.messageBar().INFO)
    progress.setValue(0)
    sleep(5)

    # Define Report text browser
    display_report = self.report_dockwidget.report_textBrowser
    # Technology checkboxes
    pv_checkBox = self.pv_generation_dockwidget.returnResults_Checkbox
    wind_checkBox = self.wind_generation_dockwidget.returnResults_Checkbox

    # For PV
    if pv_checkBox.isChecked():
        # Do some processing
        progress.setValue(50)
        if not wind_checkBox.isChecked():
            progress.setValue(99)
        sleep(5)

    # For Wind
    if wind_checkBox.isChecked():
        # Do some processing
        if not pv_checkBox.isChecked():
            progress.setValue(50)
        else:
            progress.setValue(99)
        sleep(5)

    # Do some cleanup
    progressMessageBar.setText('Complete')
    progress.setValue(100)
    sleep(1)

    # Show report
    self.report_dockwidget.show()

One idea would be to gather all your techs in a queue and iterate over that, updating the progress as you go.

Since my Python is rather Rust-y (I'm sorry for that pun), here's some pseudocode:

# Setup progress bar et cetera
# Gather techs in a queue
techs = Queue()
if pv_checkBox.isChecked():
    techs.add(pv_task)
if wind_checkBox.isChecked():
    techs.add(wind_task)

# Do the work
total_tasks = techs.count()
done_tasks = 0
while not techs.isEmpty():
    task = techs.take()
    task.doWork()
    done_tasks += 1
    new_progress = max(done_tasks / total_tasks * 100, 99)
    progress.setValue(newProgress)

# Progress is now 99
doFinalStuff()
progress.setValue(100)

This will do all the tasks in a FIFO fashion and set the progress as you wanted with the last step always being 99.

Now, it would be a good idea to gather all checkboxes in a group or layout and, by getting and filtering all child elements, add the techs via loop as well.


Handling incoming socket data Below I have some code that I use to handle incoming socket data, this is not full thing but its the part I am worried about, is there any way to improve this?

int packetPosition = 0;
                while (packetPosition < packetData.Length)
                {
                    try
                    {
                        int MessageLength = Apple.Encoding.DecodeInt32(new byte[] { packetData[packetPosition++], packetData[packetPosition++], packetData[packetPosition++] });
                        uint MessageId = Apple.Encoding.DecodeUInt32(new byte[] { packetData[packetPosition++], packetData[packetPosition++] });

                        byte[] Content = new byte[MessageLength - 2];

                        for (int i = 0; i < Content.Length && packetPosition < packetData.Length; i++)
                        {
                            Content[i] = packetData[packetPosition++];
                        }

                        IncomingPacket incomingPacket = new IncomingPacket(MessageId, Content);
                    }
                    catch (Exception e)
                    {
                        _log.Error(e);
                        Apple.Game.PlayerManager.DisposePlayer(_playerData.PlayerId);
                    }
                }

Assuming the completely broken indentation is a paste glitch (right?), this bit hits me first:

catch (Exception e)
{
    _log.Error(e);
    Apple.Game.PlayerManager.DisposePlayer(_playerData.PlayerId);
}

If DisposePlayer does what it seems to be doing, then this looks like dangerous code that's begging for ObjectDisposedException to be thrown at another, unrelated point in time.

And if your logging framework ever gets configured to throw exceptions, and _log.Error(e); actually throws (IDK, error writing to log file for example), then DisposePlayer is never going to get called.

That's why disposal of resources should happen in a finally block.

catch (Exception e)
{
    _log.Error(e);
    disposePlayer = true;
}
finally
{
    if (disposePlayer)
    {
        Apple.Game.PlayerManager.DisposePlayer(_playerData.PlayerId);
    }
}

That way you know that that bit of code will run no matter what. I added a little bool disposePlayer local variable here, because a finally block will run whether an exception is caught or not, and you only need to call that method when an exception is effectively thrown.

I'm just not comfortable seeing an object being disposed by someone else than its creator, but you haven't given us enough code to chew on for me to expand on this point.

I think these two lines take up too much horizontal space:

int MessageLength = Apple.Encoding.DecodeInt32(new byte[] { packetData[packetPosition++], packetData[packetPosition++], packetData[packetPosition++] });
uint MessageId = Apple.Encoding.DecodeUInt32(new byte[] { packetData[packetPosition++], packetData[packetPosition++] });

I would be better to give it vertical height, so that the reader can more easily see how many times packetPosition is being incremented in the process:

int MessageLength = Apple.Encoding.DecodeInt32(new byte[]
{
    packetData[packetPosition++],
    packetData[packetPosition++],
    packetData[packetPosition++]
});
uint MessageId = Apple.Encoding.DecodeUInt32(new byte[]
{
    packetData[packetPosition++],
    packetData[packetPosition++]
});