Simple Oracle connection using JDBC I'm trying to continue the learning process in Java, and I've made a simple yet useful class which will allow me to retrieve the data inside a table from an Oracle DB, using JDBC.

What I'm especially looking after in a review is:

styleguide - as in naming variables / classes, spacing, and so on)
usage of correct data types
best practices

Code:

package oraconnection;

import java.sql.*;

class OraConnection {

    public static void oracleDbConnection(
            String db_driver,
            String db_type,
            String db_driver_type,
            String db_host_name,
            String db_port,
            String db_service_name,
            String db_username,
            String db_password) {

        String connection_string = String.format("%s:%s:%s:@%s:%s/%s",
                db_driver,
                db_type,
                db_driver_type,
                db_host_name,
                db_port,
                db_service_name);

        try {
            Class.forName("oracle.jdbc.driver.OracleDriver");

            Connection connection = DriverManager.getConnection(
                    connection_string,
                    db_username,
                    db_password);

            Statement statement = connection.createStatement();
            String query = "select * from some_table";

            ResultSet result = statement.executeQuery(query);

            while (result.next()) {
                System.out.println(
                        result.getInt(1) + "  " + result.getString(2) + "  "
                        + result.getString(3));
            }
            connection.close();

        } catch (Exception e) {
            System.out.println(e);
        }

    }

    public static void main(String[] args) {
        oracleDbConnection(
                "jdbc",
                "oracle",
                "thin",
                "host",
                "port",
                "service-name",
                "user",
                "pass");
    }
}

Please bear with the fact that I'm a beginner when it comes to Java and I don't have a deep knowledge about its insides, so please make your answers as detailed as possible.

Minor detail: I prefer putting a closing parenthesis on its own line when the content between parentheses spans more than one line. (Basically, it looks like one-true-brace style with parentheses instead of braces.) I find it makes scanning vertically much simpler. If your team/company has chosen a different convention, that's fine.

The biggest problem I see with your code is that there's no division of responsibilities. For a very simple program, this doesn't matter, but in more complex application with a bunch of different queries and behaviors, it's a huge deal. The overhead of doing it isn't that big, either, so for almost any real world program, I recommend going ahead and dealing with this. (There's nothing more permanent than a temporary, quick, and dirty solution, after all.)

Creating the connection

Start by creating the connection in one place. This lets you reuse that code in multiple places in your application, and we'll see in a minute that it makes it much simpler to execute multiple queries in a transaction.

class OraConnection {
    public static Connection openConnection(
        String db_host_name,
        String db_port,
        String db_service_name,
        String db_username,
        String db_password
    ) {
        String connection_string = String.format(
            "%s:%s:%s:@%s:%s/%s",
            "jdbc",
            "oracle",
            "thin",
            db_host_name,
            db_port,
            db_service_name
        );

        Class.forName("oracle.jdbc.driver.OracleDriver");

        return DriverManager.getConnection(
            connection_string,
            db_username,
            db_password
        );
    }
}

I also recommend hard coding the "jdbc", "oracle", "thin" string here. The main reason is that the way you're opening the connection doesn't support any other values for them. For example, you can't open a PostgreSQL connection with this code because it looks for the oracle.jdbc.driver.OracleDriver class. Since the caller can't validly pass in anything else, there's no point in providing them as arguments. If your connection opening code were more general, I wouldn't advise that. However, this exposes one of the advantages of breaking things down into smaller pieces: you could change this code to be more general and allow more connection types without changing any of your other code.

Fetching results

Then create an object to represent your query results. Callers that want to execute your query and process the results shouldn't have to worry about details like a result set, what position each column is at, and what data type each column is. All that should be sorted out by the method that executes the query, and that method should return a simple object containing the results.

class SomeTable {
    private int column1;
    private int column2;
    private String column3;
    // ...and the rest of your columns

    public SomeTable(
        int column1,
        int column2,
        String column3,
        // the rest of your cols
    ) {
        this.column1 = column1;
        // etc.
    }

    public int getColumn1() { return this.column1; }
    // the rest of your columns
}

Then put the query itself in another class.

class SomeTableQueries {
    public static List<SomeTable> fetchAllSomeTableRows(Connection connection) {
        String query = "select column1, column2, column3, /* rest of your columns */ from some_table";

        try (
            Statement statement = connection.createStatement();
            ResultSet result = statement.executeQuery(query)
        ) {
            ArrayList<SomeTable> rows = new ArrayList<SomeTable>();
            while (result.next()) {
                rows.add(SomeTable(
                    result.getInt(1),
                    result.getString(2),
                    result.getString(3)),
                    // rest of your columns
                ));
            }
            return rows;
        }
    }
}

Note that it does the following:

It hides the boiler plate of building a statement and converting the data into a simpler form from callers.
It avoids SELECT *. SELECT * is okay if you have code that can automatically figure out what the result columns are and stuff them into an appropriate object, but we haven't written code like that here. Since our column set and the order is hard coded as part of reading the data, it's better to list them explicitly.
It cleans up the Statement and ResultSet objects it creates.
Names the method to describe the query it runs

Logic that uses all this

Now that we've got separate pieces of code to do smaller pieces, we can write our main method. Note that this method must be responsible for ensuring that the connection gets closed properly.

public class PrintSomeTable {
    public static void main(String[] args) {
        try (Connection conn = OraConnection.openConnection(
            "host",
            "port",
            "service-name",
            "user",
            "pass"
        )) {
            for (SomeTable r: SomeTableQueries.fetchAllSomeTableRows(conn)) {
                System.out.println(
                    r.getColumn1() +
                    " " +
                    r.getColumn2() +
                    " " +
                    r.getColumn3()
                );
            }
        }
    }
}

Note that it would be very simple to use the same connection for more queries and operations.

Closing notes

The overall theme here is that you should use methods and classes to hide complexities that callers don't really care about, which makes understanding the code much simpler. This way, you can consider the details of each operation in an isolated scope, instead of trying to understand what each "larger" logical operation is by sorting through every little detail.

Lastly, if you have a lot of queries, seriously consider using an ORM, a micro ORM, or other similar tooling to handle a good amount of this boiler plate for you.


Implementing a queue using a linked-list In my intro CS class we're reviewing data structures. I'm currently working on implementing a queue using a linked list (FIFO) in C. I'd appreciate a review of the implementation as well as of my understanding of how a queue should work

// This program is implementation of queue data structure
// via linked list (FIFO)

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct node
{
    int number;
    struct node *next;
} node;

node *enqueue(int element, node *temp);
node *dequeue(int n, node *temp);
void display(node *temp);
void destroy_queue(node *temp);

int main()
{
    node *queue = NULL;

    // add some elements to queue
    for (int i = 0; i < 5; i++)
    {
        queue = enqueue((i + 1), queue);
    }

    // display queue
    display(queue);

    // dequeue 2 elements from queue
    printf("Removing two elements from queue...\n");
    queue = dequeue(2, queue);

    //display queue
    display(queue);

    // free memory
    destroy_queue(queue);

}

node *enqueue(int element, node *temp)
{
    node *newElement = malloc(sizeof(node));
    node *head = temp;
    if (newElement == NULL)
    {
        fprintf(stderr, "No memory for the new queue element");
        return temp;
    }

    newElement->number = element;
    newElement->next = NULL;

    if (head == NULL)
    {
        return newElement;
    }
    else
    {
        while ((temp->next) != NULL)
        {
            temp = temp->next;
        }
        temp->next = newElement;
    }
    return head;
}

node *dequeue(int n, node *temp)
{
    node *aux;
    for (int i = 0; i < n; i++)
    {
        if (!temp)
        {
            fprintf(stderr,"No elements to remove!");
            return temp;
        }
        aux = temp->next;
        free(temp);
        temp = aux;
    }
    return temp;
}

void display(node *temp)
{
    while (temp)
    {
        printf("Elements in queue are: %i\n", temp->number);
        temp = temp->next;
    }
}

void destroy_queue(node *temp)
{
    node *aux = temp;
    while (temp)
    {
        aux = temp->next;
        free(temp);
        temp = aux;
    }
}

Having a separate queue structure

typedef struct {
    node * head;
    node * tail;
} queue;

will bring you several benefits:

Most important, enqueue would take a constant time (vs linear in queue length you have now).
The client would be relieved from responsibility to maintain the head node.
enqueue doesn't inform the client that it failed the allocation. fprintf is good for the human, but does nothing for the calling routine. Having the queue structure enables you to return an error code.

The specialized utility functions, like enqueue and dequeue should not print anything. They must communicate success/error to the caller, and let it act appropriately.
else in enqueue is redundant.
I don't see why do you #include <string.h>.


While loop uses extra variables - can this be cleaned up? A piece of my program allows the looping and incrementing of a targeted entity to be used in a while loop somewhere else. Here is a sample of the while loop:

while (state.World.Looping(reqComponents))
{
    TimerComponent timer = state.World.GetComponent(TimerComponent.Type);

    if (timer.IsActive)
    {
        timer.TimeLeft -= gameTime.ElapsedGameTime.TotalSeconds;

        if (timer.IsFinished)
        {
            timer.IsActive = false;
        }
    }
}

