TCP client reconnection logic running on 30 machines I am working on a service that is supposed to run on a large installation 24/7. Once the software is deployed, I will most likely not be able to make any changes anymore. This is my first time working on a network application so I want to make sure I am not making any obvious mistakes.

60 instances of this program are supposed to run on 30 machines, meaning there are 2 instances of it running on each machine. It is used to keep all the client instances (running this code) connected to the controller that gives them updates when needed.

I am wondering if there are any optimizations you guys can recommend to me or if there are any other insufficiencies that catch your eyes.

public class ControllerConnectionService {

    public ControllerConnectionService() {
        Initialize();
    }

    private Timer _pingTimer;
    private SimpleTcpClient _tcpClient;

    public event EventHandler<DataModel> ContentTypeChanged;

    private void Initialize() {
        CreateTcpClient();
        Connect();

        var messageString = GetMessageString(MessageType.SendId, MainViewModel.Id.ToString());
        SendMessage(messageString);

        _pingTimer = new Timer {Interval = 2000};
        _pingTimer.Elapsed += PingTimerOnElapsed;
        _pingTimer.Start();
    }

    private void PingTimerOnElapsed(object sender, ElapsedEventArgs elapsedEventArgs) {
        if (!IsConnected()) Reconnect();
    }

    private bool IsConnected() {
        try {
            var pingBytes = Encoding.UTF8.GetBytes(GetMessageString(MessageType.Ping, string.Empty));
            _tcpClient.TcpClient.Client.Send(pingBytes);
            return true;
        }
        catch (Exception) {
            return false;
        }
    }

    private void CreateTcpClient() {
        _tcpClient = new SimpleTcpClient();
        _tcpClient.DataReceived += TcpClientOnDataReceived;
    }

    private void Connect() {
        try {
            _tcpClient.Connect(ControllerIp, ClientControllerPort);
        }
        catch (Exception) {
            Reconnect();
        }
    }

    private void Reconnect() {
        try {
            while (!IsConnected()) {
                Thread.Sleep(500);
                CreateTcpClient();

                _tcpClient.Connect(ControllerIp, ClientControllerPort);
            }
        }
        catch (Exception) {
            Reconnect();
        }
    }

    private void SendMessage(string message) {
        try {
            _tcpClient.TcpClient.Client.Send(Encoding.UTF8.GetBytes(message));
        }
        catch (Exception) {
            Reconnect();
        }
    }

    private void TcpClientOnDataReceived(object sender, Message message) {
        if (string.IsNullOrEmpty(message.MessageString))
            return;

        var messageParts = message.MessageString.Split(';');

        Enum.TryParse(messageParts[0], out MessageType messagetype);

        if (messagetype == MessageType.UpdateContentType) {
            Console.WriteLine($@"{DateStamp} Data received: {message.MessageString} ");
            var dataModel = JsonConvert.DeserializeObject<DataModel>(messageParts[1]);

            ContentTypeChanged?.Invoke(this, dataModel);
        }
    }
}

If you require any additional information or code, feel free to ask.

catch (Exception)
{
  Reconnect();
}

The same pattern can be found in almost every method. Are you not interested in what happened? No logging or anything? Just ignore the exception?

_pingTimer = new Timer { Interval = 2000 };
Thread.Sleep(500);

Hardcoded numbers? No settings? What if this won't work and you have to tune it? You'll have to update all instances!

Enum.TryParse(messageParts[0], out MessageType messagetype);

Why isn't this inside an if?

if (messagetype == MessageType.UpdateContentType)

This is the next line following the TryParse but this can go sideways if the UpdateContentType is the first value of the enum it might be returned as a default value and the flow will enter the if even if the massage part wasn't in the correct format.

private void TcpClientOnDataReceived(object sender, Message message)

You have try/catches for everything but this method. If something goes wrong here the application will crash.

  60 instances of this program are supposed to run on 30 machines, meaning there are 2 instances of it running on each machine. It is used to keep all the client instances (running this code) connected to the controller that gives them updates when needed.

This sounds like too much. Why can't the client check for updates by itself?

  Once the software is deployed, I will most likely not be able to make any changes anymore.

If you are not able to update it later then you should start writing all kinds of unit and integration tests to make sure it's bullet-proof but even then there will still be a bug somewhere... Not being able to deploy new versions is a terrible plan.


Django: Database efficiency - currently accessed twice I have the following function that is called via signal. 1) check if incluencer exists. 2) Increase discount by +1. Currently, I hit the database twice. Is there a better way to write that? Additionally, I wonder I can check 2) all on a database level so I don't need the for-loop.

@receiver(signal=charge_succeeded)
@transaction.atomic
def create_influencer_transaction(sender, order, charge, **kwargs):
    order_exists = Transaction.objects.filter(order=order).exists()
    if order_exists:
        return None

    # 1) Check if influencer exists
    first_order_item = order.order_items.first()
    influencer = first_order_item.social_referral
    if not influencer:
        return None

    total_points_earned = order.order_items.aggregate(
        total=Sum(
            F('unit_points_earned') * F('quantity'),
            output_field=IntegerField()
        )
    )['total'] or 0

    Transaction.objects.create(
        [...]
    )

    # 2) Redeemed amount + 1
    for order_item in order.order_items.all():
        social_discount = order_item.social_discount
        if social_discount:
            social_discount.redeemed_amount = F('redeemed_amount') + 1
            social_discount.save(update_fields=['redeemed_amount'])
            break

models.py

class OrderItem(AbstractItem, AbstractDiscountItem, AbstractSocialItem):
    order = models.ForeignKey(
        [...]
    )
    social_discount = models.ForeignKey(
        'discounts.SocialDiscount',
        on_delete=models.PROTECT,
        related_name='order_items',
        null=True,
        blank=True,
    )   # PROTECT = don't allow to delete the discount if an order_item exists

class SocialDiscount(AbstractDiscount):

    event = models.OneToOneField(
        [...]
    )  # CASCADE = delete the discount if the event is deleted
    tickets = models.ManyToManyField(
        [...]
    )

Your step 2 can be done entirely in DB by filtering and limiting your order_items. And then updating it to contain the desired value. Note that the F objects documentation already mention that use of a queryset update method. So we could imagine something like:

order.order_items.filter(social_discount__isnull=False)[:1].update(social_discount__redeemed_amount=F('social_discount__redeemed_amount') + 1)

Except this doesn't work:

we cannot use update after slicing a queryset;
we cannot update fields of related models anyway.

Instead we must use the relationship backwards and filter/update on the SocialDiscount model, the filter being the subset of OrderItems we are interested in:

order_item = order.order_items.filter(social_discount__isnull=False)[:1]
# Note that order_item is still a queryset and has not hit the database yet
SocialDiscount.objects.filter(order_items__in=order_item).update(redeemed_amount=F('redeemed_amount') + 1)

Even though this only hit the database once, this will update all SocialDiscount objects that are related to the one order_item specified by the queryset above. If it ever can mean that more than one object is updated and you don't want that, you can still remove the for loop using this two-queries code:

social_discount = order.order_items.filter(social_discount__isnull=False).select_related('social_discount').first().social_discount
social_discount.redeemed_amount = F('redeemed_amount') + 1
social_discount.save()


Check whether two JavaScript arrays are equal, using basic library only I am writing a function that takes in two arrays and a test name and outputs statements (console.log) saying whether the two arrays are equal. I don't want to convert the arrays to strings or use non-core libraries to do this. I am also new to writing JS so I wanted to get a code review here.

