finding the conflicts in meetings given startTimes and end Times Code to find conflicts in a list of meetings given as input.
Meetings are defined as {startTime, endTime}. I am using a Greedy Algorithm to find the conflicts. Wanted to know if there is a more efficient way of solving this problem.

Example:

Input: `meetings[] = { {1, 5} {3, 7}, {2, 6}, {10, 15}, {5, 6}, {4, 100}}`
Output: Number of Conflicts : 4

The code:

// Meeting Class
class Meeting implements Comparable<Meeting> {
    int startTime;
    int endTime;
    public Meeting(int startTime, int endTime) {
        super();
        this.startTime = startTime;
        this.endTime = endTime;
    }
    @Override
    public int compareTo(Meeting o) {
        if(this.startTime == o.startTime) return 0;
        if(this.startTime < o.startTime) return -1;
       return 1;
    }

}

Class which computes the conflicts

public class FindConflictingMeetingsProblem {
     // static function to find conflicts
    public static int findNumberOfConflicts(List<Meeting> meetings){

        // sort the meetings in ascending order of start time
        Collections.sort(meetings,new Comparator<Meeting>(){
            @Override
            public int compare(Meeting o1, Meeting o2) {
                return o1.compareTo(o2);
            }
        });

        int conflicts = 0;
        // count the conflicts
        for(int i = 0 ; i < meetings.size();i++){
            if(i > 1 && meetings.get(i-1).endTime > meetings.get(i).startTime){
                conflicts++;
            }
        }
        return conflicts;
    }

    public static void main(String[] args) {
            Meeting m1 = new Meeting(1, 2);
            Meeting m2 = new Meeting(2, 5);
            Meeting m4 = new Meeting(4, 5);
            Meeting m5 = new Meeting(7, 8);
            List<Meeting> meetingsList = new ArrayList<Meeting>();
            meetingsList.add(m1);
            meetingsList.add(m2);
            meetingsList.add(m4);
            meetingsList.add(m5);
            System.out.println(" Number of conflicts :"+findNumberOfConflicts(meetingsList));
     }
}

Advice 1

int startTime;
int endTime;

I suggest you hide them with private access modifier:

private int startTime;
private int endTime;

Advice 2

Elaborating on Advice 1 you can add the getters for the two above fields:

public int getStartTime() {
    return startTime;
}

...

Advice 3

 Collections.sort(meetings,new Comparator<Meeting>(){
                @Override
                public int compare(Meeting o1, Meeting o2) {
                    return o1.compareTo(o2);
                }
            });

Since you already have the compareTo method in your Meeting class, you can write more succinctly:

Collections.sort(meetings);

Advice 4

What comes to Meeting.compareTo, you can write shorter:

@Override
public int compareTo(Meeting o) {
    return Integer.compare(startTime, o.startTime);
}

Advice 5

Consider writing a method that tests whether two given meetings are conflicting each other:

public boolean conflictsWith(Meeting meeting) {
    if (endTime <= meeting.startTime) {
        return false;
    }

    if (meeting.endTime <= startTime) {
        return false;
    }

    return true;
}

Advice 6

Consider checking in the constructor of Meeting whether startTime <= endTime:

private void checkStartAndEndTimes(int startTime, int endTime) {
    if (startTime >= endTime) {
        throw new IllegalArgumentException("Start time (" + startTime +
                ") >= end time (" + endTime + ").");
    }
}

Advice 7

