Timeout thrower using multithreading I designed a simple timeout thrower for a bluetooth protocol I am writing. If the packet is not received in a certain amount of time, then timeout is thrown. I made it to be as plug and play as possible.

timeout.hpp:

#ifndef __timeout_h__
#define __timeout_h__

#include <thread>

struct Timeoutthread{
    private:
        int sleep;
    public:
        Timeoutthread(int seconds);
        void time(bool* alert);
};

class Timeout{
    private:
        int sleep;
        bool alert;
    public:
        Timeout(int seconds);
        bool timeout();
};

#endif

timeout.cpp:

#include "timeout.hpp"

Timeout::Timeout(int seconds){
    sleep = seconds;
    Timeoutthread timeoutthread(sleep);
    std::thread timeout(&Timeoutthread::time,timeoutthread,&alert);
    timeout.detach();
}

bool Timeout::timeout(){
    return alert;
}

Timeoutthread::Timeoutthread(int seconds){
    sleep = seconds;
}

void Timeoutthread::time(bool* alert){
    std::this_thread::sleep_for (std::chrono::seconds(sleep));
    *alert = true;
}

simple implementation:

#include <iostream>
#include "timeout.hpp"

int main(){
     std::string message = "";
     Timeout timeout(10);
     while(message == ""){
          message = readBLE();
          if(timeout.timeout() == true)
               message = "timed out";
     }

     std::cout<<message<<std::endl;

     return 0;
}

The loop waits for a bluetooth message, but if no message is received in 10 seconds, then a timeout is thrown.

Is this an efficient way to do it. I want to make sure that I am not wasting any resources.

EDIT:

There seems to be some confusion on the readBLE() part. There is a lot more code associated with this part but it does not act like std::cin where it is waiting for a user input. Whenever readBLE() is called, if there was no message received, then it would just return an empty string. Sorry for the confusion.

The Timeout class doesn't solve the problem you have

If you write:

message = (read from bluetooth);
if(timeout.timeout() == true)
    message = "timed out";

Then it will first wait for the message to be read from Bluetooth, which might take more than 10 seconds, and then once you have the message, it will check whether more than 10 seconds have passed since the start, and if so it will discard the message you got. The fact that the timer is run in its own thread does not magically make (read from bluetooth) exit after the timer expires.

What you instead have to do is run the (read from bluetooth) command in a thread, and wait at most 10 seconds for that to complete. With C++11, you can do this very easily with std::async():

#include <future>
#include <chrono>

...

auto future = std::async(std::launch::async, [] {
    return (read from bluetooth);
});

auto status = future.wait_for(std::chrono::seconds(10));

if (status == std::future_status::ready)
    message = future.get();
else
    message = "timed out";

The problem however is that if there is a timeout, the thread running the Bluetooth read command is still running. When exiting the scope, the destructor of future will block until the thread finishes execution. So this kind of approach has a limited use.

The best solution would be to find some wait to make (read from bluetooth) itself give up after 10 seconds, or have some way to cause it to stop waiting for data.

Identifiers with double underscores are reserved

You should not use identifiers that start with underscores, or contain double underscores, as they are reserved, and might be used by the compiler and/or the standard library. This even applies to macros, so instead of:

#ifndef __timeout_h__
#define __timeout_h__

Write:

#ifndef timeout_h
#define timeout_h

Or use the following pragma that most compilers understand, and that ensure that a header file is only read once:

#pragma once

Store durations as std::chrono::duration

Avoid storing timeouts as int, this limits the resolution. Instead, consider using std::chrono::duration to store the timeout period.

Make member functions const where appropriate

If a member function doesn't modify any member variables, mark it as const, like so:

class Timeout {
    ...
    bool timeout() const;
}

Ensure variables are properly initialized

You never initialize alert to false, so a call to timeout() might return an uninitialized value.

Use std::atomic<> variables when communicating between threads

If you write:

Timeout timeout(10);
// do something
if (timeout.timeout())
    ...

Then the compiler might know that alert is set to false in the first line, and if it can prove that do something never touches the variable timeout, then it can assume in the third line that alert will always be false. To ensure the compiler does not make such assumptions when threads are involved, you have to tell it that it should atomically read and write this flag.

Do you need a separate thread at all?

The only thing your Timeoutthread does is sleep for a certain amount of time, then set a variable. You know that the threads sets that variable a given number of seconds after it starts. So instead of using a thread, you can just store the current time when an instance of Timeout is created in a separate member variable, and in timeout() just check the difference between the current time now and the time stored in that member variable.


Easy Print PHP From Data array or objects I have created a PHP class to print into html easily from an array or object.

Using this class, you can get rid of multiple echo blocks or ugly concatenation while working with data arrays to transform into html.

class PrintEasy {
    /**
     * Print single data item with html
     * Include variable names within a [] block in the html string.
     *
     * @param string $html
     * @param mixed $data Accepts both array and object format
     * @return boolean
     */
    public static function single($html, $data) {
        $newHtml = preg_replace_callback(
            '/\[([^\]]+)\]/',
            function($match) use ($data) {
                $key = $match[1];
                if(is_object($data)) {
                    return $data->$key;
                } elseif(is_array($data)) {
                    return $data[$key];
                } else return $match[0];

            },
            $html
        );
        echo $newHtml;
        return true;
    }

    /**
     * Print multiple items from a data array with html
     *
     * @param string $html
     * @param array $dataArray the dataArray can contain both array and object format data items
     * @param int $count (optional) If provided, only that number of items will be printed from the array
     * @return boolean
     */
    public static function multiple($html, $dataArray, $count = 0) {
        if(!$count) {
            $count = count($dataArray);
        }

        for($i=0;$i<$count;$i++) {
            PrintEasy::single($html, isset($dataArray[$i]) ? $dataArray[$i] : array());
        }
        return true;
    }
}

I have also created a github repo.

Are there any improvements I can make?

Readability issues

After staring at your code for about 5 minutes, I wasn't able to figure out what it was doing. So I copied it over to a PHP file and started testing it out. After some time I found out that it is replacing [name] in the HTML with the name key in an array provided as the $data parameter.

Your function names single() and multiple() are very non-descriptive. If I saw a piece of code doing PrintEasy::single($html, $data); I'd have no clue what is was doing other than printing some stuff. You want a line of code like that to be as descriptive as possible. This starts at naming your class and functions better.

On another note, I do not find the $count parameter on your second function very useful. Why wouldn't I just pass in a smaller array? The parameter most likely will only cause confusion. Furthermore, why not use the splat operator from PHP 5.6 to combine both of these methods into a single method with the same flexible functionality? This would result in a function like this:

function insert($html_string, ...$data) { // code

The name of your class is somewhat descriptive, as it does state that it will be printing something. However, we will be removing the echo with a return in a bit, so the class itself will no longer print something. So we need to come up with a better name for it. In this short period of time (good names can take time) I came up with VariableInserter, TemplateVariableInserter and PlaceholderReplacer.

I think I will be going with PlaceholderReplacer, as that is the only name that clearly indicates that the original text contains some placeholder strings that are going to be replaced.

Functionality

Your code does seem to work pretty well though. One tiny error that I noticed is that when your HTML contains for example [name], but there is no data object/array that contains this key, an undefined index/property NOTICE is raised.

Also, your PrintEasy class already decides for me what I want to do with the result. What if I want to use var_dump() instead, of if I want to use this functionality for an entirely different purpose? That is currently not possible with your implementation. Why not create a class that simply returns the replaced HTML. This makes it way more flexible. And if you do actually want to echo it, you just place echo in front of the function call.

Something else that struck me while looking at your code is that the implementation of your class is way more general than just for HTML. It can replace variables in any string. So why name it in such a specific way?

Rework of the class

Now that we have some better names for our class and functions, we can start working on improving the current code.

class PlaceholderReplacer {
    /**
     * Replaces placeholder strings such as "hello [name]" with
     * the 'name' key/property from an array/object
     *
     * @param string $string
     * @param array|object ...$data_sources
     * @return string
     */
    public static function replaceWithData($string, ...$data_sources)
    {
        $new_string = preg_replace_callback(
            '/\[([^\]]+)\]/',
            function ($match) use ($data_sources)
            {
                return self::retrieveMatchValueFromDataSources($match, $data_sources) ?: $match[0];
            },
            $string
        );

        return $new_string;
    }

    /**
     * @param array $match
     * @param array $data_sources
     * @return mixed|null
     */
    private static function retrieveMatchValueFromDataSources(array $match, array $data_sources)
    {
        $key = $match[1];

        foreach ($data_sources as $data_source)
        {
            if (is_object($data_source) && isset($data_source->$key))
            {
                return $data_source->$key;
            }

            if (is_array($data_source) && isset($data_source[$key]))
            {
                return $data_source[$key];
            }
        }

        return NULL;
    }
}

As you can see, I have extracted another private method from your original method. This method is responsible for trying to retrieve a key from any of the given data sources. By splitting up this functionality into a separate function your code becomes more readable and maintainable.

I have also added isset() checks before attempting to retrieve data from the data sources to prevent a NOTICE from being raised.

Readability of class usage

Now compare the use of your class to that of the class above. Assume these variables exist:

$html = "
 Hello [name]!
";
$data_source = ["name" => "Chuck Norris"];

Now consider

PrintEasy::single($html, $data_source);

versus

echo PlaceholderReplacer::replaceWithData($html, $data_source);

Now you might argue that the functionality of your PrintEasy::single() method could be derived from looking at the $html and $data_source variables and I would somewhat agree. But keep in mind that often you don't directly see the contents of a string like this (think of a piece of text retrieved from a database). This would make it much harder to figure out what PrintEasy::single() is actually doing.

Reusability

Furthermore, without echoing the new string directly in the function but returning it, the functionality has become much more versatile. For example, I could rebuild your class using the new PlaceholderReplacer class:

class PrintEasy {
    function single($html, $data)
    {
        echo PlaceholderReplacer($html, $data);
    }

This example shows that there was some functionality in your class that could be extraced to a class of its own, with a single responsibility (SRP). Now all that is left of the printEasy class is to echo the result of another class. Now that just seems silly, so a dedicated PrintEasy class is no longer needed since we can just use echo outside of the class.

Testing

Another great aspect of returning instead of echoing is that it makes testing your code much easier.

Consider the following test:

class PlaceholderReplacerTest {
    testSimpleExample() {
        $html = "
 Hello [name]!
";
        $data_source = ["name" => "Chuck Norris"];

        $new_html = PlaceholderReplacer::replaceWithData($html, $data_source);
        $this->assertEquals("
 Hello Chuch Norris!
", $new_html);
    }
}

Your class did not return anything, but echos it directly, making it harder to test. (Not impossible however, since you could use ob_start() and ob_get_clean() to buffer the output and get the return value, but that is generally not something you want to be working with when testing other functionality).


Repository pattern with plain old PHP object I've been reading a lot about repository patterns these days. At first, the pattern seemed easy.

Most of the examples I read over the web use an ORM like below.

interface MemberRepositoryInterface {
    public function find($id);
}

class MemberRepository implements MemberRepositoryInterface {

    // here we're passing framework sepific ORM implementation
    public function __construct(Model_Member $model)
    {
        $this->model = $model;
    }

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

}

// In a controller

class Test_Controller {

    public function __construct(MemberRepositoryInterface $memberRepository)
    {
        $this->repository = $memberRepository;
    }

    public function show()
    {
        $member = $this->repository->find(1);

        $this->template->content = View::factory("testView")->set('member', $member)->render();
    }

}

It looks fine up to this point. Now..when you pass $member variable to a view then you can access all the framework specific methods of the ORM such as...delete(), save(), and so on.

what's the best way to prevent people from using framework specific ORM methods?

I thought about it..then the answer is to use a plain old PHP object.

// Modified Repository

class MemberModel {

    public $id;
    public $firstName;
    public $lastName;

    public function __construct(array $data)
    {
        foreach($data as $key=>$value)
        {
            if(isset($this->$key))
            {
                $this->$key = $value;
            }
        }
    }
}

class ContactModel {
    public $id;
    public $firstName;
    public $lastName;

    public function __construct(array $data)
    {
        foreach($data as $key=>$value)
        {
            if(isset($this->$key))
            {
                $this->$key = $value;
            }
        }
    }
}

class MemberRepository implements MemberRepositoryInterface {

    // here we're passing framework sepific ORM implementation
    public function __construct(Model_Member $model)
    {
        $this->model = $model;
    }

    public function find($id)
    {
        $record =  $this->model->find($id)->as_array();
        return new MemberModel($record);
    }

    public function find_with_contacts($id)
    {
        //find a member and contacts then use MemberModel and ContactModel
        // then return the data
    }

}

Is this  good enough? or are there other best practices?

You are already using the repository of Kohana. They hide their repository behind static factory methods though (copied from the documentation):

// Find user with ID 20
$user = ORM::factory('User')
    ->where('id', '=', 20)
    ->find();
// Or
$user = ORM::factory('User', 20);

Yet they fail to achieve the important thing about ORM and repositories: decoupling the application from the database. This is actually two-fold: the SQL-syntax leaks into the application space (the querybuilder) and the database-structure is mapped 1:1 on the model. Changing the model or database requires to change the other respectively. Comparing this to the great explanation at MSDN: the mapper is missing and the client business logic has access to the query object (and therefore to the datasource). A major (not related) issue is the all-presence of static method calls.

What you try is to fix this (thumbs up ;)). You have your business entities (MemberModel and ContactModel) and a repository MemberRepository. But instead of delegating the the ORMs repository you should use the query builder directly:

class MemberRepository implements MemberRepositoryInterface {
    public function find($id)
    {
        $record =  DB::select(...);
    }
}

The second important task is the mapping of database fields to entity fields (and vice versa). This should not be done in your business entity! The business entity does not know anything about how it is saved:

class MemberRepository implements MemberRepositoryInterface {
    public function find($id)
    {
        $record =  DB::select(...);
        // map $record to $entity;
        return $entity;
    }
}

Other means to manage your model in the repository as well (e.g. delete, update, create), not in your business entity!

Last your model. For now it is fine. But soon it should contain your business logic (no, not in your services or controllers ;)). You'll probably start with some data validation when setting properties. I'd suggest to use setters/getters right away, saves you refactoring later on.

Two final remarks about ORM at the end:

Repositories tend to be very repetitive. Especially when they only provide only the basic CRUD methods. Pretty much every ORM Framework provides a general repository for these methods (as Kohana does and what you used).
Some explanations on ORM pass query objects to the repository. Those are not to be confused with the query builder from your dbal. The query passed to the repository is written in the business language (e.g. the fields of your model) and mapped to a query of the querybuilder by the repository. This avoids leaking of database details into the business logic.


Triangulate the divisors and divide the triangulars Project Euler 12 - Highly divisible triangular number

  The sequence of triangle numbers is generated by adding the natural numbers. So the 7th triangle number would be \$1 + 2 + 3 + 4 + 5 + 6 + 7 = 28\$. The first ten terms would be: \$1, 3, 6, 10, 15, 21, 28, 36, 45, 55, ...\$

  (...) 28 is the first triangle number to have over five divisors.

  What is the value of the first triangle number to have over five hundred divisors?

I did spend quite a bit of time on this one to figure out a faster solution than what I first attempted. My first, very naive approach, to finding the number of divisors for a number was to loop through all the numbers and count them, like this:

public static long numDivisors(long number) {
    long count = number == 1 ? 1 : 2;
    for (int i = 2; i <= number / 2; i++) {
        if (number % i == 0) {
            count++;
        }
    }
    return count;
}

I knew there was a faster approach. And I didn't give up until I found one.

I realized, after factorizing some numbers, that the fastest way is to keep a track of the unique prime factors that make up the number, and use them to calculate the number of divisors.

For example, 36. It has 9 divisors in total, 1, 2, 3, 4, 6, 9, 12, 18, 36. We know though that 36 is 2*2*3*3. That's 2 factors of 2 and 2 factors of 3. So by choosing 0-2 2's and 0-2 3's we can get the 9 divisors of 36.

The same goes for a bigger number, such as 32063349528. If we prime-factorize it, it is 2 * 2 * 2 * 3 * 3 * 3 * 7 * 7 * 17 * 19 * 83 * 113. So 4*4*3*2*2*2*2 = 768 divisors.

public class ProjEuler12 {

    public static long triangleNumberN(long n) {
        if (n <= 0) {
            throw new IllegalArgumentException("n must be positive: " + n);
        }
        return (1 + n) * n / 2;
    }

    public static int divisorCount(long number) {
        if (number <= 0) {
            throw new IllegalArgumentException("number must be positive: " + number);
        }
        int divisors = 1;
        long lastNum = 0;
        int lastCount = 0;
        while (number != 1) {
            if (number % 2 == 0) {
                lastCount++;
                number /= 2;
                continue;
            }
            for (long i = 3; i <= number; i += 2) {
                if (number % i == 0) {
                    if (lastNum != i) {
                        divisors *= lastCount + 1;
                        lastCount = 1;
                        lastNum = i;
                    } else {
                        lastCount++;
                    }
                    number /= i;
                    break;
                }
            }
        }
        return divisors * (lastCount + 1);
    }

    public static void main(String[] args) {
        long result = LongStream.iterate(1, i -> i + 1)
                .map(ProjEuler12::triangleNumberN)
                .filter(n -> divisorCount(n) > 500)
                .findFirst().getAsLong();
        System.out.println(result);
    }

}

Any improvements possible? Can it be made even faster? Is the naming just as good as you would expect?

Can it be made even faster?

Sure.

Fact number one. An n'th triangular number is \$\frac{n(n+1)}{2}\$

Fact number two. A number of divisors (aka \$\sigma(n)\$) is multiplicative, that is \$\sigma(m*n) = \sigma(m)*\sigma(n)\$ for coprime m and n.

Fact number three. \$n\$ and \$n + 1\$ are coprime.

Which means you don't have to factorize (fairly large) triangular numbers. Instead, you only need to factorize \$n\$ and \$n+1\$; in fact, since one of them is even, and contributes as a number twice that small, at most one number needs to be factorized at each iteration. You need to memoize factorizations though.

And of course a precomputed prime numbers table may help.


Listening for several connections in an async method Is there a better way to structure a method that returns a task that queues several inbound socket connections?

    public Task Start(CancellationToken token)
    {
        return Task.Run(
            () =>
            {
                var acceptCount = new SemaphoreSlim(10);
                while (!token.IsCancellationRequested)
                {
                    acceptCount.Wait(token);
                    if (token.IsCancellationRequested)
                        break;

                    var args = new SocketAsyncEventArgs();
                    args.UserToken = acceptCount;
                    args.Completed += AcceptInbound;
                    if (!this.socket.AcceptAsync(args))
                        AcceptInbound(this.socket, args);

                }
            },
            token);
    }
    private void AcceptInbound(object sender, SocketAsyncEventArgs e)
    {
        ((SemaphoreSlim)e.UserToken).Release();
        Socket connectedClient = e.AcceptSocket;
    }

I think Task.Factory.FromAsync is what you are looking for...

public async Task Start(CancellationToken token)
{
     while (!token.IsCancellationRequested)
     {
         Socket connectedClient = await AcceptAsync(this.socket).ConfigureAwait(false);
         ...
     }
}

public static Task<Socket> AcceptAsync(Socket socket)
{
     return Task.Factory.FromAsync<Socket>(socket.BeginAccept, socket.EndAccept, null);
}


Optimize Memory Java String Array I was trying to solve one programming challenge :

First input is a list of connections which is made of pair of IPv4
address.  Each IP has its own state which starts with false, each
connection also has state which is made up by logical AND of state of
its IPs
Second input is a toggle IPs list , these toggles the state of IP
address ( false to true or true to false). For each toggle action ,
calculate the impact on connections which is defined as how many
connections state changed due to change in one IP address.

I solved the above challenge by using below Java program, but I'm not sure if there could be any scope of improvement for memory consumption and run time.
Please pardon my programming knowledge, I would appreciate any positive feedback to cover corener cases
public class NumberOfDevices {

    public static void main(String[] args) {
        String [][]connections ={{"192.167.0.0","192.167.0.1"},{"192.167.0.2","192.167.0.0"},{"192.167.0.0","192.167.0.3"}};
        String []toggleIps= {"192.167.0.1","192.167.0.0","192.167.0.2","192.167.0.0", "0.0.0.0"};
        int[] result = numberOfDevices(connections, toggleIps);

        for(int i:result) {
            System.out.print(i + " ");
        }
    }

    public static int[] numberOfDevices(String[][] connections, String [] toggleIps) {
        int count =0;
        int [] result = new int[toggleIps.length];
        if(connections.length>0) { // already a given contaraint that the length of toggle IPs is >=1

            Map<String,Ipv4Address> ip = new HashMap<>();
            for(int i=0; i<connections.length; i++) {
                Ipv4Address ip1, ip2;

                if(!ip.containsKey(connections[i][0])) {
                    ip.put(connections[i][0], new Ipv4Address(connections[i][0]));
                }
                if(!ip.containsKey(connections[i][1])) {
                    ip.put(connections[i][1], new Ipv4Address(connections[i][1]));
                }

                ip1=ip.get(connections[i][0]);
                ip2=ip.get(connections[i][1]);

                ip1.addConnection(ip2);
                ip2.addConnection(ip1);

            }

            for(int j=0; j<toggleIps.length; j++) {
                if(!ip.containsKey(toggleIps[j])) {
                    result[j]=0;
                }
                else {
                    Ipv4Address ipv4 = ip.get(toggleIps[j]);
                    ipv4.setState(!(ipv4.getState()));
                    for(Ipv4Address conn:ipv4.getConnections()) {
                        if(conn.getState()) {
                            result[j]++;
                        }
                    }
                }
            }
        }

        return result;
    }

}

// class representing the IPv4 address , it has IP address, current state and the connections where its present
class Ipv4Address{
    String ipAddress;
    boolean state;
    List<Ipv4Address> connections;

    public Ipv4Address(String ipAddress) {
        this.ipAddress = ipAddress;
        this.state=false;
        connections= new ArrayList<>();
    }

    public boolean getState() {
        return this.state;
    }
    public void setState(boolean state) {
        this.state=state;
    }

    public void addConnection(Ipv4Address ipaddress) {
        this.connections.add(ipaddress);
    }
    public List<Ipv4Address> getConnections(){
        return this.connections;
    }

}

Welcome to Stack Review, it seems me there is no much space for improvement, just some little tips:
class Ipv4Address {
    String ipAddress;
    boolean state;
    List<Ipv4Address> connections;

    //setters and getters
}

Probably you included your Ipv4Address class inside the NumberOfDevices class file but for a cleaner code better create a Ipv4Address class file separated. If you have defined setters and getters, you can encapsulate fields into the class using the private access modifier like below:
public class Ipv4Address {
    private String ipAddress;
    private boolean state;
    private List<Ipv4Address> connections;

    public Ipv4Address(String ipAddress) {
        this.ipAddress = ipAddress;
        this.state = false;
        this.connections = new ArrayList<>();
    }

    //getters and setters
}

No much to say about NumberOfDevices; in its numberOfDevices method you have the following code :
public static int[] numberOfDevices(String[][] connections, String [] toggleIps) {
    int count =0;
    int [] result = new int[toggleIps.length];
    if(connections.length>0) { // already a given contaraint that the length of toggle IPs is >=1

       Map<String,Ipv4Address> ip = new HashMap<>();
       for(int i=0; i<connections.length; i++) {
          Ipv4Address ip1, ip2;

          if(!ip.containsKey(connections[i][0])) {
            ip.put(connections[i][0], new Ipv4Address(connections[i][0]));
          }
          if(!ip.containsKey(connections[i][1])) {
            ip.put(connections[i][1], new Ipv4Address(connections[i][1]));
          }

          ip1=ip.get(connections[i][0]);
          ip2=ip.get(connections[i][1]);

          ip1.addConnection(ip2);
          ip2.addConnection(ip1);

     }

      //other lines
}

You can rewrite this part of your method using a foreach construct like below:
public static int[] numberOfDevices(String[][] connections, String[] toggleIps) {
    int[] result = new int[toggleIps.length];

    if (connections.length == 0) { return result; }

    Map<String, Ipv4Address> map = new HashMap<>();

    for (String[] connection : connections) {

        for (String c : connection) {

            if (!map.containsKey(c)) {
                map.put(c, new Ipv4Address(c));
            }
        }

        Ipv4Address ip1 = map.get(connection[0]);
        Ipv4Address ip2 = map.get(connection[1]);
        ip1.addConnection(ip2);
        ip2.addConnection(ip1);
   }

   //other lines
}


Python program that gets basic information on a Github user I've created a basic Python CLI script, which takes a Github username, and returns some basic information associated with said username. I use the requests for HTTP requests (I'm new to this area) and docopt for command-line handling.

#!/usr/bin/env python3

"""Github User Info

Get basic information on a Github user

Usage:
  github_user_info.py USERNAME
  github_user_info.py (-h | --help)
  github_user_info.py (-v | --version)

Arguments:
  USERNAME     Github username

Options:
  -h --help     Show this screen.
  -v --version  Show version.
"""

from docopt import docopt
import requests

import sys

def main():
    if len(sys.argv) == 1:
        sys.argv.append('-h')  # Show help screen if no arguments are passed

    cmd_arguments = docopt(__doc__, version='Github User Info v1.0')

    username = cmd_arguments["USERNAME"]

    if len(username.split()) != 1:
        print('Github usernames must be one word in length! Exiting!')
        return

    response = requests.get(f'https://api.github.com/users/{username}')

    if response.status_code != requests.codes.ok:
        if response.status_code == 404:  # No such user was found
            print(f'No Github user was found with username {username}!')
        else:
            print(f'An unexpected error occured! Exiting with error code: '
                  f'{response.status_code}')
        return

    responses_json = response.json()

    print(f'The following information was found on user: {username}',
          end='\n\n')
    print(f'Name: {responses_json["name"]}')
    print(f'Bio: {responses_json["bio"]}')
    print(f'Followers: {responses_json["followers"]}')
    print(f'Following: {responses_json["following"]}')

if __name__ == '__main__':
    main()

I'd mainly like to know if this is the correct way to handle HTTP requests (in terms of getting them, error handling, etc.). I'd also like to make sure that I follow good Python conventions.

Just a note:
instead of using string.split() to check for spaces, you can directly check with in.

From:

if len(username.split()) != 1:
        print('Github usernames must be one word in length! Exiting!')
        return

To:

if ' ' in username:
        print('Github usernames cannot contain spaces! Exiting!')
        return

Also, instead of multiple print statements, you can use multiline strings: """...""" or '''...'''.

From:

print(f'The following information was found on user: {username}',
      end='\n\n')
print(f'Name: {responses_json["name"]}')
print(f'Bio: {responses_json["bio"]}')
print(f'Followers: {responses_json["followers"]}')
print(f'Following: {responses_json["following"]}')

To:

print(f'''
The following information was found on user: {username}

Name: {responses_json["name"]}
Bio: {responses_json["bio"]}
Followers: {responses_json["followers"]}
Following: {responses_json["following"]}''')


Project Euler problem # 3 is this the most efficient approach in Python or can I do better? The problem statement: The prime factors of 13195 are 5, 7, 13 and 29.

What is the largest prime factor of the number 600851475143 ?

This code takes 9 seconds!, is there a better way?

from time import time

def is_prime(number):
    for divisor in range(2, int(number ** 0.5) + 1):
        if number % divisor == 0:
            return False
    return True

def calculate_largest(number):
    # checking backwards to get the largest number first
    for divisor in range(int(number ** 0.5) + 1, 0, -1):
        if is_prime(divisor) and number % divisor == 0:
            return divisor

if __name__ == '__main__':
    time1 = time()
    print(calculate_largest(600851475143))
    print(time() - time1)

order
You wrote:
    if is_prime(divisor) and number % divisor == 0:

that is, if A and B:.
But B is typically false and is quickly computed.
Prefer if B and A:
stride
Rather than
    for divisor in range(int(number ** 0.5) + 1, 0, -1):

you might want to begin with an odd number and then use a stride of -2.
You needn't special case for when number is a power of two,
since the input clearly isn't.
sieve
Consider sieving for the solution.


Buffered read from a large list There is a large list of email addresses and there is a limitation of max 100 email addresses to process once. This is the code I have written.

        //Create a list of email addresses to test
        List<string> emails = new List<string>();
        for (int i = 0; i < 10000; i++)
        {
            emails.Add(string.Format("email{0}@domain.com", i));
        }

        //buffered read
        int remainingCount = emails.Count;
        int maxToReadOnce = 100;
        int startIndex = 0;
        while(remainingCount > 0)
        {
            int countToRead = (remainingCount > maxToReadOnce) ? maxToReadOnce : remainingCount;
            List<string> myemails = emails.GetRange(startIndex, countToRead);
            startIndex = startIndex + countToRead;
            remainingCount = remainingCount - countToRead;
            Process(myemails); // Method that process the email addresses
        }

Your code is pretty well written and I can't see anything too much I'd change...

(remainingCount > maxToReadOnce) ? maxToReadOnce : remainingCount

might be clearer as

Math.Min(maxToReadOnce, remainingCount)

but other than that it all looks really sensible.

That said, by mixing the batching into the processing code both are harder to read. Batching (splitting/sharding/whatever you want to call it) comes up a lot - I have an extension method lying around for it:

public static IEnumerable<IEnumerable<T>> Batch<T>(this IEnumerable<T> enumerable, int batchSize)
{
    var c = 0;
    var batch = new List<T>();
    foreach (var item in enumerable)
    {
        batch.Add(item);
        if (++c % batchSize == 0)
        {
            yield return batch;
            batch = new List<T>();
        }
    }
    if (batch.Count != 0)
    {
        yield return batch;
    }
}

There are a whole bunch of variations on the above - feel free to make any changes... The ++c % batchSize == 0 has always bothered me a bit but I like the terseness of it.

With that extension method in place you can simplify your code to:

foreach (var batchOfEmails in emails.Batch(maxToReadOnce))
{
    Process(batchOfEmails);
}

(if you don't like var the type will be IEnumerable<IEnumerable<string>>)


File(s) encrypter implementation in python I'm looking for any feedback in order to improve code styling, performance etc.
Also I want to make sure that the way I've implemented AES here is actually secure

import os
import hashlib
import sys
import base64

from cryptography.fernet import Fernet

class Cryptor:
    def __init__(self, password):
        self.__password = password

    def operate(self, file_):
        if file_.endswith("enc"):
            self.decrypt(file_)
        else:
            self.encrypt(file_)

    def decrypt(self, file_):
        file_name = "".join(file_.split(".")[:-1])

        with open(file_, "rb") as file_:
            save1 = file_.readline()
            save2 = file_.readline()
            save3 = file_.readline()
            salt = save3
            key = base64.urlsafe_b64encode(hashlib.pbkdf2_hmac("sha256", bytes(self.__password, encoding="utf-8"), salt, 100000, dklen=32))
            f = Fernet(key)
            decrypted = f.decrypt(save1)
            file_extention = f.decrypt(save2)
            save = decrypted

        with open(file_name + file_extention.decode(encoding="utf-8"), "wb") as file_:
            file_.write(save)

        os.remove(file_name + ".enc")

    def encrypt(self, file_):
        salt = os.urandom(64)
        key = base64.urlsafe_b64encode(hashlib.pbkdf2_hmac("sha256", bytes(self.__password, encoding="utf-8"), salt, 100000, dklen=32))
        f = Fernet(key)

        file_name = "".join(file_.split(".")[:-1])
        file_extention = "." + file_.split(".")[-1]

        with open(file_, "rb") as file_:
            file__ = file_.read()
            encrypted = f.encrypt(bytes(file__))
            save1 = encrypted
            save2 = f.encrypt(bytes(file_extention, encoding="utf-8"))
            save3 = salt

        with open(file_name + ".enc", "wb") as file_:
            file_.write(save1)
            file_.write(b"\n")
            file_.write(save2)
            file_.write(b"\n")
            file_.write(save3)

        os.remove(file_name + file_extention)

def main():
    password = sys.argv[1]
    files = sys.argv[2:]
    crypt = Cryptor(password)

    for file_ in files:
        crypt.operate(file_)

if __name__ == "__main__":
    main()

Ways of improving:

Splitting filename

Both methods encrypt and decrypt try to split a filename into "root" part and extension. encrypt method makes that even worse and verbose with 2 statements:

    file_name = "".join(file_.split(".")[:-1])
    file_extention = "." + file_.split(".")[-1]

Instead, that's easily achievable with os.path.splittext(path) function (split the pathname path into a pair (root, ext)):

file_name, file_ext = os.path.splitext(file_)

Generating Fernet instance

Both methods encrypt and decrypt have the same 2 statements for generating Fernet instance (with only difference in salt value):

key = base64.urlsafe_b64encode(hashlib.pbkdf2_hmac("sha256", bytes(self.__password, encoding="utf-8"), salt, 100000, dklen=32))
f = Fernet(key)

It's good to extract that common behavior into a separate internal function:

def _get_fernet_instance(self, salt):
    key = base64.urlsafe_b64encode(hashlib.pbkdf2_hmac("sha256", bytes(self.__password, encoding="utf-8"),
                                                       salt, 100000, dklen=32))
    return Fernet(key)

decrypt function.
The variables save1, save2, save3 and save reassignment are confusing things rather than clarify.The variables save1, save2, save3 can be renamed to line1, line2 and salt.
Aliasing save = decrypted gives no benefit - just refer decrypted directly.Including the above mentioned optimizations the decrypt method would look as:

def decrypt(self, file_):
    file_name, _ = os.path.splitext(file_)

    with open(file_, "rb") as f:
        line1 = f.readline()
        line2 = f.readline()
        salt = f.readline()

        fernet_ = self._get_fernet_instance(salt)
        decrypted = fernet_.decrypt(line1)
        file_ext = fernet_.decrypt(line2)

    with open(file_name + file_ext.decode(encoding="utf-8"), "wb") as f:
        f.write(decrypted)

    os.remove(file_)

encrypt function.
The variable file__ in file__ = file_.read() does not give an explicit meaning for the content being read. Let the content be content or data.The same issues around variables save1, save2, save3 and redundant aliases.

In case if there would be a need to refer the input argument file_ (in both methods) as its original value in different places in methods body - you should not reassign it in context managers with open(file_, "rb") as file_:.

os.remove(file_name + file_extention) is essentially the same as os.remove(file_)

The final optimized encrypt method would look as:

def encrypt(self, file_):
    salt = os.urandom(64)
    fernet_ = self._get_fernet_instance(salt)
    file_name, file_ext = os.path.splitext(file_)

    with open(file_, "rb") as f:
        content = f.read()
        encrypted_content = fernet_.encrypt(bytes(content))
        encrypted_ext = fernet_.encrypt(bytes(file_ext, encoding="utf-8"))

    with open(f'{file_name}.enc', 'wb') as f:
        f.write(encrypted_content)
        f.write(b"\n")
        f.write(encrypted_ext)
        f.write(b"\n")
        f.write(salt)

    os.remove(file_)


How can I optimize my login script? I've created an Ajax login for my website but I feel like I can optimize it, but I'm not sure how and where.

Questions:

How can I optimize my code?
Is the code secure? Any ways to break it (injection, etc)?

Also, when I attempt to log in, it currently takes about 1 second to process the login (on localhost). Is this long?

Here's my Ajax call:

$(document).ready(function() {
  $(document).on("submit", "form", function(event) {
    event.preventDefault();
    $.ajax({
      url: 'assets/php/login_script.php',
      type: 'POST',
      data: $(this).serialize(),
      success: function(data) {
        if (data == true) {
          window.location.href = "index.php";
        } else {
          $("input[name=password_field]").focus();
          $(".error").html(data);
        }
      }
    });
  });
});

Here's the PHP script:

<?php

  include_once("access.php");

  $cxn = mysqli_connect($host, $user, $pass, $db) or die ("Couldn't connect to the server. Please try again.");

  $username = $_POST["username"];
  $password = $_POST["password"];
  $date = date('Y-m-d h:i:s', time());
  $ip_address = get_ip_address();
  $expire = time() + 86400 * 365;

  $options = array('cost' => 12);
  $hash_password = password_hash($password, PASSWORD_BCRYPT, $options);

  /* Log the login request. */
  $stmt = $cxn->prepare("INSERT INTO login_logs (log_id, username, password, datetime, ip_address) VALUES ('', ?, ?, ?, ?)");
  $stmt->bind_param('ssss', $username, $hash_password, $date, $ip_address);
  $stmt->execute();

  /* Get user information from database. */
  $stmt = $cxn->prepare('SELECT * FROM users WHERE username = ?');
  $stmt->bind_param('s', $username);
  $stmt->execute();
  $result = $stmt->get_result();

  /* If a result exists, continue. */
  if ($result->num_rows > 0) {
    while ($row = $result->fetch_assoc()) {
      $db_username = $row['username'];
      $db_password = $row['password'];
      $random_hash = password_hash(time() . $db_username . time(), PASSWORD_BCRYPT, $options);

      /* Password matches. */
      if (password_verify($password, $db_password)) {

        /* Get user's cookie information in database. */
        $stmt2 = $cxn->prepare("SELECT * FROM cookies WHERE username = ?");
        $stmt2->bind_param('s', $db_username);
        $stmt2->execute();
        $result2 = $stmt2->get_result();

        /* If a result exists, update the cookie. */
        if ($result2->num_rows > 0) {
          $stmt = $cxn->prepare("UPDATE cookies SET hash = ? WHERE username = ?");
          $stmt->bind_param('ss', $random_hash, $db_username);
          $stmt->execute();

          setcookie("user", $db_username, $expire, "/");
          setcookie("hash", $random_hash, $expire, "/");
        } else {
          $stmt = $cxn->prepare("INSERT INTO cookies (cookie_id, username, hash) VALUES ('', ?, ?)");
          $stmt->bind_param('ss', $db_username, $random_hash);
          $stmt->execute();

          setcookie("user", $db_username, $expire, "/");
          setcookie("hash", $random_hash, $expire, "/");
        }

        echo true;
      } else {
        echo "Incorrect credentials.";
      }
    }
  } else {
    echo "Incorrect credentials.";
  }

  function get_ip_address() {
    $ip_address = '';
    if (getenv('HTTP_CLIENT_IP'))
      $ip_address = getenv('HTTP_CLIENT_IP');
    else if(getenv('HTTP_X_FORWARDED_FOR'))
      $ip_address = getenv('HTTP_X_FORWARDED_FOR');
    else if(getenv('HTTP_X_FORWARDED'))
      $ip_address = getenv('HTTP_X_FORWARDED');
    else if(getenv('HTTP_FORWARDED_FOR'))
      $ip_address = getenv('HTTP_FORWARDED_FOR');
    else if(getenv('HTTP_FORWARDED'))
      $ip_address = getenv('HTTP_FORWARDED');
    else if(getenv('REMOTE_ADDR'))
      $ip_address = getenv('REMOTE_ADDR');
    else
      $ip_address = 'UNKNOWN';

    return $ip_address;
  }

?>

How can I optimize my script to look better, be faster, etc?

I've seen quite a few programmers who don't enjoy seeing arrays coming in as arguments into the code (me personally, I'm fine with it as long as there is some sort of documentation that goes with it so you don't forget what the function can and cannot take).

password_hash($password, PASSWORD_BCRYPT, $options);

would instead be:

password_hash($password, PASSWORD_BCRYPT, $cost);

The above is purely an FYI and not my opinion.

When it comes to accepting username/passwords, you want to be sure you're on a secure location. Based on your current code, if you're only a http:// page (typically port 80) then a malicious bot can potentially sniff the username/password. Easiest way to do this, is run a check against $_SERVER['HTTPS'] but if that fails, you do want to redirect the user to another page or something like that.

You shouldn't have more than one user with the same username, so create a LIMIT 1 at the end of the SQL query, otherwise it'll scan the entire table no matter how many results you get.

$stmt = $cxn->prepare('SELECT * FROM users WHERE username = ? LIMIT 1 ');

Same concept for your cookies table lookup:

$stmt2 = $cxn->prepare("SELECT * FROM cookies WHERE username = ? LIMIT 1");

Is there a purpose of creating additional variables that are essentially the same values of the initial variables?

e.g.

$username = $_POST['username'];

What that ends up doing is you will use up additional memory blocks when it's not really needed to happen. Also, you could lose track of what $username is down the road, and re-initialize it with a different value. Same idea when you're looking at your $row data.

PASSWORD_BCRYPT is not meant to be fast, but it is meant to be secure. You're running it twice, with different values, is there a reason you're doing that for your logs considering you can't reverse the encryption? I'd just run it once and put it against $hash_password

If you're using time() multiple times across the page, that's when you may want to put that against a variable. Even though it's a very quick function, it'll be faster to look it up against a variable that has a fixed number and is stored in a particular memory block, creates a consistent value across the board in case you have to do additional comparisons against each other (compare something about the expiration date or check the value that got inserted into the log table and since that's a salt, might as well have that as a reference etc.).

If the username/password verification feels like it takes a long time, then it is taking a long time, that's the way I see it. If you don't mind how long it takes, you might be ok (as saying 1 second is relative to the actual speed of the script). However, for bottlenecks, you'd have to test pieces of your code to see where is it slowest.

Hope this helps.


Improving or replacing a long if statement I have a an if statement that works just fine, however, it has become a bit long and I need help to figure out how I can reduce the lines of code and thereby the readability.
I have a defaultMonth and set of months and a corresponding price. I need to add or reduce the price depending on the defaultMonth and month selected by the user.
Here are some examples of the output I need (note the interval is the same):
If default month is 60 - set price accordingly:
Month    -     Price
60             0.0
48             0.1
36             0.2
24             0.35
12             0.5

If default month is 48 - set price accordingly:
Month    -     Price
60             -0.1
48             0.0
36             0.1
24             0.25
12             0.4

If default month is 36 - set price accordingly:
Month    -     Price
60             -0.2
48             -0.1
36             0.0
24             0.15
12             0.3

The way I've built it now is with an if statement for each default month and nested ifs for each month.
if (defaultMonth === 60) {
    if (month === 60) {
        this.price += 0.0
    }
    if (month === 48) {
        this.price += 0.1
    }
    if (month === 36) {
        this.price += 0.2
    }
    if (month === 24) {
        this.price += 0.35
    }
    if (month === 12) {
        this.price += 0.5
    }
}
if (defaultMonth === 48) {
    if (month === 60) {
        this.price -= 0.1
    }
    if (month === 48) {
        this.price += 0.0
    }
    if (month === 36) {
        this.price += 0.1
    }
    if (month === 24) {
        this.price += 0.25
    }
    if (month === 12) {
        this.price += 0.4
    }
}
if (defaultMonth === 36) {
    if (month === 60) {
        this.price -= 0.2
    }
    if (month === 48) {
        this.price -= 0.1
    }
    if (month === 36) {
        this.price += 0.0
    }
    if (month === 24) {
        this.price += 0.15
    }
    if (month === 12) {
        this.price += 0.3
    }
}
if (defaultMonth === 24) {
    if (month === 60) {
        this.price -= 0.35
    }
    if (month === 48) {
        this.price -= 0.25
    }
    if (month === 36) {
        this.price -= 0.15
    }
    if (month === 24) {
        this.price += 0.0
    }
    if (month === 12) {
        this.price += 0.15
    }
}
if (defaultMonth === 12) {
    if (month === 60) {
        this.price -= 0.5
    }
    if (month === 48) {
        this.price -= 0.4
    }
    if (month === 36) {
        this.price -= 0.3
    }
    if (month === 24) {
        this.price -= 0.15
    }
    if (month === 12) {
        this.price += 0.0
    }
}


InternedString as alternative to regular String I need to use large strings as a dictionary keys, and I want to optimize the repetitive GetHashCode() and Equals() calls in it.

The number of keys will be quite small (<1000), but each string length will be very big (it's generated SQL queries cache), and I expect a lot of hot lookups.

My idea is to make sure that each wrapped string is interned, so that we can rely on the string references of the same strings to be always the same:

public sealed class InternedString : IEquatable<InternedString>
{
    public InternedString(string s) => String = string.Intern(s);

    public string String { get; }

    public override bool Equals(object obj) => String.Equals(obj);

    public bool Equals(InternedString other) => String.Equals(other?.String);

    public override int GetHashCode() => RuntimeHelpers.GetHashCode(String);

    public static bool operator ==(InternedString l, InternedString r) =>
        l?.String == r?.String;

    public static bool operator !=(InternedString l, InternedString r) => !(l == r);
}

Can you review this class, with special attention to safety?

UPDATE

You can check the final code and benchmarks here: https://github.com/astef/InternedString

Usage:

// O(n) operation happens only here, so we want to re-use this object
var iString = new InternedString("typically_a_very_long_string_key");

// now any call to `GetHashCode()` and `Equals(...)` will run in a constant time
mySet.Add(iString);

Assuming each time you receive a new query you create an instance of InternedString to get/add another item from/into the dictionary, the performance benefit seems questionable:

Impact of string.Intern

a) It needs to calculate a hash of the original string to make a lookup in the pool of strings

b) From official documentation -

  The memory allocated for interned String objects is not likely to be
  released until the common language runtime (CLR) terminates. The
  reason is that the CLR's reference to the interned String object can
  persist after your application, or even your application domain,
  terminates.

High rate of collisions seems to be the only beneficial scenario

The equality comparison that Dictionary does for items in the bucket is the case when you implementation might win when the bucket grows huge.

But do you really have that many collisions?

P.S.

Comparing strings you're not using String.ReferenceEquals to compare references of the interned strings, but rather == operator which for (sting vs string) leads to values comparison.


Processing files in a directory tree I'm new to Scala, and would appreciate any notes about a better approach, correctness, or style.

/**
 * Call proc(f) for each file in the directory tree rooted at dir.
 *
 * proc will not be called for directories, just for files.
 */
def traverse(dir: File, proc: File => Unit): Unit = {
    dir.listFiles foreach { (f) => {
            if(f.isDirectory) {
                traverse(f, proc)
            } else {
                proc(f)
            }
        }
    }
}

Your code is fine. Nevertheless, removing unnecessary braces can increase readability:

def traverse(dir: File, proc: File => Unit): Unit =
  dir.listFiles foreach { f => if (f.isDirectory) traverse(f, proc) else proc(f) }


Framework for composing algorithms I am implementing a game in python, which will give the user a sample encryption, and explain each step from plaintext to cipher. The task for the player is to decrypt a word by undoing each step shown above on the new word.

For this, I have created an abstract algorithm which requires an encrypt function, and a current development status. In the future I think it will require other properties such as explanation text or a difficulty level.

The abstract algorithm looks like this:

from abc import ABC, abstractmethod
from enum import Enum

class Status(Enum):
    """The current development status"""
    in_development = 0
    under_testing = 1
    production_ready = 2
    known_bugs = 3

class BaseAlgorithm(ABC):
    """This is the base on which all other algs are to be built"""
    @abstractmethod
    def encrypt(self, word):
        """Encrypt the word using an internal alg"""
        pass

    @abstractmethod
    def decrypt(self, word):
        """Decrypt the word using an internal alg
        If this is not overloaded, it is assumed that"""
        return self.encrypt(word)

    @property
    @abstractmethod
    def status(self):
        """The current progress and readiness of the alg"""
        pass

An example algorithm is implemented in rotate.py

from . import BaseAlgorithm, Status

class RotateAlg(BaseAlgorithm):
    """Takes a rotation amount  which it uses to encrypt or decrypt
    all words pass into it"""
    def __init__(self, offset):
        try:
            self.offset = int(offset)
        except ValueError:
            raise ValueError("the offset must be an integer") from None

        # Decides which direction is the default way to rotate
        self.rotate_left = True

    def encrypt(self, word):
        """Encrypts by rotating the word"""
        return self.rotate(word)

    def decrypt(self, word):
        """Decrypts by rotating the word back"""
        return self.rotate(word, inverse=True)

    def rotate(self, word, inverse=False):
        """Rotates the word to the left"""
        offset = self.offset
        # Inverse and rotate_left both flip the direction
        # If both are the same the direction remains unchanged
        if inverse == self.rotate_left:
            offset *= -1
        return rotate(word, offset)

    @property
    def status(self):
        return Status.in_development

def rotate(word, offset):
    """Rotates the word right by the amount specified"""
    offset %= len(word)
    return word[offset:] + word[:offset]

Finally how these algorithms would be composed together is a simple accumulating loop

def decode(cipher, encryptions):
    for encryption in reversed(encryptions):
        cipher = encryption.decrypt(cipher)
    return cipher

I feel like this is the wrong approach to take though, as there is a lot of boilerplate for what is essentially a function which is used in two ways. Is there a better design pattern to use in this scenario? I suspect a more functional approach would suit, such as passing the encryption functions into a class that binds them together. My concern is I lose the option of providing metadata with the function with that style.

For such a simple use case, I would drop the abc module in favor of NotImplementedError:

class BaseAlgorithm:
    """This is the base on which all other algorithms are to be built."""

    # Unless explicitly stated otherwise, algorithms are in development
    STATUS = Status.in_development

    def encrypt(self, word):
        """Encrypt the word using an internal algorithm."""
        raise NotImplementedError

    def decrypt(self, word):
        """Decrypt the word using an internal algorithm.

        If this is not overloaded, it is assumed that
        encrypting and decrypting are the same operation.
        """
        return self.encrypt(word)

Using such base, only encrypt is explicitly required to be overloaded. I also extracted the status from a method to a class constant as this is the kind of information that does not depend of any state about this class. It also allows to focus on the algorithm first and only change the status when it is ready. Lastly, I also expanded the docstrings a bit so they are more explicit; you may want to explain, in the class docstring, what a user is expected to do when inheriting from BaseAlgorithm.

As for RotateAlg:

you could name it Rotate as the Alg part is already implied from the base BaseAlgorithm class;
you could incorporate the rotate function directly in the rotate method as there shouldn't be much use for a function on their own;
you could handle the rotate_left logic better by using this information directly as a parameter to rotate.

This would become something along the lines of:

class Rotate(BaseAlgorithm):
    """Takes a rotation amount  which it uses to encrypt or decrypt
    all words pass into it"""
    def __init__(self, offset):
        try:
            self.offset = int(offset)
        except ValueError:
            raise ValueError("the offset must be an integer") from None

    def encrypt(self, word):
        """Encrypts by rotating the word"""
        return self.rotate(word, rotate_left=False)

    def decrypt(self, word):
        """Decrypts by rotating the word back"""
        return self.rotate(word, rotate_left=True)

    def rotate(self, word, rotate_left=False):
        """Rotates the word to the chosen direction"""
        offset = -self.offset if rotate_left else self.offset
        offset %= len(word)
        return word[offset:] + word[:offset]

And, unless you specify otherwise with:

class Rotate(BaseAlgorithm):
    STATUS = Status.under_testing
    ...

then Rotate.STATUS will still be Status.in_development.


Program for converting a boring text into a stylish written text I have written a program that makes it possible to write as cool as the cool kids. All you have to do is enter the boring, normal text, which is then transformed into an exciting, stylish text.
Is there a possibilty to make the code more performant without decreasing it's readability?

Example Output:

I H@Ve wRITTen @ Pr0qR@M tH@T m@KEZ IT P0ZzIblE t0 wriTE @z C00L @z the c00L KIdZ :) All Y0U h@Ve T0 D0 iZ eNter thE B0rinQ, n0rm@L teXt, whIch iZ tHeN TR@nZF0RmED int0 @n exCiTInq, ztyLIZh TExT. :D

Source code

import java.util.Random;
import java.util.Scanner;

public class Program {
    public static void main(String[] args) {
        Scanner input = new Scanner(System.in);
        System.out.print("Input: ");
        String text = input.nextLine();
        String trendyText = convertToTrendyText(text);
        System.out.println(trendyText);
    }

    public static String convertToTrendyText(String string) {
        string = string.replace("g", "q");
        string = string.replace("s", "z");
        string = string.replace("a", "@");
        string = string.replace("o", "0");
        string = string.replace(". ", " " + generateRandomSmiley() + " ");
        string = string.replace("! ", " " + generateRandomSmiley() + " ");
        string = string.replace("? ", " " + generateRandomSmiley() + " ");
        string = string.concat(" " + generateRandomSmiley());

        StringBuilder text = new StringBuilder(string);
        Random random = new Random();
        for (int i = 0; i < text.length(); i++) {
            if (random.nextBoolean()) {
                text.setCharAt(i, Character.toUpperCase(text.charAt(i)));
            }
        }

        return text.toString();
    }

    public static String generateRandomSmiley() {
        Random random = new Random();

        switch (random.nextInt(10)) {
            case 0: return ":)";
            case 1: return ":D";
            case 2: return ":*";
            case 3: return "<3";
            case 4: return "o.O";
            case 5: return "x3";
            case 7: return "xD";
            case 8: return ":o";
            default: return ";D";
        }
    }
}

Finally. A program to help me speak the same language as the cool kids. Thanks for that.

Use an array when it's good enough

The second part of convertToTrendyText needlessly uses a StringBuilder instance.
The power of StringBuilder is to efficiently build strings whose size is not known in advance.
In this method we do know in advance,
so there's no need for a StringBuilder,
an array from string.toCharArray() would be more than enough.
After replacing characters in the char[],
you could return it in a new String(...).

Destructive uppercasing

The first part of convertToTrendyText inserts some random smileys.
The second part of the method randomly uppercases some letters.
That risks ruining the following smileys: o.O, x3, xD.
I'm wondering if that's intended or not.
With this potential unintended side effect,
the text might become completely unreadable.

To avoid such destruction,
you could swap the first and second parts:
do the uppercasing first,
and insert smileys after.
That way the smileys will be unaffected by design.

One Random is enough

There's no need to create multiple instances of Random in a program.
It would be better to use just one.
That could be a good step in the direction of making the program testable,
because you will be able to set a seed to get reproducible output.

Getting a random value out of n values

The switch statement in generateRandomSmiley is a bit troublesome.
If you add a new smiley, you have to remember to increment the number in the random.nextInt(...) call, and add a correctly numbered case statement. Such a hassle.

If you use an array of smileys,
then the process of adding or removing values becomes a lot simpler,
more compact,
without having to worry about indexes.

private static final String[] SMILEYS = {
    ":)", ":D", ":*", "<3", "o.O", "x3", ";D", "xD", ":o", ";D"
};

public static String generateRandomSmiley() {
    return SMILEYS[random.nextInt(SMILEYS.length)];
}

Btw, did you notice that there is no case 6 line in your original switch statement?
That leads to getting the same value for it as the default case.
Not sure if that was intentional.
To preserve the behavior of the posted code,
I duplicated the default value ;D at index 6 (in addition to its natural index 9).

A word on performance

Every call like string = string.replace("...", "..."); has to iterate over all the content of the string.
That seems a bit wasteful.
In a toy program like this,
it doesn't really matter,
but it's worth keeping in mind.


Get Bitcoin Price and Advice I wrote this in the last hour because I was bored.  I was hoping I could get some feedback on this.  It is a function that returns the price of a bitcoin and whether you should buy or sell it. I am thinking of setting it up as a function within a chat bot to spice up the chat a little.

import urllib2
import random
import json

def bitcoin(currency, amt = 1):
    url = "https://api.coindesk.com/v1/bpi/currentprice.json"
    request = urllib2.Request(url)
    response = urllib2.urlopen(request)
    the_page = response.read()
    data = json.loads(the_page)
    if currency in ["USD","US","dollar","$"]:
        conversion = data['bpi']['USD']['rate_float']
        symbol = "USD"
    elif currency in ["EUR","EU","euro","€"]:
        conversion = data['bpi']['EUR']['rate_float']
        symbol = "EUR"
    elif currency in ["GBP","GB","UK","pound","£"]:
        conversion = data['bpi']['GBP']['rate_float']
        symbol = "GBP"
    else:
        stop("Currency not recognized.")
    advice = random.choice(["It's so LOW!!!!!!! \r\nBuy!!! Buy!!! Buy!!!!!","It's so HIGH!!!!!! \r\nSell!!! Sell!!! Sell!!!!!"])
    return "{0} Bitcoin = {1} {2} \r\n{3}".format(amt,conversion * amt, symbol, advice)

This is Schrödinger code
It can be both correct and incorrect at the same time.
random.choice is not how we offer financial advice.
What I would suggest is to monitor previous data, the longer the better, and monitor the fluctuation in price.
See this graph (Note, a 5 year graph):

at two points on the chart you can see separate events that can be somewhat predicted by the speed at which it falls/rises in comparison to previous events.
Although this could potentially expand to be a large solution, it's about as close to accurate as you can get. (Not really, but I'm not a market analyst)
These guys are, though.
Note that the graph is 5 years; data doesn't fluctuate that much in small amounts.
Although Bitcoin can, given it's financially unstable and volatile nature.
Beyond that, your python code is nice :-)
I would suggest writing it to PEP8 however, as that is the official style guide of Python, and then you can use an online checker like pep8online.