Usage: FAILURE CASE:
var expected = [1, 1, 2, 3, 5, 8, 13];
var actual = [2, 3, 5, 8, 13, 21, 34];
assertArraysEqual(actual, expected, 'generates first 7 Fibonacci
numbers');

console output:
 FAILED [generates first 7 Fibonacci numbers] Expected "1, 1, 2, 3, 5,
8, 13", but got "2, 3, 5, 8, 13, 21, 34"

function assertArraysEqual(actual, expected, testName) {

  var lengthsEqual = (actual.length === expected.length)
  var contentsEqual = true
  for (var i=0; i<actual.length; i++)
  {
    if (actual[i] !== expected[i]) contentsEqual=false
  }
  if(contentsEqual && lengthsEqual){
   console.log("passed")
  }
  else{
    console.log('FAILED [' + testName + '] Expected "' + expected + '", but got "' + actual + '"');
  }
}

Your function should be dedicated to its functionality. Have all the printing done externally. You don't need testName as a parameter.
Have your function return true or false. It's much more useful and reusable that way. Having it return also allows you to exit the function early if you know the arrays are not equal. For example:

function assertArraysEqual(actual, expected) {
    if (actual.length !== expected.length) {
        return false;
    }
    ...

If this is for something like unit testing, you should use a unit test library to like Mocha or Qunit to validate the behavior of the function.
What data types are you dealing with? Are you only concerned with numbers? Do you want strict type checking and/or null checking?

If you're only concerned with validating arrays of numbers and you know the bounds of the input, you can easily rewrite your function like so:

    function assertArraysEqual(actual, expected) {
        if (actual.length !== expected.length) {
            return false;
        }
        for (var i = 0; i < actual.length; ++i) {
            if (actual[i] !== expected[i]) {
                return false;
            }
        }
        return true;
    }


c# IndexOf string search I have this code working but its using IndexOf I am not sure if that is the best or only option for search for a string value. What I have is a list that will need to remove a world from the child name if it is the same as the name.

  Is this the best way to complete this task?

namespace DupName
{
    public class Program
    {
        static void Main(string[] args)
        {
            StringBuilder sbchildName = new StringBuilder();

            List<Item> items = new List<Item>()
            .AddAlso(new Item { Id = 1, Name = "Audi", ChildName = "Audi A3 Premium Plus" })
            .AddAlso(new Item { Id = 2, Name = "Audi", ChildName = "Audi A5 Premium Plus" })
            .AddAlso(new Item { Id = 1, Name = "Audi", ChildName = "Audi A6 Premium Plus 55 TFSI quattro" })
            .AddAlso(new Item { Id = 2, Name = "Audi", ChildName = "S5 Premium Plus 3.0 TFSI quattro" })
            .AddAlso(new Item { Id = 2, Name = "Audi", ChildName = "S3 Premium Plus 3.0 TFSI quattro" });

        foreach (var item in items)
        {
            if (item.ChildName.IndexOf(item.Name) != -1)
            {
                string temp = string.Format("{0} {1}", item.ChildName.Replace(item.Name, ""), "Car");
                Console.WriteLine(temp.Trim());
                childName.AppendFormat(temp.Trim());
            }
            else
            {
                string temp = string.Format("{0} {1}", item.ChildName, "Car");
                Console.WriteLine(temp.Trim());
                childName.AppendFormat(temp.Trim());
            }
        }
    }

    public static class Extensions
    {
        public static List<T> AddAlso<T>(this List<T> list, T item)
        {
            list.Add(item);
            return list;
        }
    }

    public class Item
    {
        public int Id { get; set; }
        public string Name { get; set; }
        public string ChildName { get; set; }
    }
}

output:

A3 Premium Plus Car
A5 Premium Plus Car
A6 Premium Plus 55 TFSI quattro Car
S5 Premium Plus 3.0 TFSI quattro Car
S3 Premium Plus 3.0 TFSI quattro Car

Because you use the returned value of IndexOf() only to check if it is > -1 a more readable way would be to use Contains() instead of IndexOf().

Internally Contains() does mostly the same than your comparison of the returned value. In the reference source of Contains() you will find

[Pure]
public bool Contains( string value ) {
    return ( IndexOf(value, StringComparison.Ordinal) >=0 );
}

while the counterpart of IndexOf() will look like so

[Pure]
public int IndexOf(String value) {
    return IndexOf(value, StringComparison.CurrentCulture);
}

The difference between these two methods by the meaning of the internally called IndexOf() method is the passed StringComparison parameter.

By reading the documentation you will read in the remarks section

  When you call a string comparison method such as String.Compare,
  String.Equals, or String.IndexOf, you should always call an overload
  that includes a parameter of type StringComparison so that you can
  specify the type of comparison that the method performs. For more
  information, see Best Practices for Using Strings.

Let us check what that means by using the german word daß and compare the results from StringComparison.CurrentCulture, StringComparison.InvariantCulture and StringComparison.Ordinal

Console.WriteLine("daß".IndexOf("daß", StringComparison.Ordinal));
Console.WriteLine("daß".IndexOf("dass", StringComparison.Ordinal));
Console.WriteLine("daß".IndexOf("dass", StringComparison.CurrentCulture));
Console.WriteLine("daß".IndexOf("dass", StringComparison.InvariantCulture));

this will yield the results

  0
  -1
  0
  0

As you see you need to know what you want to get returned by calling IndexOf(). If you need to know if the exact string is contained inside another string you shouldn't use the default IndexOf() method.


Implementation of Dijkstra's algorithm to find the shortest path in graph This is my implementation of Dijkstra's algorithm in C++.

Please clarify

Whether the class design is proper or if it needs to be improved.
I am just hard coding the vertex based on the priority of the vertex. Is there is any other way to implement it?
Please provide some suggestions to optimize the code.

class Vertex
{
public:
    int m_id;
    unsigned int m_cost;
    bool m_isVisited;

    std::map<int,unsigned int> m_edgeList; // map to store neighbour nodes and cost to reach them

    void findMinDistance(std::vector<Vertex>&);

    Vertex(int p_id)
    {
        m_id= p_id;
        if(p_id == 0)
        {
            m_cost=0;
        }
        else
        {
            m_cost=UINT_MAX; // Assigning Infinite cost
        }
        m_isVisited=false;
    }

    Vertex()
    {
    }
};

void Vertex::findMinDistance(std::vector<Vertex> &p_vertex)
{

    m_isVisited= true;
    // Looping throgh the neighbour list and update cost
    for(std::map<int,unsigned int>::iterator iter = m_edgeList.begin();
        iter!=m_edgeList.end(); iter++)
    {
        //If cost of picked neighbour is greater than current_vertex+new cost then replace cost
        if ( p_vertex.at(iter->first).m_isVisited == false && p_vertex.at(iter->first).m_cost > m_cost+(iter->second))
        {
            p_vertex.at(iter->first).m_cost=m_cost+iter->second;

            std::cout<<"\nVertex Id : "<<p_vertex.at(iter->first).m_id;
            std::cout<<"\nExisting Cost  : "<<p_vertex.at(iter->first).m_cost;
            std::cout<<"\nNew Cost : "<<p_vertex.at(iter->first).m_cost;
        }
    }
}

int main()
{
    std::vector<Vertex> l_vertex;

    for(int i=0; i<7;i++)
    l_vertex.push_back(Vertex(i)); // Push Vertex to vector container

    // updating the edges and cost to respective node
    // The below model is similar to priority queue.

    l_vertex.at(0).m_edgeList.insert(std::make_pair((l_vertex.at(5).m_id),14));
    l_vertex.at(0).m_edgeList.insert(std::make_pair((l_vertex.at(2).m_id),9));
    l_vertex.at(0).m_edgeList.insert(std::make_pair((l_vertex.at(1).m_id),7));

    l_vertex.at(1).m_edgeList.insert(std::make_pair((l_vertex.at(0).m_id),7));
    l_vertex.at(1).m_edgeList.insert(std::make_pair((l_vertex.at(2).m_id),10));
    l_vertex.at(1).m_edgeList.insert(std::make_pair((l_vertex.at(3).m_id),15));

    l_vertex.at(2).m_edgeList.insert(std::make_pair((l_vertex.at(0).m_id),9));
    l_vertex.at(2).m_edgeList.insert(std::make_pair((l_vertex.at(5).m_id),2));
    l_vertex.at(2).m_edgeList.insert(std::make_pair((l_vertex.at(3).m_id),11));
    l_vertex.at(2).m_edgeList.insert(std::make_pair((l_vertex.at(1).m_id),10));

l_vertex.at(5).m_edgeList.insert(std::make_pair((l_vertex.at(0).m_id),14));
l_vertex.at(5).m_edgeList.insert(std::make_pair((l_vertex.at(2).m_id),2));
l_vertex.at(5).m_edgeList.insert(std::make_pair((l_vertex.at(4).m_id),9));

l_vertex.at(4).m_edgeList.insert(std::make_pair((l_vertex.at(5).m_id),9));
l_vertex.at(4).m_edgeList.insert(std::make_pair((l_vertex.at(3).m_id),6));

l_vertex.at(3).m_edgeList.insert(std::make_pair((l_vertex.at(4).m_id),6));
l_vertex.at(3).m_edgeList.insert(std::make_pair((l_vertex.at(2).m_id),11));
l_vertex.at(3).m_edgeList.insert(std::make_pair((l_vertex.at(1).m_id),15));

// updating cost of each vertex by calling findMinDistance

for(std::vector<Vertex>::iterator iter = l_vertex.begin(); iter != l_vertex.end(); iter++)
{
    (*iter).findMinDistance(l_vertex);
}

for(int i=0;i<6;i++)
std::cout<<"\n\nshortest "<<i<<" "<<l_vertex.at(i).m_cost;

return 0;
}

This is a bad idea:
class Vertex
{
public:
    int m_id;
    >>> unsigned int m_cost;
    >>> bool m_isVisited;

You are conflating two things into a single class. The concept of a vertex and the concept of performing Dijkstra's algorithm on the node. You should separate the two issues into their own classes.
The main problem with this technique is that you need to reset the graph before use. ie. you can not run your algorithm twice in a row. You must reset the state of the whole graph before it can be re-used. Look up the visitor pattern.
You really want a constructor that does no initialization?
Vertex()
{
}

The POD members will be left in an undefined state.
Your have void findMinDistance(std::vector<Vertex>&); in the vertex? Find min distance to where?
Use the new C++ range based for:
for(std::map<int,unsigned int>::iterator iter = m_edgeList.begin();
    iter!=m_edgeList.end(); iter++)

// can be rewritten as:
for(auto& edge: m_edgeList)

Overall I don't think you are actually implement Dijkstra's algorithm.
Dijkstra algorithm
The algorithm should look like this (this is pseudo code).
The classic part is the frontier list (I called it boundryList) and the set of vertex we have found the shortest route to.
 RouteCostObject findRout(Graph& g, Vertex& start, Vectex& end)
 {
      OrderedListByDistance    boundryList;
      SetOfVertex              foundVertixes;

      boundryList.push_back([start, 0, []]);

      while(!boundryList.empty())
      {
          // Remove the top items from the list.
          (vertex, cost, route) = boundryList.top();       // Note: boundryList is sorted
          boundryList.pop();

          // If we have reached the end.
          // This is the shortest route.
          if (vertex == end)
          {    return [cost, route];
          }

          // Check if we have already found the shortest distance
          // to this vertex.
          if (foundVertixes.find(vertex) != vertex.end())
          {
              continue;
          }
          foundVertixes.insert(vertex);

          // Add all edges to the boundary list.
          for(auto& edge: vertex.edges)
          {
              boundryList.push_back([edge.dst, cost + edge.cost, route + edge]);
          }
      }
      // No route from start to end
      return [infinity, []];
 }


Custom HashMap Implementation in java I implemented a CustomHashMap in Java, and would like to know if my approach is good or not.

Below I am sharing the complete code of my CustomHashMap:

CustomHashMap.java

import java.util.Iterator;
import java.util.Objects;
import com.custom.utils.*;

public class CustomHashMap<K, T> implements Iterable<Entity> {

    private static final int capacity_init = 10;
    static final float DEFAULT_LOAD_FACTOR = 0.75f;
    private int indexCount = 0;

    Entity[] bucket = null;

    Entity[] bucketItr = null;

    public CustomHashMap() {
        init(capacity_init);
    }

    public CustomHashMap(int capacity) {
        init(capacity);
    }

    private void init(int intcapacity) {

        bucket = new Entity[intcapacity];
    }

    private int hash(Object key) {

        return key.hashCode() >> 1;
    }

    public void add(K key, T value) {

        int hashcode = hash(key);

        if(indexCount == bucket.length){
            Entity[] obj = bucket;
            int k = (int) (obj.length*2);
            bucket = new Entity[k];
            System.arraycopy(obj, 0, bucket, 0, indexCount-1);
        }

        if (bucket[hashcode] == null) {
            indexCount++;
            Entity head = new Entity(key, value, null);
            bucket[hashcode] = head;
        } else {
            Entity temp = bucket[hashcode];
            while (temp.next != null) {
                temp = temp.next;
            }
            Entity neEntity = new Entity(key, value, null);
            neEntity.next = temp;
            bucket[hashcode] = neEntity;

        }

        bucketItr = new Entity[bucket.length];
        System.arraycopy(bucket, 0, bucketItr, 0, bucket.length);

    }

    public T get(K key) {
        int hashcode = hash(key);
        if (bucket[hashcode] == null) {
            return null;
        } else {
            Entity temp = bucket[hashcode];
            while (temp != null) {

                if (temp.key.equals(key)) {
                    return (T) temp.value;
                }
                temp = temp.next;
            }
            return null;
        }

    }

    public void delete(K key) {
        int hashcode = hash(key);
        Entity deleteNode = bucket[hashcode];
        deleteNode = null;
        bucket[hashcode] = deleteNode;
    }

    @Override
    public Iterator<Entity> iterator() {
        // TODO Auto-generated method stub
        bucketItr = new Entity[indexCount];
        int entryCount = 0;
        for (Entity e : bucket) {

            if (Objects.nonNull(e)) {
                bucketItr[entryCount] = e;
                entryCount++;
            }

        }

        return new MapItr();
    }

    class MapItr implements Iterator<Entity> {

        int count = 0;
        Entity node = bucketItr[0];

        @Override
        public boolean hasNext() {
            // TODO Auto-generated method stub

            return count < bucketItr.length;
        }

        @Override
        public Entity next() {

            Entity result = null;

            if (hasNext()) {
                result = node;
                if ( Objects.nonNull(node) && node.next != null) {
                    node = node.next;
                } else {
                    count++;
                    if (count < bucketItr.length)
                        node = bucketItr[count];
                }

                return result;
            } else {
                return null;
            }
        }

    }

}

Entity.java

public class Entity<K, T> {
    public K key;
    public T value;
    public Entity next;

    public Entity(K key, T value, Entity next) {

        this.key = key;
        this.value = value;
        this.next = next;

    }

}

Please suggest improvements and things which you feel I need to add to it.

Here are my comments, in decreasing order of severity:

1) BUGS

Hash function: your hash function does not refer in any way to the bucket (changing) capacity.  The main problem with that is that the function may produce values that are outside of the index bounds of the bucket array (current function may even produce negative values) and then your add() method would throw an exception. but even if you protect from such cases, the function needs to refer to the current capacity in order to spread the values as evenly as possible across the buckets.
Load factor: The code does not refer to the DEFAULT_LOAD_FACTOR: nor does it allow the user to override this value, and more importantly, it doesn't consider this when deciding if the bucket array  needs to be enlarged.  In fact, the whole logic of this decision seems to me to be wrong: when all entries in the bucket are full, you decide to enlarge the size. so theoretically, on the 11th add() call, you would enlarge the size.  This logic needs to be redesigned and it needs to be based on the total number of Entity instances in all the buckets and of course on the load factor.

Note: depending on the hash function implementation, you may need to rehash the entire hash table because if the hash function depends on the capacity, then it may produce different results for existing keys when the capacity changes.  So, the simple System.arraycopy() might not suffice.  This is actually what is done in the JDK implementation.

2) Performance

Adding a new Entity to the linked list involves going through the entire list.  This seems inefficient.  Perhaps you can save the last node in a separate array that has same length as bucket?
The usage of bucketItr seems redundant.  First of all, you copy bucket into bucketItr every time add() is called.  System.arraycopy() is efficient but that doesn't mean it need not be used judiciously. Moreover, the first line in iterator() initializes bucketItr from scratch, making the copy operation in add() completely redundant. but moreover, I don't see the reason for this array at all.  couldn't you loop on bucket in MapItr and just ignore null entries?

3) Best Practices

References to generic type Entity<K,T> should be parameterized: Throughout the code, you refer to the Entity raw type, without the <K, T> generic parameters.  While it seems that this should not produce a bug, (since add() method only accept args of the correct types, this is still considered bad practice (and indeed produces compile warning).
The use of Objects.nonNull() is redundant.  why not use the simple and clear node != null? and while we are there, you first assign node to result and then ask about null. so theoretically, you could return null result.

4) Naming Conventions

an array of buckets should be named buckets or perhaps bucket_array don't you think?


Given a byte string S of length l, obtain the string D of length l-1 as D(i) = S(i) * S(i+1) Given a byte string S of length l, obtain the string D of length l-1 as D(i) = S(i) * S(i+1) (each element of D is the product of two consecutive elements of S).
here is my code.
bits 32

global start

extern exit
import exit msvcrt.dll

segment data use32 class=data
    s db 1, -3, 4, 7 ; declaring the string of bytes s
    l equ $ - s - 1 ; compute the length of the string l - 1
    d times l dw 0 ; reserve l bytes for the destination string and initialize it
segment code use32 class=code
    start:
        mov ecx, l

        mov esi, 0

        jecxz end_prog
        repeat_this:
            mov al, [s + esi] ; in al - the element on the esi position
            mov dh, [s + esi + 1] ; in dh - the element after the esi position
            imul dh ; multiplying consecutive terms
            mov [d + esi], ax; put the result in d
            inc esi ; incrementing esi to go to the next position
        loop repeat_this

        end_prog:

        push dword 0
        call [exit]

my question is did I make a mistake by declaring d times l dw 0 since I know that a byte * byte = word? Is everything else okay or should I pay attention to some details that I'm missing?

Inconsistent addressing of d
d is an array of 2-byte elements (d times l dw 0) and mov [d + esi], ax indexes into it with esi as the offset. However, esi is increased by 1 every iteration, so the resulting access pattern is that the stores keep stepping half on top of the previous store, shown by such a diagram: (storing -3, -12 and 28 in sequence)
FD FF
   F4 FF
      1C 00

This is possible to do, but probably unintentional.
Normally my recommendation would be to increase the register by 2 in the loop, but esi is also used to index s so that won't work. You can change the store to double esi though, for example:
mov [d + esi*2], ax

Or,
mov [d + esi + esi], ax

If you change d to be an array of bytes then the store should be mov [d + esi], al, with the original address but storing only 1 byte. Storing overlapping words would put the right data in d, but it's strange and stores a stray byte just after the end of d.
Calling exit
exit performs C library termination procedures, but the C library runtime environment wasn't used or even initialized (this code is in start, not main), so it's not necessary. It makes more sense to call ExitProcess (kernel32.dll), or just return (that does not work on Linux, but this is a Windows program and there it does work).
The loop instruction
loop is not great, it has some niche uses but typically it should be avoided. jecxz by contrast is OK, just unusual.
Minor things
The best way to zero esi isn't mov esi, 0, it's xor esi, esi, unless the flags must not be affected.
The two loads of the current and next element of S could be merged into a single word load, if you want. For example mov ax, [s + esi] followed by imul ah.


Dynamic visual list of images Situation
So I have made a list of PictureBoxes on my form that looks as follows:

All images have a panel behind them that is 4px lager (2xp all around the picture box). The images are named x1, x2, xn.. respectively. The panels behind the images are named y1, y2, yn.. respectively.

I want this to happen smoothly when one of the 5 PictureBoxes is clicked:

Before clicked, all pictureboxes are 48x48 and respective panels  52x52
When one gets clicked, resize the picturebox to 62x62 and the respective panels to 66x66
When one gets clicked, the image must hold the same center as previously
When one gets clicked, all other images turn back 'small'
when a 'enlarged' image gets clicked, nothing happens

What I made

When form is created, two lists are initialised:

List<PictureBox> views = new List<PictureBox> { x1, x2, x3, x4, x5 }; and
List<Panel> backviews = new List<Panel> { y1, y2, y3, y4, y5 };

and
PictureBox current which is the name of the current picturebox (x1, x2, etc.)

All PictureBoxes in views have a click event that points to x_Click

private void x_Click(object sender, EventArgs e)
        {
            PictureBox x = sender as PictureBox;

            //Only do something if clicked PictureBox is not large
            if (x.Size != new Size(62, 62))
            {
                // This keeps the PictureBox center constant
                x.Location = new Point(x.Left - 7, x.Top - 7);
                // Resize picturebox
                x.Size = new Size(62, 62);
                // Get the panel corresponding to the PictureBox
                Control y = backviews[int.Parse(x.Name.Replace("x", "")) - 1];
                // This keeps the Panel center constant
                y.Location = new Point(y.Left - 7, y.Top - 7);
                // Resize Panel
                y.Size = new Size(66, 66);

                // Find the current enlarged PictureBox and reverse what is done above
                Control z = views[int.Parse(current.Replace("x", "")) - 1];
                z.Location = new Point(z.Left + 7, z.Top + 7);
                z.Size = new Size(48, 48);
                Control w = backviews[int.Parse(z.Name.Replace("x", "")) - 1];
                w.Location = new Point(w.Left + 7, w.Top + 7);
                w.Size = new Size(52, 52);

                // Set the currently enlarged PictureBox
                current = x.Name;
            }

        }

This works, but not smoothly; I can visually intercept the Panel being enlarged after the PictureBox is being enlarged. I suspect most time is taken by finding the Panels corresponding to the PictureBoxes. Is there any way I can speed this up?

There are a few things that you should refactor to be able to optimize your code better. Currently everthing sits inside the Click event handler and it's too much to handle there.

You are not able to focus on particular task like that. You should separate the operations and create new methods and properties for them. Here's how you could start. It's not 100% perfect (like the hard coded picture box BackColor) yet but it should give you an idea how to start refactoring your current solution.

Create a new panel control with a picture box inside (you can create a property for it so that you can set the image later). In this control you can implement all the logic that should handle the resize.

Give it a few properties that you can set and calculate the size and location values dynamiclly and internally. Use a flag (like IsEnlarged) to track whether a panel is in its normal or enlarged size rather than hard coded sizes. Here you can handle the Click event and take proper actions to modify the panel and the picture box.

class ResizablePanel : Panel
{
    private bool _isEnlarged;

    public ResizablePanel()
    {
        Controls.Add(new PictureBox
        {
            BackColor = Color.PaleGoldenrod
        });

        Click += PanelOrPicktureBox_Click;
        Controls[0].Click += PanelOrPicktureBox_Click;
    }

    public bool IsEnlarged
    {
        get { return _isEnlarged; }
        set
        {
            if (value == _isEnlarged)
            {
                return;
            }

            _isEnlarged = value;
            Size = _isEnlarged ? EnlargedSize : NormalSize;
            ResizePictureBox();
            TranslatePanel();

            if (IsEnlarged)
            {
                Enlarged?.Invoke(this, EventArgs.Empty);
            }
        }
    }

    public Size NormalSize { get; set; }

    public Size EnlargedSize { get; set; }

    public Size BorderSize { get; set; }

    public void Initialize()
    {
        Size = _isEnlarged ? EnlargedSize : NormalSize;
        ResizePictureBox();
        TranslatePanel();
    }

    private void ResizePictureBox()
    {
        var pictureBox = Controls[0] as PictureBox;

        pictureBox.Location = new Point
        {
            X = BorderSize.Width,
            Y = BorderSize.Height
        };

        // multiply by 2 becasue border size specifies only its width and we
        // need to two of them, one for each side
        pictureBox.Size = new Size
        {
            Width = Size.Width - (BorderSize.Width * 2),
            Height = Size.Height - (BorderSize.Height * 2)
        };
    }

    private void TranslatePanel()
    {
        if (IsEnlarged)
        {
            Location = new Point
            {
                X = Location.X - ((EnlargedSize.Width - NormalSize.Width) / 2),
                Y = Location.Y - ((EnlargedSize.Height - NormalSize.Height) / 2),
            };
        }
        else
        {
            Location = new Point
            {
                X = Location.X + ((EnlargedSize.Width - NormalSize.Width) / 2),
                Y = Location.Y + ((EnlargedSize.Height - NormalSize.Height) / 2),
            };
        }
    }

    private void PanelOrPicktureBox_Click(object sender, EventArgs e)
    {
        IsEnlarged = !IsEnlarged;
    }
}

You also can create your panels dinamiclly and give them some initial values.

private void CreatePictureBoxes()
{
    var pictureBoxCount = 4;

    var panelNormalSize = new Size(52, 52);
    var panelEnlargedSize = new Size(66, 66);
    var borderSize = new Size(5, 5);

    for (var i = 0; i < pictureBoxCount; i++)
    {
        var panel = new ResizablePanel
        {
            NormalSize = panelNormalSize,
            EnlargedSize = panelEnlargedSize,
            BorderSize = borderSize,
            IsEnlarged = false,
            // 3 is just a random border width multiplier to create gaps between the controls
            Location = new Point
            {
                X = panelNormalSize.Width * i + borderSize.Width * 3 * (i + 1),
                Y = borderSize.Height * 3
            },
            BackColor = Color.DarkMagenta,
        };
        panel.Initialize();

        Controls.Add(panel);
    }
}

Initialize the panels:

public Form1()
{
    InitializeComponent();
    CreatePictureBoxes();
}

And this is how it looks like:

  When one gets clicked, all other images turn back 'small'

Add an event to your new panel and raise it when it gets enlarged. You can then handle this event in the main form and set all other panels to IsEnlarged = false

public event EventHandler Enlarged;

public bool IsEnlarged
{
    get { return _isEnlarged; }
    set
    {
        // prevents multiple changes
        if (value == _isEnlarged)
        {
            return;
        }

        _isEnlarged = value;
        Size = _isEnlarged ? EnlargedSize : NormalSize;
        ResizePictureBox();
        TranslatePanel();

        // raise the event (using C# 6)
        if (IsEnlarged)
        {
            Enlarged?.Invoke(this, EventArgs.Empty);
        }
    }
}

Subscribe to the event during the initialization:

panel.Enlarged += Panel_Enlarged;

Handle the event:

private void Panel_Enlarged(object sender, EventArgs e)
{
    var otherPanels = Controls.OfType<ResizablePanel>().Where(rp => rp != sender && rp.IsEnlarged);
    foreach (var otherPanel in otherPanels)
    {
        otherPanel.IsEnlarged = false;
    }
}

  when a 'enlarged' image gets clicked, nothing happens

Just modify the PanelOrPictureBoxClick event handler like this:

IsEnlarged = true;


Is my lockless data structure correct? I need multiple read access to some data which can be updated from time to time.
I created a class for managing lock-less access to the data. The class uses 2 counters, one for count of the acquired references and another one for the count of released references. The update function replaces the data pointer and then waits for the completion of the previous reads.

Can someone with more experience in lock-less programming comment on the correction and performance of the code?

template<typename T>
inline void IgnorePolicy(T* item) {}
template<typename T>
inline void DeletePolicy(T* item)
{
    delete item;
}
template<typename T>
inline void ReleasePolicy(T* item)
{
    if(item != NULL)
        item->Release();
}

template<typename T, void (* DestroyPolicy)(T* item) = IgnorePolicy>
class ReadUpdateData
{
    private:
        volatile int m_acquireCount, m_releaseCount;
        T* m_data;

        bool CheckReleaseCount(int limit) //wrap-safe check to see if release count has reached the required limit
        {
            int releaseCount = m_releaseCount;
            return (limit < 0 || releaseCount >= 0) ? limit <= releaseCount : limit >= 0 && releaseCount < 0;
        }

    public:
        ReadUpdateData() : m_acquireCount(0), m_releaseCount(0), m_data(NULL) {}
        ~ReadUpdateData()
        {
            assert(m_acquireCount == m_releaseCount);
            DestroyPolicy(m_data);
        }

        T* Acquire()
        {
            AtomicIncrement(&m_acquireCount);
            return m_data;
        }
        void Release()
        {
            AtomicIncrement(&m_releaseCount);
        }

        void Update(T* newData)
        {
            T* oldData = m_data;
            while(AtomicCompareExchangePtr(&m_data, newData, oldData) == oldData)
                oldData = m_data;

            int acquireCount = m_acquireCount;
            while(!CheckReleaseCount(acquireCount))
                Yield();

            DestroyPolicy(oldData);
        }
};

I'm not an expert, but I this line caught my eye:

volatile int m_acquireCount, m_releaseCount;

m_acquireCount and m_releaseCount will be part of the same cache line!

Therefor I expect no performance advantages compared to the simpler AtomicIncrement/AtomicDecrement on one variable.


Filling up a numpy array using data from a list I am looking to optimize the nested for-loops shown in the code below.

I have a list, mixtures, that contains points that represent 1 Gaussian Mixture (GM) on each line. My goal is to run a function kl_divergence between all combinations of GM's (with i≠j) and store the results in an array PxP.

Note: The following mixtures list  is a simplified version of the actual list. In my original code, I am working with a lot more data.

import numpy as np
from sklearn.mixture import GaussianMixture

def kl_divergence(gmm_i, gmm_j, n_samples=10**5):
    X = gmm_i.sample(n_samples)
    log_p_i = gmm_i.score_samples(X[0])
    log_p_j = gmm_j.score_samples(X[0])
    return log_p_i.mean() - log_p_j.mean()

mixtures = [[[1, 3, 4, 7], [3, 5, 9, 2], [4, 3, 6, 1], [4, 3, 6, 3]],
            [[3, 4, 8, 2], [3, 6, 3, 7], [2, 6, 8, 4]],
            [[4, 8, 9, 3], [2, 6, 5, 8], [2, 5, 3, 5]],
            [[4, 9, 0, 2], [2, 4, 8, 3], [9, 8, 2, 3], [2, 6, 8, 3]]]
#Note: Each line has a different number of points

P_comp = len(mixtures)
PxP = np.zeros((P_comp, P_comp), dtype=int)

for row_i, GM_i in enumerate(mixtures):    #Loop through mixtures using i
    gmm_i = GaussianMixture(n_components=1).fit(GM_i)
    for row_j, GM_j in enumerate(mixtures): #Loop through mixtures using j
        if GM_i is not GM_j: #Skip if same mixture
            gmm_j = GaussianMixture(n_components=1).fit(GM_j)
            PxP[row_i, row_j] = kl_divergence(gmm_i, gmm_j) #Store result at index (row_i, row_j)

print("PxP\n", PxP)

The code to establish PxP should be in a function of its own, featuring a doc string and comments where necessary (why not use GaussianMixture(covariance_type ='spherical')?)
Same goes for kl_divergence(): when(/why?) is n_samples=10**5 appropriate?

From eyeballing your code, it would seem that you instantiate a GaussianMixture() not just once for each mixture, but (len(mixtures))²?  times.
Try to turn mixtures into GaussianMixture upfront: mixtures = [GaussianMixture().fit(mix) for mix in mixtures] (n_components=1 is default - do you need covariance_type='full'?).


Finding the position in a triangle for the given challenge The LAMBCHOP doomsday device takes up much of the interior of Commander Lambda's space station, and as a result
the prison blocks have an unusual layout. They are stacked in a triangular shape, and the bunny prisoners are given
numerical IDs starting from the corner, as follows:

| 7

| 4 8

| 2 5 9

| 1 3 6 10

Each cell can be represented as points (x, y), with x being the distance from the vertical wall, and y being the height from the ground.

For example, the bunny prisoner at (1, 1) has ID 1, the bunny prisoner at (3, 2) has ID 9, and the bunny prisoner at (2,3) has ID 8. This pattern of numbering continues indefinitely (Commander Lambda has been taking a LOT of prisoners).

Write a function answer(x, y) which returns the prisoner ID of the bunny at location (x, y). Each value of x and y will be at least 1 and no greater than 100,000. Since the prisoner ID can be very large, return your answer as a  string representation of the number.

Here is my solution:

y = int(input())
x = int(input())
sum_in_x = 1
for i in range(x):
    sum_in_x = sum_in_x + range(x)[i]
    in_sum =i + 2
sum_in_y = sum_in_x
for j in range(y - 1):
    sum_in_y = sum_in_y + in_sum
    in_sum += 1
print(sum_in_y)

It works and generates the needed solutions, but I am trying to understand if there are better ways of doing this.

Your current algorithm is very inefficient. I would use the following algorithmic approach:

def answer(x, y):
    y_diff = y - 1
    corner = x + y_diff
    id = corner * (corner + 1) // 2
    id -= y_diff
    return str(id)

This method takes advantage of basic algebra. It uses the sum of the arithmetic progression of x to calculate the id of the bottom right corner, and then subtracts the difference of the input coordinate from the bottom right corner. The reason it take the y difference is because moving 45 degrees diagonally down right is equivalent to moving down (y-1) units and moving right (y-1) units. Is is much more efficient than your current method: instead of an increasing numbering of iterations through range, it does one multiplication.

Benchmark

Testing your code compared to mine, mine runs about 10,000 to 100,000 times faster for random values of x and y between 1 and 100000. This is a very significant performance difference.

Additionally, in benchmarking, I discovered a slight bug in your current implementation. I didn't analyze your implementation deeply to understand it, but it seems that your current implementation returns an output corresponding to the reversed inputs (so for example, if I input (3, 2), the output is correct for (2, 3).) I suspect this may have been missed in your personal testing because the input is reversed of traditional coordinate order, i.e. you ask for y coordinate input before you ask for x coordinate input. I fixed this in my testing by switching all occurrences of x (and related variables) with y and vice versa.

Some notes

  Write a function answer(x, y) which returns the prisoner ID of the bunny at location (x, y).

So it seems your answer should be in the form:

def answer(x, y):
    # ... algorithm
    return str(output)

Also, use consistent spacing:

in_sum =i + 2 to in_sum = i + 2

And take advantage of +=:

sum_in_x = sum_in_x + range(x)[i] to sum_in_x += range(x)[i]
sum_in_y = sum_in_y + in_sum to sum_in_y += in_sum


Parse copied data from Excel using AutoHotkey Code Objective

I'm writing an AutoHotkey script which takes in department data copied to the clipboard from a Microsoft Excel spreadsheet. It then uses this data to auto-fill forums in the web-app Kronos Workforce Central.

Source Data

When data is copied from Microsoft Excel, each cell in a row is stored in the clipboard as strings delimited by tabs \t, with each row separated by newlines \r\n.

DeptID     Job Name           Abbreviation     Service Line
0368       Administrator      ADMIN            OPS
3945       Programmer         PRGRM            NON NRSG
4596       Software Engineer  SFTWRE-ENG       NON NRSG

Current Method

Currently, I'm parsing this copied-data by splitting the data into an array of rows with StrSplit(), then using a second StrSplit() inside a for-loop to parse my data.

; Autofill data from clipboard
#d::
   addLocationsWindow := "Add Locations"
   WinActivate, %addLocationsWindow%        ; Window MUST be active

   rowArray := StrSplit(Clipboard, "`r`n")  ; Split copied rows from Excel by newlines
   for index, row in rowArray {
      tempArray := StrSplit(row, "`t")      ; Split each row by tabs
      deptID := tempArray[1]
      jobName := tempArray[2]

      If (deptID == "")                     ; skip empty cells
         continue

      IfWinNotActive(addLocationsWindow)  ; check for active window
      {
         MsgBox '%addLocationsWindow%' window not found. Stopping script...
         Exit, 1
      }

      ; DoStuff(jobName, deptDisplayName, index)
   }
Return

The Issue

My method of string parsing feels hacky and unintuitive. It means I can only manipulate a single row at a time, as each each row's data is only split temporarily. I would need to re-run the costly StrSplit() to access data from a row again.

In Java, I could store my Excel data in a single 2D array. Unfortunately, arrays are a bit tricky to use in AutoHotkey, which is why my current code is a bit messy.

I realize this is an older question, but hopefully, this will help a future asker, if not the original asker.

I agree with your assessment of using StrSplit() and would recommend using a 2D array. This can be done using a nested parsing loop. Here is a working example of the data in your example (assuming the range was copied from Excel and not from your post):

f1::
aData := {} , nCt := 0 , clipboard := RegExReplace( clipboard , "`r" , "" )
Loop , Parse , clipboard , `n
{
    nCt++
    Loop , Parse , A_LoopField , `t
        aData[ nCt , A_Index ] := A_LoopField
}
Loop , % aData.Length()
    MsgBox , % aData[A_Index,1] . "`t" . aData[A_Index,2] . "`t" . aData[A_Index,3]
