Locating contact information from a CSV spreadsheet I'm trying to find a more effective way to access variables within for loops. I felt that my attempt which works is not an effective way to access variables that are within for loops.

mylist=None is the variable in which I want to access within the for loop.

Also, I want to find a way to minimize the lines of my code either using functions or other suggested approaches and etc.

import csv
import os
from var_dump import var_dump
from orderedset import OrderedSet

# if phone or websites dont appear on spreadsheet change the row indexs

fil = 'newfiles/output8.csv'
with open(fil) as csvfile:
    readCSV = csv.reader(csvfile, delimiter=",")
    websites = OrderedSet()
    phonenumbers = OrderedSet()
    emails = OrderedSet()
    locations = list()
    mylist = None
    data = ["electric", "electricians"]
    # remove urls that have these strings,  only works with the first string
    # but not second one.
    remo = ["listings", "nationwide", "/mip", "car", "/locations", "/YB", "?utm_source", "/locallocksmithnearby",
            "key.me", "find-locksmiths-near-me", "premium-locksmiths", "locksmithnearyou24hrs"]
    for row in readCSV:
        website = row[1].strip('[]\'')
        email = row[0].lower().strip('[]\'')
        phonenumber = row[2]
        location = row[3]
        if location:
            if website not in websites:
                if phonenumber not in phonenumbers:
                    if email not in emails:
                        for x in data:
                            if x in website:
                                for r in remo:
                                    if all(r not in website for r in remo):
                                        if phonenumber not in phonenumbers:
                                            if email not in emails:
                                                websites.add(website)
                                                phonenumbers.add(phonenumber)
                                                emails.add(email)
                                                locations.append(location)
                                                mylist = zip(emails, websites, phonenumbers, locations)

    if mylist is not None:
        number = 0
        while os.path.exists('newfiles/newoutput%s.csv' % number):
            number += 1
        with open('newfiles/newoutput%s.csv' % number, 'w') as csvfile:
            writer = csv.writer(
                csvfile, delimiter=",")
            writer.writerow(
                ['Email', 'Website', 'Phone Number', 'Location'])
            # var_dump(mylist)
            for i in mylist:
                writer.writerow(list(i))

You would do well to use generator functions to break this down into parts that might be re-usable for other steps in your pipeline.

For example, the first thing you do in your loop is read a row from the CSV file and process some of the fields. That's a good candidate, right there:

def normalized_csv_rows(reader):
    """Filter CSV data, cleaning up email & web fields.        """
    for row in reader:
        email, web, phone, locn = row
        email = email.lower().strip("[]'")
        web = web.strip("[]'")
        yield email, web, phone, locn

The next obvious thing I see is that you require three of your fields to be unique. That's another good candidate for a filtering generator:

def require_unique_field(num, reader):
    seen = set()
    for row in reader:
        field = row[num]
        if field not in seen:
            seen.add(field)
            yield row

You can stack these up to require several unique fields. Finally, you require the location field to be "truthy", which generally means non-empty. So let's go with that:

def require_nonempty_field(num, reader):
    for row in reader:
        if row[num]:
            yield row

With these tools in hand, let's look at your for loop:

for row in readCSV:
    website = row[1].strip('[]\'')
    email = row[0].lower().strip('[]\'')
    phonenumber = row[2]
    location = row[3]
    if location:
        if website not in websites:
            if phonenumber not in phonenumbers:
                if email not in emails:
                    for x in data:
                        if x in website:
                            for r in remo:
                                if all(r not in website for r in remo):
                                    if phonenumber not in phonenumbers:
                                        if email not in emails:
                                            websites.add(website)
                                            phonenumbers.add(phonenumber)
                                            emails.add(email)
                                            locations.append(location)
                                            mylist = zip(emails, websites, phonenumbers, locations)

The first few lines are taken care of by our normalization function:

reader = readCSV
reader = normalized_csv_rows(reader)

The if location: is our require_nonempty_field function, with a field number of 3:

reader = require_nonempty_field(3, reader)