Stack class in Java 8 I have a exercise where I have to write a Stack class with the push and pop methods. The code compiles and works as it should. Are there better practices to use in my code? Is there something I should avoid doing?

public class Stack {

    private final int INCREMENTSIZE = 1024;
    Integer position = 0;

    int[] list = null;

    public Stack(){
        list = new int[INCREMENTSIZE];
    }

    public void push(Integer i) {
        if(position == list.length) {
            list = Arrays.copyOf(list, list.length + INCREMENTSIZE);
        }
        list[position++] = i;
    }

    public Integer pop(){
        return list[--position];
    }
}

Other answers correctly point out that using primitive types and not mixing them with Objects (=int instead of Integer), reducing visibility wherever possible (=adding private modifier to position and list) and preventing popping from empty stack are good practices. There are a few more subtleties which can be improved:

Rename INCREMENTSIZE to INCREMENT_SIZE. It's customary, when naming constants using full caps, to separate words with underscores.
Consider growing a stack by multiplying current size and start small, e.g. instead having new size be current+increment, make it current*factor, where factor can be 1.5 or 2, or even decreased as the stack grows. If you're implementing a general purpose stack, you don't know how small or large the user will want it to be—incrementing by a constant might be an overkill or too small, while starting small and growing it in multiples will conserve memory if user needs a small stack, and will grow it in large enough increments later on if user needs to store many elements. The two approaches can be mixed and fine-tuned for best performance.
Consider generifying the stack class so it can be a stack of anything, not just Integers. It's a small cost, but can be of large benefit.
If you do store arbitrarily typed objects, beware of memory leaks! The pop function as it stands won't free memory if a user decides to empty the stack. It's a merely unconservative approach when dealing with primitives or small, usually pooled Integers. It can be a real problem when storing something heavyweight—your stack will keep a reference to something which the user has popped and prevent the garbage collector from collecting it (also see Effective Java 3rd ed., Item 7). To prevent this, when popping an element, set the value of the popped element in array to null; additionally when a considerable proportion of the array is empty, deallocate a portion of it (e.g. using the Arrays.copyOf with a smaller second argument).
Guard against overflows. At some point, list.length + INCREMENTSIZE will overflow, and you'll get NegativeArraySizeException from Arrays#copyOf. Unfortunately, you can't have arrays which are indexed by longs, so best you can do is use Integer.MAX_VALUE as the new size, and throw an exception (e.g. IllegalStateException with a helpful message) if the caller wants to add more items to stack after it's grown to MAX_VALUE. You can see how ArrayDeque#grow(int) and ArrayDeque#newCapacity(int, int) are implemented by OpenJDK here.