Return


Does my Rectangle intersect code work? I just want to see if any part of one "rectangle" overlaps the other.
Is this the correct code?

 bool checkCollide(int x, int y, int oWidth, int oHeight, int x2, int y2, int o2Width, int o2Height){
    bool collide;
    collide = false;

    if(x >= x2 && x <= x2+o2Width && y >= y2 && y <= y2+o2Height){
        collide = true;
    }

    if(x+oWidth >= x2 && x+oWidth <= x2+o2Width && y >= y2 && y <= y2+o2Height){
        collide = true;
    }

    if(x >= x2 && x<= x2+o2Width && y+oHeight >= y2 && y+oHeight <= y2+o2Height){
        collide = true;
    }

    if(x+oWidth >= x2 && x+oWidth <= x2+o2Width && y+oHeight >= y2 && y+oHeight <= y2+o2Height){
        collide = true;
    }
    return collide;
}

No, this is not correct; it only verifies whether the vertices of one rectangle are inside the other, not the other way around. Try calling it with the following parameters:

checkCollide(2,2,4,4, 1,3,2,2);
checkCollide(1,3,2,2, 2,2,4,4);

They should print the same result, but they don't. This case can be seen in the picture below.

Update: even if you repeat the checks changing x/x2, y/y2, etc, this still won't catch all the cases: two rectangles may intersect even if none of their vertices are inside the other rectangle - see the other picture below.