The lines like if email not in emails: (which occurs twice - I hope somebody was drunk for St. Patty's day) can be replaced with our require_unique_field function:

reader = require_unique_field(0, reader)
reader = require_unique_field(1, reader)
reader = require_unique_field(2, reader)

So we can rewrite the top part like this:

EMAIL, WEBSITE, PHONE, LOCATION = 0,1,2,3

data = ["electric", "electricians"]
# remove urls that have these strings,  only works with the first string
# but not second one.
remo = ["listings", "nationwide", "/mip", "car", "/locations", "/YB", "?utm_source", "/locallocksmithnearby",
        "key.me", "find-locksmiths-near-me", "premium-locksmiths", "locksmithnearyou24hrs"]
fil = 'newfiles/output8.csv'

with open(fil) as csvfile:
    reader = csv.reader(csvfile, delimiter=",")
    reader = normalized_csv_rows(reader)
    reader = require_nonempty_field(LOCATION, reader)
    reader = require_unique_field(EMAIL, reader)
    reader = require_unique_field(WEBSITE, reader)
    reader = require_unique_field(PHONE, reader)

Now, however, we come to the tricky part. You have a list of "must contain" items, data. But that list includes one word that is a prefix of another word.

If electricians is part of website, then electric is also part of website, since electric is a substring of electricians. The result is that this will tend to cause those rows containing electricians to be emitted twice. I suspect this is why you have two copies of the if email in emails: conditionals included in the stack.

A better idea would be to pre-process the filter words, to eliminate the longer word:

data.sort(key=len)
data2 = []
for word in data:
    for prefix = data2:
        if word.startswith(prefix):
            break
    else:
        data2.append(word)
data = data2
del data2

Once we have a "clean" list, you can write another generator function. Better still, we can make the cleanup part of the generator function, to
make sure the user doesn't give us bad data:

def field_contains(num, strings, reader):
    """Read rows from reader and pass those which have any member of
    strings as a substring of field num.

    """
    strings_by_len = sorted(strings, key=len)
    substrings = []
    for word in strings_by_len:
        for prefix in substrings:
            if word.startswith(prefix):
                break
        else:
            substrings.append(word)

    for row in reader:
        field = row[num]
        if any(s in field for s in substrings):
            yield row

And you can add that to the constraints on the reader:

reader = field_contains(WEBSITE, data, reader)

Similarly, you have a "blocking" list, where some substrings must not be in the website field. This is a similar function to the last, except you want to yield the row when the condition fails:

if not any(s in field for s in substrings):
    yield row

Then add that to the constraints:

reader = field_does_not_contain(WEBSITE, remo, reader)

At this point, reader is a stack of generators that will only yield the rows you want. However, not a single one of them has been read! So you can just pass this in to your output code:

writer.writerows(reader)

Please note, however, that it's important to keep this nested below the with open(fil) statement, since none of the data has been read in! The structure should look like:

with open(fil) ...:
    reader = ...
    with open(whatever) as outputcsv:
        writer = ...
        writer.writerows(reader)


Is the LINQ version faster than the foreach one? Since I don't know how LINQ works under the hood, I can't decide what version is best to use in term of rapidity of execution. I've done some testing with my testing data (Point Cloud) but I can't see a clear difference between the 2. The only thing I know is that the real life data will be a larger Point Cloud so my guess is that the LINQ would be faster but this is only if LINQ doesn't do a for each under the hood. If it's the case, the 2 functions would be the same. What is your advice?

By the way, cylindre is a 3D cylinder and I want to know which point are inside.

Version 1 without LINQ

for (int i = 0; i < fpc.Vertices.Length; i++)
{
    if (cylindre.IsPointInside(fpc.Vertices[i]))
        listPoint.Add(fpc.Vertices[i]);
}

Version 2, with LINQ

var insidePoint =
    from pt1 in fpc.Vertices
    where cylindre.IsPointInside(pt1)
    select pt1;

    foreach (Point3D pt2 in insidePoint)
    {
        listPoint.Add(pt2);
    }

As has been said, the for loop will most likely be more performant, but you can still clean the code up a bit more:

for (int i = 0; i < fpc.Vertices.Length; i++)
{
    if (cylindre.IsPointInside(fpc.Vertices[i]))
        listPoint.Add(fpc.Vertices[i]);
}

Can become:

foreach (var vertice in fpc.Vertices)
{
    if (cylindre.IsPointInside(vertice))
        listPoint.Add(vertice);
}

EDIT:  As per the performance question. This code will run in LINQPad. I found that the foreach version performs a few milliseconds better than the for loop.

var elements = Enumerable.Range(0, 4000000).Select(x => true).ToArray();

var sw = new Stopwatch();
var result = new List<bool>();
int trueCount = 0;
sw.Start();
for(int i=0; i < elements.Length; ++i)
{
    if (elements[i])
    {
        ++trueCount;
        result.Add(elements[i]);
    }
}

sw.Stop();
sw.ElapsedMilliseconds.Dump();

sw.Reset();
trueCount = 0;
result = new List<bool>();
sw.Start();
foreach(var element in elements)
{
    if (element)
    {
        ++trueCount;
        result.Add(element);
    }
}
sw.Stop();
sw.ElapsedMilliseconds.Dump();

EDIT2 - Regarding the link that seems to indicate such drastically poor performance in a foreach loop

The link (http://www.schnieds.com/2009/03/linq-vs-foreach-vs-for-loop-performance.html) is dealing with removing elements from a list. In the case that you have a generic list, removing elements via a for loop is not normally the best approach. A better approach is to use the RemoveAll method. RemoveAll will remove all elements matching the predicate and then consolidate the list as opposed to RemoveAt which will require moving all elements above the removed element. For performance comparison of the removal please see the below code (once again, this can be run in LINQPad). In my testing the RemoveAll ran roughly 250x faster.

var elements = Enumerable.Range(0, 100000).Select(x => x % 2 == 0).ToArray();

var sw = new Stopwatch();
var source = new List<bool>(elements);
sw.Start();
for(int i=0; i < source.Count; ++i)
{
    if (!source[i])
    {
        source.RemoveAt(i);
        --i;
    }
}

sw.Stop();
sw.ElapsedMilliseconds.Dump();
int count = source.Count;

sw.Reset();
source = new List<bool>(elements);

sw.Start();
source.RemoveAll((bool x) => {return x;});
sw.Stop();
sw.ElapsedMilliseconds.Dump();


Is it long polling or short polling? I wrote the following script and php code for long-polling architecture by googling references, I would like to know if this is long-polling or short-polling, ...because I am not sure:

bc_test.js

jQuery(function($) {

  var data = [];
  function pullNotification() {
    var params = {};
    new RPC.Call({
        'method': 'users.getJsonUsers',
        'params': params,
        'timeout': 30000,
        'onSuccess': success,
        'onFailure' : error,
    });
  };

  function error() {
    alert('Error occured');
  }

  function success(result) {
    if (data.length !== 0) {
        for (var i = 0, item; item = result[i]; i++) {
           for (var k = 0; k < data.length; k++) {
               if (item.userid === data[k]) {
                   this.found = true;
               }
            }
            if (!this.found) {
               data.push(item.userid);
               $('
\n\\n\
                            ' + item.userid + '	\n\
                            ' + item.alias + '	\n\
                            ' + item.surname + '	\n\

').insertAfter('.nilar');
            }
            this.found = false;
        }
    } else {
        for (var j = 0; j < result.length; j++) {
            data.push(result[j].userid);
        }
    }
}

window.setTimeout(function() {
   window.setInterval(function() {
      pullNotification();
   }, 3000);
   pullNotification();
}, 1000);
});

PHP

function getJsonUsers() {
   $timeStart = time();
   $newData = false;
   $users_data = array();

   $sql = 'SELECT * FROM users';

   while (!$newData && (time() - $timeStart) < 10) {
         usleep ( 10000000 );
         $db_res = DBselect($sql);
         while ($user = DBfetch($db_res)) {
               $users_data[] = $user;
               $newData = true;
         }
   }
   return $users_data;
}

This code is performing a long-polling technique.  The server gets a client request, the server method is checking for any changes in data, and when found, returns it to the client.  In short-polling, the client is constantly sending requests for data on a timer basis; in long-polling, the client makes one request and waits for the server to return any new data it has.

For example, in your method getJsonUsers(), it is running a query that looks for new data, and upon finding it, returns it to the user.  If it doesn't find data, it waits a finite amount of time, and tries again.  The client won't get notified of anything until data is actually returned, or it will timeout.  This is a normal pattern for long-polling.  If your program was using a short-polling technique, the server method would just return (either data or no data, depending on the results of the query you're running), and not wait to actually get something to give back to the client.

Your client code on the other hand, in the case of short-polling, would keep making requests to the server, and after a certain amount of time, would make another request.  Long-polling uses an event-based mechanism (that is, it waits for data to be present prior to returning) for alerting the client of data, whereas the short-polling method just keeps hitting the server regardless of what it gets back or not.

This website has good information on polling techniques, including an example of long-polling:  CoderTalks:  Long Polling vs Short Polling


Removing duplicate values from an array Here is my code for removing duplicated values from an array. I think I tested it with the most possible cases. Any suggestions or bugs?

class duplicate {

    public static int[] removeDuplicates(int[] arr) {
        int end = arr.length;

        for (int i = 0; i < end; i++) {
            for (int j = i + 1; j < end; j++) {
                if (arr[i] == arr[j]) {
                    int shiftLeft = j;

                    for(int k = j + 1; k < end; k++, shiftLeft++) {
                        arr[shiftLeft] = arr[k];
                    }

                    end--;
                    j--;
                }
            }
        }

        int[] whitelist = new int[end];

        for (int i = 0; i < end; i++) {
            whitelist[i] = arr[i];
        }

        return whitelist;
    }
}

After some tests, it appears really inefficient because an array with 1,000,000 elements takes a very long time to end. Is there any better way to implement this on arrays?

Suggestion:

public static Integer[] removeDuplicates(Integer[] arr) {
  return new HashSet<Integer>(Arrays.asList(arr)).toArray(new Integer[0]);
}

Another solution might be:

public static int[] removeDuplicates(int[] arr) {
  Set<Integer> alreadyPresent = new HashSet<Integer>();
  int[] whitelist = new int[0];

  for (int nextElem : arr) {
    if (!alreadyPresent.contains(nextElem)) {
      whitelist = Arrays.copyOf(whitelist, whitelist.length + 1);
      whitelist[whitelist.length - 1] = nextElem;
      alreadyPresent.add(nextElem);
    }
  }

  return whitelist;
}

Here you only iterate once via arr.


E-mail MIME message parser As part of a larger Java application I'm working on, I have to retrieve emails and parse the data for the emails' content (subject, date, text, attachments, sender). In the method below, I pass a Message as a parameter, which is the Javamail abstract representation of an email. The details of the Message are recovered and then returned as a MailList. A MailList is just a holder for the five objects that describe each email.

public MailList getContent(Message message) throws MessagingException, IOException
{
    String body = "";
    String from = "";
    ArrayList<MimeBodyPart> attachments = new ArrayList<MimeBodyPart>();
    String contentType = message.getContentType();
    Address[] addresses = message.getFrom();
    if(addresses.length == 1)
        from = addresses[0].toString();
    else
    {
        for(int num = 0; num < addresses.length - 1; num++)
            from += addresses[num].toString() + ", ";
        from += addresses[addresses.length].toString();
    }
    if(contentType.contains("TEXT/PLAIN"))
    {
        Object content = message.getContent();
        if(content != null)
            body += content.toString();
    }
    else if(contentType.contains("TEXT/HTML"))
    {
        Object content = message.getContent();
        if(content != null)
            body += Jsoup.parse((String)content).text();
    }
    else if(contentType.contains("multipart"))
    {
        Multipart mp = (Multipart)message.getContent();
        int numParts = mp.getCount();
        for(int count = 0; count < numParts; count++)
        {
            MimeBodyPart part = (MimeBodyPart)mp.getBodyPart(count);
            String content = part.getContent().toString();
            if(MimeBodyPart.ATTACHMENT.equalsIgnoreCase(part.getDisposition()))
                attachments.add(part);
            else if(part.getContentType().contains("TEXT/HTML"))
                body += Jsoup.parse(content).text();
            else
                body += content;
        }
    }
    return new MailList(from, message.getSubject(), body,
            message.getSentDate().toString(), attachments);
}

This code works exactly as intended, but is very slow. Each method run can range from 0.5s to 1.5s depending on the content of the email. This means that parsing 10 emails might take up to 15s. Up to around 100 emails might be expected to be parsed at any given time, and 2 to 3 min is too long for such a process. Not sure if this could be improved to be more efficient but any advice is greatly appreciated.

if(addresses.length == 1)
    from = addresses[0].toString();
else
{
    for(int num = 0; num < addresses.length - 1; num++)
        from += addresses[num].toString() + ", ";
    from += addresses[addresses.length].toString();
}

Concatenations of strings in a loop should be done using a StringBuilder like so

StringBuilder builder = new StringBuilder(1024);
for(int i = 0; i < addresses.length ; i++)
{
    builder.append(addresses[i].toString())
           .append(", ");
}
int builderLength = builder.length();
if(builderLength > 2)
{
    // removing the last appended ", "
    builder.setLength(builderLength - 2);
}
String from = builder.toString();

if(contentType.contains("TEXT/PLAIN"))
{
    Object content = message.getContent();
    if(content != null)
        body += content.toString();
}
else if(contentType.contains("TEXT/HTML"))
{
    Object content = message.getContent();
    if(content != null)
        body += Jsoup.parse((String)content).text();
}

here it would be sufficient to just assign the content.toString() or Jsoup.parse((String)content).text() to the body variable. There is no need to use +=.

else if(contentType.contains("multipart"))
{
    Multipart mp = (Multipart)message.getContent();
    int numParts = mp.getCount();
    for(int count = 0; count < numParts; count++)
    {
        MimeBodyPart part = (MimeBodyPart)mp.getBodyPart(count);
        String content = part.getContent().toString();
        if(MimeBodyPart.ATTACHMENT.equalsIgnoreCase(part.getDisposition()))
            attachments.add(part);
        else if(part.getContentType().contains("TEXT/HTML"))
            body += Jsoup.parse(content).text();
        else
            body += content;
    }
}

As mentioned above if you are using a loop for concatenations of strings this should be done using a StringBuilder.

You will only need the String content if the execution steps into the else if or else part and because one should declare variables as near to its usage, you could use continue; after the attachment is added like so

else if(contentType.contains("multipart"))
{
    StringBuilder bodyBuilder = new StringBuilder(1024);
    Multipart mp = (Multipart)message.getContent();
    int numParts = mp.getCount();
    for(int count = 0; count < numParts; count++)
    {
        MimeBodyPart part = (MimeBodyPart)mp.getBodyPart(count);
        if(MimeBodyPart.ATTACHMENT.equalsIgnoreCase(part.getDisposition()))
        {
            attachments.add(part);
            continue;
        }

       String content = part.getContent().toString();
        if(part.getContentType().contains("TEXT/HTML"))
        {
            bodyBuilder.append(Jsoup.parse(content).text());
        } else {
            bodyBuilder.append(content);
        }
    }
    body = bodyBuilder.toString();
}


Simple Telnet chat server I am new to node.js and asynchronous programming and this is my first small project to learn it. I have written a small Telnet chat server and the program works but I want to know if this is the correct way to write programs in node.js (asynchronous programming model).

var tcp = require('net');
var Server = tcp.createServer();
var pwd=[]; // keep tracks of connected clients
var ip=[];  // keeps tracks of connected clients IP address
var Cpwd = "Rushabh"; // password that you require to login

Server.on('connection'  ,function(conn){
    conn.setEncoding('utf-8');
    conn.write("Password:");// ask user for password on their terminal
    console.log("[" + conn.remoteAddress + "] has joined the chat");

    conn.on('data' , function(data){
        if(pwd.indexOf(conn)>-1){//check if it is an old client or a new client
            console.log("[" + conn.remoteAddress + "]:"  + data);

            if(pwd.length > 0){ // check if atleast one client is connected
                sendMessage(conn , data);// broadcast message to all client connected
            }
        }
        else{//if it is a new client then server should first check for password
            data= data.toString('utf-8').trim();
            var message = " has joined the chat";
            if(Cpwd == data){ // if it is a new client than check for password
                pwd.push(conn);
                ip.push(conn.remoteAddress);
                sendMessage(conn , message);
            }
            else {
                conn.write("Password rejected:" + data);conn.end();}// disconnect client
            }
        });

      conn.on('end' , function() { // remove the client from reference array

       var i , client;
         for(i in pwd){
           client = pwd[i];
           if(!client.writable){
             pwd.splice(i,1);
             console.log(ip[i]+ " has left the chat");
             ip.splice(i,1);
           }
         }

     });

});

    function sendMessage(conn , message){ //function to send message to all connected client

      var i , client;
      for(i in pwd){
        client = pwd[i];
        if(client.writable){
          if(conn === client){
            client.write("[me]:" + message);
          }
          else
            client.write("[" + conn.remoteAddress +"]:" + message);
        }
        else{
          pwd.splice(i , 1);
          console.log(ip[i]+ " has left the chat");
          ip.splice(i,1);
        }
      }
    }
    Server.listen(8000);

From a once over:

Naming

variable names should start with a lower case letter, unless they are constructors

Server -> server
Cpwd -> password

pwd contains all the connections, any reader would guess it contains passwords

More on variables

There is no good reason to have a separate array ip, since it always contains as many entries as pwd which already contains the the ip addresses in remoteAddress.
Having the password inside your scrip is ok for a prototype, it is not for production

Functions

You encapsulated sendMessage into a function, you should also have a leave or quit function and a joinChat function.

Flow

You announce that a user joins the chat before checking the password
In on 'end' you could just use indexOf on conn instead of manually looping over all connections.

Other than that I think you are doing fine with the asynchronous model of Node.


Formatting some data about some airlines function reformat($data, $registration = false) {
    // Initializing aircompanies codes
    $iata = new iata;
    for ($i = 0; $i < count($data); $i = $i + 6) {
        if ($registration) {
            $r = str_replace('<nobr>', '', $this->prep_value($data[$i]));
            $block[$i]['reis'] = str_replace('</nobr>', '', $r);
            $a = explode(' ', $block[$i]['reis']);
            $block[$i]['company'] = $a[0];
        } else {
            $r = explode('&nbsp;', $this->prep_value($data[$i]));
            $block[$i]['reis'] = (strlen($r[0]) > 2) ? strrev(substr(strrev($r[0]), 15, 2)) . ' ' . $r[1] : $r[0] . ' ' . $r[1];
            $block[$i]['company'] = (strlen($r[0]) > 2) ? strrev(substr(strrev($r[0]), 15, 2)) : $r[0];
        }
        $block[$i]['companyname'] = $iata->aircompanies[strtoupper($block[$i]['company'])];
        if (file_exists(ROOT_HTML_PATH . '/content/aircompanies/' . $block[$i]['company'] . '.png')) {
            $block[$i]['company'] = $block[$i]['company'] . '.png';
        } else {
            /**
             * @todo Write e-mail notifications, but only once;
             */
        }
        $block[$i]['airport'] = $this->prep_value($data[$i + 1]);
        $block[$i]['terminal'] = substr(strrev($this->prep_value($data[$i + 2])), 2, 1);
        $block[$i]['shedtime'] = $this->prep_value($data[$i + 3]);
        $block[$i]['facttime'] = str_replace('&nbsp;', '', $this->prep_value($data[$i + 4]));
        $block[$i]['status'] = str_replace('&nbsp', '<br />', trim(strip_tags($this->prep_value($data[$i + 5]), '<img />'), '&nbsp;/r/n'));
        $block[$i]['status'] = str_replace(';', '', $block[$i]['status']);
    }
    return $block;
}

function reformat($data, $registration = false) {
    // Initializing aircompanies codes
    $iata = new iata;
    $block = array();
    for ($i = 0; $i < count($data); $i += 6) {
        $block[$i] = array(
            'airport'  => $this->prep_value($data[$i + 1]),
            'terminal' => substr(strrev($this->prep_value($data[$i + 2])), 2, 1),
            'shedtime' => $this->prep_value($data[$i + 3]),
            'facttime' => str_replace('&nbsp;', '', $this->prep_value($data[$i + 4])),
            'status'   => str_replace(';', '',
                              str_replace('&nbsp', '<br />',
                                  trim(
                                      strip_tags($this->prep_value($data[$i + 5]), '<img />'),
                                      '&nbsp;/r/n'
                                  )
                              )
                          ),
        );

        if ($registration) {
            $r = str_replace('<nobr>', '', $this->prep_value($data[$i]));
            $block[$i]['reis'] = str_replace('</nobr>', '', $r);
            $a = explode(' ', trim($block[$i]['reis']));
            $block[$i]['company'] = $a[0];
        } else {
            $r = explode('&nbsp;', $this->prep_value($data[$i]));
            $block[$i]['company'] = (strlen($r[0]) > 2) ? substr($r[0], -17, 2) : $r[0];
            $block[$i]['reis'] = $block[$i]['company'] . ' ' . $r[1];
        }
        $block[$i]['companyname'] = $iata->aircompanies[strtoupper($block[$i]['company'])];
        if (file_exists(ROOT_HTML_PATH . '/content/aircompanies/' . $block[$i]['company'] . '.png')) {
            $block[$i]['company'] = $block[$i]['company'] . '.png';
        } else {
            /**
             * @todo Write e-mail notifications, but only once;
             */
        }
    }
    return $block;
}

But it is many questions here:

$this->prep_value($data[$i + 2])

It is really bad idea. May be you need a little another cycle, like

for ($i = 0; $i < count($data) - 6; $i ++) { // or something for preventing array index overloading

It would be much better.
But I cannot know it for sure.


Project Euler 001 in C++ I come from a C background and have now just begun C++.
This also means that I am not focussing on the algorithm, but more on C++ idioms.

Description:

  If we list all the natural numbers below 10 that are multiples of 3 or 5, we get 3, 5, 6 and 9. The sum of these multiples is 23.

  Find the sum of all the multiples of 3 or 5 below 1000.

main.cpp:

#include <iostream>

#define UPPER_BOUND 1000

int main() {
    int sum = 0;

    for (int i = 1; i < UPPER_BOUND; ++i) {
        if (i % 3 == 0 || i % 5 == 0) {
            sum += i;
        }
    }

    std::cout << "The sum of the natural numbers below " << UPPER_BOUND << " that are multiples of 3 and 5 is " << sum;

    return 0;
}

I will add files to GitLab here and make changes to them as I receive feedback.
Feel free to make changes to existing solutions.

constexpr everything

Apart from the #define, your code is quite good. But you miss on a really simple optimization by not using constexpr, which has different meanings in different contexts:

constexpr variables are compile-time constants. They're a good replacement for constant defining macros.

#define UPPER_BOUND 1000 // bad, because preprocessing leads to a lot of bugs
constexpr int upper_bound = 1000; // good, safe

constexpr functions can be executed at compile-time if their arguments are known at compile-time; the compiler will then substitute a constant to the function call in the code.

So let's extract a function from your main() and declare it constexpr:

constexpr int sum_of_multiples_of_3_and_5_under(int upper_bound) {
    int sum = 0;
    for (int i = 1; i < upper_bound; ++i) {
        if ( i % 3 == 0 || i % 5 == 0 ) {
            sum += i;
        }
    }
    return sum;
}

Now, whenever you call it with an upper_bound you know at compile-time, it will run at compile-time.

Generalize your code

Running your function at compile-time means a lot, because if you can't and need your code to run fast at run-time, you'll have to implement better algorithms. As @Dannnno pointed out, you could have used the algorithm to compute an arithmetic progression, which is O(1) instead of O(n) for your code:

constexpr int sum_multiples_under(int ratio, int upper_bound) {
    int nb_val = (upper_bound - 1) / ratio;
    int high = nb_val * ratio;
    int extremes = ratio + high;
    return nb_val * extremes / 2;
}

You can then compute the sum of the multiples of 3 and 5 with:

auto sum = sum_multiples_under(3,  1000)
         + sum_multiples_under(5,  1000)
         - sum_multiples_under(15, 1000);

It's quite easy to implement, even if it consumes a bit more brain-power than the for-loop. But let's say that we want to generalize our code, and make it work for any number of divisors, for instance the sum of the multiples of 3, 5 and 7. Then it becomes a lot harder: you have to add all members of one set, then subtract all members of two sets, then add all members of three sets... etc. That means computing combinations, applying functions to arrangements, etc. Whereas our for-loop scales quite comfortably.

Use fold expressions

With fold expressions (which require C++17), you can generalize your code fairly easily:

template <typename... Multiples>
constexpr int sum_of_multiples_under(int upper_bound, Multiples... multiples) {
    int sum = 0;
    for (int i = 1; i < upper_bound; ++i) {
        if ( ( ... || (i % multiples == 0)) ) {
            sum += i;
        }
    }
    return sum;
}

Multiples is a variadic template parameter. It can stand for any number of arguments. You can then expand the arguments' pack inside your function. Fold expressions allow you to expand it around an operator. In this case, the operator is ||. ( ... || (i % multiples == 0)) will be expanded into:

   (i % multiples_1 == 0)
|| (i % multiples_2 == 0)
|| ...
|| (i % multiples_N == 0)

Conclusion

constexpr is a really, really useful keyword, and is a sign of modern, optimized C++ code. Use it without moderation. If you want to know everything about it, there's a keynote on YouTube named "constexpr everyting".


Moving business logic towards the domain object as a design pattern My project is composed of several parts: Repositories, Services, Domain objects, etc in a .NET MVC web application.

My repositories handle all data i/o, the services are responsible for CRUD operations as well as other logic, such as getting a count of objects from the database.

I am trying to shift from an "anemic" domain model where my domain objects are simply classes with properties to having more business logic where it belongs. In this case, I would like to have the domain object handle the logic where it checks the database for duplicates, but I also don't want to have the code that communicates with the repository inside of the domain object (I read it is bad practice to have an object responsible for persistence with the database, so I'm assuming this is the same thing.)

I don't particularly like this solution, as it just doesn't feel good, and I'd like to get a better handle on how to handle these sorts of situations. Can anybody give me some advice here?

public interface IDuplicateChecker<T> where T : class
{
    bool IsDuplicate(T t);
}

public class DuplicateDocumentChecker : IDuplicateChecker<Document>
{
    public bool IsDuplicate(Document doc)
    {
        IDocumentService documentService = new DocumentService();

        // simplified check -- really there are a few other properties to match
        if (documentService.Count(doc.DocumentNumber) > 0)
        {
            return true;
        }

        return false;
    }
}

public class Document
{
    public Document(string documentNumber)
    {
         // validation and other removed for brevity
         DocumentNumber = documentNumber;
    }

    // simplified document properties for brevity
    public string DocumentNumber { get; set; }
    public bool IsDuplicate { get; set; }

    public void CheckForDuplicates(IDuplicateChecker<Document> checker)
    {
        IsDuplicate = checker.IsDuplicate(this);
    }
}

First, I would like to say thank you for asking this question, because this is the type of situation that average enterprise devs face every day and being able to recognize that something doesn't "feel" right even though all the rules are followed is a good indicator that you're on the right track for figuring out what is going to be a good implementation and what isn't.

Getting to the code, from an overall perspective I would recommend the pattern of creating services in front of your domain layer, instead of your domain objects being fed into services that are dependencies of util/helper classes. An app layer can construct a domain Document object and pass it to a DocumentService, but encapsulating a DocumentService behind something that the app layer doesn't interact with could become hard to maintain.

A DocumentService could deal with saving the Document to the database, and as it's saving, it can then check for duplicates. Whether it checks for duplicates inline in the  method or it calls another duplicate-checking object/util to do this is up to how big the method gets and how many different things the service is doing in the method.

Looking at your existing code, I'd recommend not getting too dogmatic about "one role, one responsibility--one class!" because it can be argued that documentService.SaveDocument(document) also has one responsibility (having to check for duplicates is superfluous--it's not part of the public contract of this method, although it may require a dependency or two passed in to the constructor).

I think when you have a DocumentService inside a very specific class used for a very specific purpose, it's a bit of a smell, because your DocumentService starts seeming like a bit of a Swiss army knife you start including inside all document-related objects, because hey, it's where document-related things go!

After all of that, if you implement according to my advice, your Document may remain a glorified property bag (though I would remove the properties like IsDuplicate from the Document. They don't belong on a domain entity. IsDuplicates should be refactored most simply to a boolean hasDuplicates or duplicatesExist as a local variable inside a method that has a responsibility to check for duplicates). And I think it's okay if it stays that way for now if your domain is not very fleshed out. It would be worse to put responsibilities/functionality on a domain object that didn't belong, because that's the type of problem that once is in place is really hard to break out because it's in such critical spot.

I would recommend focusing on your domain objects as smart entities first. That will help keep your mental model of them as a business object, but keeping the "smart" aspect in mind will allow you to apply "smart" logic to your objects. For example, if you have a one-to-many relationship between your objects, how does your domain track changes between these objects? If a child object assigns its parent reference, the parent object should have a reference to the child in its collection of children. So you might implement that by writing a "smart" setter so that when an Order assigns its Customer property, the Customer property adds it to its Orders collection if it doesn't exist. I think that's more in line with a domain object that is still an "entity" of sorts. Of course, there's definitely more that comes with the evolution of an application, but I'd start there.

For the time being, I'd focus on having intuitive services sitting in front of your domain, so the application layer can call these services, using domain objects to communicate by constructing them and passing them in to the services or having them returned from the services, or both.


Template heap/priority-queue data structure with update-key operation I implemented the following priority-queue/heap with an update-key operation. The update key uses the map from STL, which still requires lg(n) time for lookup. However, replacing it with an unordered_map would require the client program to write hash and equal operations for each class, which is a problem in my case.

#ifndef IZ_HEAP_H
#define IZ_HEAP_H

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

using namespace std;

template <class T>
class IzHeap {
  class IzHeapItem {
    int key;
    T item;
  public:
    IzHeapItem(const int key, const T& item)
      : key(key), item(item) {}
    T getItem() const {return item;}
    int getKey() const {return key;}
    void updateKey(const int newKey) {key = newKey;}
    void updateItem(const T& newItem) {item = newItem;}
    IzHeapItem& operator=(const IzHeapItem& heapItem) {
      this->key = heapItem.getKey();
      this->item = heapItem.getItem();
      return *this;
    }
  };
  std::vector<IzHeapItem> heap;
  std::map<T, int> itemToHeapIndex;
  int leftChild(const int index) const {return index*2 + 1;}
  int rightChild(const int index) const {return index*2 + 2;}
  int parent(const int index) const {return (index - 1)/2;}
  void heapSwap(const int i, const int j) {
    /*
    IzHeapItem temp = heap[i];
    heap[i] = heap[j];
    heap[j] = temp;
    */
    std::swap(heap[i], heap[j]);

    itemToHeapIndex[getItem(i)] = i;
    itemToHeapIndex[getItem(j)] = j;
  }
  int getKey(const int index) const {return heap[index].getKey();}
  T getItem(const int index) const {return heap[index].getItem();}
protected:
  void shiftUp(const int index) {
    int largest{index}, p{parent(index)};
    if (p >= 0 && getKey(p) > getKey(index)) {
      largest = p;
    }
    if (largest != index) {
      heapSwap(largest, index);
      shiftUp(largest);
    }
  }
  void shiftDown(const int index) {
    int smallest{index}, lc{leftChild(index)}, rc{rightChild(index)};
    if (lc < heap.size() && getKey(lc) < getKey(smallest)) {
      smallest = lc;
    }
    if (rc < heap.size() && getKey(rc) < getKey(smallest)) {
      smallest = rc;
    }
    if (smallest != index) {
      heapSwap(smallest, index);
      shiftDown(smallest);
    }
  }
public:
  T extract() {
    assert(heap.size() > 0);
    heapSwap(0, heap.size() - 1);
    T item = heap.back().getItem();
    heap.pop_back();
    itemToHeapIndex.erase(item);
    shiftDown(0);
    return item;
  }
  T peak() {
    return heap[0];
  }
  bool empty() {
    return heap.size() == 0;
  }
  void insert(const T& item, const int key) {
    heap.push_back(IzHeapItem(key, item));
    itemToHeapIndex[item] = key;
    shiftUp(heap.size()-1);
  }
  void updateKey(const T& item, const int newKey) {
    assert(itemToHeapIndex.count(item) > 0);
    int index = itemToHeapIndex[item];
    int oldKey = getKey(index);
    heap[index].updateKey(newKey);
    if (newKey > oldKey) {
      shiftDown(index);
    }
    else {
      shiftUp(index);
    }
  }
  void heapify(const std::vector<T>& items, const std::vector<int>& keys) {
    assert(items.size() == keys.size());
    for (int i = 0; i < items.size(); ++i) {
      heap.push_back(IzHeapItem(keys[i], items[i]));
      itemToHeapIndex[items[i]] = i;
    }
    for (int i = heap.size()/2 - 1; i >=0; --i) {
      shiftDown(i);
    }
  }
  friend std::ostream& operator<<(std::ostream& out, const IzHeap<T>& heap) {
    for (auto& hi : heap.heap) {
      out << "(" << hi.getKey() << "->" << hi.getItem() << "), ";
    }
    return out;
  }
};

#endif

Please comment on any improvements that can be done on this code. This is my first time posting on code review, so please let me know of any norms I should follow.

peak() should be returning a const reference. same goes for getItem().
Extract() should not return anything. Having two functions to read the head just begs for someone to accidentally call Extract() when they meant peak().
Why is key forced to be an int? It could easily be a separate template parameter.
heap.push_back(IzHeapItem(keys[i], items[i])); should be heap.emplace_back(keys[i], items[i]); instead.
you should do a heap.reserve() at the top of heapify()
Heapifying a vector<> containing the same item twice would break this pretty badly. You should guard against this.
Unless you intend to call heapify() multiple times in a row, that should just be a constructor. Otherwise, I would rename that function addToHeap()
I prefer to avoid using the [] operator on maps in general unless I specifically want a "insert-or-replace" logic. There's a bunch of logic to handle creating a new instance, which assumes the value type of the map has a default constructor.

so:

itemToHeapIndex[items[i]] = i;

becomes

itemToHeapIndex.emplace(items[i], i);

and

assert(itemToHeapIndex.count(item) > 0);
int index = itemToHeapIndex[item];

becomes

auto found = itemToHeapIndex.find(item);
assert(found != itemToHeapIndex.end()); // Bonus! no additional lookup in debug
int index = found->second;

Super minor detail:

The peak() function name made me squint. Traditionally, peek() means "see the next value that will come out of the datastructure", which is what happens here. This is not really a problem you have to fix, more of an observation. I would personally have used top() and pop() to match established standards.


A Simple Battleship Game I have just finished learning basic-intermediate Python subjects and wanted to test myself. This Battleship game is the third game i have written.

import random

def create_random_ship():
    return random.randint(0, 5), random.randint(0, 5)

def play_again():
    try_again = input("Wanna play again? <Y>es or <N>o? >: ").lower()
    if try_again == "y":
        play_game()
    else:
        print("Goodbye!")
        return

print("Welcome to the Battleship game!"
      "\nYour main objective is to find and destroy all the hidden ships on map!\n")

print("""\nIntroductions:
\nYou have 10 ammo and there are 3 hidden ships on map.
In order to hit them, you have to enter specific numbers for that location. For example:
For the first row and first column, you have to write 1 and 1.
I wish you good fortune in wars to come!\n""")

def play_game():
    game_board = [["O", "O", "O", "O", "O"],
                  ["O", "O", "O", "O", "O"],
                  ["O", "O", "O", "O", "O"],
                  ["O", "O", "O", "O", "O"],
                  ["O", "O", "O", "O", "O"]]

    for i in game_board:
        print(*i)

    ship1 = create_random_ship()
    ship2 = create_random_ship()
    ship3 = create_random_ship()
    ships_left = 3
    ammo = 10

    while ammo:
        try:
            row = int(input("Enter a row number between 1-5 >: "))
            column = int(input("Enter a column number between 1-5 >: "))
        except ValueError:
            print("Only enter number!")
            continue

        if row not in range(1,6) or column not in range(1, 6):
            print("\nThe numbers must be between 1-5!")
            continue

        row = row - 1 # Reducing number to desired index.
        column = column - 1 # Reducing number to desired index.

        if game_board[row][column] == "-" or game_board[row][column] == "X":
            print("\nYou have already shoot that place!\n")
            continue
        elif (row, column) == ship1 or (row, column) == ship2 or (row, column) == ship3:
            print("\nBoom! You hit! A ship has exploded! You were granted a new ammo!\n")
            game_board[row][column] = "X"
            ships_left -= 1
            if ships_left == 0:
                print("My my, i didn't know you were a sharpshooter! Congratz, you won!")
                play_again()
        else:
            print("\nYou missed!\n")
            game_board[row][column] = "-"
            ammo -= 1

        for i in game_board:
            print(*i)

        print(f"Ammo left: {ammo} | Ships left: {ships_left}")

    play_again()

if __name__ == "__main__":
    play_game()

What do you think? How can I make it better? What are my mistakes?

Magic Numbers

Your biggest issue is too many magic numbers and hard-coded information.

If you added at the top of your script:

ROWS = 5
COLUMNS = 5

then you could have:

def create_random_ship():
    return random.randrange(ROWS), random.randrange(COLUMNS)

Game board can be created with:

game_board = [["O"] * COLUMNS for _ in range(ROWS)]

And you can ask for input from the user with:

row = int(input(f"Enter a row number between 1-{ROWS} >: "))

Along the same vein, you could have:

print(f"""\nIntroductions:
\nYou have {INIT_AMMO} ammo and there are {INIT_ENEMY} hidden ships on map.
...""")

so your welcome instructions can keep up-to-date with any changes to your game initial conditions.

Instructions

You have a if __name__ == "__main__": guard to prevent the code from running if imported into another script, but your instruction are unconditionally printed.  You should move the instruction printing into an instructions() function, and call that only if appropriate.

Infinite Recursion

play_game() calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), which calls play_again(), which can call play_game(), ...

Traceback (most recent call last):
  [...omitted...]
RecursionError: maximum recursion depth exceeded

Don't do that.  Much simpler is to have play_again() return a True/False result, and loop based on the return value:

def play_many_games():

    instructions()

    play_game()

    while play_again():
        play_game()

    print("Goodbye!")

if __name__ == "__main__":
    play_many_games()

No more recursion.

In the comments, @Graipher suggests an alternate method to avoid the recursion:

while True:
    play_game()
    if not play_again():
        break

Yet another alternative introduces a loop condition variable, which is pre-set before the loop:

 playing = True
 while playing:
     play_game()
     playing = play_again()

All three approaches avoid the infinite recursion.

I Saw Three Ships...

ship1 = create_random_ship()
ship2 = create_random_ship()
ship3 = create_random_ship()

# ...

    elif (row, column) == ship1 or (row, column) == ship2 or (row, column) == ship3:

What are you going to do if you change this to four ships, or even 5 ships?

With computers, there are 3 important numbers: zero, one and many.  If you have more than one ship, you have "many" ships, and should put them in a container, such as a list():

ship_coordinates = []
for _ in range(ships_left):
    ship_coordinates.append(create_random_ship())

And when you want to see if a particular row/column matches any one of the ship coordinates in the container, you use the in function:

    elif (row, column) in ship_coordinates:

There are other container types you can use, such as a set() which has faster in performance.  However, it cannot add the same value more than once, and using it would result in slightly different behaviour to what you currently have.  You've mentioned that you have a bug where ships can have the same coordinates, and I am trying very hard not to accidentally fix that.  (Code Review is for reviewing working code only, not help debugging, so I am tiptoeing around the bug, so that you can fix it yourself.)


Simplify a complex KQL Query The situation is, I have two fields in a database (or that can be derived) - when the message was received (Received, of type DateTime), and the day of the month (but not the month itself) that the message was supposed to be received (DayOfRun of type Integer, which is the last two digits of an ID column that doesn't otherwise contain date-time information). I need to know the difference (in whole days, ignoring the time) between the two. If the message arrived before the day of run, the number will be negative.
I can guarantee that the entries will not differ by more than half a month in either direction.
The pseudo-code logic I have come up with is as follows:
DayOfRun = Extract last two characters from ID Field
DayReceived = Extract DayOfMonth from Received;
DaysInPreviousMonth = Extract Count of Days in (Month before Received)
Pick the closest to 0 from the following:
A:   If DayReceived < DayOfRun then
 :      DaysInPreviousMonth + DayReceived - DayOfRun ;
 :   Otherwise
 :       DaysInPreviousMonth + DayOfRun - DayReceived ;

B:   DayReceived - DayOfRun;

Examples:

Day of Run
Received
Answer
Reasoning

01
2021-04-29
-2
29th April is 2 days before 1st May (can't be 1st April - would be too far away)

28
2021-05-02
4
02nd May is 4 days after 28th April (which gives a smaller absolute number than assuming 28th May, which would be too far away anyway)

14
2021-04-28
14
28th April is 14 days after 14th April (which gives a smaller absolute number than if we assumed 14th May)

My KQL to implement this logic is as follows:
tableName
| project DayOfRun = toint(substring(['ID Field'], -2, 2)), ['Day Of Month Received'] = datetime_part("day",ErrorTimestamp), ErrorTimestamp
| where DayOfRun <> ['Day Of Month Received']
| project ['Days In Previous Month'] =  datetime_part("day",endofmonth( datetime_add("month",-1, ErrorTimestamp))), DayOfRun, ['Day Of Month Received']
| project ['Day Of Month Received'], Diff = (['Day Of Month Received'] - DayOfRun), CrossMonthDiff = iif(['Day Of Month Received'] < DayOfRun,(['Days In Previous Month']+['Day Of Month Received']-DayOfRun),(['Days In Previous Month']+DayOfRun-['Day Of Month Received']))
| summarize ['Day Of Month Received'], ['Difference (Days)'] = iif(min_of(abs(Diff),abs(CrossMonthDiff)) == abs(Diff),Diff,CrossMonthDiff)
| order by ['Difference (Days)'] desc, ['Day Of Month Received'] asc

This query seems overly complex, and is quite slow to run. Any thoughts on how to improve either the logic or the query itself?

A few suggestions:

You're performing string manipulations, and then extracting numbers out of the string result, which is slow. The best thing to do is to ingest the data in a better format in the first place (so that the number you're extracting will be ingested in a separate numeric column). If it's not possible to do it in ingestion, then you should do it in an Update Policy.

You didn't add any filters on datetime columns, meaning you're querying the whole table. For this to work efficiently, you need all the data to be in the hot cache. Is this your case? To see the effective retention and caching policies for the table, run .show table tableName details | project RetentionPolicy, CachingPolicy.

You might want to try using summarize hint.strategy=shuffle (see this for more info).


Check if a store is currently open This is the dirty bastard of my current code block, and I'm trying to find a way to improve this code for increased readability, but also functionality.

The method checks if the store is currently open. Yet on our site we close the sites 30 minutes prior to their real closing time. Issues arise mostly because all of our stores are open past midnight.

So this function is supposed to figure out if a store is open, taking into account that we close it 30 minutes before it's actual close time, and the fact that if it closes 12:10AM the next day, we have to close it 11:40PM the prior day.

def open?(current_time: Time.now.change(sec: 0))
  current_business_hour = business_hours.where('week_day = ? AND open_at <= ? AND close_at >= ?', current_time.wday, current_time, current_time).first
  if current_business_hour
    safe_closing_hour = current_business_hour.close_at - 30.minutes
    if current_time.strftime('%H:%M') >= '23:29'
      next_opening_hour = business_hours.where(week_day: DateTime.now.tomorrow.wday).order(:open_at).first
      if next_opening_hour
        if next_opening_hour.open_at.strftime('%H:%M') == '00:00'
          if next_opening_hour.close_at.strftime('%H:%M') >= '00:29'
            true
          elsif (next_opening_hour.close_at - 30.minutes).strftime('%H:%M') < current_time.strftime('%H:%M')
            false
          else
            true
          end
        else
          false
        end
      else
        false
      end
    elsif safe_closing_hour.strftime('%H:%M') < current_time.strftime('%H:%M')
      false
    else
      true
    end
  else
    false
  end
end

It's an unappealing spiderweb of if else statements, I do hope that the code makes someone what sense when you read it, then it's just the puzzle of understanding which true and false belongs to which.

I also feel like this is awfully complicated, and I wish that there was an easier way to do this.

Each store has many business hours, which has the week_day and an open_at and close_at. The store can have several business hours in one week_day. F.ex a store can close at 1AM, but open at 11AM and go til 11:59PM.

First I must disclose that I have no experience about programming in Ruby, I hope that the following is applicable.

In a comment I suggested Unix time stamps because they represent a continuous flow of time (integer number of seconds since 1.1.1970), and are therefore easy to use in time arithmetics and comparisons. Hopefully Ruby has support for Unix timestamps.

I also suggest that you store open hours in a db table, regardless of whether they extend over midnight, as a start time and an end time.

Define Unix30min = 30 x 60

Fetch UnixCurrentTime

Fetch a record from the db where (UnixOpenHoursOpen < UnixCurrentTime) and (UnixOpenHoursClose > UnixCurrentTime)

The logic is then simplified to three if statements, without nesting. As you, I dislike deeply nested if, then elseif else end statements, they make my head hurt. If possible I split complex logic into simple statement, readability and maintainability improves significantly.

if nothing was returned (from the db), the shop is closed

if ((UnixCurrentTime > UnixOpenHoursOpen) and (UnixCurrentTime < (UnixOpenHoursClose - Unix30min)))
{
  exit(ShopIsOpen)
}

if ((UnixCurrentTime > (UnixOpenHoursClose - Unix30min)) and (UnixCurrentTime < UnixOpenHoursClose))
{
  exit(ShopIsClosing)
}

The above is pseudocode


Generating all 6-digit palindrome numbers in C++ I just finished my first homework for university where I had to create code that would generate all 6 digit palindrome numbers.

Can someone who has more knowledge give me some tips where I can improve my code?

#include <iostream>
#include <algorithm>
#include <sstream>

using namespace std;

int main(int argc, char* argv[] )
{
  string palindroms;

  for (int iter = 100000; iter <= 1000000; ++iter ) {
    stringstream ss;
    ss << iter;
    palindroms = ss.str();

    string RevStr = palindroms;
    reverse( RevStr.begin(), RevStr.end() );

    if ( RevStr == palindroms ) {
      cout << palindroms << endl;
    }
  }
return 0;
}

I have checked your "brute force" algorithm and though it solves the problem (I have not run it to check), it will have to deal with a lot of work. Why not narrowing the problem a bit? What do we know about palindroms? They read the same from left to right than from right to left.

So if we use that and the fact that our palindroms are six digits long, we would only have to generate all the combinations for the first three digits, then make sure the next three are the same reversed. So:

for( int iter=100; iter<=999; iter++ ) {
  stringstream ss;
  ss << iter;
  string pal1 = ss.str();
  string pal2 = pal1;
  reverse(pal2.begin(), pal2.end());

  cout << pal1 << pal2 << endl;
}


Celsius and Fahrenheit Converter I'm very new to programming in C and I attempted to make a program that converts temperatures to and from Fahrenheit and Celsius. I have been running the program and I have realized that not all conversions are correct. For example, if I use the boiling point of water (100C or 212F), I get an incorrect answer. I thought that I used the correct formulas. I was wondering if you wouldn't mind reviewing this quick program.

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

int main(void) {

    float c;
    float f;
    int w;

    printf("Type 1 if you would like to convert to Farenheit\n");
    printf("Type 2 if you would like to convert to Celcius\n");

    scanf("%i", &w);

// Loop to make sure value is 1 or 2

    while(w != 1 && w !=2) {

        fprintf(stderr, "You didn't input a valid number. Please Try Again\n");
        scanf("%i", &w);
    }

// Celsius --> Fahrenheit
    if(w == 1){
        printf("Input the Celcius Temperature\n");
        scanf("%f", &c);
        printf("You entered %f\n", c);
        f = c * (9./5.) + 32.;
        printf("%f in Fareheit is %f\n", c, f);
     }

// Fahrenheit --> Celsius
    if(w == 2){
        printf("Input the Farenheit Temperature\n");
        scanf("%f", &f);
        printf("You entered %f\n",f);
        c = (f - 32.) * (5./9.);
        printf("%f in Celcius is %f\n", f, c);
    }

    return 0;
}

Spelling "Celsius" (3 places)

printf("Type 2 if you would like to convert to Celcius\n");

Better to get return value of input functions (4 places).  Even in learner code, it saves time during debug

// scanf("%i", &w)
int ret = scanf("%i", &w);
assert(ret == 1);

Infinite loop should user enter non-numeric input as the offending input is not consumed.

while(w != 1 && w !=2) {
  fprintf(stderr, "You didn't input a valid number. Please Try Again\n");
  // scanf("%i", &w);
  ret = scanf("%i", &w);
  assert(ret == 1);
}

Style: Suggest .0.  Also since the input and output are float, code could use float constants.  In general, unless floating point math is intensive or has larges array of data, recommend using double as that is C's default FP type.

// f = c * (9./5.) + 32.;
f = c * (9.0/5.0) + 32.0;
f = c * (9.0f/5.0f) + 32.0f;

Functional simplification: Rather than enter a number to indicate the desired temperature scale, enter 'C' or 'F'.

char scale;
char scale_new;
ret = scanf("%f %c", &temperature, &scale);
assert(ret == 2);

switch (scale) {
  case 'C':
  case 'c':
    temperature_new = temperature*9/5 + 32;  // math is all FP
    scale_new = 'F';
    break;
  case 'F':
  case 'f':
    temperature_new = (temperature - 32)*5/9;  // math is all FP
    scale_new = 'C';
    break;
  default:
    // Handle bad scale
}
printf("... converts to %f %c\n", temperature_new, scale_new);


Wrap a js function with a fixed this arg If I use node.addEventListener('click',Wyg.Editor.nodeClickedEvent); then when node is clicked, nodeClickedEvent has this===node. Since nodeClickedEvent is a static function in a class, I want this to === Wyg.Editor, so I wrote a helper function to simplify it.

Helper Function:

function fixThisFunc(object,functionName){
    const fixedFunc = function(){
        object[functionName].apply(object,arguments);
    };
    return fixedFunc;
}

How it's used:

let node = document.getElementById('myFavoriteNode');
node.addEventListener('click', fixThisFunc(Wyg.Editor,'nodeClickedEvent'));

How it looked before:

node.addEventListener('click',
    function(event){
        Wyg.Editor.nodeClickedEvent(event);
    }
);

The Wyg.Editor class is like:

Wyg.Editor = class {

    static nodeClickedEvent(event){
        const clickedNode = event.target;
        console.log(this); //successfully outputs the Wyg.Editor object/class
        this.getEditableNode(clickedNode);
    }
//there are more functions, of course
};

I'm looking for a review of the Helper Function fixThisFunc

Are there any particular problems I should be worried about? is there a better way to achieve this functionality?

is there a better way to achieve this functionality?

You can use Function.bind() to create a function with the this context bound to Wyg.Editor:

const node = document.getElementById('myFavoriteNode');
node.addEventListener('click', Wyg.Editor.nodeClickedEvent.bind(Wyg.Editor));

See this demonstrated in the snippet below.

Notice that const was used instead of let - unless there is a reason to re-assign node, use const. This will avoid accidental re-assignment.

const Wyg = {};
Wyg.Editor = class {

    static nodeClickedEvent(event){
        const clickedNode = event.target;
        console.log('nodeClickedEvent() - this: ', this);

    }
};
const node = document.getElementById('myFavoriteNode');
node.addEventListener('click', Wyg.Editor.nodeClickedEvent.bind(Wyg.Editor));
<button id="myFavoriteNode">click this favorite node</button>


Create shapes in a console I've done a job interview assignment on shapes (surprising ah?!) which is a C# console application which creates a list of shapes basically. It's available here, but for my question the only relevant part is the following:

This is where shapes are created - AddNewShape method (I presented this in the interview):

switch (pressedKey)
{
    case ConsoleKey.D1:
        shape = new Square(GetNumFromConsoleAndVerifyIt());
        break;
    case ConsoleKey.D2:
        double height = GetNumFromConsoleAndVerifyIt();
        double width = GetNumFromConsoleAndVerifyIt();
        shape = new Rectangle(height, width);
        break;
    case ConsoleKey.D3:
        shape = new Circle(GetNumFromConsoleAndVerifyIt());
        break;
    case ConsoleKey.D4:
        double triangleHeight = GetNumFromConsoleAndVerifyIt();
        double triangleWidth = GetNumFromConsoleAndVerifyIt();
        shape = new RightTriangle(triangleHeight, triangleWidth);
        break;
    default:
        throw new ArgumentException();
}

The problem that was discussed about this section is that it's not "Open for Extension" (Personally I prefer the 'Predicted Variations' version of this principle). So later on I refactored this part:

// Use reflection to get the type and it's required params:
Type shapeType = shapeTypes.Where(t => t.FullName.ToLower().Contains(shapeNameEntered.ToLower())).First();
var ctor = shapeType.GetConstructors()[0];

List<object> sides = new List<object>();
foreach (ParameterInfo pi in ctor.GetParameters())
{
    sides.Add(GetNumFromConsole(pi.Name));
}
shape = (I2DShape)ctor.Invoke(sides.ToArray());

And now you can clone my project and add whatever shape you want, implement I2DShape (also - have only one ctor) and change nothing in the Program.cs etc...

So, does this follow the principle? Did I get it right using reflection or should I have used a different approach?

I find neither approach is good.

What's wrong with the switch?

It needs to be modified with each change and even only if you want to do something with only one shape.
It's bound to key-codes.
You cannot test it.

What's wrong with reflection?

Reflection is usually the last resort solution. I think you are not at a dead end yet.
The way you implemented it is very unsafe because you use the FullName and Contains. What if a Rectangle would be in a namespace like CirclesAndSqures and you are looking for a circle? You'll find something that is not a circle but a rectangle only because its namespace has the name circle in it.
You cannot test it.

Instead I suggest the following. Let's say you have an abstract Shape

abstract class Shape { }

and some concrete types

class Circle : Shape { }

For each type you can write a factory that implements a common interface:

interface IShapeFactory
{
    Shape CreateShape();
}

Each factory of course can depend on other services like reading from the console etc.

class CircleFactory : IShapeFactory
{
    public Shape CreateShape()
    {
        // read params etc
        return new Circle();
    }
}

Finally you create a dictionary with all the shape factories

var shapeFactories = new Dictionary<string, IShapeFactory>(StringComparer.OrdinalIgnoreCase)
{
    [nameof(Circle)] = new CircleFactory()
};

var shapeName = // read shape name...

if (shapeFactories.TryGetValue(shapeName, out IShapeFactory shapeFactory))
{
    var shape = shapeFactory.CreateShape();
}

You can now add as many shapes and factories as you want without touching any other shapes or common code.


SQL Procedure that will calculate the Last Price paid for a certain Part I am trying to write an SQL Procedure that will calculate the Last Price paid for a certain Part.

DECLARE @ItemId int;

SET @ItemId = 1;

SELECT
SD.Price
FROM SalesOrderHeaders AS SH
INNER JOIN SalesOrderDetails AS SD
ON SH.Id = SD.HeaderId
WHERE (SD.LineItemId = @ItemId)
AND (SH.OrderDate = (SELECT
    MAX(SH.OrderDate) AS Expr1
FROM SalesOrderHeaders AS SH
INNER JOIN SalesOrderDetails AS SD
    ON SH.Id = SD.HeaderId
WHERE (SD.LineItemId = @ItemId))
)

This does produce the results I wanted but, I am doing everything twice. Would someone point me in a better direction?

Considering that your subselect is basically the same as your main select, chances are that this subselect is indeed a bit of overkill. Looking at your logic, what you want to achieve is to get the Price of the SalesOrderDetail linked to the SalesOrderHeader with the most recent OrderDate, so let's use an ORDER BY DESC and select only the first answer.

I think this should do the trick:

SELECT TOP 1 SD.Price
FROM SalesOrderHeaders AS SH
INNER JOIN SalesOrderDetails AS SD
    ON SH.Id = SD.HeaderId
WHERE SD.LineItemId = @ItemId
ORDER BY SH.OrderDate DESC


Simple spatial grid for particle system I am going to simulate a particle system, the particles are infinitesimal points which apply forces on the neighbors and I need a fast way of making proximity checks, they are going to have a maximum check radius but not a minimum and will be almost evenly spaced.

The simulation will be very similar to this: https://youtu.be/SFf3pcE08NM

I thought a spatial grid would be the easiest approach for it and I need it to be as cost efficient as possible to matter how ugly it gets.

Is there any optimization I can make on this code?

Is there a way to compute the optimal cell size from the average distance between particles?

_empty_set = set()

class SpatialHash:
    def __init__(self, cell_size=0.1):
        self.cells = {}
        self.bucket_map = {}
        self.cell_size = cell_size

    def key(self, co):
        return int(co[0] / self.cell_size), int(co[1] / self.cell_size), int(co[2] / self.cell_size)

    def add_item(self, item):
        co = item.co
        k = int(co[0] / self.cell_size), int(co[1] / self.cell_size), int(co[2] / self.cell_size)
        if k in self.cells:
            c = self.cells[k]
        else:
            c = set()
            self.cell_size[k] = c
        c.add(item)
        self.bucket_map[item] = c

    def remove_item(self, item):
        self.bucket_map[item].remove(item)

    def update_item(self, item):
        self.bucket_map[item].remove(item)
        self.add_item(item)

    def check_sphere(self, co, radius, exclude=()):
        r_sqr = radius * radius
        for x in range(int((co[0] - radius) / self.cell_size),
                       int((co[0] + radius) / self.cell_size) + 1):
            for y in range(int((co[1] - radius) / self.cell_size),
                           int((co[1] + radius) / self.cell_size) + 1):
                for z in range(int((co[2] - radius) / self.cell_size),
                               int((co[2] + radius) / self.cell_size) + 1):
                    for item in self.cells.get((x, y, z), _empty_set):
                        if item not in exclude and (item.co - co).length_squared <= r_sqr:
                            yield item

You could rewrite your key() method and use it to much greater effect, reducing repeated code.

def key(self, co, radius=0):
    return tuple((c + radius) // self.cell_size for c in co)

You can then call key() in both add_item() and check_sphere(). In add_item() it will replace k, while in check_sphere() you can use it to define your ranges.

After that I would look for a way to flatten those nested for loops which will likely be an algorithm change. Hopefully someone else has some ideas there.


Suggestions for Ruby string parsing I need to add quotes to each word in a string.  Here is something that works but it looks ugly to me;

"this is a test".split.to_s.delete("[],")

produces

"\"this\" \"is\" \"a\" \"test\""

split adds the quotes, to_s turns the array back to a string, then the delete removes the array stuff.  The downside is the case where the data includes [] or ,

I welcome your responses!

Here's the "naïve" way to go

"this is a test".split.map { |word| "\"#{word}\"" }.join(" ")

But a better way is to use a regular expression, since those are made specifically for string manipulation/substitution

"this is a test".gsub(/\S+/, '"\0"')

The expression matches 1 or more (the +) non-whitespace characters (the \S) in a row, and replaces the match with the same string (the \0) but surrounded by quotes.


Click on an image to zoom in I'm still a newbie when it comes to javascript, so I don't know yet how to optimize this code.

customize.js

 // Get the modal
  var modal = document.getElementById('myModal');
  var modal2 = document.getElementById('myModal2');
  var modal3 = document.getElementById('myModal3');

  // Get the image and insert it inside the modal - use its "alt" text as a      caption
  var img = document.getElementById('myImg');
  var modalImg = document.getElementById("img1");
  var captionText = document.getElementById("caption");

  var img2 = document.getElementById('myImg2');
  var modalImg2 = document.getElementById("img2");
  var captionText2 = document.getElementById("caption2");

  var img3 = document.getElementById('myImg3');
  var modalImg3 = document.getElementById("img3");
  var captionText3 = document.getElementById("caption3");

  img.onclick = function(){
 modal.style.display = "block";
 modalImg.src = this.src;
 captionText.innerHTML = this.alt;
  }

  img2.onclick = function(){
 modal2.style.display = "block";
 modalImg2.src = this.src;
 captionText2.innerHTML = this.alt;
  }

  img3.onclick = function(){
 modal3.style.display = "block";
 modalImg3.src = this.src;
 captionText3.innerHTML = this.alt;
  }
  // Get the  element that closes the modal
  var span = document.getElementsByClassName("close")[0];
  var span2 = document.getElementsByClassName("close")[1];
  var span3 = document.getElementsByClassName("close")[2];
  // When the user clicks on  (x), close the modal
  span.onclick = function() {
 modal.style.display = "none";
  }

  span2.onclick = function(){
 modal2.style.display = "none";
  }

  span3.onclick = function(){
 modal3.style.display = "none";
  }

And this how my html looks like

HTML

HTML

I noticed that you didn't put the ul or ol tag in your code, you need to put your li tags inside the a ul or ol in order to have a more semantic structure.

CSS

I did some very basic styling for the modal to look more attractive in the demo.

.modal {
    display: none;
    background: #000;
    position: fixed;
    top: 0;
    left: 0;
    width: 100%;
    height: 100%;
    opacity: .8;
}

.modal .modal-content {
    position: absolute;
    top: 50%;
    left: 50%;
    margin-top: -120px;
    margin-left: -150px;
    color: #fff;
}

.modal img, .modal span {
    z-index: 10;
}

JavaScript

I put all the images inside the images variable and also put the modal element in it's own variable.

Then I looped through each image using the forEach(), defining that for each image, which the user has clicked, it will put a display: block to the modal element and insert a piece of HTML containing an  tag and a  tag with the alt property in it.

And last but not less important, everytime that the user clicks in the modal area with the display: block activated, it will put a display: none automatically on it.

var images = document.querySelectorAll('#my-images img'),
    modal = document.querySelector('.modal');

// Loops through the all the images selected...
images.forEach(function (image) {
    // When the image is clicked...
    image.addEventListener('click', function(event) {
        modal.innerHTML = '

' + event.target.alt + '
';
        modal.style.display = 'block';
    });
});

// When the user clicks somewhere in the "modal" area it automatically closes itself
modal.addEventListener('click', function () {
    this.style.display = 'none';
});

I didn't bother to put any effect on the modal element, I was simply controlling the hide and show properties and didn't cared about fadeIn or fadeOut that were mentioned in your code.

The easiest and fastest way to make some animations, would be making use of some third-party animation library such as Animate.css that comes with tons of cool effects to work with.


Dynamic vector of void* in C I have implemented a simple dynamic vector library in C. It is a header-only library.

Full library listing -
void_vector.h:

#ifndef VOID_VECTOR_H
#define VOID_VECTOR_H

typedef enum {
    VV_SUCCESS,
    VV_MALLOC_FAILURE
} vv_err;

typedef struct vv {
    size_t length;
    size_t size;
    void *data[];
} void_vector;

void_vector* new_void_vector(size_t size);
vv_err vv_push(void_vector **vv, void *el);
void *vv_pop(void_vector *vv);
const void* vv_read_index(void_vector *vv, size_t index);
void delete_void_vector(void_vector *vv, void (del_el)(void*));

#ifdef VOID_VECTOR_IMPL
#undef VOID_VECTOR_IMPL

#include <stdlib.h>

#define defualt_size 16ul

void_vector*
new_void_vector(size_t size)
{
    if (!size) size = defualt_size;
    void_vector* vv = malloc(sizeof(void_vector) + sizeof(void*) * size);
    if (vv) {
        vv->size = size;
        vv->length = 0;
    }
    return vv;
}

vv_err
vv_push(void_vector **vv, void *el)
{
    if ((*vv)->length >= (*vv)->size) {
        void_vector *new_vv = realloc((*vv), sizeof(void_vector)
                                            + sizeof(void*) * (*vv)->size * 2);
        if (!new_vv) return VV_MALLOC_FAILURE;
        (*vv) = new_vv;
        (*vv)->size *= 2;
    }
    (*vv)->data[(*vv)->length] = el;
    (*vv)->length++;
    return VV_SUCCESS;
}

void*
vv_pop(void_vector *vv)
{
    if(vv->length == 0) return NULL;
    vv->length--;
    return vv->data[vv->length];
}

const void*
vv_read_index(void_vector *vv, size_t index)
{
    if (index > vv->length) return NULL;
    return (vv->data[index]);
}

void
delete_void_vector(void_vector *vv, void (del_el)(void*))
{
    if (!del_el) del_el = &free;
    for (int i = vv->length; i; i--) {
        del_el(vv->data[i-1]);
    }
    free(vv);
}

#endif /* VOID_VECTOR_IMPL */
#endif /* VOID_VECTOR_H */

I am using this as test bench - void_vector_tb.c

#include <stdio.h>

#define VOID_VECTOR_IMPL
#include "void_vector.h"

int
main (int argc, char **argv)
{
    void_vector* vv = new_void_vector(4);

    char *strings[10];

    for (int i = 0; i < 10; i++) {
        strings[i] = malloc(32);
        snprintf(strings[i], 32, "This is String: %d", i);
    }

    for (int i = 0; i < 10; i++) {
        vv_push(&vv, strings[i]);
    }

    for (int i = 0; i < vv->length; i++) {
        printf("%d:\t%s\n", i+1, vv_read_index(vv, i));
    }

    char *s;
    while(s = (char*) vv_pop(vv)) {
        printf("%s\n", s);
    }

    for (int i = 0; i < 10; i++) {
        vv_push(&vv, strings[i]);
    }

    delete_void_vector(vv, NULL);
}

I am not using a make file. Compilation is performed by gcc -ggdb void_vector_tb.c -o void_vector_tb

I would greatly appreciate your time and any comments but I'm particularly interested in the following points:

Am I doing anything dangerous in this?
Is the code clear, what can I do to make it easier to follow?
Is there an obvious way to greatly improve the efficiency?
General style comments
I'm not overly concerned about the contents of void_vector_tb.c.

I ran this through valgrind and got the following output:

==7009== HEAP SUMMARY:
==7009==     in use at exit: 0 bytes in 0 blocks
==7009==   total heap usage: 14 allocs, 14 frees, 1,616 bytes allocated
==7009==
==7009== All heap blocks were freed -- no leaks are possible
==7009==
==7009== For counts of detected and suppressed errors, rerun with: -v
==7009== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

I am trying to minimize use of external libraries (although I don't think it would be possible to avoid stdlib.h).

Overall, fairly good.

  Am I doing anything dangerous in this?

Lack of comments

I'd expect at least some commentary about the roll of the functions and how to use them near the function declaration section.

Assume a user will read the header and not delve into implementation.

Unexpected type change

The stored ellement is a void *, changing the return type to const is not good.

// const void* vv_read_index(void_vector *vv, size_t index)
void* vv_read_index(void_vector *vv, size_t index)

Overuse of NULL

vv_pop(), vv_read_index() return NULL as an error indicator and can return NULL as a valid return.  If NULL is not to be allowed (which I think is a bad idea) as a storable void *, vv_push(&vv, NULL) should error.

I'd re-work calls to return an error code on vv_pop(), like vv_err vv_pop(void_vector **vv, void **el).

It seems odd to have 2 forms of error signaling: error codes and NULL.

"default size"

I see no real value in a default size of 16.  Suggest setting aside size == 0 as something special and allow an initial vector size of 0.  Add code to handle doubling array size when size is 0.

Create a VV_DEFAULT_SIZE 16u if desired for the user to call with new_void_vector(VV_DEFAULT_SIZE).

Overflow

I'd detect overflow in size computations.

Tolerate freeing NULL

free(NULL) is well defined.  I'd expect the same with delete_void_vector(NULL, foo).  Currently code dies on this.

  Is the code clear, what can I do to make it easier to follow?

Naming convention

Recommend to use the same one prefix with all external types, names and functions.

// void_vector.h
vv.h

// VOID_VECTOR_H
VV_H

// void_vector* new_void_vector(size_t size);
void_vector* vv_new(size_t size);

Or use void_vector instead of vv throughout, not both.

Allocate to the size of the object, not type

Allocating to the size of the referenced object is 1) less error prone, 2) easier to review 3) easier to maintain than allocating to the type.

// void_vector* vv = malloc(sizeof(void_vector) + sizeof(void*) * size);
void_vector* vv = malloc(sizeof *vv + sizeof *vv->data * size);

Avoid unnecessary suffixes

An L, l, LL, ll is needed a lot less than one thinks.  In the following case, L certainty not needed.  The u is useful though.  I am curios as to why OP thought is was needed - what issue was of concern?

/// #define defualt_size 16ul
#define defualt_size 16u

Use const

const would help convey that the function is not altering *vv

// vv_read_index(void_vector *vv ...
vv_read_index(const void_vector *vv ...

  Is there an obvious way to greatly improve the efficiency?

Allocations only increase as vv_pop() does not reduce things. This can make for an inefficiency of memory usage.

IMO, a more advanced  scheme could decrease by 1/2 when size needed falls below 25% or 33%.

  General style comments

Spell check

// #define defualt_size 16ul
#define default_size 16ul

! vs. >

Minor: With arithmetic concerns, > is more readily understood that ! negation.

// if (!size)
if (size > 0)

  I'm not overly concerned about the contents of void_vector_tb.c.

Avoid naked magic numbers

Replace 10 with #define VV_STR_N 10,  32 with #define VV_STR_SZ 32,

Cast not needed

// while(s = (char*) vv_pop(vv)) {
while(s = vv_pop(vv)) {


JavaScript/ECMAscript 6 classes organization I have been trying to wrap my head around all the new options in ECMAscript 6 and to do this I tried to develop a simple setup which would allow me to place absolutely positioned <div>s on the page.

main.js:

import {Screen} from './objects/screen';
import {Position} from './objects/position';

var welcomeScreen = new Screen("Hello World");
welcomeScreen.title = "Hello World2";
welcomeScreen.position = new Position(100,100);
//Make sure the following does work if you have any advices, because there is a simple mistake which breaks this:
console.log(welcomeScreen.position.x);

var secondScreen = new Screen("Second screen", new Position(200,200));

objects/baseobject.js:

export class BaseObject{
    on(eventname,func){
        if(typeof this.listeners == "undefined"){
            this.listeners = {};
        }
        if(typeof this.listeners[eventname] == "undefined"){
            this.listeners[eventname] = [];
        }
        this.listeners[eventname].push(func);
        return this;
    }
    trigger(eventname){
        if(this.listeners && this.listeners[eventname]){
            for(let func of this.listeners[eventname]){
                func.call(this);
            }
        }
    }
}

objects/screen.js:

import {BaseObject} from './baseobject';
// private properties:
var pp = {
    position: Symbol(),
    title: Symbol()
};
export class Screen extends BaseObject{
    constructor(title,position = new Position(0,0)){
        this.element = document.createElement("div");
        this.element.classList.add("screen");
        this.title = title;
        this.position = position;
        document.body.appendChild(this.element);
    }
    reposition(){
        this.element.style.left = this.position.x + "px";
        this.element.style.top = this.position.y + "px";
    }
    set position(position){
        var that = this;
        this[pp.position] = position;
        this[pp.position].on("positionchange",function(){
            that.reposition();
        }).trigger("positionchange");
    }
    get position(){
        return this[pp.position];
    }
    set title(value){
        this[pp.title] = value;
        this.element.textContent = value;
    }
    get title(){
        return this[pp.title];
    }
}

objects/position.js:

import {BaseObject} from './baseobject';
// private properties:
var pp = {
    x: Symbol(),
    y: Symbol()
};
export class Position extends BaseObject{
    constructor(x,y){
        this[pp.x]=x;
        this[pp.y]=y;
    }
    set x(value){
        this[pp.x]=x;
        super.trigger("positionchange");
    }
    get x(){
        return this[pp.x];
    }
    set y(value){
        this[pp.y]=y;
        super.trigger("positionchange");
    }
    get y(){
        return this[pp.y];
    }
}

You can transpile ES6 to ES3 (I think it is 3) using (Google) Traceur.

Points of interest:

Are there nice ES6 language features I am forgetting?
Is there a nicer way to handle the 'set triggers' (-> private property construction)?
Should I be using modules? Combine classes into the same files? etc.

Most interesting,

In BaseObject, you should really have a constructor that builds this.listeners, it would make your code much cleaner afterwards
I am not sure why your listeners are not private in BaseObject ?
I am not sure what is more important to you, learn EC6 or position divs efficiently, if the latter is more important, than this code is massive overkill ;)

If you want to avoid having a constructor for BaseObject, then I would re-write on like this:

on(eventname,func){
    this.listeners = this.listeners || [];
    this.listeners[eventname] = this.listeners[eventname] || [];
    this.listeners[eventname].push(func);
    return this;
}


Is this an ok implementation of an UnorderedPair? I want to create pair, where new UnorderedPair(ObjA, ObjB) == UnorderedPair(ObjB, ObjA) returns true. I also want to avoid any (un)boxing.

Have I gone about this in the correct way?

public struct UnorderedPair<T> : IEquatable<UnorderedPair<T>>
{
    public T A;
    public T B;

    public UnorderedPair(T a, T b)
    {
        A = a;
        B = b;
    }

    public override int GetHashCode()
    {
        return A.GetHashCode() ^ B.GetHashCode();
    }

    public bool Equals(UnorderedPair<T> other)
    {
        return (other.A.Equals(A) && other.B.Equals(B)) ||
               (other.A.Equals(B) && other.B.Equals(A));
    }

    public override bool Equals(object obj)
    {
        return Equals((UnorderedPair<T>)obj);
    }

    public static bool operator ==(UnorderedPair<T> a, UnorderedPair<T> b)
    {
        return a.Equals(b);
    }

    public static bool operator !=(UnorderedPair<T> a, UnorderedPair<T> b)
    {
        return !(a == b);
    }
}

One thing: I would make the struct immutable since mutable structs have really bad unintended effects. Otherwise, this is a good, solid implementation.
public struct UnorderedPair<T> : IEquatable<UnorderedPair<T>>
{
    private readonly T a;

    private readonly T b;

    public UnorderedPair(T a, T b)
    {
        this.a = a;
        this.b = b;
    }

    public T A
    {
        get
        {
            return this.a;
        }
    }

    public T B
    {
        get
        {
            return this.b;
        }
    }

    public override int GetHashCode()
    {
        return this.a.GetHashCode() ^ this.b.GetHashCode();
    }

    public bool Equals(UnorderedPair<T> other)
    {
        return (other.A.Equals(this.a) && other.B.Equals(this.b)) ||
               (other.A.Equals(this.b) && other.B.Equals(this.a));
    }

    public override bool Equals(object obj)
    {
        return this.Equals((UnorderedPair<T>)obj);
    }

    public static bool operator ==(UnorderedPair<T> a, UnorderedPair<T> b)
    {
        return a.Equals(b);
    }

    public static bool operator !=(UnorderedPair<T> a, UnorderedPair<T> b)
    {
        return !(a == b);
    }
}

and because time marches on, here's a more compact and performant C# 7.2 version:
public readonly struct UnorderedPair<T> : IEquatable<UnorderedPair<T>>
{
    public UnorderedPair(in T a, in T b) => (this.A, this.B) = (a, b);

    public T A { get; }

    public T B { get; }

    public override int GetHashCode() => this.A.GetHashCode() ^ this.B.GetHashCode();

    public bool Equals(UnorderedPair<T> other) => (other.A.Equals(this.A) && other.B.Equals(this.B))
        || (other.A.Equals(this.B) && other.B.Equals(this.A));

    public override bool Equals(object obj) => this.Equals((UnorderedPair<T>)obj);

    public static bool operator ==(in UnorderedPair<T> a, in UnorderedPair<T> b) => a.Equals(b);

    public static bool operator !=(in UnorderedPair<T> a, in UnorderedPair<T> b) => !(a == b);
}


Personal book library Very new to OOP with Python - and feel like I still don't totally get OOP. However I thought I'd put together a personal online book library to practice inheritance, methods, docstrings etc.
Program allows one to add books (comics or novels - with comics inheriting from novels) to one's own library, and create a custom reading lists.
There are a few different methods. For example, a library method which will return all books over a specified length.
Note that I've removed quite a lot from this, just so it's not 5 pages long! I would normally have several different types of books, attributes such as Author etc. But hopefully from the below you get a good idea of the structure.
Speaking of structure, I would just like to know if mine is any good. Also there is an issue whereby if you remove a book from the library, it will still show up in any reading lists I created. I don't know if there's a way to deal with this, or if I'm thinking about this the wrong way.
class Library:

    def __init__(self, books = None):
        if books is None:
            self.books = []
        else:
            self.books = books

    def add_book(self, book):
        self.books.append(book)

    def remove_book(self, book):
        self.books.remove(book)

    def pages_over_n(self, number):
        """Returns books that have more than specified number of pages"""
        return [book for book in self.books if book.pages > number]

    def get_library(self):
        """Returns all books in Library"""
        return self.books

    def __repr__(self):
        return f"Library({self.books})"

    def __str__(self):
        return f"Library has {len(self.books)}"

class Novel:

    def __init__(self, title, pages, publicationDate):
        self.title = title
        self.pages = pages
        self.publicationDate = publicationDate

    def __repr__(self):
        return f"Novel({self.title}, {self.pages}, {self.publicationDate})"

    def __str__(self):
        return f"Media: {self.title}"

class ComicBook(Novel):

    def __init__(self, title, pages, publicationDate, artist):
        super().__init__(title, pages, publicationDate)
        self.artist = artist

    def get_artist(self):
        return f"Artist: {self.artist}"

    def __repr__(self):
        return (f"ComicBook({self.title}, {self.pages}, "
                f"{self.publicationDate}, {self.artist})")

class ReadingList:

    def __init__(self):
        self.list = []

    def add_book(self, book):
        """Add book to reading list"""
        self.list.append(book)

    def remove_book(self, book):
        """Remove book from reading list"""
        self.list.remove(book)

    def total_pages(self):
        """Returns total pages in reading list"""
        total = 0
        for book in self.list:
            total += book.pages
        return total

    def __repr__(self):
        return f"ReadingList({self.list})"

    def __str__(self):
        return f"Reading list of size {len(self.list)}"

# Initialise Library
library = Library()

# Create a few books
novel1 = Novel("Harry Potter", 500, 1991)
novel2 = Novel("LotR", 1000, 1960)
novel3 = Novel("Game of Thrones", 2000, 2018)
comic1 = ComicBook("Batman", 100, 2020, "Stan Lee")

# Add books to library.
library.add_book(novel1)
library.add_book(novel2)
library.add_book(novel3)
library.add_book(comic1)

# Create a new reading list.
readingListOne = ReadingList()

# Add a couple of books to reading list.
readingListOne.add_book(novel1)
readingListOne.add_book(comic1)

Simplify the constructor of class Library
You have a default value of None for the parameter books, but then immediately proceed to convert that None to an empty list. Why not use [] as the default value?
class Library:
    def __init__(self, books = []):
        self.books = list(books)

    ...

Note that to prevent the issue of mutable default arguments as mentioned by ojdo, you should make a copy of the initial list.
Be consistent
You take an optional list of books in the constructor of class Library, but there is no such thing in class ReadingList. That is a bit surprising.
Also be consistent with naming things. In class Library, the list of books is called books, but in class ReadlingList you named it list. I would name it books in both cases.
Also be consistent with the names of functions:

get_library() should be get_books(), since it only returns the list of books, not an object of type Library.
pages_over_n() should be get_books_with_over_n_pages() or something similar, to indicate that it returns Books, not pages.
total_pages() should be renamed get_total_pages() or calculate_total_pages(). Prefer using verbs for function names, and nouns for variables.

And I also wonder why a ComicBook has an artist, but a Novel has no author? And why is there a get_artist() but no get_title(), get_publication_date() and so on? Making everything consistent will make using your classes much easier.
Removing books from the Library
As you noticed, you can remove a book from the library, but it won't be removed from any reading lists. But that is fine. Why would a book magically disappear from a real life reading list if your library removes that book? Note that your code is perfectly fine, Python won't actually delete the Book object from memory until the last reference to it is gone.
However, if you want a book to disappear from the ReadingList when it is removed from the Library, then somehow there needs to be some communications between those two objects. There are two options:

A Library keeps a list of ReadingLists that reference its Books, and when you call remove_book() on a Library, it will in turn iterate over all the ReadingLists and call remove_book() on them as well.

A ReadingList keeps a reference to the Library whose Books it contains. Whenever you call a member function that accesses the list of books on the reading list, this member function first has to filter out all the books that are no longer in the Library.


Evaluate an expression binary tree - Daily Coding Challenge Here is my solution for the Daily Coding Challenge 50

  Given an arithmetic expression in the form of a binary tree, write a function to evaluate it

Example

    *
   / \
  +    +
 / \  / \
3  2  4  5

Return >> 45

Is this the most efficient way to do this?

"""Build an operation tree and calculate the result"""

class Tree:
    def __init__(self, value, left=None, right=None):
        """Initialise values"""
        self.value = value
        self.left = left
        self.right = right

    def determine_sum(self, total):
        if self.left == None and self.right == None:
            return int(self.value)
        if self.value == "*":
            total += self.left.determine_sum(total) * self.right.determine_sum(total)
        if self.value == "+":
            total += self.left.determine_sum(total) + self.right.determine_sum(total)
        if self.value == "-":
            total += self.left.determine_sum(total) - self.right.determine_sum(total)
        if self.value == "/":
            total += self.left.determine_sum(total) / self.right.determine_sum(total)
        return total

if __name__ == "__main__":  #
    n = Tree("*")
    n.left = Tree("+")
    n.right = Tree("+")
    n.left.right = Tree("+")
    n.right.right = Tree("+")

    n.left.left = Tree("3")
    n.left.right.left = Tree("4")
    n.left.right.right = Tree("5")

    n.right.left = Tree("6")
    n.right.right.left = Tree("7")
    n.right.right.right = Tree("4")

    sum = n.determine_sum(0)
    print(sum)

dict > ifs
operator contains all the functions you need.
Taking total as an argument is unneeded.
I would personally split the 'Tree' which is actually a Node into two types, operators and values. But that may go against the challenge.
Use is to compare to None.

import operator

operators = {
    '*': operator.mul,
    '+': operator.add,
    '-': operator.sub,
    '/': operator.truediv,
}

class Tree:
    def __init__(self, value, left=None, right=None):
        """Initialise values"""
        self.value = value
        self.left = left
        self.right = right

    def determine(self):
        if self.left is None and self.right is None:
            return int(self.value)

        return operators[self.value](
            self.left.determine(),
            self.right.determine()
        )