Search script code efficiency Is there a more efficient way to write this? This script will be used heavily and I want to make sure I do not have any memory leaks or speed issues.

This script gets an input from a form and searches the database for that record.

<?php

date_default_timezone_set('America/Los_Angeles');
$hostname='xxx';
$username='xxx';
$password='xxx';
$dbname='xxx';

$db = mysql_connect($hostname,$username, $password) OR DIE ('Unable to connect to       database! Please try again later.');
mysql_select_db($dbname);

$input_id = $_POST['id'];

$sql = "SELECT chart_id, CONCAT(first_name,' ',last_name) as full_name, expiration_date as exp, NOW() as now, DATE_FORMAT(date_of_birth,'%m-%d-%Y') as date_of_birth, DATE_FORMAT(expiration_date,'%M %d, %Y') as expiration_date, DATE_FORMAT(recommendation_date,'%M %d, %Y') as recommendation_date, notes
    FROM chart
    WHERE chart_id = '" . mysql_real_escape_string($input_id) . "'
   ";

$today = date("F j, Y");

$result = mysql_query($sql) or die(mysql_error());

if (mysql_num_rows($result) > 0) {
    $row = mysql_fetch_assoc($result);

if ($row['exp'] < $row['now']) {
    $row['status'] = 0;
}
else {
    $row['status'] = 1;
}

mysql_free_result($result);
}
else {
    $row['status'] = 'none';
}