public Meeting(int startTime, int endTime) {
                super();
                ...

Above super does not buy you anything, consider removing it.

Finally

The main algorithm is now a little bit more readable:

class FindConflictingMeetingsProblem {

    public static int findNumberOfConflicts(List<Meeting> meetings) {
        Collections.sort(meetings);
        int conflicts = 0;

        for (int i = 0; i < meetings.size() - 1; ++i) {
            if (meetings.get(i).conflictsWith(meetings.get(i + 1))) {
                conflicts++;
            }
        }

        return conflicts;
    }
}

Algorithm limitations

Consider this:

Meeting m1 = new Meeting(1, 10);
Meeting m2 = new Meeting(5, 9);
Meeting m3 = new Meeting(4, 7);
List<Meeting> meetingsList = new ArrayList<>();
meetingsList.add(m1);
meetingsList.add(m2);
meetingsList.add(m3);

The above will report 2, but there is 3 pairs of meetings that conflict each other.

Hope that helps.


Optimized thumbnail generation Here is a snippet of code that generates a thumbnail from a given file.  It works well, but is much slower than desired.  Without using an external library (which I'm trying to avoid for learning purposes), is there anything that can be done to increase the speed?

I know the fastest way would be to retrieve the stored thumbnail.  I've looked into reading for any EXIF thumbnail data by parsing out the embedded information, but gave up since it was taking me a lot of effort with no finished end result.  Again, I'd prefer not to rely on a library specifically because I want to learn more about the format itself.

I know many will state right away I shouldn't be messing with EXIF data unless I use an external library, so as an alternative, what can be done specifically with this snippet to make things (significantly) faster?

Feel free to give me feedback about my coding style as well, except for the use of JavaDoc comments.

class ThumbIcon extends ImageIcon implements Runnable {
   public int    thumbWidth;
   public int    thumbHeight;

   Image         image;
   Graphics2D    g2dBuffer;
   ThumbButton   parent;
   BufferedImage scaledImage;

   public ThumbIcon(final ThumbButton theParent) {
      super();
      parent = theParent;
   } // End ThumbIcon(ThumbButton, String)

   public void run() {
                         image   = Toolkit.getDefaultToolkit().getImage(filePath);
      final MediaTracker tracker = new MediaTracker(new Container());
      tracker.addImage(image, 0);
      try {
         tracker.waitForID(0);
      } catch (InterruptedException e) {
         System.out.println("Interrupted getting thumb");
      } // End try

      // Get dimensions from the now-loaded image
      final int width   = image.getWidth(null);
      final int height  = image.getHeight(null);

      // Use the width to determine if the image is legitimate and loaded
      // Limit importable images to <54MiB or <51.5MP (9000px x 6000px)
      if ((width > 0) && (width * height < 54000000)) {
         // This is an actual image, so display on screen

         // Set the thumbnail size depending on image orientation
         // We are assuming a square thumbnail
         if (width > height) { // Landscape
            thumbWidth  = THUMBS_SIZE;
            thumbHeight = (int)((double)height / (double)width * THUMBS_SIZE);
         } else {              // Portrait
            thumbWidth  = (int)((double)width / (double)height * THUMBS_SIZE);
            thumbHeight = THUMBS_SIZE;
         } // End if

         scaledImage = new BufferedImage(thumbWidth, thumbHeight,
                                         BufferedImage.TYPE_INT_RGB);

         // Simultaneously load image into g2d buffer and scale to desired size
         g2dBuffer = scaledImage.createGraphics();
         g2dBuffer.setRenderingHint(RenderingHints.KEY_INTERPOLATION,
                                    RenderingHints.VALUE_INTERPOLATION_NEAREST_NEIGHBOR);

         //g2dBuffer.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
         //                           RenderingHints.VALUE_ANTIALIAS_ON);

         // Size image (no matter initial size) to destWidth x destHeight
         g2dBuffer.drawImage(image, 0, 0, thumbWidth, thumbHeight, null);
         ...
         // Create the thumbnail
         setImage(Toolkit.getDefaultToolkit().createImage(scaledImage.getSource()));
         scaledImage.flush();
         image.flush();
         //g2dBuffer.finalize();
         g2dBuffer = null;
      } else {
         ...
      } // End if
   } // End run()
   ...
} // End ThumbIcon

I shaved off some time by disabling anti-aliasing.  It looks nasty, but it serves its purpose.

You probably reached the maximum speed you can achieve with the API you are using. You could try the following extra hints, just in case they’re not the default values:

g2dBuffer.setRenderingHint(RenderingHints.KEY_RENDERING,
                                              RenderingHints.VALUE_RENDER_SPEED);
g2dBuffer.setRenderingHint(RenderingHints.KEY_ANTIALIASING,
                                              RenderingHints.VALUE_ANTIALIAS_OFF);
g2dBuffer.setRenderingHint(RenderingHints.KEY_DITHERING,
                                              RenderingHints.VALUE_DITHER_DISABLE);
g2dBuffer.setRenderingHint(RenderingHints.KEY_COLOR_RENDERING,
                                              RenderingHints.VALUE_COLOR_RENDER_SPEED);

Also, try replacing TYPE_INT_RGB with TYPE_INT_BGR or TYPE_3BYTE_BGR to experiment with how efficiently the Java graphics libraries do their byte packing. But it’s pretty unlikely you’ll see a noticeable difference.


Dynamic module import in Python with exception handling import nltk

req_modules = {'from nltk import punkt': 'punkt',
               'from nltk.corpus import stopwords': 'stopwords',
               'from nltk import pos_tag': 'averaged_perceptron_tagger',
               'from nltk import ne_chunk': 'maxent_ne_chunker'}

for m in req_modules:
    try:
        print("Trying: '%s'" % m)
        exec(m)    # try to import the package
        print("Success.")
    except (LookupError, ImportError):
        # if data not found (not already installed), download it
        print("Tried: %s. Resource '%s' was not available \
              and is being downloaded.\n" % (m, req_modules[m]))
        nltk.download(req_modules[m])
        exec(m)

I have some code I'm distributing to folks at work, and the lines above are at the top of the file. In order to run the code underneath (which harnesses several subpackages in nltk), the user needs to have downloaded these subpackages. Is this is safe/reliable way of doing dynamic module import? It has worked for me thus far.

I mean I'm not a fan of it since it's basically just a list of
statements to execute, but then again, the solution via globals and
importlib
doesn't look that much better:

import importlib
import nltk

req_modules = {'nltk.punkt': 'punkt',
               'nltk.corpus.stopwords': 'stopwords',
               'nltk.pos_tag': 'averaged_perceptron_tagger',
               'nltk.ne_chunk': 'maxent_ne_chunker'}

def try_load(module, name):
    print("Trying to load: '%s'" % module)
    globals()[name] = importlib.import_module(module)
    print("Success.")

for module, name in req_modules.items():
    try:
        try_load(module, name)
    except (LookupError, ImportError):
        # if data not found (not already installed), download it
        print("Tried to load: '%s'. Resource '%s' was not available \
               and is being downloaded.\n" % (module, name))
        nltk.download(name)
        try_load(module, name)

N.b. I'd say use a list of module/name pairs instead to enforce the
order to make diagnostics a bit easier (or load dependencies for one of
the later modules first).

Now that I read it again, this seems alright if it's in a module and reusable, otherwise doesn't look like it's less safe than other options that involve nltk.download and it provides informative output while it gets things, so that's nice.


Review the vimrc for a Python programmer I am primarily a Python programmer and have finally ditched the IDE in favour of vim and I will admit, I am loving it !

My vimrc file looks like this:

autocmd BufRead,BufNewFile *.py syntax on
autocmd BufRead,BufNewFile *.py set ai
autocmd BufRead *.py set smartindent cinwords=if,elif,else,for,while,with,try,except,finally,def,class
set tabstop=4
set expandtab
set shiftwidth=4
filetype indent on

Any changes I should make to make my Python vim experience more pleasant?

I like to add the following:

" Allow easy use of hidden buffers.
" This allows you to move away from a buffer without saving
set hidden

" Turn search highlighting on
set hlsearch

" Turn on spelling
" This auto spell checks comments not code (so very cool)
set spell

" tabstop:          Width of tab character
" expandtab:        When on uses space instead of tabs
" softtabstop:      Fine tunes the amount of white space to be added
" shiftwidth        Determines the amount of whitespace to add in normal mode
set tabstop     =4
set softtabstop =4
set shiftwidth  =4
set expandtab


PHP MySQL connection class I'm trying to build a class that connects to the database. It does the job but it is not very elegant. I also have no understanding of singleton class or dependency injection.

Is this safe? Can you explain how I can make this more elegant in simple language?

Also, I use the $logs array instead of commenting. I got some remarks on it, while it makes debugging easy for me (I call print_r() on the logs of certain classes during development). What do you guys think?

<?php
  class database{
    private $host = DB_HOST;
    private $user = DB_USER;
    private $pass = DB_PASS;
    private $name = DB_NAME;

    public $db_connection = false;
    public $logs = array();

    public function __construct(){
      $this->logs[] = "Checking if the database is already open.";
            if($this->db_connection != false){
                $this->logs[] = "The database connection is already open.";
            }
            else{
                $this->logs[] = "The database connection is not already open.";
                try{
                    $this->db_connection = new PDO('mysql:host='. DB_HOST .';dbname=' . DB_NAME .';charset=utf8', DB_USER, DB_PASS);
                    $this->logs[] = "A new database connection has been established.";
                }
                catch (PDOException $e){
                    $this->logs[] = "A new database connection could not be established.";
                }
            }
    }
  }
?>

I use the class like this:

if($db_connection){
 $query_search = $db_connection->prepare('SELECT * FROM ...');
 $query_search->bindValue(':sqlvar', $phpvar, PDO::PARAM_STR);
 $query_search->execute();
 $query_result = $query_search->fetch();
}
else{
 // Failed connection
}

From the things I see in your code it looks to me like you are quite new to Object Oriented Programming (OOP), so I will try to keep that in mind during this review.

Basic OOP

The __construct() method of a class is called only once per object of that class, when it is instantiated like so: $db = new Database(). Your class contains a field $db_connection, which default to false. Since the __construct() method is called only when the object is being instantiated, there is no reason to check if ($this->db_connection != false), since $db_connection will always be false for that object.

Static vs non-static

It seems to me like you are misunderstanding how regular class fields and static class fields work. Lets do a quick example based on your code:

$database1 = new Database();
$database2 = new Database();

I now have two instantiated Database objects. Lets assume that when I created $database1, the connection succeeded, so a PDO object will be assigned to $db_connection:

// Will print some stuff about the PDO object
print_r($database1->db_connection);

However, when creating $database2, the $db_connection field will be set to false for $database2, because it is in no way related to the $db_connection field of any other instantiated Database object.

If you were to make $db_connection a static field, then they would be shared between all class instances, because it would be a field on the actual class itself, and not on the instantiated objects.

Using fields

What else is weird is that you assign the global variables DB_HOST, DB_NAME, DB_PASS and DB_USER to some private fields of the class, and then in the constructor you do not use the private fields, but you use the global variables again. This is very unusual and should definitely be removed like so:

$this->db_connection = new PDO('mysql:host='. $this->host .';dbname=' . $this->user .';charset=utf8', $this->name, $this->pass);

Now that we have that out of the way, you should not be setting private fields of a class based on some global variable that might be there. You should change it so they have to be passed to the constructor like so:

public function __construct($host, $user, $password, $name) {
    // Use the variables provided as arguments to the constructor here
}

This makes your class less dependent on your specific setup and global variables and allows for easier reuse and testing of your class. If you for whatever reason need to setup a connection to another database, you can reuse the class with different parameters. You can now use it like this:

$database = new Database(DB_HOST, DB_USER, DB_PASS, DB_NAME);

Furthermore, now that these values are passed as arguments to the constructor, you no longer need the fields to store them in, because you only use them in the constructor!

At this stage into the code review, your class would look something like this:

<?php
class database {
    public $db_connection = false;
    public $logs = array();

    public function __construct($host, $user, $pass, $name) {
        $this->logs[] = "Attempting to connect to the database.";

        try{
            $this->db_connection = new PDO('mysql:host='. $host .';dbname=' . $name .';charset=utf8', $user, $pass);
            $this->logs[] = "A new database connection has been established.";
        }
        catch (PDOException $e) {
                $this->logs[] = "A new database connection could not be established.";
        }
    }
}

One small nitpick, you should get into the habit of not ending PHP files with the ?> closing tag. See this question for an explanation.

Logging with interfaces

One final thing I will just briefly mention: You shouldn't be logging like this. You should be logging to an interface. Consider the interface:

interface Logger {
    public function log($data)
}

Now your Database class could take an optional Logger object in the constructor like so:

public function __construct($host, $user, $pass, $name, Logger $logger = NULL) {
    $this->logger = logger;
}

Now you can replace $this->logs[] = "..."; with $this->logger->log("...");. The beauty of this is that your Database class no longer cares how the data is being logged, only that it is being logged. You could create a ConsoleLogger that logs to the console or a FileLogger, that logs the data to a file. Any of these could be passed to the constructor of a new Database object, as long as they implement the Logger interface.

Edit: The Singleton

Since you verified that you attempted to make a singleton, I will talk about singletons for a bit. By definition, a singleton is a class that can only be instantiated once. What does this mean? This means that you should not be able to do this:

$db1 = new DatabaseSingleton();
$db2 = new DatabaseSingleton();

You have to ensure that only one instance of this class is made. You might get sneaky and attempt to do something like this:

// Don't do this!
class DatabaseSingleton() {
    private static $instance_count = 0;

    public function __construct() {
        if (self::$instance_count > 0) {
            throw new Exception("This singleton has already been instantiated!");
        }

        self::$instance_count += 1;
    }
}

This would technically do the trick, as a second instantiation of the class would throw an exception. Notice how I have used a static variable to achieve this.

The "singleton" above is actually a very bad implementation and should be avoided. The generally accepted way of creating a singleton in PHP looks something like this:

public class DatabaseSingleton {
    private static $instance;

    private __construct() {

    }

    public function getInstance() {
        if (empty(self::$instance)) {
            self::$instance = new DatabaseSingleton();
        }

        return self::$instance;
    }
}

A few clever tricks have been used here. First of all, I have made the constructor private! This means that the constructor can only be called from within the class itself, which sounds weird, but is the key to singletons. It means you can no longer do this outside of the DatabaseSingleton class:

// An error occurs because the constructor is private
$db = new DatabaseSingleton();

I have also created a public and static getInstance() method. This method first checks if the static field self::$instance already contains something. If it does not, then it instantiates a new DatabaseSingleton object and stores it in the self::$instance field. Finally it returns the field. The second time this method is called, the if statement will be skipped because self::$intsance is no longer empty.

The instance can now be retrieved as follows from anywhere in the code:

$db = DatabaseSingleton::getInstance();

Tada! There you have your singleton. But notice how I said anywhere. This basically means that you now have a global variable, but enclosed in a class. Singletons can sometimes be useful, but you should think real hard about whether or not you really need/want one before using this design pattern.

I hope this little extra section was informative for you!


Get last word of the string We have to print the last word of the given string. How to improve and optimise this code?

#include <iostream>

std::string get_last_word(const std::string& str)
{
    if (str.length() == 0)
    {
        std::cerr << "No string\n";
        return 0;
    }

    int len = str.length();
    int i = len - 1;
    while (i >= 0 && str[i] != ' ')
    {
        i--;
    }
    std::string last_word;
    for (int j = i + 1; j < len; j++)
    {
        last_word += str[j];
    }
    return last_word;
}

int main()
{
    std::string str;
    std::cout << "Enter string :\n";
    std::getline(std::cin, str);
    std::cout << "Last word :\n";
    std::cout << get_last_word(str);
    std::cout << std::endl;
}

Be careful with types. For example, your get_last_word function is meant to return a string but in your error case you return 0. If you want to return an empty string, use return "";

I'm guessing that you might be working under restrictions about what library functions you're allowed to use here, but you don't say so.

If you're allowed to use anything in the standard library, have a look at std::string::find_last_of as a replacement for your first loop.

Similarly, look at std::string::substr as a replacement for your second loop. Building a string one character at a time is generally discouraged because it's quite inefficient. Something like substr will be more efficient because it can, for example, allocate all the memory needed by your string in one go.


Parsing data from JSON API in Pandas I made a little program to parse data from an API.
I do not have experience with Pandas. It is working but I would like to know how to do that better and more efficiently.

class Albion_data():
    def __init__(self,item):
        self.location = f'Bridgewatch,Thetford,FortSterling,Martlock,Bridgewatch,Lymhurst'
        self.item = item
    @property
    def get_data(self):
        url = f"https://www.albion-online-data.com/api/v1/stats/Prices/{self.item}?locations={self.location}"
        response = (requests.get(url).text)
        response_json = json.loads(response)
        if response_json != []:
            response_json = pd.DataFrame(response_json)[["item_id", "city", "sell_price_min"]]
            max = response_json.loc[response_json["sell_price_min"].idxmax()]
            min = response_json.loc[response_json["sell_price_min"].idxmin()]
            gain = max[2] - min[2]
            data = pd.DataFrame(
                [[max[0], max[1], min[1], min[2], max[2], gain]],
                columns=["ITEM", "CITY max ", "CITY min", "MIN_PRICE", "MAX PRICE", "GAIN"])

            return data

def get_data_frame():
    item_list = pd.read_csv('items.txt', sep=':', names=['nuber', 'item']['item']
    item_price = pd.DataFrame(columns=["ITEM", "CITY max ", "CITY min","MIN_PRICE", "MAX PRICE", "GAIN"])
    for item in item_list:
        item_price = item_price.append(Albion_data(item).get_data)
        item_price.to_csv('data.csv')
    return item_price
```

Do you need a class?

Right now, I'm not sure you need a class. Your class has two methods, one of which is __init__, so this could be refactored to be a function

def get_data(item):
    location = f'Bridgewatch,Thetford,FortSterling,Martlock,Bridgewatch,Lymhurst'
    url = f"https://www.albion-online-data.com/api/v1/stats/Prices/{item}?locations={location}"
    # you also don't need these parentheses
    response = requests.get(url).text
    response_json = json.loads(response)
    if response_json != []:
        response_json = pd.DataFrame(response_json)[["item_id", "city", "sell_price_min"]]
        max = response_json.loc[response_json["sell_price_min"].idxmax()]
        min = response_json.loc[response_json["sell_price_min"].idxmin()]
        gain = max[2] - min[2]
        data = pd.DataFrame(
                [[max[0], max[1], min[1], min[2], max[2], gain]],
                columns=["ITEM", "CITY max ", "CITY min", "MIN_PRICE", "MAX PRICE", "GAIN"])

        return data

This minimizes the cost of constructing the class just to get a dataframe.

Checking the Value of json.loads

Now, I'm not completely sure what response.text will look like every time, but reading your code, it will always be a list. The fact that there's no else means that if your code doesn't satisfy the if statement, it will return None, which could lead to TypeErrors in your dataframe construction. It might be easier to make a generator, that way you can easily skip return values:

# set the argument to be an iterable

def get_data(items):
    columns=["ITEM", "CITY max ", "CITY min", "MIN_PRICE", "MAX PRICE", "GAIN"])
    location = f'Bridgewatch,Thetford,FortSterling,Martlock,Bridgewatch,Lymhurst'

    for item in items:
        url = f"https://www.albion-online-data.com/api/v1/stats/Prices/{item}?locations={location}"
        response = requests.get(url).text
        response_json = json.loads(response)

        # rely on the falsiness of empty lists and enforce that it will indeed be
        # a list type using isinstance
        if response_json and isinstance(response_json, list):
            # since response_json is a list, don't create a dataframe,
            # you can instead provide a key to the max and min functions
            maxval = max(response_json, key=lambda x: x['sell_price_min'])
            minval = min(response_json, key=lambda x: x['sell_price_min'])

            # Now you don't need to rely on an index
            gain = maxval['sell_price_min'] - minval['sell_price_min']
            vals = [maxval.pop('item_id')]
            for k in ('city','sell_price_min'):
                vals.append(maxval[k])
                vals.append(minval[k])
            vals.append(gain)
            yield dict(zip(vals, columns))

Now, in case the if statement isn't True, you won't get None from the function:

def func(a):
   if a<2:
       return [1,2]

df = pd.DataFrame([func(x) for x in range(4)], columns = ['a', 'b'])
# TypeError: object of type 'NoneType' has no len()

The benefit here is that now your get_data_frame function can be a lot easier to leverage:

def get_data_frame():
    item_list = pd.read_csv('items.txt', sep=':', names=['nuber', 'item']['item']

    # You also don't need to initialize the item_price dataframe

    # You can use a list comprehension, and because your items are
    # dictionaries, you don't need to specify the columns
    item_price = pd.DataFrame([a for a in get_data(item_list)])
    item_price.to_csv('data.csv')
    return item_price

read_csv

Now, the only other issue I can see is that you are collecting everything into memory multiple times with pd.read_csv. You really only want the item_id's, so just iterate over a file handle directly:

def file_iterate():
    with open('items.txt') as fh:
        for line in fh:
            # item is the second entry, so split on ':' and grab index 1
            yield line.split(':')[1].strip()

def get_data_frame():
    items = file_iterate()
    item_price = pd.DataFrame([a for a in get_data(items)])
    item_price.to_csv('data.csv')
    return item_price

The generators will allow you to keep one dataframe in-memory, and produces values one at a time.

try/except

The last thing I'd do is to ensure the program doesn't crash on a bad web request or attempt at json.loads:

from requests.exceptions import HTTPError

def get_data(items):
    columns=["ITEM", "CITY max ", "CITY min", "MIN_PRICE", "MAX PRICE", "GAIN"])
    location = f'Bridgewatch,Thetford,FortSterling,Martlock,Bridgewatch,Lymhurst'

    for item in items:
        url = f"https://www.albion-online-data.com/api/v1/stats/Prices/{item}?locations={location}"
        # wrap in try/except here
        try:
            response = requests.get(url).text
            response_json = json.loads(response)
        except JSONDecodeError as e:
            print(f'bad json for {item}')
            continue # skip it so it doesn't ruin your process, or you can raise it
        except HTTPError as e:
            print(f'HTTPError on {url}'
            continue # skip or you can raise
        except Exception: # Unexpected error
            raise
        # rest of function


Vanilla ES6 Todo/Task List with LocalStorage I'm making a simple todo list app with ES6. I'm looking for some feedback on the code, I feel like there is some improvement possible. Maybe there is a better way to attach eventListeners to input and button, but to keep in mind they have to be set not just when a new element is added but on window load when the items from localStorage are retrieved. I tried to keep it short as best I could.

import TaskItem from './TaskItem'; //constructor for id, title, and completed boolean
import * as helpers from './Helpers'; //two extra methods

export default class TaskList {

    constructor()
    {
        this.inputField= document.getElementById('task-input');
        this.addButton = document.getElementById('add-task');
        this.tasksElem = document.getElementById('tasks');
        this.tasks = JSON.parse(localStorage.getItem('userTasks')) || [];

        this.displayTasks()
        this.initializeEventListeners()

    }

    initializeEventListeners()
    {
        this.tasksElem.addEventListener('click', (event) => {
            const taskId = event.target.parentNode.getAttribute('data-id');

            switch (event.target.localName) {
                case 'button':
                    this.removeTask(taskId);
                    break;
                case 'input':
                    this.toggleCompleted(taskId);
                    break;
                default:
            }
        })

        this.addButton.addEventListener('click', this.addNewTask.bind(this))

    }

    addNewTask()
    {
        const newTaskTitle = this.inputField.value.trim();

        if (newTaskTitle)
        {
            let newTaskID = helpers.getRandomId().toString(); //still need to check if id exists
            const newTask = new TaskItem(newTaskID, helpers.htmlEscape(newTaskTitle))
            this.tasks.push(newTask)
            this.inputField.value = '';
            this.saveTasks()
            this.displayTasks()
        }

    }

    removeTask(idToRemove)
    {
        this.tasks = this.tasks.filter((task) => {
            return task.id !== idToRemove
        });

        this.saveTasks()
        this.displayTasks()
    }
    toggleCompleted(idToToggle)
    {
        this.tasks.forEach((task)=>{
            if (task.id === idToToggle) {
                task.completed = !task.completed;
            }
        })

        this.saveTasks()
        this.displayTasks()
    }

    saveTasks()
    {
              localStorage.setItem('userTasks',JSON.stringify(this.tasks));
    }

    displayTasks()
    {
        this.tasksElem.innerHTML = this.tasks.reduce((acc, task) => {
            return acc + this.returnTaskTemplate(task)
        },'');

    }

    returnTaskTemplate(task)
    {
        return `

                   <input class="toggle" type="checkbox" ${task.completed ? 'checked' : ''}>
                   ${task.title}
                   <button>Remove</button>

`;
    }
}

Any feedback is appreciated.

I would prefer a better extensibility (for example, easy to add new actions) and to get the taskId only if I’m going to use it.

initializeEventListeners() {
    const actions = {
        button: 'removeTask',
        input: 'toggleCompleted'
    };

    this.tasksElem.addEventListener('click', (event) => {
        const action = actions[event.target.localName];
        if (action) {
            const taskId = event.target.parentNode.getAttribute('data-id');
            this[action](taskId);
        }
    })

    this.addButton.addEventListener('click', this.addNewTask.bind(this))
}


Safely setting object properties with dot notation strings in JavaScript What my function does:
Safely sets object properties with dot notation strings in JavaScript.
That is to say, it allows setting a nested property on an object using a string such as "a.b.c".
For example: set(obj, "a.b.c", value) would be equivalent to a.b.c = value
Some notes on intended behavior

When setting a property at a given path, if any portion of that path doesn't exist, it should be created
When creating a path portion, if the next key in the path is NOT an integer, the current portion should be created as an object
When creating a path portion, if the next key in the path is an integer, the current portion should be created as an array

Why?
I'd like to be able to set deeply nested properties on an object in an environment where I can't be sure that all the properties in the path will always exist and I don't want to deal with the logic of checking for and setting each key manually in more than one place. This allows me to quickly set the needed value without caring if the keys were present previously or not.

/**
 * Set the value for the given object for the given path
 * where the path can be a nested key represented with dot notation
 *
 * @param {object} obj   The object on which to set the given value
 * @param {string} path  The dot notation path to the nested property where the value should be set
 * @param {mixed}  value The value that should be set
 * @return {mixed}
 *
 */
function set(obj, path, value) {
    // protect against being something unexpected
    obj = typeof obj === 'object' ? obj : {};
    // split the path into and array if its not one already
    var keys = Array.isArray(path) ? path : path.split('.');
    // keep up with our current place in the object
    // starting at the root object and drilling down
    var curStep = obj;
    // loop over the path parts one at a time
    // but, dont iterate the last part,
    for (var i = 0; i < keys.length - 1; i++) {
        // get the current path part
        var key = keys[i];

        // if nothing exists for this key, make it an empty object or array
        if (!curStep[key] && !Object.prototype.hasOwnProperty.call(curStep, key)){
            // get the next key in the path, if its numeric, make this property an empty array
            // otherwise, make it an empty object
            var nextKey = keys[i+1];
            var useArray = /^\+?(0|[1-9]\d*)$/.test(nextKey);
            curStep[key] = useArray ? [] : {};
        }
        // update curStep to point to the new level
        curStep = curStep[key];
    }
    // set the final key to our value
    var finalStep = keys[keys.length - 1];
    curStep[finalStep] = value;
};

/** Usage **/

console.log('setting non existant a.b.c to "some value":');
var test = {};
set(test, 'a.b.c', 'some value');
console.log(test);

console.log('updating a.b.c to "some new value":');
set(test, 'a.b.c', 'some new value');
console.log(test);

console.log('setting non existant a.b.0 to "some value":');
var test = {};
set(test, 'a.b.0', 'some value');
console.log(test);

console.log('updating a.b.0 to "some new value":');
set(test, 'a.b.0', 'some new value');
console.log(test);

obj = typeof obj === 'object' ? obj : {};

When obj argument points to something other than object you will create one. Like in set("foo", "a.b.c", "Yay!"). I guess you want to return it at function end or it will be lost.

Other notes:

/^\+?(0|[1-9]\d*)$/ allows + in integer key inputs
you may use isNaN(+nextKey) instead of regular expression, but beware float or signed numbers
finalStep is a bit misleading name. You may use key = keys[keys.length - 1] instead.


Optimizing timeline generation I have a bunch of processes that are being executed on different virtual machines. All of those processes have a StartDate, EndDate and Resource properties. Resource is a specific virtual machine it was ran on. I generated the actual timeline for items and found out that all of those resources have "holes" in timeline that could fit other processes running on other machines at the same time, which means I could live with less machines. So I am now trying to come up with an algorithm that tries to fit all those processes to timeline. It adds a timeline and first item to timeline, for every next item, it checks if the EndDate of the last item in timeline is greater than current items StartDate, if so Add a new timeline, else put to existing timeline. The current implementation:

public List<TimelineDto> GetOptimizedTimeline()
{
    var allItems = GetInterestingItems().OrderBy(x => x.StartDate).ToList();
    if (!allItems.Any())
    {
        return null;
    }
    // Add the first item to first resource
    var firstitem = allItems.FirstOrDefault();
    var timelineCount = 1;
    var timeline = new List<TimelineDto>
    {
        new TimelineDto
        {
            itemId = firstitem.Id,
            Resource = timelineCount.ToString(),
            StartTime = firstitem.StartDate,
            EndTime = firstitem.EndDate
        }
    };
    allItems.Remove(firstitem);

    foreach (var item in allItems)
    {
        var added = false;
        for (var i = 1; i <= timelineCount; i++)
        {
            // as items are ordered by date, take the last item in current resource and see if the new items "fits" to that resource
            var last = timeline.Last(x => x.Resource.Equals(i.ToString()));
            if (item.StartDate > last.EndTime)
            {
                // Add to existing
                timeline.Add(new TimelineDto
                {
                    itemId = item.Id,
                    Resource = i.ToString(),
                    StartTime = item.StartDate,
                    EndTime = item.EndDate
                });
                added = true;
                break;
            }
        }
        // suitable resource already found, cool.
        if (added)
            continue;
        // Current item did not fit into any existing resource, add a new one.
        timelineCount++;
        timeline.Add(new TimelineDto
        {
            itemId = item.Id,
            Resource = timelineCount.ToString(),
            StartTime = item.StartDate,
            EndTime = item.EndDate
        });
    }
    return timeline;
}

Now this does what I need, but for 100k items it takes minutes to complete. I am processing 1M items daily so it's not that I can't wait several minutes, but it would be great to optimize it. Can I parallelize this?

Compound words like firstitem should be camelCase.

Why is the Resource property of TimelineDto a string, when you seem to be filling it with ints?

allItems is a bad name, item is even worse. I have no idea what this represents.

I also have to object to TimelineDto, especially when timeline = new List<TimelineDto>.

var firstitem = allItems.FirstOrDefault(); can return a null, yet you never check this.

Why are you even using the OrDefault version, considering that the first line sorts the result of GetInterestingItems() by StartDate, which would throw an exception if one of the items was null.

Is there a point to all of this:

// Add the first item to first resource
var firstitem = allItems.FirstOrDefault();
var timelineCount = 1;
var timeline = new List<TimelineDto>
{
    new TimelineDto
    {
        itemId = firstitem.Id,
        Resource = timelineCount.ToString(),
        StartTime = firstitem.StartDate,
        EndTime = firstitem.EndDate
    }
};
allItems.Remove(firstitem);

Except for var timelineCount = 1; I don't see how this isn't handled by the logic inside foreach (var item in allItems). Of course you need to rethink var last = timeline.Last(x => x.Resource.Equals(i.ToString())); and if (item.StartDate > last.EndTime), but that seems a more elegant solution than the 10+ lines required to add a first entry to timeline.

This code is repeated three times, so it should be moved to a method:

new TimelineDto
{
    itemId = item.Id,
    Resource = timelineCount.ToString(),
    StartTime = item.StartDate,
    EndTime = item.EndDate
}

WRT your question, I don't see how you can parallelize it without running the risk of duplicate entries.

I do wonder whether it wouldn't be easier (and increase performance) to maintain a Dictionary<int, DateTime> instead of doing var last = timeline.Last(x => x.Resource.Equals(i.ToString())); each time. Key of that dictionary is the Resource, value is the EndTime.

So you'd end up with something like this:

public List<TimelineDto> GetOptimizedTimeline()
{
    var items = GetInterestingItems().OrderBy(x => x.StartDate).ToList();
    if (!items.Any())
    {
        return null;
    }

    var timeline = new List<TimelineDto>();
    var endTimeByResource = new Dictionary<int, DateTime>();
    var timelineCount = 1;

    foreach (var item in items)
    {
        var added = false;
        for (var i = 1; i <= timelineCount; i++)
        {
            DateTime endTime;
            if(endTimeByResource.TryGetValue(i, out endTime)
            {
                if (item.StartDate > endTime)
                {
                    timeline.Add(CreateTimelineDto(item, i));
                    endTimeByResource[i] = item.EndTime;
                    added = true;
                    break;
                }
            }
        }

        if (added)
            continue;

        timelineCount++;
        timeline.Add(CreateTimelineDto(item, i));
        endTimeByResource[i] = item.EndTime;
    }

    return timeline;
}

Beware: I have not tested this code, so there might be some part of the logic missing! I suspect the if(endTimeByResource.TryGetValue(i, out endTime) needs an else where you add a new TimelineDto.

I would even consider getting rid of timelineCount completely and instead do for (var i = 1; i <= endTimeByResource.Keys.Count; i++), that way you might even be able to get rid of added, something like this:

foreach (var item in items)
{
    for (var i = 1; i <= endTimeByResource.Keys.Count; i++)
    {
        DateTime endTime;
        if(endTimeByResource.TryGetValue(i, out endTime)
        {
            if (item.StartDate > endTime)
            {
                timeline.Add(CreateTimelineDto(item, i));
                endTimeByResource[i] = item.EndTime;
                break;
            }
        }
    }
}

Again: this is untested.

I think it can even be reduced to this:

    for (var i = 1; i <= endTimeByResource.Keys.Count; i++)
    {
        DateTime endTime;
        endTimeByResource.TryGetValue(i, out endTime);

        if (item.StartDate > endTime)
        {
            timeline.Add(CreateTimelineDto(item, i));
            endTimeByResource[i] = item.EndTime;
            break;
        }
    }


Is there a simpler way of copying and pasting one inputs value into other input with the same name I am trying to write something that will copy the current <input>s value and enter it into any <input> that start with the same name.

<input> names will follow this pattern: price-0, price-1, price-2, upc-0, upc-1, upc-2.

So if a user enters a value in <input name="price-0"> and hits copy the value should be transferred over to all input whos name start with price

This is the code I've written:

$(document).on('click', '.--copy', function () {
    var input_name = $(this).closest('div').find('input').attr('name').split('-')[0];
    $('input[name^=' + input_name + ']').val($(this).closest('div').find('input').val());
});

A fiddle to make everyones life easier: http://jsfiddle.net/6jGLD/

I feel like there are too many selectors being called upon and the code is somewhat difficult to read.

One minor improvement I can think of is to create a variable to hold the input element since the element is used twice like

$(document).on('click', '.--copy', function () {
    var $input = $(this).closest('div').find('input'), input_name = $input.attr('name').split('-')[0];
    $('input[name^=' + input_name + ']').val($input.val());
});

Demo: Fiddle


SQL server efficiency, comparing date time of latest record and the one under it I have a snippet of SQL that compares the last two records and gives the datediff in seconds. However, the way I have it is quite slow, taking up to 20 seconds to execute depending on how many controllerIDs I need to check. What would be a more efficient way of doing this?

 select
                T.controllerID,
                datediff(ss, T.Max_dtReading, T1.Max_dtReading) As ElaspedTime
            from
                (
                    select
                        controllerID,
                        max(dtReading) as Max_dtReading
                    from ReaderData
                    where CardID = 'FFFFFFF0' AND (controllerID in(2,13,28,30,37,40))
                    group by controllerID
                ) as T
                outer apply(
                    select max(T1.dtReading) as Max_dtReading
                    from ReaderData as T1
                    where
                        T1.CardID = 'FFFFFFF0' AND (controllerID in(2,13,28,30,37,40))
                        and T1.controllerID = T.controllerID
                        and T1.dtReading < T.Max_dtReading
                ) as T1

Aliasing
The use of T and T1 as aliases in this query made me take twice as long to work out what it is actually doing. I would give these a more meaningful alias.
Brackets
The Where clause contains this:
AND (controllerID in(2,13,28,30,37,40))

The brackets around this are unnecessary and a little confusing. I would only recommend brackets in the Where clause when you are using both AND and OR.
Removing a select
Firstly, let's ask ourselves what this query is doing.
Step 1, get the controllerID's and MAX(dtReading)'s from the ReaderData table.
Step 2, do a datediff of the MAX(dtReading) and the next highest dtReading.
First lets write a simple query to do step 1:
SELECT
    base.controllerID,
    MAX(base.dtReading) AS [Most Recent Reading Date]
FROM ReaderData AS base
WHERE base.CardID = 'FFFFFFF0'
AND base.controllerID IN (2,13,28,30,37,40)
GROUP BY base_data.controllerID

Now let's bring in the second dtReading:
SELECT
    base.controllerID,
    MAX(base.dtReading) AS [Most Recent Reading Date],
    prior.[Prior Reading Date]
FROM ReaderData AS base
OUTER APPLY
(
    SELECT MAX(extra.dtReading) AS [Prior Reading Date]
    FROM ReaderData AS prior
    WHERE prior.CardID = base.CardID
    AND prior.controllerID = base_data.controllerID
    AND prior.dtReading < base_data.Max_dtReading
) AS prior
WHERE base.CardID = 'FFFFFFF0'
AND base.controllerID IN (2,13,28,30,37,40)
GROUP BY base_data.controllerID

You'll notice that I changed the CardID = 'FFFFFFF0' and controllerID IN (2,13,28,30,37,40) in the apply to check if it is equal to the value in the main select. It is easier to understand this way, you want to make sure that they are equal, plus it lets you avoid the possible mistake of forgetting to update of the sets of values.
Then we add the datediff:
SELECT
    base.controllerID AS [Controller ID],
    DATEDIFF(SECOND,MAX(base.dtReading),prior.[Prior Reading Date]) AS [Elasped Time]
FROM ReaderData AS base
OUTER APPLY
(
    SELECT MAX(extra.dtReading) AS [Prior Reading Date]
    FROM ReaderData AS prior
    WHERE prior.CardID = base.CardID
    AND prior.controllerID = base_data.controllerID
    AND prior.dtReading < base_data.Max_dtReading
) AS prior
WHERE base.CardID = 'FFFFFFF0'
AND base.controllerID IN (2,13,28,30,37,40)
GROUP BY base_data.controllerID

Here I took the liberty of aliasing the returned columns to make them a little easier on the eye for the end user, as well as capitalising keywords.
There is no agreed upon standard to SQL, so feel free to re-lowercase everything, but conventionally SQL is written with keywords in ALL CAPS
Performance Increases
The query I walked through above should perform better than the one in the question as it avoids the sub-select.
I would make sure that the ReaderData table is properly indexed, this can and will drastically speed up queries and make sure that the statistics on the table are up to date.


MaxCounters Codility Challenge I've been doing some Codility challenges lately and the MaxCounters challenge got me stumped:

  You are given N counters, initially set to 0, and you have two
  possible operations on them:

  increase(X) — counter X is increased by 1,
  max counter — all counters are set to the maximum value of any counter.

  A non-empty array A of M integers is given. This array represents consecutive operations:

  if A[K] = X, such that 1 ≤ X ≤ N, then operation K is increase(X),
  if A[K] = N + 1 then operation K is max counter.

  [...]
  The goal is to calculate the value of every counter after all operations.

Below is my solution in Python.  I can't reach 100% on it even though I am pretty confident it is an O(n+m) complexity solution. Codility fails it and says its an O(n*m) complexity solution.

def solution(N, A):
    counter = [0] * N
    max_val = 0

    for v in A:

        if v <= N and v >= 1:
            if counter[v-1] < max_val+1:
                counter[v-1] = max_val+1
            else:
                counter[v-1] += 1
        else:
            max_val = max(counter)

    for i in range(len(counter)):
        if counter[i] < max_val:
            counter[i] = max_val

    return counter

A couple of hints, to avoid spoiling the problem.

Hint 1

   What would happen if every entry in A were N + 1?

Hint 2

   What is the complexity of max(counter)?


Remove points in a straight line I wrote this code to remove points that create a straight line when plotted.

For instance, this has many points in a straight line:

import matplotlib.pyplot as plt
import numpy as np

pattern = np.array(
    [ 9, 10, 11, 10,  9,  8,  6,  5,  4,  3,  4,  5,  7,  6,  5,  4,  2,
      1,  2,  3,  4,  5,  7,  8,  9, 10,  8,  7,  8,  9, 10, 11, 10, 11,
     12, 13, 12, 11, 10,  8,  7,  6,  5,  4,  6,  5,  3,  2,  3,  2,  0,
      1,  2,  3,  4,  5,  6,  8,  9, 10, 11, 12, 13, 14, 15, 16, 17, 19,
     21, 19, 18, 16, 15, 14, 12, 11, 12, 11, 10, 11, 12, 13, 14, 15, 13,
     12, 11, 10,  9,  8,  7,  6,  5,  6,  5,  4,  6])

plt.plot(pattern)
plt.plot(pattern, 'bo')
plt.show()

Using the degrees function I can remove any lines that create an angle of
180deg between the two neighboring points.

from numpy.lib.stride_tricks import as_strided

def moving_slice(a, k):
    a = a.ravel()
    return as_strided(a, (a.size - k + 1, k), 2 * a.strides)

def degrees(x, y):
    x_window, y_window = moving_slice(x, 3), moving_slice(y, 3)
    x_window = np.concatenate((x_window, x_window[:, None, 0]), axis=1)
    y_window = np.concatenate((y_window, y_window[:, None, 0]), axis=1)

    triangle_sides = np.sqrt(np.square(np.diff(x_window)) + np.square(np.diff(y_window)))
    squared_sides = np.square(triangle_sides)

    cos_num = np.sum(squared_sides[:, :2], axis=1) - squared_sides[:, 2]
    cos_den = 2 * np.prod(triangle_sides[:, :2], axis=1)
    return np.degrees(np.arccos(cos_num / cos_den))

This is what I'm trying to achieve:

# My review function
angles = degrees(np.arange(len(pattern)), pattern)

pattern = pattern[1:-1] # <- No angles between the end points
less_than_180 = angles < 180
plt.plot(pattern[less_than_180])
plt.plot(pattern[less_than_180], 'bo')
plt.show()

You can see that it's pretty good. Missed a few points to remove (due to decimal accuracy) but otherwise the function does what I want. I'd like to know if there is a simpler way to achieve the desired result without calculating all the angles. What do you think?

A look at the second plot in the post shows that something has gone wrong. There are four points in a line here:

This shouldn't be possible, since "points that create a straight line when plotted" have been removed. How did this happen? A look at the \$x\$ axis shows the cause. The first plot has the \$x\$ coordinate running from 0 to 96, but the second plot has the  \$x\$ coordinate running from 0 to 43, because the \$x\$ values for the omitted points have also been omitted.

To get the correct plot you have to remember the original \$x\$ coordinates and mask them at the same time as you mask the \$y\$ values, like this:

x = np.arange(len(pat))[less_than_180]
y = pat[less_than_180]
plt.plot(x, y)
plt.plot(x, y, 'bo')

See below for the corrected plot.
The name degrees could be better chosen — there are already functions math.degrees in the standard library, and numpy.degrees in NumPy, and a reader might well imagine that your function does something similar. A name like vertex_angle would be better (see Wikipedia).
The line:

pattern = pattern[1:-1] # <- No angles between the end points

discards the endpoints of the data. But surely you want to keep the endpoints in the plot?
Given three points \$A, B, C\$, the code finds the vertex angle \$∠ABC\$ and compares it with 180°. An alternative approach is to linearly interpolate between the endpoints, finding \$B′ = ½(A + C)\$, and see if \$B′\$ is close enough to \$B\$. Like this:

# Boolean mask for the points we are going to plot.
mask = np.ones_like(pattern, dtype=bool)

if len(pattern) > 2:
    # Linearly interpolate between points that are two apart.
    interpolants = (pattern[:-2] + pattern[2:]) / 2

    # Don't plot points that are equal to their interpolant.
    mask[1:-1] = interpolants != pattern[1:-1]

x = np.arange(len(pattern))[mask]
y = pattern[mask]
plt.plot(x, y)
plt.plot(x, y, 'bo')
plt.show()

Here's the resulting plot:

If your data is floating-point then you will need some tolerance in the comparison of points to their interpolants. For example, you might use numpy.isclose:

# Don't plot points that are close their interpolant.
mask[1:-1] = ~np.isclose(interpolants, pattern[1:-1])


Circular nesting dictionary function How can I re-factorise this circular nesting dictionary function?

The challenge:

  I recently received a data extract with a list of folder names and associated subfolders. The challenge was to produce a reusable function that could provide a summary of the unique folder names and all the nested subfolders.

  The data source was an excel spreadsheet containing 2 columns:

  Parent: Folder name

  Child: Subfolder name

  Note: I have recreated spreadsheet data using pandas so the code can be easily tested.

Create table:

import pandas as pd

data = {'Parent': ['A', 'B', 'C', 'D', 'E', 'F', 'C', 'C'],
        'Child': ['B', 'C', 'E', 'E', 'Z', 'Z', 'B', 'A']}

df = pd.DataFrame(data)

print(df):

Parent  Child
0   A   B
1   B   C
2   C   E
3   D   E
4   E   Z
5   F   Z
6   C   B
7   C   A

My solution:

def relationship_dictionary(dataframe, key_column_name, values_column_name):
    """
    The key_column_name is the primary data source that should be considered the
    start of the nested relationship.

    The values_column_name is the subfolder

    Creates a dictionary of unique relationships to each key.
    """

    parent = key_column_name
    child = values_column_name

    d = {}
    for i, row in dataframe.iterrows():
        key = row[parent]
        value = row[child]
        if key in d.keys():
            d[key].append(value)
        else:
            d[key] = [value]

    for k, values in d.items():
        for v in values:
            if v in d.keys():
                for each in d[v]:
                    if (each not in d[k]) and (each != k):
                        d[k].extend([each])
    return d

Result:

relationship_dictionary(df, "Parent", "Child")
{'A': ['B', 'C', 'E', 'Z'],
 'B': ['C', 'E', 'A', 'Z'],
 'C': ['E', 'B', 'A', 'Z'],
 'D': ['E', 'Z'],
 'E': ['Z'],
 'F': ['Z']}

Feedback

  I'm happy to say it works after mitigating the circular nesting issue but I can't help thinking there is a far simpler way of doing this so I thought I'd put it out there for critique so feedback would be welcome... :)

You could create a graph, and use graph theory algorithms to find all nodes (children) you can visit from your current node (parent). The graph would be a directed, acyclic graph due to the nature of the file system.

In fact, this sounds like a great example of how to use graph theory in practice, so I will implement it given slightly more time.

I am including a simplified version of your code without using graphs (still had to use a nested loop, but only of depth 2!):

def relationship_dictionary(dataframe, key_column_name, values_column_name):
    """
    The key_column_name is the primary data source that should be considered the
    start of the nested relationship.

    The values_column_name is the subfolder

    Creates a dictionary of unique relationships to each key.
    """
    # ['A', 'B', 'C', 'D', 'E', 'F', 'C', 'C']
    parents = dataframe[key_column_name].to_list()

    # ['B', 'C', 'E', 'E', 'Z', 'Z', 'B', 'A']
    children = dataframe[values_column_name].to_list()

    # [('A', 'B'), ('B', 'C'), ('C', 'E'), ('D', 'E'), ('E', 'Z'), ('F', 'Z'), ('C', 'B'), ('C', 'A')]
    queue = tuple(zip(parents, children))

    # Create a parent -> empty set mapping to avoid using "if parent in mapping then ..., else ..."
    mapping = {parent: set() for parent in parents}

    # Iterate over each parent, child pair
    for parent, child in queue:

        # Always register a new pair has been processed
        mapping[parent].add(child)

        # Need to iterate over current pairs to make sure situations such as
        # 1. Pair A -> {B} added
        # 2. Pair B -> {C} added
        # result in A -> {B, C} instead of A -> {B}
        #
        # This essentially checks that if the parent in the current pair has been a child somewhere, the child in
        # current pair should also be added to wherever the parent was a child (if confusing follow sample above),
        # excluding cases (such as the last C -> {A} pair being included into A -> {'C', 'B', 'E', 'Z'} mapping)
        # in which the child is also the parent
        for current_parent, current_children in mapping.items():
            if parent in current_children and child != current_parent:
                current_children.add(child)

    return mapping

for k, v in relationship_dictionary(df, "Parent", "Child").items():
    print(k, v)

Result:

A {'E', 'B', 'Z', 'C'}
B {'E', 'A', 'Z', 'C'}
C {'A', 'B', 'Z', 'E'}
D {'Z', 'E'}
E {'Z'}
F {'Z'}

I have only tested it with your example but you might want to verify the code works with some more samples!

Hope it helps!


De-/Serialization of structs representing TCP messages I wrote two template functions to serialize and deserialize structs representing messages to be sent and received via a TCP socket:
/*
 * Convert a byte string into a packet.
 */
template <typename PacketType>
PacketType deserialize(std::string &buf, bool partialProcessing = false)
{
    PacketType packet;
    size_t bufSize = buf.length();
    size_t packetSize = sizeof packet;

    if (packetSize > bufSize)
        std::cerr << "Packet larger than buffer size. Partial read: " << (packetSize - bufSize) << std::endl;

    if (packetSize < bufSize && !partialProcessing)
        std::cerr << "Packet smaller than buffer size. Bytes unprocessed: " << (bufSize - packetSize) << std::endl;

    auto ptr = reinterpret_cast<char*>(&buf[0]);
    memcpy(&packet, ptr, (packetSize < bufSize) ? packetSize : bufSize);
    return packet;
}

/*
 * Convert a packet into a byte string.
 */
template <typename PacketType>
std::string serialize(PacketType &packet)
{
    auto ptr = reinterpret_cast<char*>(&packet);
    return std::string(ptr, ptr + sizeof packet);
}

The code currently works within a larger project quite well, but I am unsure as to whether it might produce memory leaks due to my usage of pointers and memcpy.
How can I improve this code?
Addendum
Since some of the (great) answers seem to need more context, this is but an excerpt of this revision of a header file within the respective project. The template functions are used internally only i.e. do not constitute an external API.

Your code produces no memory leaks since you're not manually allocating and deleting memory.

Include all headers
Always include all the headers. In this case, you need <iostream>, string, and cstring.

Pass by const reference
You should pass your objects by const reference. For example, const std::string& buf and const PacketType& packet. You can use reinterpret_cast<const char*>.

Use std::memcpy instead of memcpy
If you're including <cstring> (which you should be since you're using C++), you should qualify your C function calls with the std namespace.

Including the C++-style header () guarantees that the entities will be found in namespace std and perhaps in the global namespace.

Compile time checks for POD structs
std::memcpy only does a shallow copy, so it only works on POD structs (i.e. C-style structs).

A Plain Old Data Structure in C++ is an aggregate class that contains only PODS as members, has no user-defined destructor, no user-defined copy assignment operator, and no nonstatic members of pointer-to-member type.

For example,
struct Example
{
    int a;
    std::string b;
};

is a non-POD struct since std::string is a non-POD type (has a non trivial copy constructor, non-trivial destructor).
You should make sure that PacketType is a POD struct. You can do this with a compile time check using static_assert.
You can check whether a type is POD, using is_pod type trait (deprecated in C++20) or using is_trivial && is_standard_layout. Here is a good description of what these terms mean and how they relate to POD types.


Verify user account on button click with an AJAX call I have the following code that works perfectly other than the fact that it has to use async false, to wait for a response from ajax. Is there a way of restructuring this by using callbacks, or deferment?

playbtn.on('mousedown', function () { // functions when clicking play button

    function checkUser() {
        $.ajax({
            url: "/submit/checkuser/",
            async: false,
            success: function (userStatus) {
                if (userStatus == 'Disabled') { // check if user if disabled
                    if (Shadowbox.isOpen()) {
                        Shadowbox.close(); setTimeout(function () { accountDisabled(); }, 750);
                    } else {
                        Shadowbox.clearCache(); accountDisabled();
                    };
                    setTimeout(function () { location.replace('/'); }, 10000);
                } else if (userStatus == 'Deleted') { // check if user if deleted
                    if (Shadowbox.isOpen()) {
                        Shadowbox.close(); setTimeout(function () { accountDeleted(); }, 750);
                    } else {
                        Shadowbox.clearCache(); accountDeleted();
                    };
                    setTimeout(function () { location.replace('/'); }, 10000);
                } else {
                    Shadowbox.setup();
                };
            },
            error: function (e) {
                if (e.status != 403) {
                    if (!Shadowbox.isOpen()) {
                        Shadowbox.clearCache(); connectionError();
                    };
                };
            },
            statusCode: {
                403: function () {
                    if (Shadowbox.isOpen()) {
                        Shadowbox.close(); setTimeout(function () { locationNotAuthorized(); }, 750);
                    } else {
                        Shadowbox.clearCache(); locationNotAuthorized();
                    };
                }
            }
        });
    };

    Shadowbox.clearCache(); // disable shadowbox to check user status
    checkUserTimer = setInterval(function () { checkUser(); }, 600000);
    checkUser();
});

From: http://api.jquery.com/jquery.ajax/

  Deprecation Notice: The jqXHR.success(), jqXHR.error(), and jqXHR.complete() callbacks are deprecated as of jQuery 1.8. To prepare your code for their eventual removal, use jqXHR.done(), jqXHR.fail(), and jqXHR.always() instead.

So, it looks like you should do:

$.ajax({ url: "/submit/checkuser" })
 .done(function(userStatus) {
     ... function to complete when done ...
 }).fail(function(e) {
     ... function to complete on error ...
 }).always(function() {
     ... move all your code that needs to be done regardless of error or completion,
     and re-run your timer here
 });

Otherwise, if you are using a version of jQuery that does not include this more-normal looking functionality, there is a "complete" callback available that you can supply just as you are supplying "success" and "error" callbacks, which you can use to do all your cleanup and re-start your timer.


Rails middleware initialization block I'm working on a simple superfeedr powered rails app here.

Based on the superfeedr-rack gem documentation, I'm doing this to initialize the middleware (snippet from application.rb config block):

Configuration = YAML.load_file(Rails.root.join('config', 'config.yml'))

config.middleware.use Rack::Superfeedr, { :host => Configuration["general"]["hostname"], :login => Configuration["superfeedr"]["username"], :password => Configuration["superfeedr"]["password"]} do |superfeedr|
  Superfeedr = superfeedr
  superfeedr.on_notification do |notification|
    Article.create_from_raw_notification(notification)
  end
end

I'm looking for a better way to do it - I don't like to load my configuration file there instead of in an initializer and I think the block with the article creation callback smells. Any ideas?

Based on tapajos comment, I was able to to refactor the code using initializers. First I created one to load the configuration file:

APP_CONFIG = YAML.load_file(File.expand_path('../../config.yml', __FILE__))

Then I created another initializer, like this:

superfeedr_config =  {
      :host     => APP_CONFIG["general"]["hostname"],
      :login    => APP_CONFIG["superfeedr"]["username"],
      :password => APP_CONFIG["superfeedr"]["password"]
}

Rails.configuration.middleware.use Rack::Superfeedr, superfeedr_config do |superfeedr|
    Superfeedr = superfeedr
    superfeedr.on_notification {|notification| Article.create_from_raw_notification(notification)}
end

I also applied the logic described in this SO question to ensure the load order: How do I change the load order of initializers in Rails 3?.


How to append values into existed Vec with iterator I have a Vec with value default [0], and then I want to append into the vec with an iterator which is read from stdin, now my code is not performant since I use an insert to shift all the values behind index 0, how to improve this code with rusty style?
#[allow(unused_imports)]
use std::cmp::{max, min};
use std::io::{stdin, stdout, BufWriter, Write};

#[derive(Default)]
struct Scanner {
    buffer: Vec<String>,
}
impl Scanner {
    fn next<T: std::str::FromStr>(&mut self) -> T {
        loop {
            if let Some(token) = self.buffer.pop() {
                return token.parse().ok().expect("Failed parse");
            }
            let mut input = String::new();
            stdin().read_line(&mut input).expect("Failed read");
            self.buffer = input.split_whitespace().rev().map(String::from).collect();
        }
    }
}

fn main() {
    let mut scan = Scanner::default();

    let n: usize = scan.next();
    // How to improve these lines?
    let mut arr = (0..n).map(|i| scan.next()).collect();
    arr.insert(0, 0);
}

Playground link

I have got an answer with chain
fn main() {
    let mut scan = Scanner::default();
    let n: usize = scan.next();
    let mut arr = vec![0];
    let mut arr: Vec<i32> = arr.into_iter().chain((0..n).map(|i| scan.next())).collect();
    println!("{:?}", arr);
}


Implementing range() in JavaScript For fun, I implemented a version of Python's range function in JavaScript. I found that underscore.js implements it as well, but with notable differences. I'm hoping others can comment on the differences. I assume that underscore's implementation is, if not more optimal, at least more idiomatic. And I'd like to build my intuition for the best way to do things in JavaScript.

My version:

var range = function () {
    var results = [];

    // Allow comparison to be variable
    var lessThan = function (a, b) {return (a < b); };
    var greaterThan = function (a, b) {return (a > b); };

    if (arguments.length === 1) {
        var start = 0;
        var stop = arguments[0];
        var step = 1;
        var comparisonTest = lessThan;
    } else {
        var start = arguments[0];
        var stop = arguments[1];
        var step = arguments[2] || 1;
        var comparisonTest = ((step > 0)? lessThan : greaterThan);
    };

    for (var i = start; comparisonTest(i, stop) ; i += step) {
        results.push(i);
    };

    return results;
};

And underscore's implementation:

_.range = function(start, stop, step) {
    if (arguments.length <= 1) {
      stop = start || 0;
      start = 0;
    }
    step = arguments[2] || 1;

    var length = Math.max(Math.ceil((stop - start) / step), 0);
    var idx = 0;
    var range = new Array(length);

    while(idx < length) {
      range[idx++] = start;
      start += step;
    }

    return range;
  };

I see that not specifying function parameters - which made it easier to deal with variable lengths of arguments, ended up being more verbose than the way underscore handles it. Is there any other reason to always specify parameters?

And how about the actual building up of the range - underscore declares an Array constructor with length. Is that necessary?

Any insight or comments is appreciated!

I see that not specifying function parameters - which made it easier to deal with variable lengths of arguments

But it didn't really make it easier to deal with variable length args, as underscore's code demonstrates. It's true, though, that it makes it a lot easier to write the function definition without worrying about whether you should write function (start), function (start, end), or function (start, end, step)- but that's about it.

But regardless of how you write it in code document it.

Since JS doesn't really care too much about what you put in the args list, docs are the what you have to rely on.

Anyway, you can basically put whatever you want in the args list. But I'd lean toward following underscore's example, and write out the full list of arguments. Worst case scenario is that someone skips the docs, looks at the args list, and sees 3 arguments. So he/she writes calls the function with all of those argument, even though 1 or 2 would have achieved the same thing. But that's just fine. But if you don't write anything in the args list and don't provide docs, whomever wants to use your code (e.g. you yourself in 2 months) suddenly has to play JavaScript interpreter and read the code itself to figure it out. While that shouldn't take more than a couple of seconds it's still an annoyance.

Alternatively, you can do something like

function (start /*, [stop, [end]] */)

to clue a developer in on what the function expects. But that's still not a substitute for just documenting it well. Because even if you are looking at the function's code itself and figuring it out, you have no idea what arguments[1] might be, because there's nothing to indicate there even is a 2nd argument. You have to work backwards to understand it. Underscore has named arguments, that, even if they're undefined, just make the code itself a lot more readable right off the bat.

  underscore declares an Array literal with length. Is that necessary?

Necessary? No. A good idea? Yes.

By declaring the length right away, you save the computer the trouble of keeping up with a growing array. When you declare the length, the computer can start allocating the necessary memory right away, whereas if you build it up, it may have to allocate more memory for each push(), and copy chunks of memory around and similar busywork.

Will you ever notice the difference? Depends on what you're doing, but I'd say wager that no, you won't ever notice. But it's a pretty simple optimization to make, so why not? Not that this is something you have to always do when building arrays (and sometimes you just can't know ahead of time how big the array will get), but for a low-level function like this with potentially heavy usage? Yeah, might as well. It's still very readable, so it's a win-win, really.

Next up: Variables. Just declare your variables in one place - with sensible default values, if possible. You declare the same 4 vars in two different places. While it doesn't matter much, it's just basic code cleanliness to not repeat yourself.

Next up: The loop. You loop basically does this:

declare and initialize i
call comparisonTest with a couple of arguments

compare those arguments
return boolean

if returned value is false, exit the loop
if returned value is true:

append i to the array
increment i  by step

repeat

Whereas underscore's does this:

compare idx and length
if false, exit the loop
if true:

insert start at index idx
increment idx
increment start by step

repeat

Your way seems to save a variable (underscore has both idx and start), but it's a losing battle: underscore's way saves an entire function call for every iteration of the loop.

Calling that comparison function means duplicating 2 variables (the arguments) plus the whole rigamarole of setting up return address, switching to the function's context, setting up caller and callee, arguments and all the other stuff a JS runtime does when you shove something onto the stack. Then it does the same sort of thing again when calling push. Since push is a native function, the calling overhead is no doubt lower,  but there's still the aforementioned overhead of (potentially) having to reallocate memory for a bigger array and all that.

Oh, and the engine also has to create those comparison functions when you call range() (I'd wager this is also pretty well optimized, but worst case scenario is a lot of extra work on every call just to do a simple comparison)

It's naturally more efficient to just not do any of that, as in underscore's solution.

Now, let me just make it clear that you rarely need to care about all of this. Once you start micro-optimizing your code, it's a deep, deep rabbit hole to go down, and most of the time it really doesn't matter. You're just spinning your wheels. All of this stuff is worth being aware of, but don't go overboard. "Premature optimization is the root of all evil" as the saying goes.

So in the end, there's nothing really terribly wrong with your code at all, and I'm sure it can serve you well without any changes (except maybe some documentation). But it's of course hard to improve upon a battle-tested, built-for-speed library like underscore.


Changing the background color when a link is clicked What I'm trying to do is to change the background color when a link is clicked (in this case a list item). When it is no longer clicked, it should revert back to its normal color.

I was able to get this to work via the jQuery addClass and removeClass methods, but this seems to be hardcoding it.

jsFiddle

$(document).ready(function() {
    $(".nav > li:first-child").click(function(){
      $(this).addClass("highlighted");
      $(".nav > li:nth-child(2), .nav > li:nth-child(3), .nav > li:last-child").removeClass("highlighted");
    });
    $(".nav > li:nth-child(2)").click(function(){
        $(this).addClass("highlighted");
        $(".nav > li:first-child, .nav > li:nth-child(3), .nav > li:last-child").removeClass("highlighted");
    });
    $(".nav > li:nth-child(3)").click(function(){
        $(this).addClass("highlighted");
        $(".nav > li:first-child, .nav > li:nth-child(2), .nav > li:last-child").removeClass("highlighted");
    });
    $(".nav > li:last-child").click(function(){
        $(this).addClass("highlighted");
        $(".nav > li:first-child, .nav > li:nth-child(2), .nav > li:nth-child(3)").removeClass("highlighted");
    });
});

The opening and closing tags don't match in your JSFiddle.

It's not a good idea to use presentational like highlighted for a CSS class name.  It would be better to use current or selected, and let the stylesheet decide how to indicate which item is currently selected — whether by highlighting, underlining, or whatever visual hint it chooses.

It would be a lot simpler if you removed the highlighted class from all items, then added it back for just the selected item.

$(document).ready(function() {
    $(".nav > li").click(function() {
        $(".nav > li").removeClass('current');
        $(this).addClass('current');
    });
});
* {
 margin: 0px;
 padding: 0px;
}

nav {
 background-color: #e4801c;
 height: 100%;
}
.nav > li {
 text-align: center;
 text-transform: uppercase;
}

.current {
 background: #faaf5e;
}

Clients

Services

Add

Time

In addition, I suggest using text-transform: uppercase to make the text all caps, as that is also a presentation style choice.


Trim leading zeroes from second component of a string I have a collection of strings as follows:

  44.01
  44.02
  44.03
  44.04
  44.12
  46.05
  46

... and so on.

I want to remove leading zeros from the component of the string after the full stop.

As you can see from above, not all strings have a sub-component and not all sub-components will have leading zeros.

Is there a more elegant way of doing that from my attempt?

    List<string> codesWithSubCodes = new List<string>
    {
        "44.01", "44.02", "44.03", "44.04", "44.05", "44.06", "44.07", "44.08"
    };

    var codes = new List<string>();
    foreach (var code in codesWithSubCodes)
    {
        if (code.Contains('.'))
        {
            var prefix = code.Split('.')[0];
            var subCode = code.Split('.')[1].TrimStart('0');
            codes.Add(prefix + '.' + subCode);
        }
        else
        {
            codes.Add(code);
        }
    }

    foreach (var code in codes)
    {
        Debug.WriteLine(code);
    }

The biggest thing I can see, which may or may not be an issue, is the manner in which you process the strings.

    if (code.Contains('.'))
    {
        var prefix = code.Split('.')[0];
        var subCode = code.Split('.')[1].TrimStart('0');
        codes.Add(prefix + '.' + subCode);
    }
    else
    {
        codes.Add(code);
    }

This could be rewritten as:

var codeSections = code.Split('.');
var code = codeSections[0];

if (codeSections.Length == 2)
    code += "." + codeSections[1].TrimStart('0');

codes.Add(code);

This saves you LoC, and probably makes it faster. (You could also possible make this a StringBuilder as well.) And in my opinion, this is more readable/concise/descriptive.

Another alternative:

code = code.Replace(".0", ".");

Both should have the same effect on the cases you provided.

However, if, for whatever reason, one of the cases ends up as 46.00, then the second method will only make it 46.0. Choose that method with great caution.

As the OP pointed out in an edit that should have been a comment, this code can all be done as LINQ:

var codes = codesWithSubCodes.ConvertAll(code => code.Replace(".0", "."));


12-week staffing forecast LINQ query I have a LINQ query that I am having trouble optimizing and takes about 5.5 seconds to run. I am using a view called StaffingResourceData and a table called StaffingForecasts.

Each StaffingResource has a ResourceId, a Division, and a Type. A StaffingForecast has a ResourceId, Project, Date (represents a Monday of a week), Hours. A StaffingResource can have 0-many StaffingForecasts.

For each StaffingResource, I need a list of their total forecasted hours for the next 12 weeks. Here is what I have right now:

// Get list of dates
var dates = new List<DateTime>();
var start = Utilities.GetStartOfWeek(DateTime.Today);
for (var i = 0; i < 12; i++)
{
    dates.Add(start.AddDays(i * 7));
}
var end = dates[11];

// Get resources
var resources = (from r in context.StaffingResourceDatas
                 where r.EmployeeId != null
                     && !exclusionList.Contains(r.ResourceTitleId)
                 join f in context.StaffingForecasts.Where(x => x.Date >= start && x.Date <= end) on r.ResourceId equals f.ResourceId into g1
                 from f in g1.DefaultIfEmpty()
                 group new { f.Date, f.Hours } by r into g2
                 select new ChartResourceModel
                 {
                     ResourceId = g2.Key.ResourceId,
                     Division = g2.Key.ResourceDivision,
                     Type = g2.Key.ResourceType,
                     Dates = dates.Select(d => new ChartDateModel
                     {
                         Date = d,
                         Available = (g2.Where(f => f.Date == d).Any() ? g2.Where(f => f.Date == d).Sum(f => f.Hours) : 0) < 24
                     }).ToList()
                 })
               .ToList();

Any ideas on how I could speed this up?

You are combining an IQueryable with a local sequence, dates. It is possible to do that, but Entity Framework1 needs a lot of code to convert the sequence into something that behaves like a SQL table. I'm sure this is a major performance killer.

From what I see, you can do without this local sequence. You know the begin and end date. So it's possible to get the forecasts between those dates. If you group those forecasts by their Date, you achieve the same grouping:

from r in context.StaffingResourceDatas
where r.EmployeeId != null && !exclusionList.Contains(r.ResourceTitleId)
let forecasts = context.StaffingForecasts
                       .Where(f => r.ResourceId == f.ResourceId
                                && f.Date >= start && f.Date <= end)
select new ChartResourceModel
{
    ResourceId = r.Key.ResourceId,
    Division = r.Key.ResourceDivision,
    Type = r.Key.ResourceType,
    Dates = (from fc in forecasts
             group fc by d.Date into fcg
             select new ChartDateModel
             {
                 Date = fcg.Key,
                 Available = fcg.Sum(f => f.Hours) < 24
             })
}

1 I assume this is EF, because Linq-to-Sql doesn't allow local sequences in a LINQ query.


Using jQuery to set the class of an element corresponding to the selected radio button I'm a bit of a newb with jQuery but can get it working to perform the functionality that I'm after.

The below function checks the value of the radio input and then adds a class, depending on the selection. But it feels like there is a more efficient way of writing this.

$('input[name=choice]').change(function(){
 var inputVal = $('input[name=choice]:checked').val();

 if(inputVal == 'Option A') {
  $(".img-wrapper").addClass('option-a');
 } else {
  $(".img-wrapper").removeClass('option-a');
 }

 if(inputVal == 'Option B') {
  $(".img-wrapper").addClass('option-b');
  $(".img-wrapper").removeClass('option-a');
 } else {
  $(".img-wrapper").removeClass('option-b');
 }

 if(inputVal == 'Option C') {
  $(".img-wrapper").addClass('option-c');
  $(".img-wrapper").removeClass('option-a');
 } else {
  $(".img-wrapper").removeClass('option-c');
 }

 if(inputVal == 'Option D') {
  $(".img-wrapper").addClass('option-d');
  $(".img-wrapper").removeClass('option-a');
 } else {
  $(".img-wrapper").removeClass('option-d');
 }
});

   <input type="radio" name="choice" value="Option A">
   <input type="radio" name="choice" value="Option B">
   <input type="radio" name="choice" value="Option C">
   <input type="radio" name="choice" value="Option D">

https://jsfiddle.net/fhaLztL6/

You could use labels for your radio inputs and use the actual class names for the values. Then on change, you'd just remove all the option classes and pass in your inputVal for the class to add. See the snippet below (colors added to the classes via CSS and text added in .img-wrapper to show it working).

$('input[name=choice]').change(function(){
 var inputVal = $('input[name=choice]:checked').val();

 $(".img-wrapper")
  .removeClass('option-a option-b option-c option-d')
  .addClass(inputVal);
});
.option-a
{
  color: red;
}

.option-b
{
  color: green;
}

.option-c
{
  color: blue;
}

.option-d
{
  color: yellow;
}

   Option A
   <input type="radio" name="choice" value="option-a" checked>
   Option B
   <input type="radio" name="choice" value="option-b">
   Option C
   <input type="radio" name="choice" value="option-c">
   Option D
   <input type="radio" name="choice" value="option-d">

Blah


Program to output the same highest numbers in an array Here is my program to find the largest number(s) in a list of elements, possibly non-unique (in Python):

item_no = [5, 6, 7, 8, 8]
max_no = 0
for i in item_no:
    if i > max_no:
        max_no = i
        high = [i]
    elif i == max_no:
        high.append(i)

Example input: [5, 6, 7, 8, 8]

Example output: [8, 8]

How can I make this program more effective and shorter?

Any help would be highly appreciated.

This code is based on help received on Stack Overflow.

Note: After posting my answer I found out that the question is a duplicate of this question.

Readability

If you have no values then your code will error when you try to interact with high. You should define high outside the loop.
You should use a function so it's easier to test your code.
You should use full words for variable names.

def largest_orig_pretty(values):
    max_no = 0
    highest = []
    for value in values:
        if value > max_no:
            max_no = value
            highest = [value]
        elif value == max_no:
            highest.append(value)
    return highest

Performance

This however isn't shorter or faster than your code.

When optimizing Python it's almost guaranteed that all C functions are faster than the Python equivalent. And so we can test some changes out:

Remove max_no to make your code smaller.
Remove max_no and use iter to not incur \$O(n)\$ memory.
Try using collections.Counter, in the hopes it's written in C.
Use list.sort to sort the input so we can just take the first elements that are the same.

I did this in multiple ways:

Using a for loop. largest_sort.
Using a functional/iterator approach, but using lambda. largest_sort_semifunctional.
Using a functional/iterator approach, using functools.partial and operator.eq. largest_sort_functional.

Using sorted to not mutate the input list. And otherwise using the same code as largest_sort.
Using max and build a new list using list.count.

Most of the answers in the duplicate question derive from this method.

Allan's answer is the same as my original one. largest_max_count.
Ev.Kounis's answer suggests using a comprehension to build the output list.
Daweo's answer suggested not using list.count and instead use a list comprehension. There are two varients one when they use max once and one where it's used on every iteration.

Grijesh Chauhan's answer suggested storing only the value and a count, rather than building a list.

I have not tested the output of these functions, only the timings. The sample data is also uniform, and so may not accurately describe the expected timings if this assumption is not true.

Disclaimer, graphtimer was written and is maintained by me. It is only to produce the graph.

import collections
import itertools
import operator
import functools

import matplotlib.pyplot as plt
import numpy as np

from graphtimer import Plotter, MultiTimer

def largest_orig(values):
    max_no = 0
    for i in values:
        if i > max_no:
            max_no = i
            high = [i]
        elif i == max_no:
            high.append(i)
    return high

def largest_orig_pretty(values):
    max_no = 0
    highest = []
    for value in values:
        if value > max_no:
            max_no = value
            highest = [value]
        elif value == max_no:
            highest.append(value)
    return highest

def largest_no_max(values):
    if not values:
        return []
    highest = [values[0]]
    for value in values[1:]:
        if value > highest[0]:
            highest = [value]
        elif value == highest[0]:
            highest.append(value)
    return highest

def largest_no_max_iter(values):
    values = iter(values)
    highest = [next(values)]
    for value in values:
        if value > highest[0]:
            highest = [value]
        elif value == highest[0]:
            highest.append(value)
    return highest

def largest_counter(values):
    if not values:
        return []
    c = collections.Counter(values)
    k = max(c)
    return [k] * c[k]

def largest_sort(values):
    if not values:
        return []
    values.sort(reverse=True)
    value = values[0]
    for i, v in enumerate(values):
        if value != v:
            return [value] * i
    return values

def largest_sort_semifunctional(values):
    if not values:
        return []
    values.sort(reverse=True)
    value = values[0]
    return list(itertools.takewhile(lambda i: i == value, values))

def largest_sort_functional(values):
    if not values:
        return []
    values.sort(reverse=True)
    value = values[0]
    return list(itertools.takewhile(functools.partial(operator.eq, value), values))

def largest_sorted(values):
    if not values:
        return []
    values = sorted(values, reverse=True)
    value = values[0]
    for i, v in enumerate(values):
        if value != v:
            return [value] * i
    return values

# Same as https://stackoverflow.com/a/55216417
def largest_max_count(values):
    if not values:
        return []
    maximum = max(values)
    return [maximum] * values.count(maximum)

# Derived from Ev. Kounis' answer: https://stackoverflow.com/a/55216309
def largest_max_count_comprehension(values):
    if not values:
        return []
    maximum = max(values)
    return [maximum for _ in range(values.count(maximum))]

# Derived from Daweo's answer: https://stackoverflow.com/a/55216401
def largest_max_comprehension(values):
    if not values:
        return []
    return [value for value in values if value == max(values)]

# Derived from Daweo's answer: https://stackoverflow.com/a/55216401
def largest_max_comprehension_once(values):
    if not values:
        return []
    maximum = max(values)
    return [value for value in values if value == maximum]

# Grijesh Chauhan's answer: https://stackoverflow.com/a/55254044
def largest_grijesh_chauhan(iterable):
    max = None
    count = 0
    for index, value in enumerate(iterable):
        if index == 0 or value >= max:
            if value != max:
                count = 0
            max = value
            count += 1
    return count * [max]

def main():
    fig, axs = plt.subplots()
    axs.set_yscale('log')
    axs.set_xscale('log')
    (
        Plotter(MultiTimer([
            largest_orig,
            largest_orig_pretty,
            largest_no_max,
            largest_no_max_iter,
            largest_counter,
            largest_sort,
            largest_sort_semifunctional,
            largest_sort_functional,
            largest_sorted,
            largest_max_count,
        ]))
            .repeat(10, 10, np.logspace(0, 5), args_conv=lambda n: list(np.random.rand(int(n))))
            .min()
            .plot(axs, title='Maximums')
    )
    fig.show()

    fig, axs = plt.subplots()
    axs.set_yscale('log')
    axs.set_xscale('log')
    (
        Plotter(MultiTimer([
            largest_orig_pretty,
            largest_sort,
            largest_max_count,
            largest_max_count_comprehension,
            largest_max_comprehension,
            largest_max_comprehension_once,
            largest_grijesh_chauhan,
        ]))
            .repeat(10, 10, np.logspace(0, 3), args_conv=lambda n: list(np.random.rand(int(n))))
            .min()
            .plot(axs, title='Maximums max count derived')
    )
    fig.show()

if __name__ == '__main__':
    main()

Resulting in the following timings:

The following is a comparison of the SO answers:

Conclusion

[Performance] sort is the fastest method when there are ~10 or more elements in the list. Despite the \$O(n\log{n})\$ big-O time complexity it's the fastest as it's written in C.

It mutates the input which may not be desirable, and using sorted destroys the performance gain.

Using a plain for loop is faster than itertools.
[Performance] Using max and list.count is the fastest method if you are not allowed to mutate the list.
[Size] Using max and list.count results in the smallest amount of lines of code.

And so depending on your need I'd use either largest_max_count or largest_sort.


Prepare data for a contour plot with matplotlib My question concerns the following Python code which is already working. As far as I have seen, there are very elegant solutions of compacting code. Do you have any ideas on how to make the following code look smoother?

mom = [0.,0.13,0.27,0.53,0.67]
strings = ['overview_files/root/file_' + str(e) for e in mom]
myfile = [np.loadtxt(s) for s in strings]
nbinp = len(mom)
nbinomega = len(myfile[0][:,0])

x, y, z = (np.empty(nbinp*nbinomega) for i in range(3))
for i in range(nbinomega):
  for j in range(nbinp):
    i_new = i + j*nbinomega
    y[i_new] = myfile[j][i,0] - 1.4
    x[i_new] = mom[j]
    z[i_new] = myfile[j][i,1]

You may want to follow PEP8 a bit more closely to learn how to write code that is easily understood by most Python developers. Eg. mom = [0.3, 0.13] instead of mom = [0.3,0.13] and four spaces indentation.

Try to be more careful about variable names. I couldn't understand what most of them meant, which is probably because I don't much about the code you're writing. But think about your readers (including you in three months) and wonder what are the best ways to convey information in your variable names. For example, 'myfile'  suggest that this is not a collection. And 'my' doesn't provide any useful information.

There's a common idiom in Python to avoid dealing with ranges explicitely: enumerate().

for i, this in enumerate(myfile):
    for j, that in enumerate(myfile[i]):
        i_new = ...
        y[i_new] = ...
        x[i_new] = ...
        z[i_new] = ...

If you often deal with ranges, it will certainly help you at some point.


Dynamic variables in loop I have this working code. I think this must be easier with a while loop.

if not c.e1.nil?
  extrapreis = number_to_currency Extra.find(c.e1).aufschlag, :unit=>"EUR "
  array.push Extra.find(c.e1).name + " (" + extrapreis.to_s + ")"
  extpreis = extpreis + Extra.find(c.e1).aufschlag
end
if not c.e2.nil?
  extrapreis = number_to_currency Extra.find(c.e2).aufschlag, :unit=>"EUR "
  array.push Extra.find(c.e2).name + " (" + extrapreis.to_s + ")"
  extpreis = extpreis + Extra.find(c.e2).aufschlag
end
if not c.e3.nil?
  extrapreis = number_to_currency Extra.find(c.e3).aufschlag, :unit=>"EUR "
  array.push Extra.find(c.e3).name + " (" + extrapreis.to_s + ")"
  extpreis = extpreis + Extra.find(c.e3).aufschlag
end
if not c.e4.nil?
  extrapreis = number_to_currency Extra.find(c.e4).aufschlag, :unit=>"EUR "
  array.push Extra.find(c.e4).name + " (" + extrapreis.to_s + ")"
  extpreis = extpreis + Extra.find(c.e4).aufschlag
end
if not c.e5.nil?
  extrapreis = number_to_currency Extra.find(c.e5).aufschlag, :unit=>"EUR "
  array.push Extra.find(c.e5).name + " (" + extrapreis.to_s + ")"
  extpreis = extpreis + Extra.find(c.e5).aufschlag
end

It is always the same code, except for the variable e (1, 2, 3, 4, 5). How can I put this in a while loop and put the counter together with e?

(1..5).each do |i|
  e = c.send("e#{i}")
  next if e.nil?
  found = Extra.find(e)
  array.push("#{found.name} (#{number_to_currency(found.aufschlag, unit: "EUR ")})")
  extpreis += found.aufschlag
end

If you can be certain that c.e1 etc. never becomes false, then:

(1..5).each do |i|
  next unless e = c.send("e#{i}")
  found = Extra.find(e)
  array.push("#{found.name} (#{number_to_currency(found.aufschlag, unit: "EUR ")})")
  extpreis += found.aufschlag
end