Here is the actual loop function, which increments the current entity (which is grabbed by state.World.GetComponent():

public bool Looping(ComponentType reqComponents)
{
    if (!isLooping)
    {
        isLooping = true;
        CurrentEntity = 0;
    }
    else if (stopLooping)
    {
        isLooping = false;
        stopLooping = false;

        return false;
    }
    else if (++CurrentEntity == EntityCount - 1)
    {
        stopLooping = true;
    }

    if (!HasComponents(reqComponents))
    {
        return Looping(reqComponents);
    }

    return true;
}

The very top part of the Looping() feels really ugly to me. Is there a better way to do this?

Basically, this code controls everything except the body of the loop: initialization, increment, and destruction. This suggests that the actual while should be inside State.World.Looping, and it should take another parameter, an Action or equivalent delegate, which the caller should pass in. For instance, the call would be rewritten:

state.World.Looping(reqComponents, () =>
{
    TimerComponent timer = state.World.GetComponent(TimerComponent.Type);

    if (timer.IsActive)
    {
        timer.TimeLeft -= gameTime.ElapsedGameTime.TotalSeconds;

        if (timer.IsFinished)
        {
            timer.IsActive = false;
        }
    }
});

and Looping itself could be modified to:

public void Looping(ComponentType reqComponents, Action loopBody)
{
    if (!HasComponents(reqComponents))
    {
       return;
    }
    CurrentEntity = 0;

    do
    {
       loopBody();
    } while (++CurrentEntity != EntityCount - 1);
}

I think I got the number of iterations right but I'm not completely sure.


Finding kth min/max in BST Please critique my code. (Specifically, I'm not sure if my use of member variables is appropriate.)

Here is the main structure of my BST class:

public class bst {

    private Node root;

    public class Node {

        int data;
        Node left, right;

        private Node(int data) {
            this.data = data;
            left = null;
            right = null;
        }
    }
}

The bst class contains the following methods to find kth max/min element

private static int min;
private static int max;
private static int target;
private static boolean isFound;
private static int level;
private static int nodes;

public int KthMin(int k) {

    //initializing helper variables
    target = k;
    isFound = false;
    min = 0;
    level = 1;
    nodes = 0;

    KthMinHelper(root);

    if (target > nodes) {
        throw new IndexOutOfBoundsException();
    }

    return min;
}

private void KthMinHelper(Node n) {

    if (isFound) {
        return;
    }

    nodes++;

    if (n.left != null) {
        KthMinHelper(n.left);
    }

    if (level == target) {
        min = n.data;
        isFound = true;
    }
    level++;

    if (n.right != null) {
        KthMinHelper(n.right);
    }
}

public int KthMax(int k) {

    //initializing helper variables
    target = k;
    max = 0;
    isFound = false;
    level = 1;
    nodes = 0;

    KthMaxHelper(root);

    if (target > nodes) {
        throw new IndexOutOfBoundsException();
    }

    return max;
}

private void KthMaxHelper(Node n) {

    if (isFound) {
        return;
    }

    nodes++;

    if (n.right != null) {
        KthMaxHelper(n.right);
    }

    if (level == target) {
        max = n.data;
        isFound = true;
    }
    level++;

    if (n.left != null) {
        KthMaxHelper(n.left);
    }

}

This is how my test driver looks like:

 public static void main(String[] args) {
    bst tree = new bst();
    int[] a = {11, 5, 2, 9, 7, 6, 3};

    for (Integer n : a) {
        tree.insert(n);
    }

    System.out.println("\nkth min:" + tree.KthMin(3));
    System.out.println("kth max:" + tree.KthMax(2));
}

First of all, do not use static variables to store anything that is not directly singleton related. All your variables - min, max, target, isFound, level, nodes - should be handed around in the call, not stored in class static variable. This not only makes the code utterly unreadable, but it will not be possible to make concurrent calls on the BST, even though search should be thread safe operation.

Namely:
target does not need to exist at all and can be replaced by level only, max|min as return value respectively or in some other way, level can be decremented with every found minimum to leave out target, nodes is really clunky way to mark - not found, simply put, if there is k elements in the BST, trere must be k-th minimum or maximum, so before even going into recursion, you can compare target with number of elements in the tree (updated in class variable on every insert/remove)

Adding suffix "Helper" to your methods is redundant with them being private, and it actually makes them bit more obscured.

Avoid using of one letter variable names. Exception would be some math problem where these variables are commented right before use, or indexes in loops.

It is common convention to have names of methods start with lower case letter, while upper case is for classes. this way, you should have kthMax. For class Bst or better, since it is abbreviation, BST or for clarity, since BST can be anything, BinarySearchTree.

It would be really complicated to work around level and isFound being returned. In this case, search state class seems to be a good idea.

private class SearchState{
    public int level;
    public boolean found = false;
    public final boolean isMin;
    public int value = 0;

    public SearchState(boolean isMin, int level){
        this.isMin = isMin;
        this.level = level;
    }

    public Node first(Node in){
        return isMin?in.left:in.right;
    }

    public Node last(Node in){
        return isMin?in.right:in.left;
    }

    public void check(Node in){
        if (this.level == 0) {
            this.value = in.data;
            this.found = true;
        }
        this.level--;
    }
}

As you can see, it contains all necessary information and methods for distinguishing between min and max search.

Unified method for search of kth number would be then

private void kthRecursion(Node node, SearchState state) {
    if (state.found || node==null) {
        return;
    }

    kthRecursion(state.first(node), state);

    state.check(node);

    kthRecursion(state.last(node), state);
}

and its use

public int kthMin(int k){
    return kthInit(k, true);
}

public int kthMax(int k){
    return kthInit(k, false);
}

private int kthInit(int k, boolean isMin){
    if(k>size || k<=0){
        throw new Exception("Invalid argument "+k+"!");
    }

    SearchState state = new SearchState(isMin, k-1);
    kthNumber(root, state);
    return state.value;
}

Do not know whether this code works, since I do not have rest of your class, but it should work. Please let me know if it does not along with remaining parts of your class so I can fix it.

Hope it helps, regards.


PDO prepared statement to insert data into MySQL I am changing all mysqli queries to use PDO now and just want to make sure I'm doing it right.  Here is an old one:

$sql = "INSERT into profile (profileid, name, description) values ('$profileid', '$name', '$description')";
        $sql= mysqli_query($connection,$sql);
        if (!$sql) {
        die("Database query failed: " . mysqli_error($connection));
        } else {
        redirect_to('/my-account');
        }

And here is how I rewrote it:

$stmt = $pdo->prepare("INSERT into pools (profileid, name, description) values (:profileid, :name, :description)");

$stmt->execute([':profileid' => $profileid, ':name' => $name, ':poolname' => $poolname, ':description' => $description]);

redirect_to('/my-pools');

It seemed to work fine when I tested, just want to make sure I did EVERYTHING right.  Does it all look good?

Also, prior to my old mysqli query, I would do this to "sanitize" the data:

$description = mysqli_real_escape_string($connection,$_POST['description']);

With PDO, I do NOT have to have that AT ALL anymore, is that correct?  Just want to confirm before I delete all the escaping stuff from my code after switching to PDO.

Yes, you did everything right, save for the obvious typo with the extra parameter. You can keep with it all right.

The only improvement (which is the point of this site) I can think of is that you can make this code less bloated, with help of positional placeholders.

$sql = "INSERT into pools (profileid, name, description) values (?,?,?)";
$pdo->prepare($sql)->execute([$profileid, $name, $description]);
redirect_to('/my-pools');

To my taste, this is the most clear and readable approach: now it fits onto screen without horizontal scrolling and you still can visually control the parameter order.


Euler's Method in C# I was just looking for some feedback for my c# program to run Euler's method. I was just wondering on any possible improvements, refactoring, inefficiencies... the usual stuff you probably deal with here.

I am using YAMP, a math parser that can evaluate string inputs (so I can run things like calculating the derivative), and is capable of assignment operators (so I can replace x= with its actual value). Here is my code:

class Program
{
    static void Main(string[] args)
    {
        string derivativeinput;
        string[] xyvals;
        int xval;
        int yval;
        int numxval;
        int stepby;
        int stepto;
        derivativeinput = GetDerivative();
        xyvals = GetInitVals();
        Parser.Load();
        Console.WriteLine("Estimate what value of x?");
        stepto = int.Parse(Console.ReadLine());
        Console.WriteLine("Step by=?");
        stepby = int.Parse(Console.ReadLine());
        yval = Int32.Parse(xyvals[1]);
        xval = Int32.Parse(xyvals[0]);
        LoadInitVals(xval, yval);
        for (int i = 1; i <= stepto; i+= stepby)
        {
            int derivative = Int32.Parse(Parser.Parse(derivativeinput).Execute().ToString());

            yval = yval + derivative * stepby;
            Parser.Parse("y =" + yval.ToString()).Execute();

            xval = xval + stepby;
            Parser.Parse("x = " + xval.ToString()).Execute();
        }
        Console.WriteLine(yval);
        Console.ReadKey();
    }

    static string GetDerivative()
    {
        Console.WriteLine("Dy/Dx=");
        return Console.ReadLine();
    }

    static string[] GetInitVals()
    {
        Console.WriteLine("Initial value: X,Y");
        var initval = Console.ReadLine();
        return initval.Split(',');
    }

    static void LoadInitVals(int initx, int inity)
    {
        Parser.Parse("x = " + initx.ToString()).Execute();
        Parser.Parse("y =" + inity).Execute();
    }

}

I realize there probably isn't much to evaluate (the parser does a lot of the work). I guess some of the things I'm interested in is handling input, converting (I believe this parser will only evaluate and output a string at the moment), and my methods (for instance, GetDerivative- do I return a string, like I am? Or do I go ahead and set the 'derivative' string to the derivative in the function).

Anything else is welcome as well.

Your code is essentially fine, but stylistically, I would nitpick a few of your patterns.
Merging the declaration with the assignment of variables
You declare (almost) all of your variables at the start of your method, and then assign them later.  In general, it's better to declare the variable as late as possible, as it makes it more clear when the variable is actually relevant.  (of course, this is a matter of opinion)  Furthermore, it makes the code more concise with no sacrifice in readability, which makes it easier to reason about the code later.
Standard C# naming conventions
For variables in C#, the convention is camcelCasing.  Yet all of your variables are strictly lowercase even when they clearly represent multiple words.  For example, stepby ought to be stepBy, etc.
Using the built-in keywords when applicable
When using static methods on the int primitive, prefer to use the reserved word. Instead of:
Int32.Parse(xyvals[1]);

Prefer:
int.Parse(xyvals[1]);

If, for the sake of discussion, you prefer Int32, then be consistent and avoid using int to declare your variables.  It makes it more clear when you reference the same type consistently in your code.
Use string interpolation if using C# 6
String interpolation is a new feature in C# 6 and in general I would recommend you prefer it to string concatenation (there are performance caveats, but you should focus on that only when that performance becomes an issue over readability).  Instead of:
Parser.Parse("y =" + yval.ToString()).Execute();

Use string interpolation:
Parser.Parse($"y = {yval}").Execute();

The code becomes more concise, and (I believe) a little easier to read.  Also, the .ToString() is unnecessary as that call will be performed implicitly if the type of the variable is not already a string. (as a side-note, for some reason you format x = ... differently than y =... -- it should probably be consistent.)

I admit a lot of this guidance is somewhat subjective, but I hope you find it helpful. :)


Socket listener for chat client I'm working on chat client for mobile devices, and I need to get new messages from server as fast, as possible and, if it possible, keep battery alive.

I've created such class, that listens for server on it's own thread. Here is run method of Thread:

@Override
public void run(){
    try{
        //actually these lines defined in constructor
        String address="127.0.0.1";
        int port=46969;
        int reconnectInterval=15000;
        int interval=1000;
        Socket socket;
        //--- constructor's end

        SocketAddress addr=new InetSocketAddress(address, port);
        InputStream is=null;
        OutputStream os=null;
        ByteBuffer buf=ByteBuffer.allocate(2048);

        int beat=0;
        while(!interrupted()){
            //if socket not created - create it
            //if socket not connected - connect it
            if(socket==null || !socket.isConnected()){
                socket=new Socket();
                while(!socket.isConnected()){
                    try{
                        socket.connect(addr,reconnectInterval);
                    }catch(IOException e){
                        Log.d("AppLog", "Can't connect to socket. Reconnecting...");
                        socket=new Socket(); //recreate socket (if connection failed)
                        sleep(reconnectInterval);
                    }
                    continue;
                }
                try{
                    is=socket.getInputStream(); //open socket streams
                    os=socket.getOutputStream();
                }catch(IOException e){
                    Log.d("AppLog","Can't open socket streams - IOException");
                    interrupt();
                    break;
                }
                Log.d("AppLog", "Socket connected");
            }

            while(socket.isConnected()){
                sleep(interval);

                //send heart beats to socket to check connection
                beat++;
                if(beat>30)
                    try{
                        os.write(1);
                    }catch(IOException e){
                        Log.d("AppLog", "Can't send heartbeat");
                        socket=null;
                        break;
                    }finally{
                        beat=0;
                    }

                //read from socket
                try{
                    if(is.available()>0 && buf.remaining()>0){
                        while(is.available()>0 && buf.remaining()>0)
                            buf.put((byte) is.read());
                    }else if(buf.position()>0){
                        get(extractBytes(buf));
                        buf.clear();
                    }else buf.clear();
                }catch(IOException e){
                    Log.d("AppLog", "Can't read from socket - IOException");
                }
            }
        }
    }catch(InterruptedException e){
        Log.d("AppLog", "Interrupted exception has occured");
    }
}

Questions:

1) Would sleep method save resources (and battery)?