mysql_close($db);

?>

Efficiency

Nothing to worry about. It's only a linear script, there are no loops anywhere. You don't need to worry about efficiency.

Security and best-practices horrible practices

I should really have you write down this sentence 100 times on a piece of paper:

DO NOT USE THE MYSQL_* FUNCTIONS, THEY ARE DEPRECATED
DO NOT USE THE MYSQL_* FUNCTIONS, THEY ARE DEPRECATED
DO NOT USE THE MYSQL_* FUNCTIONS, THEY ARE DEPRECATED
DO NOT USE THE MYSQL_* FUNCTIONS, THEY ARE DEPRECATED
(...)

Although your SQL is technically safe, I think you can write down this sentence 2 times, and read up on what it is about:

GIVE ME PARAMETRIZED SQL OR GIVE ME DEATH
GIVE ME PARAMETRIZED SQL OR GIVE ME DEATH

There are other positive things with prepared queries / parametrized SQL than just security issues. I strongly recommend switching to prepared queries and not looking back. It's easy to miss using the mysql_real_escape_string function (and in my opinion it has an overly long function name). Switching to prepared queries helps with all these things, plus a couple of others.

There is also the fact that the mysql_* methods will be entirely removed in future versions of PHP. You are not the only one still using mysql_* methods unfortunately, and it is not the first time I'm saying this.