A more generic solution should use a segment intersection routine to check whether for all segments in one rectangle whether they intersect with any the segments on the other one.


Google homepage beginner project I was hoping to get some feedback on my basic recreation of the google homepage. Im doing the Odin Project learning course and this was the first project trying to use the basics of HTML/CSS

HTML file:

    <!DOCTYPE html>

    <title>Google</title>
    <link rel="stylesheet" href="style.css">

        About
        Store

        Gmail
        Images
        Apps
        <button type="button" name="button" id="signin">Sign In</button>

      <input type="text" name="userinput" id="userinput">

      <button type="button" id="searchbutton">Google Search</button>
      <button type="button" id="searchbutton">Im Feeling Lucky</button>

        Advertising
        Business

        Privacy
        Terms
        Settings

CSS file:

    #userinput{
  height: 30px;
  width: 500px;
  border-radius: 24px;
  box-shadow:5px 10ox;
}

#signin{
  background-color: blue;
  color: white;
  border-radius: 3px;
  border: 1px solid blue;
}

#searchbutton{
  margin-top: 20px;
  opacity: .60;
  padding: 0 12px;
}

.right{
  float: right;
}

.left{
  float: left;
}

.middle{
  margin-left: 130px;
  padding-top: 200px;
}

button{
  line-height: 28px;
}