2) Is it efficient way, to read from socket and keep connection alive?

I'm not sure about your first question - I've never programmed on a mobile device where power was an issue.

In answer to your second question, one of the options for sockets that is contained in the specification for TCP/IP is SO_KEEPALIVE. When this is set to true, the internal socket mechanism takes care of keeping the connection alive so that you don't have to worry about heart beats or anything like that. The Socket class has accessors for SO_KEEPALIVE.

Also, you need to seriously refactor this code and extract out the two meaningful parts into functions (one does the connecting, one the R/W). As it is the code is too long and complicated to be able to easily follow the logic.


Creating Simple New Elements, Object Instances and Assigning IDs I've created a simple demo that creates new elements on a page. After clicking the button, new elements are created, and when clicking remove, the relevant element is deleted. Each element has a 'data-id' data attribute, which matches the id object property that's also created.

Check out the code on this page, or see this codepen demo I created: https://codepen.io/anon/pen/xMqodd

I'm essentially looking for ways which this code can be improved. Could anyone advise me here? Thanks for any help - the code is below:

HTML:

<button id='btn' onclick='createEntry()'>Create</button>

CSS:

button { display: block }
    .entry {
        width: 100%;
        display: block;
        padding: 10px;
        border: 1px solid #f5f5f5;
    }
    span {
        display: block;
        width: 100%;
    }
    section { background: lightgreen }

JS:

// note: beware of using eccessive nested functions as they can cause problems
    var id = 0, objects = [], removes;

    function createEntry() {
        id++;

        // create new element, append to #container & create new object
        var container = document.querySelector('#container'),
            newEntry = document.createElement("div"),
            title = 'title text here',
            description = 'description text here',
            remove = 'remove',
            dataId = id,
            obj = new Entry(title, description, remove);
        newEntry.classList.add("entry");
        newEntry.innerHTML = '' + title + '' + description + '' + remove + '';
        container.appendChild(newEntry);
        newEntry.setAttribute('data-id', id);

        updateElements();

        // constructor & push into array
        function Entry(title, description, remove) {
            this.title = title;
            this.description = description;
            this.remove = remove;
            this.id = id;

            objects.push(this);
        }

        // tests
        console.log('JSON.stringify(obj): ' + JSON.stringify(obj));
        console.log('obj.id: ' + obj.id);
    }

    function updateElements() {
        removes = document.querySelectorAll(".remove");
        listenForRemoves();

        function listenForRemoves() {
            for (let remove of removes) {
                remove.removeEventListener("click", removeElements);
                remove.addEventListener("click", removeElements);
            }
        }
    }

    function removeElements() {
        let removedId = this.parentNode.getAttribute('data-id'),
            objToRemove = objects.find(obj => obj.id == removedId); // not used

        this.parentNode.remove(); console.log('removed id ' + removedId);
            console.log('objects before: '); for (let object of objects) { console.log(JSON.stringify(object))};
            objects = objects.filter(obj => obj.id != removedId); // doesn't use objToRemove
            console.log('objects now: '); for (let object of objects) { console.log(JSON.stringify(object))};
    }

Not sure why you need an id, I think you can do the same without any id.

Even the Entity object array is not clear why you need it.

But the first point is to wrap up your code in a function, so you can contain it safely.

So add a function that contains all the code and just design what you need to pass to that function and what you need to have as a public API from your module.

Another point is about your html:

  <button id='btn' onClick='createEnty()'>Create</button>

You should do the bind on the javascript code instead. As you already provided an id for the button, you can easely access to the object and bind on the click event.

So your module can be like so:

function MakeEntryModule(buttonSelector, containerSelector) {
    // code here

    // here the return object with the public methods
    return {
       myMethod: myFunction
    }
}

// then you can do so:
var entryModule = MakeEntryModule('#btn', '#container');

You can also use an anonymous function that is called directly:

var entryModule = (function(a,b){
    // code goes here
})('#btn', '#container');

This is fine, but the previous is more clear when you have parameters.

So here is a rewrite of your code:

function MakeEntryModule(buttonSelector, containerSelector) {
    // cache the container element to avoid
    // performing the search each time.
    var container = document.querySelector(containerSelector);

    // just pass title and description to the createEntry
    // function to be able to make it more generic.
    function createEntry(titleText, descriptionText) {
        var entry = document.createElement("div");
        entry.classList.add("entry");
        entry.appendChild(createSpan("title", titleText));
        entry.appendChild(createSpan("description", descriptionText));
        entry.appendChild(deleteButton(entry));

        container.appendChild(entry);
    }

    function createSpan(className, text) {
        var spn = document.createElement("span");
        spn.classList.add(className);
        spn.appendChild(document.createTextNode(text));
        return spn;
    }

    // the delete element can be make so:
    function deleteButton(toDelete) {
        var btn = createSpan("remove", "remove");
        var _listener = btn.addEventListener("click", function () {
            btn.removeEventListener("click", _listener);
            toDelete.remove();
        });

        return btn;
    }

    // here you bind your createEntry to the button
    var _listener = document.querySelector(buttonSelector)
        .addEventListener("click", function () {
            createEntry("title text here", "description text here");
        });

    // the only public API needed is the remove listener
    // to avoid memory leak
    return {
        removeListener: function () {
            document.removeEventListener("click", _listener);
        }
    }
}

You can even make it shorter using modern Javascript syntax:

function MakeEntryModule(buttonSelector, containerSelector) {
    const container = document.querySelector(containerSelector);

    function createEntry(titleText, descriptionText) {
        const entry = document.createElement("div");
        entry.classList.add("entry");
        entry.appendChild(createSpan("title", titleText));
        entry.appendChild(createSpan("description", descriptionText));
        entry.appendChild(deleteButton(entry));

        container.appendChild(entry);
    }

    function createSpan(className, text) {
        const spn = document.createElement("span");
        spn.classList.add(className);
        spn.appendChild(document.createTextNode(text));
        return spn;
    }

    function deleteButton(toDelete) {
        const btn = createSpan("remove", "remove");
        const _listener = btn.addEventListener("click", () => {
            btn.removeEventListener("click", _listener);
            toDelete.remove();
        });

        return btn;
    }

    const _listener = document.querySelector(buttonSelector)
        .addEventListener("click", () =>
            createEntry("title text here", "description text here"));

    return {
        removeListener: () =>
            document.removeEventListener("click", _listener)
    }
}

You should use const and let instead of var if possible, use var only if you have browser compatibility requirements.

About arrow functions, they are very compact and elegant, but use with carefull as they have a different behaviour compared with normal functions.

They are good for event handlers as they bind the declaration context, with the "correct" this object scope.

But they can drive to performance issues in some situations.

I get rid of the objects array, if you really need you can add it without problem.


Turning numbers into a string of words - challenge I just completed this challenge at CodeEval.

  You are given a positive integer number. This represents the sales made that day in your department store. The payables department however, needs this printed out in english. NOTE: The correct spelling of 40 is Forty. (NOT Fourty)

My solution works, and it works pretty well. However I think there has to be an easier way to do this:

import sys

def nums_to_words(number):
    units = ['', 'one', 'two', 'three', 'four', 'five', 'six', 'seven', 'eight', 'nine']
    teens = ['', 'eleven', 'twelve', 'thirteen', 'fourteen', 'fifteen', 'sixteen',
             'seventeen', 'eighteen', 'nineteen']
    tens = ['', 'ten', 'twenty', 'thirty', 'forty', 'fifty', 'sixty', 'seventy',
            'eighty', 'ninety']
    thousands = ['', 'thousand', 'million', 'billion', 'trillion', 'quadrillion',
                 'quintillion', 'sextillion', 'septillion', 'octillion',
                 'nonillion', 'decillion', 'undecillion', 'duodecillion',
                 'tredecillion', 'quattuordecillion', 'sexdecillion',
                 'septendecillion', 'octodecillion', 'novemdecillion',
                 'vigintillion']
    words = []

    if number == 0:
        words.append("zero")
    else:
        number_string = "%d" % number
        length_of_number_string = len(number_string)
        number_groups = (length_of_number_string + 2) / 3
        new_number_string = number_string.zfill(number_groups * 3)
        for i in range(0, number_groups * 3, 3):
            x, y, z = int(new_number_string[i]), int(new_number_string[i + 1]), int(new_number_string[i + 2])
            w = number_groups - (i / 3 + 1)
            if x >= 1:
                words.append(units[x])
                words.append("hundred")
            if y > 1:
                words.append(tens[y])
                if z >= 1:
                    words.append(units[z])
            elif y == 1:
                if z >= 1:
                    words.append(teens[z])
                else:
                    words.append(tens[y])
            else:
                if z >= 1:
                    words.append(units[z])
            if (w >= 1) and ((x + y + z) > 0):
                words.append(thousands[w])

    name = []

    for word in words:
        name.append(word.title())

    return ''.join(name) + "Dollars"

if __name__ == '__main__':
    with open(sys.argv[1], "r") as numbers:
        for num in numbers.readlines():
            print(nums_to_words(int(num)))

Is there anything that is obviously noticeable that I can do better?

I think the code in the post is fine: the English language rules are quite complicated, and it's going to take a certain amount of computing effort however you implement it.
The variables units, teens, tens, and so on are always the same, so they could be global constants, rather than being constructed again every time the function is called.
It's conventional in Python for constants to have names written in all capital letters (see PEP8), so UNITS instead of units.
Instead of writing:

UNITS = ['', 'one', 'two', 'three', 'four', 'five', 'six', 'seven', 'eight', 'nine']

I think this is easier to read:

UNITS = 'zero one two three four five six seven eight nine'.split()

It's convenient to combine UNITS and TEENS into a single list giving all the names for numbers less than 20.
The code reads a string from standard input, converts it to an integer (by calling int(num), then converts the number back to a string (by calling "%d" % number), then splits the string into pieces, and converts each piece back to a number (by calling int(new_number_string[i])). This back-and-forth conversion is unnecessary, because you can split a number into 3-digit groups by repeatedly calling divmod until you get down to zero:

>>> divmod(123456789, 1000)
(123456, 789)
>>> divmod(123456, 1000)
(123, 456)
>>> divmod(123, 1000)
(0, 123)

The function nums_to_words does two things: (i) it converts a number to English words; and (ii) it capitalizes the words, joins them and appends "Dollars". The principle of separation of concerns suggests that these should be two functions. And that would be useful, because then the function implementing part (i) could call itself recursively.

It's also slightly more convenient to generate the results (by calling yield or yield from) instead of constructing a list. Then you could write this for concern (i):

UNITS = """
    zero one two three four five six seven eight nine ten eleven
    twelve thirteen fourteen fifteen sixteen seventeen eighteen
    nineteen
""".split()
TENS = "zero ten twenty thirty forty fifty sixty seventy eighty ninety".split()
THOUSANDS = """
    zero thousand million billion trillion quadrillion quintillion
    sextillion septillion octillion nonillion decillion undecillion
    duodecillion tredecillion quattuordecillion sexdecillion
    septendecillion octodecillion novemdecillion vigintillion
""".split()

def english_words(n):
    """Generate words (excluding "and") naming the non-negative number n
    in English.

    """
    if n < 20:
        yield UNITS[n]
    elif n < 100:
        tens, units = divmod(n, 10)
        yield TENS[tens]
        if units:
            yield UNITS[units]
    elif n < 1000:
        hundreds, remainder = divmod(n, 100)
        yield UNITS[hundreds]
        yield "hundred"
        if remainder:
            yield from english_words(remainder)
    else:
        groups = []             # Groups of three digits from n.
        while n:
            n, group = divmod(n, 1000)
            groups.append(group)
        for i, group in reversed(list(enumerate(groups))):
            if group:
                yield from english_words(group)
                if i:
                    yield THOUSANDS[i]

and this for concern (ii):

def num_to_words(n):
    return ''.join(w.title() for w in english_words(n)) + "Dollars"


Mars Rover challenge in Ruby I have written a simplified version of the Mars Rover challenge in Ruby. Please review it for best coding practices.

A rover’s position and location is represented by a combination of x and y co-ordinates and a letter representing one of the four cardinal compass points. The plateau is divided up into a grid to simplify navigation. An example position might be 0, 0, N, which means the rover is in the bottom left corner and facing North.
In order to control a rover , NASA sends a simple string of letters. The possible letters are ‘L’, ‘R’ and ‘M’. ‘L’ and ‘R’ makes the rover spin 90 degrees left or right respectively, without moving from its current spot. ‘M’ means move forward one grid point, and maintain the same heading.

class Rover
  def initialize(x, y, direction)
    @x = x
    @y = y
    @direction = direction
  end

  def instruction(position)
    position.each do |input|
      if input == 'L'
        left
      elsif input == 'R'
        right
      else
        move
      end
    end
      puts "this is where the rover should be: x= #{@x} y= #{@y} facing= #{@direction}"
  end

  def right
    if @direction == 'S'
        puts "the rover is facing West."
      @direction = 'W'
    elsif @direction == 'N'
        puts "the rover is facting East"
      @direction = 'E'
    elsif @direction == 'W'
        puts "the rover is facting North"
      @direction = 'N'
    else
      puts "the rover is facing South"
      @direction = 'S'
    end
  end

  def left
    if @direction == 'N'
        puts "the rover is facing North."
      @direction = 'W'
    elsif @direction == 'W'
        puts "the rover is facting West"
      @direction = 'S'
    elsif @direction == 'E'
        puts "the rover is facting East"
      @direction = 'N'
    else
      puts "the rover is facing South"
      @direction = 'E'
    end
  end

  def move
    if @direction == 'N'
      @y += 1
    elsif @direction == 'E'
      @x += 1
    elsif @direction == 'S'
      @y -= 1
    else
      @x -= 1
    end
  end
end

#instance of our rovers with direction x, y, direction facing N, E, S, or W
mars_rover_a = Rover.new(0,0,'N')
# mars_rover_a.move()
mars_rover_b = Rover.new(1,1,'E')

#call the instruction for each instance of the object rover
mars_rover_a.instruction(['L','M','R','M','L','M','R','R','M'])
mars_rover_b.instruction(['R','M','M','L','M','L','L','M'])

I'd suggest that the position, and maybe orientation also, ought to be its own object ... "Location"?

I'd also prefer to use symbols rather than strings where possible, and rely on data rather than logic (see use of hashes below).

You might be interested in implementing a builder method, in which the methods called simply return the original object, which allows commands to be daisy-chained ...

class Location
  LEFT = {
    n: :w,
    e: :n,
    s: :e,
    w: :s
  }

  RIGHT = {
    n: :e,
    e: :s,
    s: :w,
    w: :n
  }

  def initialize(x:, y:, direction:)
    @x         = x
    @y         = y
    @direction = direction
  end

  def left
    @direction = LEFT.fetch(@direction)
    self
  end

  def right
    @direction = RIGHT.fetch(@direction)
    self
  end

  def move
    case direction
    when :n
      self.y += 1
    when :e
      self.x += 1
    when :s
      self.y -= 1
    when :w
      self.x -= 1
    end
    self
  end

  attr_reader :x, :y, :direction

  private

  attr_writer :x, :y, :direction
end

Hence:

2.4.5 :125 >   l = Location.new(x: 0, y: 0, direction: :n)
 => #<Location:0x00007f9f5329e858 @x=0, @y=0, @direction=:n>
2.4.5 :126 > l.left.move.right.move.left.move.right.right.move
 => #<Location:0x00007f9f5329e858 @x=-1, @y=1, @direction=:e>


Login System using PHP and PDO Prepared Statement vir.php

<?php

require_once '../db.php';

session_start();

if($_SERVER['REQUEST_METHOD'] == 'POST'){

$username = trim($_POST['username']);

try{

  $Query = "SELECT * FROM users WHERE username = :username";
  $statement = $conn->prepare($Query);
  $statement->bindValue(':username', $username);
  $statement->execute();
  $user = $statement->fetch(PDO::FETCH_ASSOC);
  $RowCount = $statement->rowCount();

} catch (PDOerrorInfo $e){

    die('QuerySCD Error '.$e->getMessage());

  }

  if( $RowCount == 0 ){
   // User doesn't exist
    $_SESSION['message'] = "error!";
    header("location: error-login.php");

  } else{ // User exists

      if( password_verify($_POST['password'], $user['password'])){

        $_SESSION['username']  = $user['username'];
        $_SESSION['active']    = $user['active'];

        $_SESSION['logged_in'] = true;

        header("location: riscar.php");

      } else {

          $_SESSION['message'] = "error!";
          header("location: error-login.php");

        }
    }
}

$conn = NULL;

?>

<!DOCTYPE html>

    <title>Login</title>
    <meta charset="UTF-8">

    <form action="vir.php" method="post" autocomplete="off">

        <input type="username" required autocomplete="off" name="username">

        <input type="password" required autocomplete="off" name="password">

        <button name="login">
            Logar
        </button>

    </form>

ir.php

<?php
/* Log out process, unsets and destroys session variables */
session_start();
session_unset();
session_destroy();
?>

<!DOCTYPE html>

    <title>Login</title>
    <meta charset="UTF-8">

<?= 'You have been logged out!'; ?>

error-login.php

<?php
/* Displays all error messages */
session_start();
?>

<!DOCTYPE html>

    <title>Login</title>
    <meta charset="UTF-8">

        <?php
        if( isset($_SESSION['message']) AND !empty($_SESSION['message']) ):
            echo $_SESSION['message'];
        else:
            header( "location: vir.php" );
        endif;
        ?>

Only me will use this login sytem and only me will have the link to this login system, i will use this to insert records on my database.

I don't know if someone could find the link to this login, to i used PDO prepared statement, so prevent against SQL Injection.

How safe is my code?

How safe is my code?

I would suggest if you protect the login form from

CSRF (Cross site request forgery) Attacks by generating a unique code for each session and assigning it to a hidden field in the form, and check every post request whether it matches the one from the session or not.

<input type = "hidden" name="token" value="' . $_SESSION['csrf_token'] . '">

Example: ignore naming convention

   class token {

     public static function generate_csrf_token() {
           if (!isset($_SESSION['csrf_token'])) {
              $_SESSION['csrf_token'] = bin2hex(random_bytes(32));
           }
        }

    public static function check_csrf_token($token) {
        return $token === $_SESSION['csrf_token'];
    }
 }

Then you want to check when the form is submitted and compare both token.

Timing attacks, disable logins requests under a second, mostly bots and robots.

    //Gets the value when the page loads and set it in a session
    $start = microtime(true);

    if (!isset($_SESSION['microtime'])) {
     $_SESSION['microtime'] = $start;
    }

and after the login button is clicked, check and perform the same and then compare the time

if (isset($_POST['submit'])) {
   $finish = microtime(true);
   $seconds_passed = $finish - $_SESSION['microtime'];
   if ($seconds_passed < 1) {

?>

Limit requests from an ip, so for example say, 3 requests maximally allowed per a second, to protect from lots of attack.
Putting captcha and login attempts will be useful.
Session Hijacking multiple things:

Have an identification method that is making each user unique (perhaps hashing user agent and ip) and check every request if they match to see if the current user is the same. And have a session requests tracker that would re-assign a new session id after 25 requests for example.

 if (!isset($_SESSION['session_clicked_times'])) {
            $_SESSION['session_clicked_times'] = 1;
        } else {
            if (($_SESSION['session_clicked_times']) >= 25) {
                $_SESSION['session_clicked_times'] = 0;
                session_regenerate_id(true);
            }
            $_SESSION['session_clicked_times'] += 1;
        }

Also generate a new id once logged in, and so on.

This code:

 if( $RowCount == 0 ){
   // error
  } else{ // User exists

      if(password_verify($_POST['password'], $user['password'])){

     //login

      } else {
         //error
        }
    }
}

to just this (since they both handle the same thing in your case)

 if( $RowCount == 0 || !password_verify($_POST['password'], $user['password']) ){
       // error
      } else {
       //login
    }


Packing and unpacking two 32-bit integers into an unsigned 64-bit integer Does this look good for combining/retrieving two 32 bit integers (a type and an index) into/from an unsigned 64 bit integer (a unique ID)?

The point is to hide in the API the composition of the unique ID and allow its calculation to change later without changing the API (e.g. possibility to change later to consecutive 64 bit ids without changing API).

#include <limits>

uint64_t combine(uint32_t low, uint32_t high)
{
     return (((uint64_t) high) << 32) | ((uint64_t) low);
}

uint32_t high(uint64_t combined)
{
    return combined >> 32;
}

uint32_t low(uint64_t combined)
{
    uint64_t mask = std::numeric_limits<uint32_t>::max();
    return mask & combined; // should I just do "return combined;" which gives same result?
}

Or would a union approach like below be better? Is this union guaranteed to fit into 64 bits (e.g. guarantee no padding in the struct)?

union Id
{
    struct
    {
       uint32_t index; // lower 32 bits
       uint32_t type;  // upper 32 bits
    } split;

    uint64_t unique_id;
};

Here is some code I used to test:

#include <iostream>
#include <assert.h>
#include <sstream>

template <typename T>
std::string bits(T num)
{
    const int num_bits = sizeof(num) * 8;

    T maxPow = T(1) << (num_bits - 1);

    std::stringstream ss;

    for(int i=0; i < num_bits; ++i)
    {
        // print last bit and shift left.
        ss << (num & maxPow ? 1 : 0);
        if ((i+1) % 8 == 0) ss << " ";
        num = num << 1;
    }

    return ss.str();
}

void test1()
{
    {
        int in_low = -3;
        int in_high = 99;
        uint64_t combined = combine(in_low, in_high);
        assert( bits(in_high) + bits(in_low) == bits(combined) );
        assert( in_low == low(combined) );
        assert( in_high == high(combined) );
    }

    {
        uint32_t in_low = 3;
        int in_high = -99;
        uint64_t combined = combine(in_low, in_high);
        assert( bits(in_high) + bits(in_low) == bits(combined) );
        assert( in_low == low(combined) );
        assert( in_high == high(combined) );
    }

    {
        uint32_t in_low = std::numeric_limits<uint32_t>::max();
        int in_high = std::numeric_limits<int32_t>::min();
        uint64_t combined = combine(in_low, in_high);
        assert( bits(in_high) + bits(in_low) == bits(combined) );
        assert( in_low == low(combined) );
        assert( in_high == high(combined) );
    }
}

void test2() {
    Id in; // would "Id in = {-3, 99};" be better? Is this legal C++03? Would a constructor be better?
    in.split.type = -3;
    in.split.index = 99;

    std::cout << in.unique_id << std::endl; // prints 18446744060824649827
    std::cout << bits(in.split.type) << bits(in.split.index) << std::endl;
    std::cout << bits(in.unique_id) << std::endl; // prints 11111111 11111111 11111111 11111101 00000000 00000000 00000000 01100011
    assert(bits(in.split.type) + bits(in.split.index) == bits(in.unique_id));

    Id out;
    out.unique_id = in.unique_id;
    std::cout << int(out.split.type) << ", " << out.split.index << std::endl; // prints -3, 99

    assert(in.split.type == out.split.type);
    assert(in.split.index == out.split.index);
}

Union provides no guarantee of memory arrangement for this.  The C++ standard merely says that it's no bigger than the storage required for the largest element, and that it's designed to only have one active data member.

If you are only targeting one compiler and one architecture it may be okay to use it to translate between types, but it is definitely skiing off-piste.

I'd be tempted to wrap the whole thing up in a class and provide getter methods.  This insulates the user from changes in the implementation of the API in the most flexible way. And, use the bitwise operations (>>, <<, | and &) inside the class.  These operations will happen on a register, rather than in memory, and therefore do not suffer for endian issues.


Print all combinations Question:

Input String 1 = {1, 2, 3, ... , 26}

Input String 2 = {A, B, C, ... , Z}

Output Result 1 = {1, A, 2, B, 3, C, ... , 26, Z}

Output Result 2 = {1, A, 2, A, B, 3, A, B, C, ... , 26, A, B, C, . , Z}

Solution:

public class Combinations {

    private static String stringNumbers[];                  //1, 2, 3, ... , 26
    private static String stringAlphabets[];                //A, B, C, ... , Z

    private static String resultNumberAlphabet[];           //1, A, 2, B, 3, C, ... , 26, Z
    private static String resultNumberAlphabets[];          //1, A, 2, A, B, 3, A, B, C, ... , 26, A, B, ... , Z

    private static final int MAX_NUMBERS = 26;
    private static final int MAX_ALPHABETS = 26;

    public static void main(String[] args) {
        initVariables();
        findCombinations();
        printResults(resultNumberAlphabet);
        printResults(resultNumberAlphabets);
    }

    private static void initVariables() {
        stringNumbers = new String[MAX_NUMBERS];
        stringAlphabets = new String[MAX_ALPHABETS];
        resultNumberAlphabet = new String[stringNumbers.length + stringAlphabets.length];
        resultNumberAlphabets = new String[stringNumbers.length + (stringAlphabets.length * (stringAlphabets.length + 1)/2)];   //  Summation Equation = [n*(n+1)/2];

        for (int i = 0; i < 26; i++) {
            stringNumbers[i] = (i + 1) + "";
            stringAlphabets[i] = Character.toString((char)(i + 65));
        }
    }

    private static void findCombinations() {
        int k = -1;
        int l = -1;
        for (int i = 0; i < stringNumbers.length; i++) {
            resultNumberAlphabet[++k] = stringNumbers[i];
            resultNumberAlphabet[++k] = stringAlphabets[i];
            resultNumberAlphabets[++l] = stringNumbers[i];
            for (int j = 0; j <= i; j++) {
                resultNumberAlphabets[++l] = stringAlphabets[j];
            }
        }
    }

    private static void printResults(String result[]) {
        for (int i = 0; i < result.length; i++) {
            System.out.print(result[i]);
            if ((i + 1) == result.length) {
                System.out.println(".");
            } else {
                System.out.print(", ");
            }
        }
    }

}

Any review comments or suggestions?

Why a string array . Just a String would suffice.
Do we really need to store it in arrays. The computed arrays would only be usde to display. So we can work towards a display method instead of storing and displaying them.

int i;
for(i=1;i<26;i++){
    System.out.print(i+", ");
    System.out.print(Character.toString((char)(64+i))+", ");
}
System.out.print(i+", ");
System.out.println(Character.toString((char)(64+i))+".");

StringBuilder next = new StringBuilder();
for(i=1;i<26;i++){
    System.out.print(i+", ");
    next.append(Character.toString((char)(64+i)) + ", ");
    System.out.print(next);
}
System.out.print(i+", ");
next.append(Character.toString((char)(64+i)));
System.out.println(next);
// Time complexity: O(n)
// Space complexity: O(n+m*(m+1)/2))

Assuming we are striving to have a generic implementation wherein we have two input arrays and we wish to provide the output. Even in that case we can carry forward with above implementation with slight changes.
Having the last condition check seems an unneeded overhead. Instead we loop in for last-but-one index and print the last outside.

if ((i + 1) == result.length)


Merge Sort Algorithm in Python I'm implementing basic sorting algorithms to learn them, and coding, better. Criticisms and possible optimizations welcome.

import unittest
import random

def merge_sort(seq):
    """Accepts a mutable sequence. Utilizes merge_sort to sort in place, return
    a sorted sequence"""
    if len(seq) == 1:
        return seq
    else:
        #recursion: break sequence down into chunks of 1
        mid = len(seq)/2
        left = merge_sort(seq[:mid])
        right = merge_sort(seq[mid:])

        i, j, k = 0, 0, 0 #i= left counter, j= right counter, k= master counter

        #run until left or right is out
        while i < len(left) and j < len(right):
            #if current left val is < current right val; assign to master list
            if left[i] < right[j]:
                seq[k] = left[i]
                i += 1; k += 1
            #else assign right to master
            else:
                seq[k] = right[j]
                j += 1; k += 1

        #handle remaining items in remaining list
        remaining = left if i < j else right
        r = i if remaining == left else j

        while r < len(remaining):
            seq[k] = remaining[r]
            r += 1; k += 1

        return seq

class test_mergesort(unittest.TestCase):
    def test_mergesort(self):
        test = [random.randrange(0, 10000) for _ in range(2000)]
        self.assertEqual(merge_sort(test), sorted(test))

if __name__ == '__main__':
    unittest.main()

You currently have:

i, j, k = 0, 0, 0 #i= left counter, j= right counter, k= master counter

Don't write minified code. Use variable names.

left_counter, right_counter, master_counter = 0, 0, 0


Better method structure for insertion in doubly linked list Here is my insertion method for a doubly linked list. It works, but passing two parameters for the method seems cumbersome. Can anyone suggest a cleaner approach than this?

   LinkedList list = new LinkedList();
    Link link1 = new Link("A");
    list.insert(list.head,link1);
    Link link2 = new Link("B");
    list.insert(link1, link2);
    Link link3 = new Link("C");
    list.insert(link2, link3);
    list.display_List_Start_From_Head();

public void insert(Link currentNode, Link newNode)
    {
        if(head == null)
        {
            head = newNode;
            tail = newNode;
        }
        else
        {
            currentNode.next = newNode;
            newNode.prev = currentNode;
            tail = newNode;
        }

    }

if(head == null)
    {
        head = newNode;
        tail = newNode;
    }
    else
    {
        currentNode.next = newNode;
        newNode.prev = currentNode;
        tail = newNode;
    }

hmmm

    if(head == null)
    {
        head = newNode;
    }
    else
    {
        currentNode.next = newNode;
        newNode.prev = currentNode;
    }
    tail = newNode;

Yep, that's shorter.

Okay, and now a more serious answer:

A LinkedList, no matter if it's doubly linked or not, always is gonna require two things for insertAt: The object to insert, and some means of specifying where it goes. There's no way to circumvent this. Best you can do is take a Node (or Link, in your case) and give it an insertAfter method.

Oh yeah, back to your code example:

LinkedList list = new LinkedList();
Link link1 = new Link("A");
list.insert(list.head,link1);
Link link2 = new Link("B");
Link link3 = new Link("C");
list.insert(link2, link3);
list.insert(link1, link2);
list.display_List_Start_From_Head();

You have some serious bugs in there.

This should work fine, of course, but try iterating it backwards; you won't get the correct elements back.

Here's another way your list blows up:

LinkedList list = new LinkedList();
Link link1 = new Link("A");
list.insert(list.head,link1);
Link link2 = new Link("B");
list.insert(link1, null);//NullPointerException
list.insert(link1, link2);//If it didn't crash just now, this will clear the list and leave link2 as head


Generating social media links I'm building an option panel for a Wordpress theme. I'm just learning PHP and I wonder if there is any better (shorter) way to generate the social media links. Here's what I came up with and even tho it works, I think there are cleaner way to write it

<?php if ( of_get_option('mm_sm_vimeo') ) { ?>

<a href="<?php echo of_get_option('mm_sm_vimeo'); ?>"><img src="<?php echo  get_template_directory_uri(); ?>/images/socials/ico_vimeo.png" alt="Vimeo"/>Vimeo

<?php } ?>

<?php if ( of_get_option('mm_sm_gplus') ) { ?>

<a href="<?php echo of_get_option('mm_sm_gplus'); ?>"><img src="<?php echo get_template_directory_uri(); ?>/images/socials/ico_gplus.png" alt="Google Plus"/>Google Plus

Etc... etc... over 20 times.

Maybe a loop or something? The problem is that I have a different Option name for each and I don't know how to handle it.

You could take the common bits out into a function:

<?php
function my_thing($name, $title) {
   if ( of_get_option("mm_sm_$name") ) {
      ?>

      <a href="<?php echo of_get_option("mm_sm_$name"); ?>"><img src="<?php echo  get_template_directory_uri(); ?>/images/socials/ico_<?php echo $name ?>.png" alt="<?php echo $title ?>"/><?php echo $title ?>

      <?
   }
}

my_thing('vimeo', 'Vimeo');
my_thing('gplus', 'Google Plus');
?>


Find neighbours of a vector To find neighbors of a vector and return array of Vectors which are neighbors of a Vector:
vector<XY> neighboursOf(XY coord){
    short x = coord.x;
    short y = coord.y;

    vector<XY> neighbours;
    neighbours.reserve(9);

    neighbours.push_back(XY(x-1, y));
    neighbours.push_back(XY(x-1, y-1));
    neighbours.push_back(XY(x-1, y+1));

    neighbours.push_back(XY(x+1, y));
    neighbours.push_back(XY(x+1, y-1));
    neighbours.push_back(XY(x+1, y+1));
    neighbours.push_back(XY(x+1, y-1));

    neighbours.push_back(XY(x, y-1));
    neighbours.push_back(XY(x, y+1));

    return neighbours;
}
}

XY is a class which has members x and y:
class XY{
    public:
    XY(short x=0, short y=0);
    short x;
    short y;
};

XY::XY(short x, short y) {
    this->x = x;
    this->y = y;
}

The param is Vector of the yellow square, the neighbor array is supposed to be the black squares

The XY class looks like it would be better as a plain struct:
struct XY
{
    short x;
    short y;
};

Then we don't need to declare a constructor, because it can be aggregate-initialised.
I'll assume that your vector is an alias for std::vector here, since there's no definition visible.
Eight of the statements to populate the vector look reasonable.  The duplicate of x+1, y-1 is probably unintended, though.
You might profitably use emplace_back rather than writing those constructors.  We could create the vector directly, with all the element values as its initializer-list:
std::vector<XY> neighboursOf(XY coord)
{
    auto const x = coord.x;
    auto const y = coord.y;

    return { {x-1, y-1}, {x, y-1}, {x+1, y-1},
             {x-1, y  },           {x+1, y  },
             {x-1, y+1}, {x, y+1}, {x+1, y+1} };
}

See how the formatting here helps us see that each neighbour is included exactly once?
It is possible to write a clever loop, but that's going to be harder to read and no more efficient than the straightforward code.
Note that if we have a finite grid, then we might want special behaviour at the edges.  And if we have an infinite grid, we'll suffer arithmetic overflow (Undefined Behaviour) when we reach the limits of short.


Updating the player statistics text I'm developing a game in Unity 5.3.5, I have a player statistics system with 10 different stats, as in probably every game the user is able to see the points put in each stats and also the benefit each stats is giving him. I have 2 panels with 2 text the first one is responsible for the points in each stats and the second one for the increase each stats is giving. The player has a class which holds all the stats in there and also implements <IEnumberable> so I can be able to iterate over this class picking all the different stats, I also have an enum which is used only for the displaying of those statistics and it's in such order that it matches the player's class that holds all the stats. Only 1 panel can be active at a time. That's how I'm updating the statistics info :

    private void UpdateStatusLabels(PanelType panelType)
    {
        List<Stats.Stats> playerStats = Player.BaseStats.ToList();
        string playerStatsText = string.Empty;
        Array values = Enum.GetValues(typeof(StatsOrders));
        string[] names = (from object value in values select value.ToString()).ToArray();
        for (int i = 0; i < names.Length; i++)
        {
            playerStatsText += panelType == PanelType.Stats
                ? names[i].SplitOnCapitalLetters() + @" : " + playerStats[i].Increase(playerStats[i].PointsInStats)
                : names[i].SplitOnCapitalLetters() + @" : " + playerStats[i].PointsInStats;
            playerStatsText += Environment.NewLine;
        }
        SetText(panelType == PanelType.Points ? panelPoints : panelStats, "EditorOnly", playerStatsText);
    }

    private static void SetText(GameObject inputGameObject, string textTag, string textToSet)
    {
        inputGameObject.GetComponentsInChildren<Text>().Single(comp => comp.tag == textTag).text = textToSet;
    }

here are some extra objects you might need :

public enum StatsOrders
{
    MagicPower,
    DebuffDuration,
    Defense,
    MagicDefense,
    CriticalPower,
    CriticalChance,
    Multistrike,
    CooldownReduction,
    Evasion,
    Resistance
}
private enum PanelType
{
    Stats,
    Points
}

private GameObject panelStats;
private GameObject panelPoints;

What concern's me is that it looks over-complicated, can the code be shorten or improved in some way ?

Your update status method does too much. It has to decide which panel to use, which string to create and even how to create it.

If you separated them you could focus on exacly one task at a time:

The entry method should just decide which specialized update method to use.

It passes the required parameters to them and lets them do the job.

After reciving the result it passes it to the SetText method.

private void UpdateStatusLabels(PanelType panelType)
{
    var playerStats = Player.BaseStats;
    var values = Enum.GetValues(typeof(StatsOrders));
    var names = from object value in values select value.ToString();

    switch (panelType)
    {
        case PanelType.Stats:
            var playerStatsText = CreateStatsPanelText(playerStats, names);
            SetText(panelStats, "EditorOnly", playerStatsText);
        break;

        case PanelType.Points:
            // ...
        break;
    }

}

Everything the specialized methods do is to create the new string. (this is an example for one of them).

You don't need to use arrays and indexes. As you are looping over both collections synchronously you can make your life easier and use the Zip extension to iterate over both of them at the same time.

For creating the status text you should use the StringBuilder which makes the string creation cleaner.

private void CreateStatsPanelText(IEnumerable<Stats.Stats> playerStats, IEnumerable<string> names)
{
    var playerStatsText = new StringBuilder();

    foreach (playerStats.Zip(names, (playerStat, name) => new { playerStat, name }))
    {
        playerStatsText
            .Append(name.SplitOnCapitalLetters())
            .Append(" : ")
            .Append(playerStat.Increase(playerStat.PointsInStats))
            .AppendLine();
    }

    return playerStatsText.ToString();
}


Laravel - Repository and Entity patterns I've spent quite a bit of time studying the repository and entity patterns and this is what I came up with. I would appreciate it if you could post suggestions and critique.

User entity:

<?php
    namespace App\Entities\Entities\User;

    use
    App\Entities\Abstracts\AbstractEntity,
    App\Repositories\Interfaces\UserRepositoryInterface as UserRepository,
    App\Services\Validation\Laravel\UserLoginValidator,
    Str,
    Exception,
    ExceptionHandler,
    stdClass,
    MessageBag;

    class UserEntity extends AbstractEntity
    {
        protected $loginValidator;

        public function __construct(
            UserRepository $repository
            UserLoginValidator $loginValidator
            )
        {
            $this->repository = $repository;
            $this->loginValidator = $loginValidator;
        }

        public function login(array $input)
        {
            $result = $this->validate($this->loginValidator, $input);
            if(!$result instanceof MessageBag)
            {
                try
                {
                    $user = $this->repository->authenticate(array(
                        'email'     =>  $input['email'],
                        'password'  =>  $input['password']
                        ), false);
                    $this->save($user);
                    $user->type = $user->getGroups()[0]->id;

                    $result = new stdClass;
                    $result->errors = false;
                    $result->status_code = 23;
                    $result->user = $user->toArray();
                    return $result;
                }
                catch (Exception $e)
                {
                    return ExceptionHandler::returnException($e);
                }
            }
            else
                return $this->loginValidator->returnErrors();
        }

    }

User Repository:

<?php

    namespace App\Repositories\Eloquent\User;

    use
    App\Repositories\Abstracts\AbstractRepository,
    App\Repositories\Interfaces\UserRepositoryInterface,
    Cartalyst\Sentry\Sentry as Sentry,
    Cartalyst\Sentry\Users\Eloquent\User as SentryUser;

    class EloquentUserRepository extends AbstractRepository implements UserRepositoryInterface
    {
        public function __construct(Sentry $model)
        {
            $this->model = $model;
        }

        public function find($id)
        {
            return $this->model->findUserById($id);
        }

        public function save(SentryUser $model)
        {
            return $model->save();
        }

        public function getAccessToken()
        {
            return $this->model->getUser()->access_token;
        }
    }
?>

User controller:

<?php
    namespace Controllers\API;
    use
    BaseController,
    Input,
    ResponseSender,
    App\Entities\Entities\User\UserEntity;

    class UserController extends BaseController
    {
        protected $user;

        public function __construct(UserEntity $user)
        {
            $this->user = $user;
        }

        public function postLogin()
        {
            return ResponseSender::send($this->user->login(Input::all()));
        }

    }
?>

Maybe I'm just thinking of a different approach, but I normally would've reversed the dependency between UserEntity and UserRepository.  I would expect the UserEntity to represent a singular record in the database and that I would use the UserRepository to retrieve one.

In this case, I would inject the UserRepository into the controller and call UserRepository.Login(userId, password) that would actually perform the lookup against the DB and return a UserEntity representing the logged in user.

Also, I would suggest defining interfaces for your injected classes so that it's easier to provide future implementations


Adding several text parts to one file in sequence In my script, node.js reads text files and generates text itself. At the end, I have five "text parts", and want to add them to one file. For this step, it must be considered that the sequence is important, so unfortunately the great async technique of node is obstructive. How would you handle this?

The five text parts consists of:

Read File "head.txt" and than write/append its text (not much data) to the file
Text (not much data) generated by node itself -> write/append its text to the file
Text (large data) generated by node itself (loop)
Text (large data) generated by node itself (loop)
Read File "footer.txt" and than write/append its text (normal size) to the file

var fs = require('fs');

var filename = "content.txt";

// (1) Read 'head.txt'
fs.readFileSync('head.txt', 'utf8', function (err, head) {
    if (err) {
        return console.log('Could not read file.');
    }
    fs.appendFile(filename, head);
    //var writeStream = fs.createWriteStream(filename);
    //writeStream.write(head);
});

// (2)
var a = '...';
var b = '...';
var ab = a + b;
//var writeStream = fs.createWriteStream(filename);
//writeStream.write(ab);
//writeStream.end();
fs.appendFileSync(filename, ab);

// (3)
var writeStream = fs.createWriteStream(filename);
for (i = 0; i < xzy.length; i++) {
    writeStream.write('...' + xyz[i] + '...');
}
//writeStream.end();

// (4)
var writeStream = fs.createWriteStream(filename);
for (i = 0; i < xyz.length; i++) {
    writeStream.write('...' + xyz[i] + '...');
}
//writeStream.end();

// (5) Read 'footer.txt'
fs.readFileSync('footer.txt', 'utf8', function (err, footer) {
if (err) {
    return console.log('Could not read file.');
}
var writeStream = fs.createWriteStream(filename);
writeStream.write(footer);
//writeStream.end();
});

/*writeStream.on('finish', function () {
    console.log('finish');
});
writeStream.on('close', function () {
    console.log('close');
});*/

Here we go, I would create some "util" namespace to write some helper code:

./util/index.js

const bPromise = require('bluebird');

const fs = require('fs');

const readFile = bPromise.promisify(fs.readFile);

const appendFile = bPromise.promisify(fs.appendFile);

const createWS = filename => {
    var writer = fs.createWriteStream(filename, {defaultEncoding: 'utf8', flags : 'a'});
    return {
        write : bPromise.promisify(writer.write.bind(writer))
    }
}

module.exports = {
    readFile,
    appendFile,
    createWS
}

Thanks to bluebird we can convert functions with callback functions into functions that returns a resolved or rejected Promise with the result value. So that is what it's done in "./util/index.js".

In your main file:

var { readFile, appendFile, createWS} = require('./util');

var filename = "content.txt";
var writer = createWS(filename);
var xyz = ['x', 'y', 'z'];

// (1) Read 'head.txt'
readFile('head.txt', 'utf8')
.then( head => appendFile(filename, head))
.then( ()=>{
    // Head appended
    var a = '...';
    var b = '...';
    var ab = a + b;
    return appendFile(filename, ab);
})
.then( ()=>{
    // ab appended
    //return xyz.reduce((a, b)=> a.then( ()=> writer.write('...' + b + '...') ), Promise.resolve(null) )
    // or just
    return writer.write(xyz.map(el=>'...' + el + '...').join(''));
})
.then( ()=>{
    return writer.write(xyz.map(el=>'...' + el + '...').join(''));
})
.then( ()=>{
    return readFile('footer.txt', 'utf8')
        .then( footer => writer.write(footer))
})
.then( ()=>{})
.catch(console.error.bind(console));

Now looks very nice and it's clear. There's no need (Like in your code) to execute synchronous functions.

Check the ES5 code.

./util/index.js

var bPromise = require('bluebird');

var fs = require('fs');

var readFile = bPromise.promisify(fs.readFile);

var appendFile = bPromise.promisify(fs.appendFile);

var createWS = function createWS(filename) {
    var writer = fs.createWriteStream(filename, { defaultEncoding: 'utf8', flags: 'a' });
    return {
        write: bPromise.promisify(writer.write.bind(writer))
    };
};

module.exports = {
    readFile: readFile,
    appendFile: appendFile,
    createWS: createWS
};

index.js

var _require = require('./util');

var readFile = _require.readFile;
var appendFile = _require.appendFile;
var createWS = _require.createWS;

var filename = "content.txt";
var writer = createWS(filename);
var xyz = ['x', 'y', 'z'];

// (1) Read 'head.txt'
readFile('head.txt', 'utf8').then(function (head) {
    return appendFile(filename, head);
}).then(function () {
    // Head appended
    var a = '...';
    var b = '...';
    var ab = a + b;
    return appendFile(filename, ab);
}).then(function () {
    // ab appended
    //return xyz.reduce((a, b)=> a.then( ()=> writer.write('...' + b + '...') ), Promise.resolve(null) )
    // or just
    return writer.write(xyz.map(function (el) {
        return '...' + el + '...';
    }).join(''));
}).then(function () {
    return writer.write(xyz.map(function (el) {
        return '...' + el + '...';
    }).join(''));
}).then(function () {
    return readFile('footer.txt', 'utf8').then(function (footer) {
        return writer.write(footer);
    });
}).then(function () {}).catch(console.error.bind(console));


CodeWars kata 'Sum Of Pairs' I need help in optimizing my code for this challenge. I think my algorithm is fine and I pass all sample tests but get took longer than 12000ms to complete error for final one.

I guess there are a lot of unnecessary variables but this is all my beginners Ruby brain could think of. :)

I have started learning coding 2 weeks ago so style-wise this is most certainly horrible.

Kata link

  Instructions :

  Given a list of integers and a single sum value, return the first two values (parse from the left please) in order of appearance that add up to form the sum.
  Negative numbers and duplicate numbers can and will appear.

  NOTE: There will also be lists tested of lengths upwards of 10,000,000 elements. Be sure your code doesn't time out.

def sum_pairs(numbers, sum)
  # Idea was :
  #  1. To iterate over array elements take their sum and add
  #     it to hash if it is equal to "sum".
  #  2. Sort hash by value to find lowest second parameter since
  #     we need entire pair to be first not just first element.
  #  3. Convert that hash to array so that I could take value
  #      of those elements and return actual numbers.
  #     (Didn't know to do it from hash directly :))
  x = 0
  h = {}
  while x < numbers.length
    y = 0
    while y < numbers.length
      sum_test = numbers[x] + numbers[y]
      if sum_test == sum && x != y && x < y
        h[x] = y
      end
      y += 1
    end
    x += 1
  end
  if h.empty?
    return nil
  else
    final_positions = h.sort_by {|key, value| value}.first.to_a
    return numbers[final_positions[0]], numbers[final_positions[1]]
  end
end

You have two nested while loops, so your solution is O(n2). A good solution should be possible in O(n) — that is, one pass through numbers.

Also, the while loops are rather clumsy. Idiomatic Ruby would usually use #each to iterate, or #each_with_index if you need the indexes as well. Here, you don't care about the indexes. If you return as soon as you have encountered a solution, it will necessarily be the leftmost solution.

require 'set'

def sum_pairs(numbers, sum)
  seen = Set.new
  numbers.each do |n|
    return [sum - n, n] if seen.include?(sum - n)
    seen.add(n)
  end
  nil
end


Using enum to create, subscript and loop an array of structs in Swift 5.0 I'm trying to "build" a few "spaceships" in my code. I'm feeling it's a good idea to enumerate all the spaceships and use that as a subscript to build and use an array that holds the spaceships' properties. After researching Stack Overflow and other places, I found a solution that works, but I'm feeling it's not very elegant and I'm probably not making good use of the Swift language. I'm also unhappy with having to define the name (string) of the spaceship again (and not using the enum case for that).

Do I miss the obvious cleaner solution? Any suggestions for a better solution?

enum SpaceshipName: Int, CaseIterable {
  case Orion
  case Vega
  case Gemini
}

struct SpaceshipProperties {
  var name: String = "TBD"
  var fuel: Int = 0
  var speed: Int = 0
}

// Initialize the spaceships:

var spaceShips = [SpaceshipProperties]()

for s in SpaceshipName.allCases {
  spaceShips.append(SpaceshipProperties())
}

// Now start using the spaceships:

spaceShips[SpaceshipName.Orion.rawValue].name = "Orion"
spaceShips[SpaceshipName.Orion.rawValue].fuel = 100
spaceShips[SpaceshipName.Gemini.rawValue].speed = 2200
// ... and so forth

// Ultimately, I want to iterate through the spaceships in the following manner:

for s in SpaceshipName.allCases {
  // Perform calculations with all the spaceships here, for example:
  let newSpeed = spaceShips[s.rawValue].speed + 10
  spaceShips[s.rawValue].speed = newSpeed
}
```

Maybe you should create enum with property? Something like this, I don't know if it is what you want but hope it will be helpful.

enum Ship {

    case first(ShipPropperties), second(ShipPropperties), third(ShipPropperties)

    static let allValues: [Ship] = [first(ShipPropperties()), second(ShipPropperties()), third(ShipPropperties())]

    var properties: ShipPropperties {
        switch self {
        case .first(let properties): return properties
        case .second(let properties): return properties
        case .third(let properties): return properties
        }
    }

    func changeProperties(to: ShipPropperties) {
        properties.changeProperties(name: to.name, fuel: to.fuel, speed: to.speed)
    }

}

class ShipPropperties {

    var name: String = "TBD"
    var fuel: Int = 0
    var speed: Int = 0

    func changeProperties(name: String, fuel: Int, speed: Int) {
        self.name = name
        self.fuel = fuel
        self.speed = speed
    }

}


Set the entitlement of a case based on record type This is my first salesforce code. It seems to me there is too much data lookup but I don't know enough about saleforce apex coding to say.

The purpose of the code is to set the entitlement based on the record type when inserting a new Case record.

The business reason for this code is the proper assignment of Entitlements (SLAs) for the various Case types.

The code runs before the record is inserted into the table. It enforces the relationship between Case type and Entitlement type.

trigger SetCaseEntitlementOnInsert on Case (before insert) {
    System.debug('SetCaseEntitlementOnInsert executed');

    // 1) Loop through the Case(s) to be inserted.
    // 2) Get the Entitlement for each based on the RecordType.
    // 3) Retrieve the Entitlement.
    // 4) Assign the Entitlement to the Case

    for(Case c : Trigger.New) {
        RecordType recordType = [SELECT Id, Name FROM RecordType WHERE Id =: c.RecordTypeId];
        String recordTypeName = recordType.Name;

        if(recordTypeName == 'Trouble Tickets') {
            Entitlement entitlement = [SELECT Id, Name FROM Entitlement WHERE Name = 'Priority 1'];
            c.EntitlementId = entitlement.Id;
        } else if (recordTypeName == 'Access Request/New Hire') {
            Entitlement entitlement = [SELECT Id, Name FROM Entitlement WHERE Name = 'Priority 2'];
            c.EntitlementId = entitlement.Id;
        } else if (recordTypeName == 'Purchase Order') {
            Entitlement entitlement = [SELECT Id, Name FROM Entitlement WHERE Name = 'Priority 3'];
            c.EntitlementId = entitlement.Id;
        } else {
            // Termination
            Entitlement entitlement = [SELECT Id, Name FROM Entitlement WHERE Name = 'Priority 4'];
            c.EntitlementId = entitlement.Id;
        }
    }
}

Well done on your first try for Apex code.

A few pointers:

Record types aren't always available for all users, when doing logic on Record types by Name I usually add a Formula to retrieve the name (one less query and I can control the permissions)
Don't add queries in for loops, you could / would hit SOQL limits very very quicly

I haven't proven that this will work but here is an example of what you should do:

trigger SetCaseEntitlementOnInsert on Case (before insert) {
    System.debug('SetCaseEntitlementOnInsert executed');

    // 1) Loop through the Case(s) to be inserted.
    // 2) Get the Entitlement for each based on the RecordType.
    // 3) Retrieve the Entitlement.
    // 4) Assign the Entitlement to the Case

    // Assuming you create a custom field with a Formula to retrieve RecordType.Name with API name RecordTypeName__c

    // Collection of Entitlement Names
    Set<String> theEntitlementNames = new Set<String> {
        'Priority 1',
        'Priority 2',
        'Priority 3',
        'Priority 4'
    };

    // Query the Entitelments
    List<Entitlement> theEntitlements = [SELECT Id, Name FROM Entitlement WHERE Name IN :theEntitlementNames];

    // If we have Entitlements
    if (!theEntitlements.isEmpty()) {

        // Map the Entitlement Ids by Name
        Map<String, Id> theEntitlementIdsByName = new Map<String, Id>();
        for (Entitlement theEntitlement : theEntitlements) {
            theEntitlementIdsByName.put(theEntitlement.Name, theEntitlement.Id);
        }

        // Now for each Case
        for (Case theCase : Trigger.New) {
            if (theCase.RecordTypeName__c == 'Trouble Tickets') {
                theCase.EntitlementId = theEntitlementIdsByName.get(theEntitlementNames[0]);
            } else if (theCase.RecordTypeName__c == 'Access Request/New Hire') {
                theCase.EntitlementId = theEntitlementIdsByName.get(theEntitlementNames[1]);
            } else if (theCase.RecordTypeName__c == 'Purchase Order') {
                theCase.EntitlementId = theEntitlementIdsByName.get(theEntitlementNames[2]);
            } else {
                theCase.EntitlementId = theEntitlementIdsByName.get(theEntitlementNames[3]);
            }
        }

    }
}


replace() for translating color names I have some code to translate some color names, and this is what I did, quick and dirty.
But I'm guessing there might be some function to replace arrays with arrays maybe, like in PHP, where you can have str_replace($array_of_colors, $replace_array_of_colors, $colors);.

var color = colors[c].replace(/sort/i, "Black");
color = color.replace(/brun/i, "Brown");
color = color.replace(/hvit/i, "White");
color = color.replace(/oransje/i, "Orange");
color = color.replace(/gul/i, "Yellow");
color = color.replace(/beige/i, "Beige");
color = color.replace(/Mørk/i, "Dark");
color = color.replace(/Rosa/i, "Pink");
color = color.replace(/rød/i, "Red");
color = color.replace(/Blå/i, "Blue");
color = color.replace(/grå/i, "Gray");
color = color.replace(/Lys/i, "Light");
color = color.replace(/lilla/i, "Purple");

You're mixing up data and logic. Separate your replaces into an associative array and iterate over that instead: (My JS is extremely bad, so this will be more pseudocode-ish)

var associations;
associations['/brun/i'] = "Brown";
associations['/sort/i'] = "Black";
//add all the other associations and then

associations.forEach(runReplace);

function runReplace(value, regex, array) {
   color.replace(regex, value);
}

Additionally I suggest you (just like me here) use the Array.prototype.forEach() when iterating over your colors


Find the cheapest shipping option based on item weight I've been taking the Computer Science course on Codecademy. The course includes this project where we have to create a python script to find the cheapest shipping method based on the weight of an item and I'm just wondering if this is the best way to code the solution?

def ground_shipping_cost(weight):
    flat_cost = 20
    premium_cost = 125
    if weight <= 2:
        flat_cost += weight * 1.50
    elif weight > 2 and weight <= 6:
        flat_cost += weight * 3.00
    elif weight > 6 and weight <= 10:
        flat_cost += weight * 4.00
    elif weight > 10:
        flat_cost += weight * 4.75
    return flat_cost, premium_cost

def drone_shipping_cost(weight):
    cost = 0
    if weight <= 2:
        cost = weight * 4.50
    elif weight > 2 and weight <= 6:
        cost = weight * 9.00
    elif weight > 6 and weight <= 10:
        cost = weight * 12.00
    elif weight > 10:
        cost = weight * 14.25
    return cost

def cheapest_shipping(weight):
    ground_cost, premium_cost = ground_shipping_cost(weight)
    drone_cost = drone_shipping_cost(weight)
    if drone_cost < ground_cost and drone_cost < premium_cost:
        return "You should use drone shipping as it will only cost " + str(drone_cost)
    if ground_cost < drone_cost and ground_cost < premium_cost:
        return "You should use standard ground shipping as it will only cost " + str(ground_cost)
    if premium_cost < ground_cost and premium_cost < drone_cost:
        return "You should use premium shipping as it will only cost " + str(premium_cost)

print(cheapest_shipping(4.8)) # You should use standard ground shipping as it will only cost 34.4
print(cheapest_shipping(41.5)) # You should use premium shipping as it will only cost 125
print(cheapest_shipping(1.5)) # You should use drone shipping as it will only cost 6.75
print(cheapest_shipping(4.0)) # You should use standard ground shipping as it will only cost 32.0

The issue I see is maybe there's too many if, elif statements and maybe there is a way to simplify the code

premium_cost

There is no reason to include the return of this premium_cost in ground_shipping_cost. Use a different method here

DRY

You are correct. There are too many if statements. If you want to introduce an new threshold for the cost, this will include a adding an extra clause, and changing the limits for the preceding and following thresholds. This is just waiting for errors to pop up. Better would be to keep a dict of the thresholds, and iterate over them to get this price factor:

def ground_shipping_cost(weight):
    thresholds = {2: 1.5, 6: 3.0, 10: 4.0, float("inf"): 4.75}
    flat_cost = 20
    for threshold, factor in sorted(thresholds.items()):
        if weight <= threshold:
            break
    return flat_cost + weight * factor

thresholds is a dict, with the cost per weight unit as value, and the threshold as key.

The drone_shipping_cost can be tackled comparably.

Now you have 2 methods that, starting from a list of thresholds, tries to determine the cost factor. We can easily refactor this out:

def get_factor(thresholds, value):
    for threshold, factor in sorted(thresholds.items()):
        if value <= threshold:
            return factor

def ground_shipping_cost(weight):
    thresholds = {2: 1.5, 6: 3.0, 10: 4.0, float("inf"): 4.75}
    flat_cost = 20
    return flat_cost + get_factor(thresholds, weight)

def drone_shipping_cost(weight):
    thresholds = {2: 4.5, 6: 9.0, 10: 12.0, float("inf"): 14.75}
    return weight * get_factor(thresholds, weight)

Cheapest costs

Your cheapest_shipping method calculates the costs for the different shipping methods, finds the cheapest one and formats this into a string. This string formatting is also very repetitive, and should be done somewhere else. cheapest_shipping will be the most clear if it only returned which method is the cheapest, and the corresponding cost. This also allow you to test this method with unit tests further on.

Since methods are real objects in Python, and you can pass references to them on and store these in dicts, calculating the costs for the different methods can be simplified a lot:

def cheapest_shipping(weight):
    methods = {
        "drone": drone_shipping_cost,
        "standard ground": ground_shipping_cost,
        "premium ground": lambda weight: 125,
    }
    results = {method: calculation(weight) for method, calculation in methods.items()}

To look for the cheapest option among those, you can use the built-in min:

cheapest_method = min(results, key=lambda method: results[method])

return cheapest_method, results[cheapest_method]

Note: the lambda weight: 125 is the equivalent of

def premium_shipping(weight):
    return 125

and "premium ground": premium_shipping, in the methods dict

And this can be called and formatted using str.format or f-strings in Python 3.6+

method, cost = cheapest_shipping(4)
f"You should use {method} shipping as it will only cost {cost}"

'You should use standard ground shipping as it will only cost 23.0'

Further refactoring

You can even refactor this one step further, and have 1 generalized cost calculation method that take a flat_cost and thresholds as arguments

def get_shipping_cost(weight, thresholds=None, flat_cost=0):
    if thresholds is None:
        return flat_cost
    return flat_cost + weight * get_factor(thresholds, weight)

then you can define the different shipping methods like this:

shipping_methods = {
    "drone": {"thresholds": {2: 4.5, 6: 9.0, 10: 12.0, float("inf"): 14.75}},
    "standard ground": {
        "flat_cost": 20,
        "thresholds": {2: 1.5, 6: 3.0, 10: 4.0, float("inf"): 4.75},
    },
    "premium ground": {"flat_cost": 125},
}

def cheapest_shipping2(methods, weight):
    results = {
        method: get_shipping_cost(weight, **parameters)
        for method, parameters in methods.items()
    }
    cheapest_method = min(results, key=lambda method: results[method])
    return cheapest_method, results[cheapest_method]

method, cost = cheapest_shipping2(shipping_methods, 4)

different min

Instead of using min with the cost as key, you can reverse the results dictionary:

def cheapest_shipping2(methods, weight):
    results = {
        get_shipping_cost(weight, **parameters): method
        for method, parameters in methods.items()
    }

    cost, method = min(results.items())
    return method, cost

In case of an ex aequo, this can have different results than the other method

even further refactoring.

Now all shipping methods are calculated with get_shipping_cost. If you want to use different functions for the different shipping methods, you can do something like this:

def cheapest_shipping3(methods, weight, default_cost_method=get_shipping_cost):
    results = {
        parameters.pop("method", default_cost_method)(
            weight, **parameters
        ): method
        for method, parameters in methods.items()
    }
    cost, method = min(results.items())
    return method, cost

shipping_methods2 = {
    "drone": {"thresholds": {2: 4.5, 6: 9.0, 10: 12.0, float("inf"): 14.75}},
    "standard ground": {
        "flat_cost": 20,
        "thresholds": {2: 1.5, 6: 3.0, 10: 4.0, float("inf"): 4.75},
    },
    "premium ground": {"method": lambda weight: 125},
}

method, cost = cheapest_shipping3(shipping_methods2, 4)

please note that the parameters.pop mutates the original shipping_methods2 after the execution:

{
    "drone": {"thresholds": {2: 4.5, 6: 9.0, 10: 12.0, inf: 14.75}},
    "standard ground": {
        "flat_cost": 20,
        "thresholds": {2: 1.5, 6: 3.0, 10: 4.0, inf: 4.75},
    },
    "premium ground": {},
}

To prevent this, we need to make a copy of the methods:

def cheapest_shipping3(methods, weight, default_cost_method=get_shipping_cost):
    methods_copy = {
        method: parameters.copy()
        for method, parameters
        in methods.items()
    }
    results = {
        parameters.pop("method", default_cost_method)(
            weight, **parameters
        ): method
        for method, parameters in methods_copy.items()
    }
    cost, method = min(results.items())
    return method, cost