Start converting your code to use mysqli or PDO NOW. Nothing Else Matters! (Feel free to listen to that song while you convert your code).


Counting the number of domino tilings for a rectangular grid A domino tiling of a grid is a tiling of the grid using domino pieces. Below is a C++ program to count the number of possible tilings of a grid of a given size.

The program constructs the board row by row, representing each row as a string. In each row, d represents the top half of a domino pointing downwards, u represent the bottom half of a domino pointing upwards, r represents the left half of a domino pointing to the right, and l represents the right half of a domino pointing to the left.

#include <iostream>
#include <vector>
#include <map>

using namespace std;

class DominoTilingCounter {
    int height, width;
    bool done;
    int tilingCount;

    map<string, vector<string>> dictionary;

    vector<string> firstRow(int width) {
        vector<string> prev;

        prev.push_back("d");
        prev.push_back("r");

        for (int col = 1; col < width; col++) {
            vector<string> next;

            while (!prev.empty()) {
                string candidate = prev.back();
                prev.pop_back();

                switch (candidate.back()) {
                case 'd':
                case 'l':
                    next.push_back(candidate + 'd');
                    if (col != width - 1)
                        next.push_back(candidate + 'r');
                    break;
                case 'r':
                    next.push_back(candidate + 'l');
                    break;
                }
            }

            prev = next;
        }

        return prev;
    }