body{
  position: relative;
  text-align: center;
  font-size: 14px;
}

footer{
  padding-top: 200px;
}

a{
  color: black;
  text-decoration: none;
}

div{
  word-spacing: 40px;
  display: inline-block;
}

Your code is good, but I have some suggestion to improve it.

HTML

You used semantic HTML for the header and footer, but for the main content you did not use any semantic html. If you use semantic HTML tags for your main content like main tag and section tag, it will be more interesting.

In the below picture you can see you followed everything except the main content.

CSS

You can order your CSS code. I would order by layout; first your body CSS, then your header CSS and then finally your footer CSS.
Alternately you can order by other things like alphabet.

body{
  position: relative;
  text-align: center;
  font-size: 14px;
}

a{
  color: black;
  text-decoration: none;
}

div{
  word-spacing: 40px;
  display: inline-block;
}

.right{
  float: right;
}

.left{
  float: left;
}

#signin{
  background-color: blue;
  color: white;
  border-radius: 3px;
  border: 1px solid blue;
}

.middle{
  margin-left: 130px;
  padding-top: 200px;
}

 #userinput{
  height: 30px;
  width: 500px;
  border-radius: 24px;
  box-shadow:5px 10ox;
}

button{
  line-height: 28px;
}

#searchbutton{
  margin-top: 20px;
  opacity: .60;
  padding: 0 12px;
}

footer{
  padding-top: 200px;
}


Find all concatenated words in a given list of words This is a Leetcode problem -

  Given a list of words (without duplicates), write a program that
  returns all concatenated words in the given list of words.

  A concatenated word is defined as a string that is comprised entirely
  of at least two shorter words in the given array.

  Note -

  The number of elements of the given array will not exceed 10,000.
  The length sum of elements in the given array will not exceed 600,000.
  All the input string will only include lower case letters.
  The returned order of elements does not matter.

Here is my solution to this challenge (DFS solution) -

def find_all_concatenated_words_in_a_dict(words):
    """
    :type words: List[str]
    :rtype: List[str]
    """
    words = set(words)
    output = []
    for w in words:

        if not w:
            continue

        stack = [0]
        seen = {0}
        word_len = len(w)

        while stack:
            i = stack.pop()
            if i == word_len or (i > 0 and w[i:] in words):
                output.append(w)
                break
            for x in range(word_len - i + 1):
                if w[i: i + x] in words and i + x not in seen and x != word_len:
                    stack.append(i + x)
                    seen.add(i + x)
    return output

Here is an example output -

#print(find_all_concatenated_words_in_a_dict(["cat", "cats", "catsdogcats", "dog", "dogcatsdog", "hippopotamuses", "rat", "ratcatdogcat"]))

>>> ['catsdogcats', 'ratcatdogcat', 'dogcatsdog']

Explanation -

"catsdogcats" can be concatenated by "cats", "dog" and "cats";

"dogcatsdog" can be concatenated by "dog", "cats" and "dog";

"ratcatdogcat" can be concatenated by "rat", "cat", "dog" and "cat".

Here is the time taken for this output -

%timeit find_all_concatenated_words_in_a_dict(["cat", "cats", "catsdogcats", "dog", "dogcatsdog", "hippopotamuses", "rat", "ratcatdogcat"])
41.1 µs ± 2.93 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

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

I took me a while to understand your code since I didn't know from the beginning what each variable really meant:

i is probably an int, maybe an index
x is probably a number (maybe even a float), or something unknown, or a placeholder
seen is a set of something, and this something could really be anything
stack will only be used by calling append and pop, but what does it contain? ints, strings, complex objects? The name stack by itself doesn't give any clue.
w could also be spelled word, but that one is already the best of the variable names

Therefore, to really understand what each variable stands for, I had to run your code using only pen and paper, which was a good exercise since I really don't do that often.

By doing that I noticed:

the only purpose of the if not w: continue is to prevent the empty word from being output
the only purpose of the w[i:] in words is to speed up the implementation; it is not necessary for the pure algorithm
the expression i + x appears several times. I wondered whether the Python runtime would be able to apply common subexpression elimination to it, or whether it would make the program faster if that expression were saved into a separate variable
I wanted to replace the x != word_len with the simpler i != 0, but I quickly noticed that this would break the whole algorithm
using a stack is great because the larger string indices are pushed at the end, which means they are popped and checked first, which speeds up the program. At least in the catsdogcats case
the range should not start at 0, but at 1, since for the empty word, seen[i + x] is always True
the code is really efficient

After finishing the manual analysis, each and every little expression made sense. I didn't discover anything superfluous. Therefore: nice work. You should make the variable names a little more suggestive though.

i could be left
i + x could be right
x could be subword_len
stack could be indices_to_test
seen could be seen_indices

One idea I had during the analysis was that the code might become easier to understand if you used a nested function:

def find_all_concatenated_words_in_a_dict(words: List[str]) -> List[str]:
    words = set(words)

    def is_concatenated(word: str) -> bool:
        ...
        return True

    return [word for word in words if word and is_concatenated(word)]

After executing your algorithm using pen and paper, I thought that another implementation might be even more efficient, and maybe lead to shorter code. My idea was:

def is_concatenated(word: str) -> bool:
    word_breaks = [True] + [False] * len(word)

    for start in range(len(word_breaks)):
        if word_breaks[start]:
            for sub_len in range(1, len(word_breaks) - start):
                if sub_len != len(word) and word[start:start + sub_len] in words:
                    word_breaks[start+sub_len] = True

    return word_breaks[-1]

This code also starts at the beginning of the long word and marks all reachable word breaks. It uses fewer variables though.

The code can be further optimized:

for the sub_word length, only iterate over the lengths that actually appear in the word set
return early as soon as word_breaks[-1] becomes True

But even with these optimizations, the time complexity stays at \$\mathcal O({\text{len}(\textit{word})}^3)\$, which is quite much when \$\text{len}(\textit{word})\$ can be up to 10_000. Space complexity is \$\mathcal O(\text{len}(\textit{word}))\$, and the initial part of the word_breaks[:start] list could be thrown away early.

The \$n^3\$ is because of the 2 nested for loops, and deeply nested in these loops is a string comparison of the subword, which also depends on \$\text{len}(\textit{word})\$.

I'd rather have an \$\mathcal O(n^2)\$ or even \$\mathcal O(n)\$ algorithm though. I just don't know whether one exists.


Writing strcat (string concatenate) in C char *cat_string(char *to_cat, char *destination) {
  char *copy = destination;
  while (*copy != '\0') {
    *copy++;
  }
  while (*to_cat != '\0') {
    *copy++ = *to_cat++;
  }
  *copy = '\0';
  return destination;
}

I would like to know if this is an efficient and effective way of writing this function. It seems silly to have to iterate through the whole array just to find the \0 character.

Also, should I be returning the pointer to the destination? What's the best practice there?

You renamed strcat() to cat_string(), so I suppose it's fitting that you reversed the traditional order of the parameters as well.  ☺

I would make three changes:

Changing the first argument to const helps prevent users notice if they call the function with the parameters swapped (but doesn't prevent it in all cases).
You have a stray pointer dereference.  It was misleading, but luckily harmless.  (In the first while loop, you just want to advance the copy pointer.  You don't care about what value it points to as you advance; you only care when you check for the NUL terminator.)
You can remove a statement just by changing the loop structure.
Edit (Rev 3): Previous recommendation was buggy, and has been retracted.  Credit to @MarcvanLeeuwen.

To answer your questions…

The only way to find out where to start appending is to walk the entire destination string to locate where it ends.  That's just how C-style strings work.
Yes, returning destination seems appropriate.  It's analogous to what strcat() does.

One final note: two spaces for indentation is too stingy, in my opinion.  It's insufficient for readability, and it also encourages inappropriately deep nesting (which would be a symptom of poorly organized code).

char *cat_string(const char *to_cat, char *destination) {
    char *copy = destination;
    while (*copy != '\0') {
        copy++;                 /* Removed superfluous dereference. */
    }
    …
}


Get local datetime from UTC time One of the app is inserting date in UTC format (column CreateDate). When reporting from this table, I used a difference in hours between the inbuilt GetUTCDate() and GetDate() and added the same to the CreateDate column. I was hoping to not worry about the daylight time. Is this approach OK?

SELECT GetUTCDate()
SELECT GetDate()
DECLARE @DifferenceInHoursBetween INT
SELECT @DifferenceInHoursBetween = DATEDIFF(HH,GetUTCDate(), GETDATE())
SELECT  USER_ID, CreateDate AS [DateTime IN UTC], DATEADD(HOUR,@DifferenceInHoursBetween,CreateDate) AS [LocalDateTime] FROM EVENTLOG(nolock)

/Tests/

SELECT GetUTCDate() AS [UTCDateTime]
SELECT GetDate() AS [LocalDateTime]
DECLARE @DifferenceInHoursBetween INT
SELECT @DifferenceInHoursBetween = DATEDIFF(HH,GetUTCDate(), GETDATE())
SELECT @DifferenceInHoursBetween AS [Difference in Hours]

-- UTCDateTime

2019-01-22 16:34:46.943

--LocalDateTime

2019-01-22 08:34:46.943

-- Difference in hours
-8

Review

You don't need to calculate the difference from two temporary values GetUTCDate and GETDATE.

SELECT @DifferenceInHoursBetween = DATEDIFF(HH,GetUTCDate(), GETDATE())

Proposed Solution

Instead, you could apply the local time zone offset to the specified datetime.

SELECT
    USER_ID,
    CreateDate AS [DateTime IN UTC],
    CONVERT(datetime,
        SWITCHOFFSET(CONVERT(datetimeoffset, CreateDate),
            DATENAME(TzOffset, SYSDATETIMEOFFSET()))) AS [LocalDateTime]
FROM EVENTLOG(nolock);

Fiddle: SQL Server 2017

Daylight Savings Time

Both methods, OP and solution don't take into account DST. And unfortunately, SQL Server does not come with a built-in conversion from DST. You'd have to make a function yourself. More about DST in SQL Server


Compile-time sieve of Eratosthenes There are many instances of prime number sieve implementation both here and other places on the web, but I wanted something a little different.  In particular, I wanted to create a static array of the first 1024 prime numbers at compile time as a simple reusable structure.  I also wanted to allow for relatively simple creation of larger or smaller arrays.

I combined the code from this question with a little bit of macro magic from this other question to come up with code that meets my goal.

Questions:

I'd like to be able to use something more elegant than the stack of macros (templates perhaps), and would prefer to have the result be something more like an STL container than a raw array.  Comments on how to do those things, or comments generally to clean up the code would be welcome.

primeconsttest.cpp

#include "primeconst.h"
#include <vector>
#include <iterator>
#include <iostream>

int main() {
    std::vector<int> prime{std::begin(primes), std::end(primes)};
    for (unsigned i=0; i < prime.size(); ++i)
        std::cout << i << '\t' << prime[i] << '\n';
}

primeconst.h

extern const int primes[1024];

primeconst.cpp

constexpr bool isPrimeLoop(int i, int k) {
    return (k*k > i)?true:(i%k == 0)?false:isPrimeLoop(i, k + 1);
}

constexpr bool isPrime(int i) {
    return isPrimeLoop(i, 2);
}

constexpr int nextPrime(int k) {
    return isPrime(k)?k:nextPrime(k + 1);
}

constexpr int getPrimeLoop(int i, int k) {
    return (i == 0)?k:
        (i % 2)?getPrimeLoop(i-1, nextPrime(k + 1)):
        getPrimeLoop(i/2, getPrimeLoop(i/2, k));
}

constexpr int getPrime(int i) {
    return getPrimeLoop(i, 2);
}

static_assert(getPrime(511) == 3671, "computed incorrectly");

#define K(x) J(x) J(x + 512)
#define J(x) I(x) I(x + 256)
#define I(x) H(x) H(x + 128)
#define H(x) G(x) G(x +  64)
#define G(x) F(x) F(x +  32)
#define F(x) E(x) E(x +  16)
#define E(x) D(x) D(x +   8)
#define D(x) C(x) C(x +   4)
#define C(x) B(x) B(x +   2)
#define B(x) A(x) A(x +   1)
#define A(x) getPrime(x) ,

extern constexpr int primes[] = { K(0) };

Output

0   2
1   3
2   5
3   7
4   11
5   13
6   17

...

1017    8093
1018    8101
1019    8111
1020    8117
1021    8123
1022    8147
1023    8161

I believe I can help with these things:

Obliterate macros
Obliterate raw arrays

Macros can be replaced with std::integer_sequence (C++14), and raw arrays can be replaced with std::array (C++11).

#include <array>
#include <utility>

template <typename T, T... Is>
constexpr auto gen_primes_helper(std::integer_sequence<T, Is...>) {
    return std::array<T, sizeof...(Is)>{{getPrime(Is)...}};
}

// T: integer type
// N: number of elements
// return: std::array<T,N>
template <typename T, T N>
constexpr auto gen_primes() {
    return gen_primes_helper(std::make_integer_sequence<T,N>());
}

constexpr auto primes = gen_primes<int,1024>();

However, we have a problem: Compilers currently implement std::make_integer_sequence in O(N) complexity.

GCC limits N to 900, unless you add the -ftemplate-depth= compiler option. See this bug.

Clang limits N to a pathetic 256, but also provides the same -ftemplate-depth= option.

To solve the problem, std::make_integer_sequence can be implemented in O(log(N)). I have done this as follows:

#include <array>
#include <utility>

namespace logseq {

// alias for std::integer_sequence, for brevity
template <typename T, T... Is>
using intseq_t = std::integer_sequence<T, Is...>;

template <typename A, typename B>
struct concat;

// A: intseq_t
// B: intseq_t
// return: typename intseq_t
//
// Example:
//   concat_t< intseq_t<int,0,1,2>, intseq_t<int,0,1,2,3> >
//     => intseq_t<int,0,1,2,3,4,5,6>
template <typename A, typename B>
using concat_t = typename concat<A,B>::type;

template <typename T, T... As, T... Bs>
struct concat<intseq_t<T, As...>, intseq_t<T, Bs...>> {
    using type = intseq_t<T, As..., (sizeof...(As) + Bs)...>;
};

template <typename T, T N, typename = void>
struct logseq;

// T: integer type
// N: number of elements
// return: typename std::make_integer_sequence<T,N>
// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66059
template <typename T, T N>
using logseq_t = typename logseq<T,N>::type;

template <typename T, T N, typename>
struct logseq {
    using type = concat_t<logseq_t<T,N/2>,logseq_t<T,N-N/2>>;
};

template <typename T, T N>
struct logseq<T,N,typename std::enable_if<N==0>::type> {
    using type = intseq_t<T>;
};

template <typename T, T N>
struct logseq<T,N,typename std::enable_if<N==1>::type> {
    using type = intseq_t<int,0>;
};

} // namespace logseq

template <typename T, T... Is>
constexpr auto gen_primes_helper(std::integer_sequence<T, Is...>) {
    return std::array<T, sizeof...(Is)>{{getPrime(Is)...}};
}

// T: integer type
// N: number of elements
// return: std::array<T,N>
template <typename T, T N>
constexpr auto gen_primes() {
    using logseq::logseq_t;
    return gen_primes_helper(logseq_t<T,N>());
}

constexpr auto primes = gen_primes<int,1024>();

Namespace logseq provides a metafunction logseq_t that returns a typename std::integer_sequence<T, 0, /* ... */, N-1>. It is a drop-in replacement for std::make_integer_sequence, which returns the same type.

logseq_t allows for infinite (read: really freaking big) values of N.


Fibonacci series - a different way I am a C++ student (1.5 months into it). Please give feedback to this different way I have thought of for Fibonacci series. If there are any improvements needed, please suggest them also.

#include<iostream>
using namespace std;
void fibseries(long int n)
{
    double x=0;double y=1;
    for (long int i=1;i<=n;i++)
     {
        if(i%2==1)
         {
            cout<<x<<" ";
            x=x+y;
         }
        else
         {
            cout<<y<<" ";
            y=x+y;
         }
     }
}
main()
{
    long int n=0;
    cout<<"The number of terms ";
    cin>>n;
    fibseries(n);
    return 0;
}

You should change this double x=0;double y=1; add newline after first ;
Using ++i is a best practice no matter if the type is int, so use ++i instead i++ and learn difference between them.
change main to int main, It is required to be standard compliant.
Why do you use doubles as x and y. Fibonacci numbers are always integers.
There are many fibonacci algorithms, you can look here

https://stackoverflow.com/questions/14661633/finding-out-nth-fibonacci-number-for-very-large-n

http://fusharblog.com/solving-linear-recurrence-for-programming-contest/


Finding non-bouncy numbers under 10^x I am trying to find non bouncy numbers under \$10^x\$ where \$1\le x\le 1000\$.
\$123456\$ is an increasing number where every digit is greater than or equal to the previous digit (\$1223344\$ is also counted as an increasing number).
Similarly a decreasing number is one where every digit is smaller than or equal to the previous digit (\$443322\$ is also counted as an decreasing number).

155224 such numbers, which are neither increasing, nor decreasing, are bouncy numbers, such that 12951 numbers below one-million that are not bouncy. I want to find number of non bouncy number under \$10^x\$.

My code works perfectly fine, but when \$n\$ is greater than 7, it takes way too much time to give the output! So, it needs performance optimizations.

import java.util.*;
class A{
static String s;
public static void main(String z[]){
    int n,i;
    Scanner sc=new Scanner(System.in);
    n=sc.nextInt();
    int input[]=new int[n];
    int output[]=new int[n];
    for(i=0;i<n;i++){
        input[i]=sc.nextInt();
    }
    for(i=0;i<n;i++){
        output[i]=noOfBouncy(input[i]);
    }
    for(i=0;i<n;i++){
        System.out.println(output[i]);
    }
}
public static boolean isIncreasing(String s){
     int len=s.length();
     for(int i=0;i<len-1;i++){
         if(s.charAt(i)>s.charAt(i+1)){
             return false;
         }
    }
return true;
}
public static boolean isDecreasing(String s){
     int len=s.length();
     for(int i=0;i<len-1;i++){
         if(s.charAt(i)<s.charAt(i+1)){
             return false;
         }
    }
return true;
}
public static int noOfBouncy(int k){
int limit=(int)Math.pow(10,k);
int count=0;
int num=1;
while(num<limit){
    s=new String(Integer.toString(num));
    if(isIncreasing(s) || isDecreasing(s) ){
    }
    else{
       count++;
    }
    num++;
  }
count=limit-count;
return count-1;
}
}

This is not a review but an expanded comment.

Brute force is bad. Do some math first.

Consider a non-decreasing number of length \$n\$ formed by  exactly \$k\$ given digits. It has exactly \$k - 1\$ transitions from a digit to a larger one, and there are exactly \$n - 1\$ places where the transition may happen, so there are \$\binom{n-1}{k-1}\$ such numbers. The \$\binom{10}{k}\$ ways to pick \$k\$ digits give a subtotal of \$\binom{n-1}{k-1}\binom{10}{k}\$, and a grand total is \$\sum_{k=1}^{10}\binom{n-1}{k-1}\binom{10}{k}\$.

So the problem nails down to efficient calculation of binomial coefficients, which in this case is fairly straightforward, since you never need more than 10 factors.


Optimizing Bingo Caller Method I am dong a competitive programming problem I have passed all the tests.
but my program times out. I need help optimizing my code so it can run faster.

  Task:

  Finish method:   BingoCaller.GetNumber()
  Return all numbers in the range of 1 until 75 once and in random    order
  If there are no numbers left, return an empty string
  The numbers are returned one by one in Bingo style:

  "I27", "N40", "B5", "B12", "I28", "O69", "B1", ...
  These are the ranges that you must follow:

  A number within range 1 to    15 starts with a 'B'
  A number within range 16 to 30 starts with an    'I'
  A number within range 31 to 45 starts with an 'N'
  A number within range 46 to 60 starts with a 'G'
  A number within range 61 to 75 starts with an 'O'

  Use System.Random to generate your random numbers. Pass via the    constructor for testing purposes.

My code below

       using System;
       using System.Collections.Generic;

       public class BingoCaller
        {

            private Random random;

            static List<int> List = new List<int>();

            public BingoCaller(Random random)
            {
                this.random = random;

            }

            public string GetNumber()
            {

            Start:

             var item = random.Next(75);

                if (List.Contains(item))
                {
                  goto Start;
                }
                else
                {
                    List.Add(item);

                    if (item >= 61)
                    {
                        return "O" + item;
                    }
                    else if (item >= 46)
                    {
                        return "G" + item;
                    }
                    else if (item >= 31)
                    {
                        return "N" + item;
                    }
                    else if (item >= 16)
                    {
                        return "I" + item;
                    }
                    else
                    {
                        return "B" + item;
                    }

                }

            }
        }

It is considered bad practice to use goto-statements in modern programming because it quickly gets hard to follow the structure, so avoid that. Instead you could use a while-loop in a kind of this manner:

while (list.Count < 75)
{
  var number = random.Next(75);
  if (list.Contains(number))
    continue;
    ...
}

var item = random.Next(75);

This statement sets item to a number in the interval of \$[0, 75[\$, but you were supposed to handle the interval \$[1, 75]\$

instead of if (item >= 61) it is more readable to write if (item > 60)

It seems that GetNumber() is missing a stop-condition. It will run forever between goto Start and Start: when the list is full. (That may be your "time out"-problem).

Instead of the long list of if-statements, you could use the simple relation between the prefix's position in the word "BINGO" and the random number to extract it as:

string bingo = "BINGO"; // Make it a class field.
int index = ...
char prefix = bingo[index]

(it has something to do with 15).

The pattern of filling a list with found numbers tends to be slower and slower in response time as the size of the list grows because the next random number is more and more likely to be in the list. Potentially a valid number may never be found...

A better approach could be to create the list of the 75 numbers in the constructor in advance and then remove them one by one from the list when found by random.Next():

...
var number = list[random.Next(0, list.Count)];
list.Remove(number);
...

This will decrease in response time and you avoid the looping.

A solution could be:

  public class BingoCaller
  {
    private Random random;
    List<int> list;
    string bingo = "BINGO";

    public BingoCaller(Random random)
    {
      this.random = random;
      list = Enumerable.Range(1, 75).ToList();
    }

    public string GetNumber()
    {
      if (list.Count == 0)
        return "";

      var number = list[random.Next(0, list.Count)];
      list.Remove(number);
      char prefix = bingo[(number - 1) / 15];
      return $"{prefix}{number}";
    }
  }


Video player UI in Javascript/JQuery I am a Javascript/JQuery beginner working on my first ever project. Everything in this program is working as it should and I am happy with its performance. I have pieced it together from a variety of sources and lots of trial and error.

Because this is so new to me I wanted some experienced opinions on the way I have used the various Javascript/JQuery components in my project. The code I have included here has been edited and simplified for clarity but in its entirety it is a working program so I'm not necessarily looking to have it rewritten but more looking for feedback on what I have done to see where I can improve my programming skills.

In particular am I following good programming protocols with my structure and layout of various elements and functions etc.?
Should I change, combine or separate components etc. to simplify coding whilst still allowing me to increase functionality should I want to extend the program further?
Are there better ways through HTML or otherwise to more efficiently achieve the outcomes I have noted in comments?

   <video id="videoElement" ><source src="video.mp4" > </video>

 <button on="" onclick="playVid();">Start video</button>

Click video to respond

This script mixes jQuery DOM lookups (e.g. $(".respond")) with native DOM lookups (e.g. document.getElementById('videoElement')). It is best to use only one style to avoid confusion (e.g. for anyone reading your code, including your future self). If you are going to load jQuery on the page then you might as well use it. Otherwise if you don't need it then it could reduce the page load time slightly if it is removed. You might find this article interesting.

Indentation is inconsistent, though maybe that was a mistake during insertion of the code into your post.

The first line in playVid() references videoElement which looks up the element by id attribute implicitly.

function playVid() {
    videoElement.play();

This can also confuse readers of your code. Because myVid points to that same element, that variable could be used instead.

function playVid() {
    myVid.play();


No more Fizzbuzz. How about Hello World instead? Write a program that prints the numbers from 1 to 100. But for multiples of three print “Fizz” instead of the number and for the multiples of five print “Buzz”. For numbers which are multiples of both three and five print “FizzBuzz”

It seems that we're shooting for one fizzbuzz question for each language. I'm sick of FizzBuzz, so I'm killing two birds with one stone. This code works in both vb6 and vba.

My goal here was to write a flexible Fizzbuzzer; one that could double as a Hello World program (of sorts). Is there anything left to say about it, or are we all FizzBuzzed out?

FizzBuzzer (Standard Module):

Option Explicit

Public Function Convert(ByVal number As Integer, Optional ByVal fizzDivisor As Integer = 3, Optional ByVal buzzDivisor As Integer = 5, Optional ByVal fizzWord As String = "Fizz", Optional ByVal buzzWord = "Buzz")

    If (number Mod fizzDivisor = 0) And (number Mod buzzDivisor = 0) Then
        Convert = fizzWord & buzzWord
    ElseIf (number Mod fizzDivisor = 0) Then
        Convert = fizzWord
    ElseIf (number Mod buzzDivisor = 0) Then
        Convert = buzzWord
    Else
        Convert = number
    End If

End Function

Example Calls:

Public Sub Main()

    Debug.Print "Standard Fizz Buzz"
    Dim i As Integer
    For i = 1 To 100
        Debug.Print (FizzBuzzer.Convert(i))
    Next i

    Debug.Print "Change the Numbers"
    For i = 1 To 100
        Debug.Print (FizzBuzzer.Convert(i, 4, 6))
    Next i

    Debug.Print "Change the Words"
    For i = 1 To 100
        Debug.Print (FizzBuzzer.Convert(i, fizzWord:="Hello", buzzWord:="World"))
    Next i

End Sub

Just a slight optimization: instead of evaluating the modulo twice just declare two booleans an evaluate them upfront.

Public Function Convert(ByVal number As Integer, _
                        Optional ByVal fizzDivisor As Integer = 3, _
                        Optional ByVal buzzDivisor As Integer = 5, _
                        Optional ByVal fizzWord As String = "Fizz", _
                        Optional ByVal buzzWord = "Buzz")

    Dim fizzState As Boolean
    fizzState = (number Mod fizzDivisor = 0)

    Dim buzzState As Boolean
    buzzState = (number Mod buzzDivisor = 0)

    If fizzState And buzzState Then
        Convert = fizzWord & buzzWord
    ElseIf fizzState Then
        Convert = fizzWord
    ElseIf buzzState Then
        Convert = buzzWord
    Else
        Convert = number
    End If

End Function

Also you may not want to include as many newlines in the arguments as I did but you don't want a line over 200 characters long or use a horizontal scroll-bar.


Secure upload script I am creating a social network that let's users upload a profile picture. I just want to know if this is a secure way of doing it. Thanks.
<?php

include 'includes/header.php';
include 'includes/form_handlers/settings_handler.php';

//$userPic = '';
$date_time = date('Y-m-d_H-i-s');

if(!empty($userLoggedIn)) {

    if (isset($_FILES['fileToUpload'])) {
        $errors= array();
        $file_name = $_FILES['fileToUpload']['name'];
        $file_size = $_FILES['fileToUpload']['size'];
        $width = 1500;
        $height = 1500;
        $file_tmp = $_FILES['fileToUpload']['tmp_name'];
        $file_type = $_FILES['fileToUpload']['type'];
        $tmp = explode('.',$_FILES['fileToUpload']['name']);
        $file_ext=strtolower (end ($tmp));

        $extensions = array( "jpeg", "jpg", "png", "gif");

        if(in_array($file_ext,$extensions)=== false){

            $errors[]="extension not allowed, please choose a JPEG or PNG file.";
        }

        if ($file_size > 8097152) {

            $errors[] = 'File size must be 2 MB';
        }

        if ($width > 1500 || $height > 1500) {

            echo"File is to large";
        }

        if(!$errors) {

            $userPic = md5($_FILES["fileToUpload"]["name"]) . $date_time . " " . $file_name;
            $profilePic = move_uploaded_file($file_tmp,"assets/images/profile_pics/" . $userPic);

            $file_path = "assets/images/profile_pics/" . $userPic;

            $stmt = $con->prepare("UPDATE users SET profile_pic = ? WHERE username = ?");
            $stmt->bind_param('ss', $file_path, $username);
            $stmt->execute();
            $stmt->close();

            header('Location: settings.php');
            exit();

        }
    }
} else {

    echo "Invalid Username";
}

?>

This is my personal opinion, but I'd say the following:

The code should be formatted, I'd personally look at PSR-12 as this standard should be followed when possible.
move_uploaded_file doesn't protect against directory traversal. You should use basename on $_FILES['fileToUpload']['tmp_name'] and some other forms of validation
Checking the file extension with if(in_array($file_ext,$extensions)=== false) doesn't prevent a user from uploading a malicious file they could for instance use a magic byte to trick the server into thinking it's a certain type of file. You should take a look at finfo and the first example on file upload
You're create an array of errors, currently that's being checked in an if statement and is then thrown away. If you aren't planning on using it you might be better just returning out of the function early rather than continuing execution.
Depending on how unique the filename should be you might want to use something like uniqid(mt_rand(), true)
move_uploaded_file will replace a file if it already exists, you might want to check that this exists before you overwrite an existing file. Depending on your naming solution it's very unlikely to occur but under high load for long periods of time this could happen more often than you think.
You're using UPDATE users SET profile_pic = ? WHERE username = ? I'd assume that this value exists in the database as the user needs to be logged in. However, if you aren't sure if the field exists or not (I haven't seen the database) I'd personally use: INSERT INTO users (profile_pic, username) VALUES (?,?) ON DUPLICATE KEY UPDATE profile_pic=?, username=? this will insert into the table if the row doesn't exist but will update it if it does.
You've set a local variable called width and height and are comparing them to the same value. I assume this was meant to check the actual file dimensions?

I hope this helps in some way :)


C++ smart pointer class #include <iostream>

template <typename T>
class my_smart_ptr {
    T* raw;
    unsigned * p_count;
public:
    my_smart_ptr(T* t):
    raw(t),
    p_count (new unsigned(1))
    {

    }

    my_smart_ptr(const my_smart_ptr<T> & p):
    raw(p.raw),
    p_count(p.p_count)
    {
        ++(*p_count);
    }

    const my_smart_ptr<T> & operator=(const my_smart_ptr<T>& p)
    {
        if (p == *this)
        {
            return *this;
        }

        raw = p.raw;
        --(*p_count);
        ++(*(p.p_count));
        return *this;

    }

    ~my_smart_ptr()
    {
        --(*p_count);
        if (*(p_count) == 0)
        {
            delete raw;
        }
    }
};

class A{
public:

    A()
    {
        std::cout<< "A()" << std::endl;
    }

    ~A()
    {
        std::cout<< "~A()" << std::endl;
    }

};
int main()
{
    my_smart_ptr<A> a (new A);
    return 0;
}

I suppose that your class is trying to simulate a shared_ptr, however, I'm missing the part of overloading the dereference methods * and ->, because without them, your class seems useless (I can't do much with the pointed object).


Hide the tree menu at a certain depth I have created this piece of js purely for learning purposes and I was hoping you could code review to see any mistakes / improvements I can make.

Basically I have a tree structure and I will use the depth variable to work out what level of the menu I will display.

Here is the code example: http://jsfiddle.net/nqGbw/5/

HTML

The Tree

 Root

 Folder 1

Folder 1.1

Folder 1.2

 Folder 2

Folder 2.1

 Folder 2.2

Folder 2.2.1

Folder 2.2.2

Folder 2.2.3

JS

var calcDepth = function (root) {
    var $parent = $(root).parents('ul');
    var depth = 0;
    while ($parent.length > 0) {
        $parent = $parent.parents('ul');
        depth += 1;
    }
    return depth;
};

var func = function (rootUL, maxDepth) {
    var selectedUL = rootUL;
    $(selectedUL.children('li')).each(function () {
        var Me = $(this);
        if (calcDepth(Me.parent()) == maxDepth) {
            Me.hide();
        }
        if (Me.children('ul').length > 0) {
            var selectedUL = Me.children('ul');
            func(selectedUL, maxDepth);
        }
    });
};
var depth = 2; //Change this to set the depth of the menu
var selectedUL = $("ul");
func(selectedUL, depth);

It would be much easier just to let the CSS selector engine select the elements to hide:

var hideDepth = function(root, depth) {
   root.find('ul').show(); // Make sure all sublists are visible
   var selector = Array(depth + 1).join(' > li > ul'); // Repeat string
   root.find(selector).hide();
}

var depth = 2;
var selectedUL = $("#tree");

hideDepth(selectedUL, depth);

NOTE: Add the id tree to the top level ul. Your selector $('ul')selects all lists in the document, not just the top one, so your code was unnecessarily called repeatedly for each sub list.

For the string repeating see: https://stackoverflow.com/questions/1877475/repeat-character-n-times.


Get data from user input as workbook or worksheet I had two different functions for populating a data array. I tried to refactor them using a single argument as to whether or not the user should be selecting a worksheet or just the workbook.

The primary issue being whether I need to prompt the user to select a range or not. The main difference between the two before was if using just a sheet I didn't need xlApp.

I came at the refactoring a little backwards, so I imagine it can be improved.

Option Explicit
Private Function ImportDataFromExternalSource(ByVal pickSheet As Boolean, Optional ByVal numberOfColumns As Long = 0) As Variant
    Dim lastRow As Long
    Dim fileName As String
    Dim xlApp As New Application
    Set xlApp = New Excel.Application
    Dim targetBook As Workbook
    Dim targetSheet As Worksheet
    Dim targetDataRange As Range
    On Error GoTo ErrorHandler
    fileName = File_Picker

    Set targetBook = xlApp.Workbooks.Open(fileName)
    Set targetSheet = targetBook.Sheets(1)

    If pickSheet Then
        xlApp.ActiveWorkbook.Windows(1).Visible = True
        xlApp.Visible = True
        targetBook.Activate
        targetBook.Sheets(1).Activate
        Set targetDataRange = xlApp.InputBox("Pick a cell on the sheet you would like to import", Type:=8)
        Set targetSheet = targetDataRange.Parent
    End If

    If numberOfColumns = 0 Then numberOfColumns = targetSheet.Cells(1, Columns.Count).End(xlToLeft).Column
    lastRow = targetSheet.Cells(Rows.Count, 1).End(xlUp).Row
    ImportDataFromExternalSource = targetSheet.Range(targetSheet.Cells(1, 1), targetSheet.Cells(lastRow, numberOfColumns))

CleanExit:
    If pickSheet Then
        xlApp.Quit
        Exit Function
    End If
    ThisWorkbook.Activate
    targetBook.Close
    Exit Function

ErrorHandler:
    MsgBox "you've cancelled"
    Resume CleanExit
End Function

Public Function File_Picker() As String
    Dim workbookName As String
    Dim selectFile As FileDialog
    Set selectFile = Application.FileDialog(msoFileDialogOpen)
    With selectFile
        .AllowMultiSelect = False
        .Title = "Select the file with your data."
        .Filters.Clear
        .Filters.Add "Excel Document", ("*.csv, *.xls*")
        .InitialView = msoFileDialogViewDetails
        If .Show Then File_Picker = .SelectedItems(1)
    End With
    Set selectFile = Nothing
End Function

I was using this little procedure to test -

Public Sub test()
    Dim x As Variant
    x = ImportDataFromExternalSource(False, 1)
    Debug.Print x(1)
End Sub

This is a cleaned-up version I would suggest:

Option Explicit

Private Function ImportDataFromExternalSource(ByVal boolPickSheet As Boolean, Optional ByVal lngNumberOfColumns As Long = 0) As Variant
    Dim lngLastRow As Long
    Dim strFileName As String
    Dim xlApp As New Excel.Application
    Dim wbkTarget As Excel.Workbook
    Dim shtTarget As Excel.Worksheet

    ' ask the user to select a file and try to open it with Excel
    Set xlApp = New Excel.Application
    Do While wbkTarget Is Nothing
        strFileName = FilePicker()
        If strFileName = "" Then Exit Function ' user requested abort
        On Error Resume Next
            Set wbkTarget = xlApp.Workbooks.Open(strFileName)
            If Err <> 0 Then MsgBox "An error occurred while opening the file" & vbNewLine _
                                   & strFileName & vbNewLine _
                                   & vbNewLine _
                                   & Err.Description _
                                   , vbCritical
        On Error GoTo 0
    Loop
    Set shtTarget = wbkTarget.Sheets(1)

    ' let the user select the sheet to import from
    If boolPickSheet Then
        xlApp.Visible = True
        Set shtTarget = xlApp.InputBox("Pick a cell on the sheet you would like to import", Type:=8).Parent
    End If

    ' determine the data that should be imported
    If lngNumberOfColumns = 0 Then lngNumberOfColumns = shtTarget.Cells(1, Columns.Count).End(xlToLeft).Column
    lngLastRow = shtTarget.Cells(Rows.Count, 1).End(xlUp).Row
    ImportDataFromExternalSource = shtTarget.Range(shtTarget.Cells(1, 1), shtTarget.Cells(lngLastRow, lngNumberOfColumns)) ' convert Range to Array

    ' cleanup
    wbkTarget.Close False
    xlApp.Quit
End Function

Public Function FilePicker() As String
    With Excel.Application.FileDialog(msoFileDialogOpen)
        .AllowMultiSelect = False
        .Title = "Select the file with your data."
        .Filters.Clear
        .Filters.Add "Excel Document", ("*.csv, *.xls*")
        .InitialView = msoFileDialogViewDetails
        If .Show Then FilePicker = .SelectedItems(1)
    End With
End Function

Being called like this:

Public Sub Test()
    Dim x As Variant

    x = ImportDataFromExternalSource(False, 1)
    If IsEmpty(x) Then
        MsgBox "you've cancelled", vbInformation
    Else
        Debug.Print x(1)
    End If
End Sub

If you are interested in the reasons behind the changes I made, I can provide some detailed information if you like.

Issues that still needs to be addressed

It would be preferable to decouple the user interaction (file and sheet selection) from the import logic, known as the Model-view-controller pattern. Have one function asking the user for the file and (optionally) the sheet, returning a Worksheet object. Now you can feed that object to another function that retrieves the array from the given sheet.

This way you are more flexible if, for example, you decide at one point that the sheet should also be able to be automatically retrieved by some other means. Just implement that in a way that it returns a Worksheet object, and you are good to go.
The way you determine the last cell in the sheet will fail in some circumstances (e.g. if the sheet is empty). Have a look at https://stackoverflow.com/a/11169920/6216216 for a thourough discussion of more robust alternatives.

Notes on Error Handling

You switched error handling on at the beginning of the function - as it is often recommended. There is no good way to do proper error handling in VBA (one of VBA's bigger flaws), but I thoroughly dislike the variant of catching all errors and handling them in one monolithic error handling routine at the end of the function/sub.

This is why:

The handling of an error gets disconnected from the origin of the error. This makes the code harder to read and easier to mess up during maintanance. Furthermore, debugging becomes a pain.
You may cast too broad a net, you might catch errors that you didn't want to catch.

Especially the second issue can become a serious problem when you start handling unexpected errors the same way as expected errors, since the error handling routine has no way to tell them apart.

Exactly this happened in your original code: If you call ImportDataFromExternalSource(False) and you cancel the file picker, fileName contains the empty string "". Trying to open that as an Excel workbook will fail and the error handler is called. This seems to be expected, because a message is displayed and the program gracefully terminated by jumping to the CleanExit label. During cleanup however, targetBook.Close is called, which will fail, because there is no targetBook, and an unexpected error is raised. Since your error handler cannot distinguish this from the expected error, the same routine is called: the messagebox is displayed again, the code continues with the CleanExit label and... now you are stuck in an infinite loop, because targetBook.Close will again raise an error.

The paradigm I followed is to apply the error handler to as litte code as possible, that is, the statements that I expect to fail. In this case, that is the Workbooks.Open function. The error handling code follows directly afterwards. If the handling code is more complex you should move it after the On Error GoTo 0, otherwise errors occuring in that code would just be silently ignored.

Do the same to other parts of the function where you expect errors (generally speaking; there aren't any in this code), and you have a clean separation of error handling for expected and unexpected errors: Expected errors are handled right away where they occurred, unexpected errors are bubbled up the calling stack until some code handles them that is better suited to do so. The worst case would be a runtime error that is displayed to the user indicating what exactly went wrong, but that would be preferable in most cases to exhibiting undefined behavior (as the infinite loop in your code).

This way your code becomes more robust, fast-failing if something went wrong instead of hiding the error and resulting in arbitrary consequences.