    vector<string> nextRows(const string& row) {
        if (dictionary.find(row) != dictionary.end())
            return dictionary[row];

        vector<string> prev;

        if (row.front() == 'd')
            prev.push_back("u");
        else {
            prev.push_back("d");
            if(row.length() > 1)
                prev.push_back("r");
        }

        for (int col = 1; col < row.length(); col++) {
            vector<string> next;

            while (!prev.empty()) {
                string candidate = prev.back();
                prev.pop_back();

                switch (row[col]) {
                case 'd':
                    if (candidate[col - 1] != 'r')
                        next.push_back(candidate + 'u');
                    break;
                case 'u':
                case 'r':
                case 'l':
                    if (candidate[col - 1] == 'r')
                        next.push_back(candidate + 'l');
                    else {
                        next.push_back(candidate + 'd');
                        if (col != row.length() - 1)
                            next.push_back(candidate + 'r');
                    }
                }
            }

            prev = next;
        }

        return prev;
    }

    int countTiling(int height, int width, const string& previousRow, int currentRow) {
        if (currentRow == height) {
            for (char cell : previousRow)
                if (cell == 'd')
                    return 0;
            return 1;
        }

        int count = 0;

        vector<string> rows;

        if (currentRow == 0)
            rows = firstRow(width);
        else
            rows = nextRows(previousRow);

        for (string row : rows)
            count += countTiling(height, width, row, currentRow + 1);

        return count;
    }

public:
    DominoTilingCounter(int height, int width) : height(height), width(width) {
        if (height <= 0 || width <= 0 || height * width % 2) {
            tilingCount = 0;
            done = true;
        }
    }

    int count() {
        if (!done) {
            tilingCount = countTiling(height, width, "", 0);
            done = true;
        }

        return tilingCount;
    }
};

int main() {
    DominoTilingCounter tc(4, 7);

    cout << tc.count() << endl;

    return 0;
}

Looks like a decent translation of your algorithm into C++. However, if you're looking for speed, this code is very "heavyweight".

It uses strings such as "drldurlrld" to represent each row of the grid, when bit-strings would suffice. (If you mark each "d" with the bit 1, and everything else with the bit 0, you'll still have an unambiguous encoding. This trick is similar to the trick used by Eller's maze-generation algorithm.)
It copies around vector<string> to memoize the "next possible rows" for a given row.
It doesn't use move semantics.

Now, I said that your code memoizes "next possible rows" — this is the purpose of dictionary — but if you look closely, you'll see that the variable dictionary is unused!

I can tell from context that what happened is you forgot to add a single line of the form dictionary[row] = prev; at the end of nextRows. But if the compiler can't read your mind as well as I can, then you'll have to learn to be more on guard — otherwise your code will run slowly and you won't know why!

On the plus side, this one-line difference is nice because it means you can test your code with and without the memoization and see if the memoization is actually worth it. I think it will be worth it, but I'm not 100% sure.

    if (currentRow == height) {
        for (char cell : previousRow)
            if (cell == 'd')
                return 0;
        return 1;
    }

This code, which rejects tilings with dominos that stick off the bottom of the grid, is important enough that I would recommend putting a comment on it.

    if (currentRow == height) {
        // Count this tiling, unless it dangles off the edge of the grid.
        return (previousRow.find('d') == std::string::npos) ? 1 : 0;
    }

To address the lack of move semantics: There are a couple of places in firstRow where you make copies of strings (or even whole vectors of strings), when you actually don't care about the right-hand side of the assignment anymore and might as well just move the string (or vector) into the left-hand variable. For example:

    string candidate = prev.back();
    prev.pop_back();

    switch (candidate.back()) {
    // ...
    case 'r':
        next.push_back(candidate + 'l');
        break;
    }
}

prev = next;

Here once you've finished the switch statement you don't care about the value of candidate, so you could

next.push_back(std::move(candidate) + 'l');

to save a copy. (Yes, this will in fact do the efficient thing: see overload #11 on the cppreference page.)

And you certainly don't need to make a copy of next when you assign it to prev! Replace that vector copy with a quick vector move:

prev = std::move(next);

Move semantics are more subtle (and probably less important) than I'm making them sound here; I won't go into detail here. But there is no shortage of material for learning more about move semantics if you want/need to.

int count() {
    if (!done) {
        tilingCount = countTiling(height, width, "", 0);
        done = true;
    }

    return tilingCount;
}

This is a common anti-pattern I see in a lot of student code for some reason: you want to compute a value, so you make a ValueComputor class, give it a computeTheValue() method, and then add memoization so that the computeTheValue() method won't be so darn slow the second time you call it.

This is an anti-pattern because it takes a notionally const operation (counting the number of tilings) and makes it a non-const member function. That is, it breaks this reasonable-looking code:

DominoTilingCounter tc(4, 4);
const DominoTilingCounter& param = tc;
std::cout << param.count() << std::endl;  // oops! can't call count() on a const object!

But it's also super easy to fix. Just take that code that computes the number of tilings, and move it straight into the constructor of your class!

DominoTilingCounter(int height, int width) : height(height), width(width)
{
    if (height <= 0 || width <= 0 || height * width % 2) {
        tilingCount = 0;
    } else {
        tilingCount = this->countTiling(height, width, "", 0);
    }
}

int count() const { return tilingCount; }

Much cleaner! And a tiny bit faster in the case that you call count() multiple times, because you don't have to check done anymore. (And much slower in the case that you construct a DominoTilingCounter object and never call the count() method — but why on earth would you construct this single-purpose object if you're never going to use it?)

Now notice that you don't use this->height or this->width anywhere in the code — instead, you pass around the caller's original height and width as parameters. And with the refactoring above, you don't use done. So what you're left with here is a class DominoTilingCounter that is effectively the same thing as an int. So, get rid of the useless class!

int count_domino_tilings(int height, int width);

This is the function you want to be writing. There's no reason to drag object-orientation into it at all. Your main function had been multiple lines long because of the mutation involved (and honestly it's not clear from reading this code that all the expensive work occurs on line 2 — so that's another downside to the anti-pattern above)...

int main() {
    DominoTilingCounter tc(4, 7);
    std::cout << tc.count() << std::endl;
}

...but with a nice clean function, it becomes simply

int main() {
    std::cout << count_domino_tilings(4, 7) << std::endl;
}

With such a drastic savings in lines of code, you have room left over to write some test cases! :)


Find occurrences of strings in view B in column from table A I'm trying to find the occurrences of strings in view B in a string in table A. However, the performance is horrendous (takes about 17 minutes to run) and I was wondering if anyone had any suggestions to improve it.

view B has 3570 records and has one column of type varchar named MISubCode

Table A has 160081 records and has the schema in the attached image.

There's also a third table, (Table C) which is largely used to filter the results of table A since I don't need to find occurrences in any of those records.

My rational to solve this was as follows:

Count the number of characters in the string in the desired search column in table A, producing one row for each character
Filter the unneeded rows using view C
Run each row generated in result set from 2. and use the Number column to find the occurrences from view B in the string in column from Table A until the end of the string in column from table A.

Code:

 SELECT *
FROM
(
SELECT Number, A.StatusInputPoint_DESC, A.StatusInputPoint_CODE, A.StatusInputPoint_CORE_ID
FROM
(
SELECT Number, A.StatusInputPoint_DESC, A.StatusInputPoint_CODE,  A.StatusInputPoint_CORE_ID
FROM
tblStatusInputPoints_CORE A
CROSS JOIN
(
    SELECT ROW_NUMBER() OVER (ORDER BY [object_id])
    FROM sys.all_objects
) AS n(Number)
WHERE Number >= 1 AND Number <= CAST(LEN(A.StatusInputPoint_DESC) AS INT) AND System_ID = 1
) AS A
LEFT JOIN
vwAlarmLibrary_MI_StatusPoint_Mask_Match_NoWildCards C
ON
A.StatusInputPoint_CORE_ID = C.StatusInputPoint_CORE_ID
WHERE C.StatusInputPoint_CORE_ID IS NULL
) AS RemainingStatusPoints
LEFT JOIN
dbo.vwAlarmLibrary_MI_Substation_Abbreviations B
ON
SUBSTRING(RemainingStatusPoints.StatusInputPoint_DESC, Number, LEN(B.MISubCode)) = B.MISubCode
WHERE B.MISubCode IS NOT NULL

I'm at a loss as to how to make this faster. I tried several different methods and join combinations and I can't seem to get it to perform well. I'm not an expert in SQL Server so I don't know all the features.

I'm currently using SQL Server 2016

EXAMPLE:

For clarity, lets say I have these strings in view B

 ABC
 EFGH
 IJ

And I have these strings in table A

 123 ABC 45 IJ
 IJ
 IJ EFGH 22

The result should be this:

 Pos   String          Occurrence
 5   | 123 ABC 45 IJ | ABC
 12  | 123 ABC 45 IJ | IJ
 1   | IJ            | IJ
 1   | IJ EFGH 22    | IJ
 4   | IJ EFGH 22    | EFGH

I think you could just simplify this with CHARINDEX and CROSS APPLY().  See how this method stacks up in your environment and paste your execution plans if it happens to be slower, if you can.

To give a concrete example, create these tables:

declare @ViewB table (myStrings varchar(16))
insert into @ViewB
values
('ABC'),
('EFGH'),
('IJ')

declare @TableA table (colToSearch varchar(256))
insert into @TableA
values
('123 ABC 45 IJ'),
('IJ'),
('IJ EFGH 22')

Then this CROSS APPLY() query should be more efficient:

select
    charindex(b.myStrings,a.colToSearch) as Pos
    ,a.colToSearch as String
    ,b.myStrings as Occurance
from
    @ViewB b
cross apply (select * from @TableA) a
where
    charindex(b.myStrings,a.colToSearch) > 0
order by
    String, charindex(b.myStrings,a.colToSearch)

Alternatively, you could use CROSS JOIN:

select
    charindex(b.myStrings,a.colToSearch) as Pos
    ,a.colToSearch as String
    ,b.myStrings as Occurance
from
    @ViewB b
cross join @TableA a
where
    charindex(b.myStrings,a.colToSearch) > 0
order by
    String, charindex(b.myStrings,a.colToSearch)

RESULTS

+-----+---------------+-----------+
| Pos |    String     | Occurance |
+-----+---------------+-----------+
|   5 | 123 ABC 45 IJ | ABC       |
|  12 | 123 ABC 45 IJ | IJ        |
|   1 | IJ            | IJ        |
|   1 | IJ EFGH 22    | IJ        |
|   4 | IJ EFGH 22    | EFGH      |
+-----+---------------+-----------+


Makefile -- Platform Dependency My application has two parts -- a client and a server.  Within this, there are different versions for different platforms.  The user specifies this in the args, i.e. "make PLATFORM=x11".

However, I feel like I'm separating the client and server build operations in a very inefficient way, and that I have too many variables.  It works, but I'm not sure how to improve it.

Any suggested improvements would be much appreciated!

CC=gcc
CFLAGS=-c -Wall
LDFLAGS=
BUILD_DIR = ../build/
OBJ_DIR = $(BUILD_DIR)obj/
LIBS=

CLIENT_NAME_DIR = client/
CLIENT_SRC_FILES := $(wildcard $(CLIENT_NAME_DIR)*.c)
CLIENT_PLATFORM_DIR = $(PLATFORM)/
CLIENT_SRC_FILES += $(wildcard $(CLIENT_NAME_DIR)$(CLIENT_PLATFORM_DIR)*.c)
CLIENT_OBJ_DIR = $(OBJ_DIR)$(CLIENT_NAME_DIR)
CLIENT_OBJ_FILES := $(addprefix $(CLIENT_OBJ_DIR),$(notdir $(CLIENT_SRC_FILES:.c=.o)))
CLIENT_LIBS = $(LIBS) -lX11
CLIENT_EXECUTABLE_NAME = outcast-client
CLIENT_EXECUTABLE=$(BUILD_DIR)$(CLIENT_EXECUTABLE_NAME)

SERVER_NAME_DIR = server/
SERVER_SRC_FILES := $(wildcard $(SERVER_NAME_DIR)*.c)
SERVER_OBJ_DIR = $(OBJ_DIR)$(SERVER_NAME_DIR)
SERVER_OBJ_FILES := $(addprefix $(SERVER_OBJ_DIR),$(notdir $(SERVER_SRC_FILES:.c=.o)))
SERVER_LIBS = $(LIBS)
SERVER_EXECUTABLE_NAME = outcast-server
SERVER_EXECUTABLE=$(BUILD_DIR)$(SERVER_EXECUTABLE_NAME)

all: clean client server

client: $(CLIENT_SRC_FILES) $(CLIENT_EXECUTABLE)

server: $(SERVER_SRC_FILES) $(SERVER_EXECUTABLE)

$(CLIENT_EXECUTABLE): $(CLIENT_OBJ_FILES)
    $(CC) $(LDFLAGS) $(CLIENT_OBJ_FILES) -o $@ $(CLIENT_LIBS)

$(CLIENT_OBJ_DIR)%.o: $(CLIENT_NAME_DIR)$(CLIENT_PLATFORM_DIR)%.c
    $(CC) $(CFLAGS) -c -o $@ $<

$(CLIENT_OBJ_DIR)%.o: $(CLIENT_NAME_DIR)%.c
    $(CC) $(CFLAGS) -c -o $@ $<

$(SERVER_EXECUTABLE): $(SERVER_OBJ_FILES)
    $(CC) $(LDFLAGS) $(SERVER_OBJ_FILES) -o $@ $(SERVER_LIBS)

$(SERVER_OBJ_DIR)%.o: $(SERVER_NAME_DIR)%.c
    $(CC) $(CFLAGS) -c -o $@ $<

clean:
    rm -rf $(BUILD_DIR)
    mkdir -p $(CLIENT_OBJ_DIR)
    mkdir -p $(SERVER_OBJ_DIR)

Conner, I have a few minor points.

Firstly, you have no includes (ie, no -Idir). Do you really have no headers?
You should make the non-targets 'PHONY' so make knows they will never exist:

.PHONY clean
clean:
        rm -f ...

Same with all, server and client
It is not normal to clean everything on each build - usually you let make decide what is out of date and needs to be rebuilt.  For that you need to have all the dependencies declared to make (eg headers)
client and server depend upon the build directories and the executables, but not directly upon the sources.
the clean target remakes the build directories.  To me, these directories should be made when you build the targets:

$(CLIENT_OBJ_DIR) $(SERVER_OBJ_DIR):
        mkdir -p $@

client: $(CLIENT_OBJ_DIR) $(CLIENT_EXECUTABLE)

server: $(SERVER_OBJ_DIR) $(SERVER_EXECUTABLE)

-Wall does not really give you 'all' warnings. There are many more that are worth enabling.


Shopping cart with Ajax I build a cart for an ecommerce website, and I want to know if I did well.

I want to record the changes in the DB and display the information from the DB to the customer. To be sure that what he sees is the good information. I use an ajax request and JSON to do so.

Please give me your comments. I especially want to know is my code safe?

The client side:

    function save_to_db(cartId, newQuantity) {
  $.ajax({
    url: "updateCart.php",
    data: "cartId=" + cartId + "&newQuantity=" + newQuantity + "&memberId=" + <?php echo $memberId?>,
    type: 'post',
    dataType: "json",
    success: function(response) {
      if (response === "Empty cart") {
        $("#total-quantity").text(0);
        $("#total-price").text(0);
        $("#shopping-cart-table").html("
Your cart is empty.
");
      }

      if (newQuantity == 0) {
        $("#item-container-" + cartId).remove();
      }

      var totalQuantity = 0;
      var totalItemPrice = 0;
      $.each(response, function(key, cartDetails) {
        totalItemPrice = totalItemPrice + parseInt(cartDetails.price) * parseInt(cartDetails.quantity);
        totalQuantity = totalQuantity + parseInt(cartDetails.quantity);

        if (cartDetails.cartId == cartId) {
          $("#input-quantity-" + cartId).val(cartDetails.quantity);
          $("#cart-price-" + cartId).text("$" + cartDetails.price * cartDetails.quantity);
        }
      });

      $("#total-quantity").text(totalQuantity);
      $("#total-price").text(totalItemPrice);
      $("#total-price-foot").text(totalItemPrice);
    }
  });
}

The server side:

    <?php
require 'connectDB.php';

$memberId = $_POST["memberId"];

if($_POST["newQuantity"] == 0){
    $stmt = mysqli_prepare($conn,"DELETE FROM tbl_cart WHERE id = ? AND member_id = ?");

    mysqli_stmt_bind_param($stmt, "ii", $_POST["cartId"], $memberId);
    mysqli_stmt_execute($stmt);
    mysqli_stmt_close($stmt);
}
elseif ($_POST["newQuantity"] == 1 || $_POST["newQuantity"] == -1) {
    $stmt = mysqli_prepare($conn,"UPDATE tbl_cart SET quantity =
        (CASE WHEN quantity + ? > 0 THEN quantity + ? ELSE quantity END)
        WHERE id= ? AND member_id = ?");

        mysqli_stmt_bind_param($stmt, "iiii",
            $_POST["newQuantity"],
            $_POST["newQuantity"],
            $_POST["cartId"],
            $memberId);
        mysqli_stmt_execute($stmt);
        mysqli_stmt_close($stmt);
    }

    if ($stmt = mysqli_prepare($conn,"SELECT tbl_product.id, tbl_product.price, tbl_cart.quantity, tbl_cart.id
        AS cartId FROM tbl_product, tbl_cart WHERE tbl_product.id = tbl_cart.product_id AND tbl_cart.member_id = ?")) {

            mysqli_stmt_bind_param($stmt, "i", $memberId);
            mysqli_stmt_execute($stmt);

            $result = mysqli_stmt_get_result($stmt);
            while ($row = mysqli_fetch_assoc($result)){
                $cartDetails[] = $row;
            }

            mysqli_stmt_close($stmt);
        }

        if(isset($cartDetails)){
            $response = json_encode($cartDetails);
            echo $response;
        }
        else{
            $response = json_encode("Empty cart");
            echo $response;
        }
        ?>

So far what I see is you aren't either casting nor sanitizing your $_POST variables but simply placing them into your SQL statement - it's better you perform some sort of filtering on those arguments (e.g. remove whitespaces, fixing invalid characters like '\' to '\\', etc.) and ensure you are passing numbers (according to your example you'd maybe use intval()), not a string or NULL value (unless that's intended, which I doubt).

In addition, the server-end should separate the database calls from parsing and filtering content - either as separate functions, files and/or both; if you were going the OOP route, then you'd set up a class(es) for either activity and use a layering system like MVC, N-Tier, MVVC.

All of this goes into making your code more reusable and easier to manage plus adds a layer of security via "Abstraction".


Merge sort implementation in JS Just for learning and practicing I've created a completely recursive version of the merge sort algorithm.

I've tested it a little bit and it seems working, but I'm quite sure that it can be largely improved. In particular, I'm hating the long if-else conditional inside the sort function. I think it's really bad written but I can't figure out a way to "beautify" the code without using a loop.

function merge(left, right, array){
    if (left.length == 0 && right.length == 0) {
        return array;
    } else if (left.length == 0) {
        return array.concat(right);
    } else if (right.length == 0) {
        return array.concat(left);
    } else if (left[0] < right[0]) {
        array.push(left.shift());
    } else if (left[0] > right[0]) {
        array.push(right.shift());
    } else {
        array.push(left.shift());
        right.shift();
    }
    return merge(left, right, array);
}

function mergeSort(array){
    if (array.length > 1) {
        return merge(mergeSort(array.slice(0, Math.ceil(array.length/2))),
            mergeSort(array.slice(Math.ceil(array.length/2))), []);
    } else {
        return array;
    }
}

mergeSort([2, 1, 5]) -> [1, 1, 2, 5] ?

You have a bug:

> mergeSort([2, 1, 5]);
[1, 1, 2, 5]

> mergeSort([2, 1, 5, 1, 9]);
[1, 1, 1, 1, 2, 5, 5, 9]

The problem is here:

sort(mergeSort(array.slice(0, Math.ceil(array.length/2))),
            mergeSort(array.slice(Math.floor(array.length/2, array.length))), []);

For example for the array [1, 2, 3],
the first slice will be [1, 2] and the second will be [2, 3],
so 2 gets duplicated.
The more ranges you have with odd length,
the more duplicates will creep in.

I'm also wondering what was the intention of this:

Math.floor(array.length/2, array.length)
                           ^^^^^^^^^^^^  typo?

It seems that changing the slices to this will fix it:

array.slice(0, Math.ceil(array.length / 2)))
array.slice(Math.ceil(array.length / 2), array.length))

Naming

The sort method doesn't "sort".
It merges.
As such, I'd rename it to merge.

Beauty

  i think it's really bad written but i can't figure out a way to "beautify" the code without using a loop

I think it's fine.
This is clear, easy to read, not really ugly.
But I'd use spaces more generously around parentheses, like this:

function merge(left, right, array){
    if (left.length == 0 && right.length == 0) {
        return array;
    } else if (left.length == 0) {
        return array.concat(right);
    } else if (right.length == 0) {
        return array.concat(left);
    } else if (left[0] < right[0]) {
        array.push(left.shift());
    } else {
        array.push(right.shift());
    }
    return merge(left, right, array);
}