Custom component organization system I'm rewriting an old Swing project that I did a few months ago, a fairly simple copy-cat of MS paint. I've been doing a pretty good job of keeping the code maintainable and organized, but I've hit a wall on one class. All I need is some general tips on organization on this one class.

The way I have my program set up is that each component of my painting program (buttons for colors, eraser button, JPanel to paint on) is actually a relative (extended from) of another swing component class (for instance PaintPanel, the panel you paint on, extends JPanel, or colorButton is an extension of JButton). This just allows me to keep things encapsulated and organized.

So, I needed a JFileMenu for the top of the program in order to save, open files, edit, etc. I created a new Class called PaintMenuBar, extended it from JMenuBar, and started creating the JMenus and JMenuItems. Now, each JMenuItem obviously needs an ActionListener, so I just gave each one a different ActionListener. I'm skeptical about this because I know it can be kind of disorganized looking.

Basically, I just want somebody to let me know if this practice is all right, or if there is a better alternative that doesn't involve me making 20 new classes.

Here's the entire class:

import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

import javax.swing.JMenu;
import javax.swing.JMenuBar;
import javax.swing.JMenuItem;

public class PaintMenuBar extends JMenuBar
{
    private static final long serialVersionUID = 1L;

    JMenu fileMenu;
    JMenu editMenu;
    JMenu aboutMenu;

    public PaintMenuBar()
    {
        setUpFileMenu();
        setUpEditMenu();
        setUpAboutMenu();
    }

    // Set up menu items & such

    private void setUpFileMenu()
    {
        fileMenu = new JMenu("File");

        // File menu items
        JMenuItem newMenuItem = new JMenuItem("New");
        newMenuItem.addActionListener(new ActionListener()
        {
            @Override
            public void actionPerformed(ActionEvent e)
            {

            }
        });

        JMenuItem openMenuItem = new JMenuItem("Open");
        openMenuItem.addActionListener(new ActionListener()
        {
            @Override
            public void actionPerformed(ActionEvent e)
            {

            }
        });

        JMenuItem saveMenuItem = new JMenuItem("Save");
        saveMenuItem.addActionListener(new ActionListener()
        {
            @Override
            public void actionPerformed(ActionEvent e)
            {

            }
        });

        JMenuItem saveAsMenuItem = new JMenuItem("Save As");
        saveAsMenuItem.addActionListener(new ActionListener()
        {
            @Override
            public void actionPerformed(ActionEvent e)
            {

            }
        });

        JMenuItem printMenuItem = new JMenuItem("Print");
        printMenuItem.addActionListener(new ActionListener()
        {
            @Override
            public void actionPerformed(ActionEvent e)
            {

            }
        });

        add(fileMenu);
    }

    private void setUpEditMenu() // Create Edit Menu
    {
        editMenu = new JMenu("Edit");
    }

    private void setUpAboutMenu() // Create About Menu
    {
        aboutMenu = new JMenu("About Paint 2.0");
    }
}

I am aware of some of the junk that is in this class that I still need to clean up, but just try and ignore it.

+1 to @Gilbert Le Blanc's comment. Creating

I'd consider extracting out methods like createOpenMenuItem(), createSaveMenuItem(), createPrintMenuItem() for better readability and higher abstraction:

private JMenuItem createOpenMenuItem() {
    JMenuItem openMenuItem = new JMenuItem("Open");
    openMenuItem.addActionListener(new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent e) {

        }
    });
    return openMenuItem;
}

Then the setUpFileMenu would be a lot cleaner:

private void setUpFileMenu() {
    fileMenu = new JMenu("File");

    // File menu items
    fileMenu.add(createNewMenuItem());
    fileMenu.add(createOpenMenuItem());
    fileMenu.add(createSaveMenuItem());
    fileMenu.add(createSaveAsMenuItem());
    fileMenu.add(createPrintMenuItem());

    add(fileMenu);
}

A maintainer would be helpful, it's easier to get an overview what the method does (without the details) and you can still check them if you are interested in. For example, finding code of save menu is much faster than before.

  Now, each JMenuItem obviously needs an ActionListener,
  so I just gave each one a different ActionListener.
  I'm skeptical about this because I know it can be kind of disorganized looking.

I've found this practice easier to work with than having one ActionListener for multiple components (with a switch-case inside).

  Basically, I just want somebody to let me know
  if this practice is all right, or if there is a better
  alternative that doesn't involve me making 20 new classes.

Having a lot of classes doesn't hurt if they have clear responsibilties.


Simple read/write lock implementation without starvation I've built a read/write lock and have been testing it without encountering any problems. It was made to avoid writer starvation, but I believe it works against reader starvation as well. I've seen alternatives online, but was wondering if this is a solid implementation.
If you use a normal shared mutex, new read actions can still be queued, which will prevent write actions from ever being processed while there is any read action present. This will cause starvation. I used a second mutex which will be locked by the write process and prevents any new read processes to be queued. Thank you!
class unique_priority_mutex
{
public:

    void lock_shared(void)
    {
        // If there is a unique operation running, wait for it to finish.
        if( this->_is_blocked ){
            // Use a shared lock to let all shared actions through as soon as the unique action finishes.
            std::shared_lock<std::shared_mutex> l(this->_unique_mutex);
        }

        // Allow for multiple shared actions, but no unique actions.
        this->_shared_mutex.lock_shared();
    }

    void unlock_shared(void)
    {
        this->_shared_mutex.unlock_shared();
    }

    void lock(void)
    {
        // Avoid other unique actions and avoid new shared actions from being queued.
        this->_unique_mutex.lock();

        // Redirect shared actions to the unique lock.
        this->_is_blocked = true;

        // Perform the unique lock.
        this->_shared_mutex.lock();
    }

    void unlock(void)
    {
        this->_shared_mutex.unlock();
        this->_is_blocked = false;
        this->_unique_mutex.unlock();
    }

    std::shared_mutex _shared_mutex;
    std::shared_mutex _unique_mutex;
    std::atomic<bool> _is_blocked = false;
};

You'd need _shared_mutex and _is_blocked to flip together 'atomically' in order for this code to work, no?
Imaging one thread in lock() just flipped _unique_mutex and before it flips _is_blocked, another thread in lock_shared() is testing _is_blocked.


Pivot pandas DataFrame while removing index duplicates The goal of my code is to pivot a pandas DataFrame which is shown below. The idea is to use the Individual column as the column IDs and the VAF column as the values. I would like to combine the data such that the values from the columns Loc, Change, Chrom are used as the new index. To do this I made two new columns, newindex, and uniques in order to remove duplicates and then pivot.

Original DataFrame:

  Chrom        Loc WT Var Change ConvChange  AO     DP          VAF IntEx  \
0  chr1  115227855  T   A    T>A        T>A   5  19346  0.000258451  TIII
1  chr1  115227855  T   C    T>C        T>C   4  19346  0.000206761  TIII
2  chr1  115227856  C   T    C>T        C>T  14  19377  0.000722506  TIII
3  chr1  115227857  C   A    C>A        C>A   3  19377  0.000154823  TIII
4  chr1  115227857  C   T    C>T        C>T  15  19377  0.000774114  TIII

    Gene Upstream Downstream Individual          newindex             uniques
0  TIIIa        T          C          1  115227855T>Achr1  115227855T>Achr1_1
1  TIIIa        T          C          1  115227855T>Cchr1  115227855T>Cchr1_1
2  TIIIa        T          C          1  115227856C>Tchr1  115227856C>Tchr1_1
3  TIIIa        C          A          1  115227857C>Achr1  115227857C>Achr1_1
4  TIIIa        C          A          1  115227857C>Tchr1  115227857C>Tchr1_1

Code:

x = x.set_index(['uniques'])
x = x[~x.index.duplicated(keep='first')]
x = x.pivot(index='newindex', columns='Individual', values='VAF')

So in the above code I set the new index that uses the uniques column that is the same as the newindex column but with the values from Individual appended. Then remove the duplicate indices, and finally pivot into the final form shown below.

Individual                  1            2            3            4  \
newindex
106155152T>Achr4  0.000120685  0.000383835  0.000383224  0.000136617
106155152T>Cchr4  0.000603427  0.000575753  0.000694594  0.000461081
106155152T>Gchr4         None         None  0.000143709  6.83083e-05
106155153G>Achr4         None  0.000355969  0.000239257  0.000392398
106155153G>Tchr4         None  5.47645e-05         None  3.41215e-05

Individual                  5            6            7
newindex
106155152T>Achr4  0.000403506  0.000443759  0.000477926
106155152T>Cchr4  0.000560425  0.000570547   0.00155326
106155152T>Gchr4         None  8.45255e-05         None
106155153G>Achr4  0.000179127  0.000274534  0.000298436
106155153G>Tchr4  6.71727e-05         None         None

To clarify, I'm guessing there is probably a much more efficient way of making the new indexes that don't contain duplicates than making the newindex and uniques columns like I do.

I would use the built-in drop_duplicates() functionality.

x.drop_duplicates(['Loc', 'Change', 'Chrom'], keep='first', inplace=True)
x = x.pivot(index='newindex', columns='Individual', values='VAF')


Copy range up to delimiter The problem I was trying to solve sounded pretty basic, but turned out to be a bit hairy.

I need to select some lines of arbitrary length from an input stream (file or standard input) and print them to output. In other words, a function that just echos the line would do the job. The input stream is encoded in UTF-8. For reasons that I don't quite understand, the normal <string> mangles the stream, so I couldn't use:

std::string line;
std::readline(line);
std::cout << line;

I did solve it using basic_istream::getline(); however, this function writes to a buffer, and I need to tell it how big the buffer is. To make it deal with lines of arbitrary length, I need to read to a buffer in a loop, checking whether failbit is set (when the end of the line wasn't reached).

Then, the easier solution was to use iterators on the input and the output stream, like this:

template<typename I, typename O, typename T>
I copy_until(I first, I last, O d_first, T delim)
{
    while (first != last && *first != delim) {
        *d_first = *first;
        ++first;
        ++d_first;
    }
    return first;
}

This copies from first up to last at most, and stops if the delimiter is encountered. It returns a pointer to the first character in the input range that was not copied.

It can be used like this to copy all input to output:

int main()
{
    std::istreambuf_iterator<char> first(std::cin);
    std::istreambuf_iterator<char> last;
    std::ostreambuf_iterator<char> d_first(std::cout);

    while (first != last) {
        first = copy_until(first, last, d_first, '\n');
        std::cout.put(*first);
        ++first;
    }
}

The third argument of copy_until could of course be a unary predicate, then instead of while (first != last && *first != delim) it would say while (first != last && !p(*first)).

More important questions:

Is there an algorithm in <algorithm> that I have overlooked? copy_if and partition_point come close, but they don't do exactly that, and partition_point followed by copy_n will not work with forward iterators.
I am not sure how to declare that the two iterators need to point to the same type as the type of the delimiter or the argument to the unary predicate.
Not certain about the return value. This one is the most useful for my particular use case.
Or maybe there is a much, much easier way that avoids this altogether. But copy_until might be useful in other contexts?

The third argument of copy_until could of course be a unary predicate, then instead of while (first != last && *first != delim) it would say while (first != last && !p(*first)).

Even better: remove the I last argument entirely! Just use the predicate for the whole thing, and the condition becomes while (!p(first)).

You're returning I (the place you stopped parsing the input stream), but similar functions in the STL return O (the place they stopped outputting to the output stream). The latter is more useful in STL cases, because typically the caller knows where he stopped parsing the input: it's last!

In your case, both I and O are useful; so why not return both?

template<typename I, typename O, typename Pred>
std::pair<I, O> copy_until(I first, O d_first, Pred pred)
{
    while (!pred(first)) {
        *d_first = *first;
        ++first;
        ++d_first;
    }
    return {first, d_first};
}

Your main thus becomes:

int main()
{
    std::istreambuf_iterator<char> first(std::cin);
    std::istreambuf_iterator<char> last;
    std::ostreambuf_iterator<char> d_first(std::cout);

    while (first != last) {
        std::tie(first, std::ignore) = copy_until(
            first, d_first, [=](auto it){ return it == last || *it == '\n'; }
        );
        if (first == last) break;  // Whoops!
        std::cout.put(*first);
        ++first;
    }
}

The // Whoops! comment indicates the fix for a crashing bug in your original main.

Boost.Algorithm actually has both copy_until and its inverse, copy_while: http://marshall.calepin.co/copy_while.html
Strangely, Boost's version takes last as a parameter exactly the way yours does. In Boost's case, I think that might be because they're trying to target C++03, which doesn't have lambdas, so there's no way that's both easy and lightweight to express [last](auto it) { return it != last && *it != delim; } — in C++03 you had to do that with boost::bind and maybe boost::function as well.

  I am not sure how to declare that the two iterators need to point to the same type as the type of the delimiter or the argument to the unary predicate.

The short answer is "don't"; complicated dependencies like that usually indicate that you've put the abstraction boundary in the wrong place (e.g. in this example, passing First, Last, Delim instead of First, Pred). But just for the record, you could do something crazy with enable_if, which can go on the return type of the function or on one of its parameters or on one of its template parameters, your choice.

auto copy_until(I first, I last, O d_first, T delim)
    -> std::enable_if_t<std::is_same_v<decltype(*first), T>, I>
{
    // ...
}

I'm pretty sure you don't want to assert that decltype(*d_first) is T, because that would prevent you from using something like a back_insert_iterator<vector<T>> with this algorithm.


Class project: user-editing page for a haiku website I am making a website using just HTML and CSS for a classroom project (we are in the 2nd week of boot-camp.) I have my site made, and want to get others' opinion on the code. I am mostly looking to have the site scale properly on different browsers, but other input is appreciated as well.

body {
    font-family: 'Anton', sans-serif;
    margin: auto;
}

.top {
    background-color: #F25266;
    border: #666561 solid 1em;
}

h1 {
    color:#F4EF6B;
    text-decoration: underline;
    text-align: center;
}

.middle {
    background-color:#ECECEC;
    border-left: #666561 1em  solid;
    border-right: #666561 1em solid;
}

.pic {
   display: block;
  margin: auto;
  width: 40%;
}

form > label{
    color: #2E90BF;
    column-count: 1
}

form > input {
    background-color: #2E90BF;
    border-color: #666561;
    width: 14.2%;
    column-count: 1
}

#button_one {
    font-family: 'Anton', sans-serif;
    color:#FFF;
    background: #2E90BF;
    border-color: #000;
    height: 2em;
    width: 100%;
    font-size: 1em;
    cursor: pointer;
    margin-top: 1em;
}

#button_one:active {
  background-color: #2E90BF;
  transform: translateY(4px);
}

.bottom {
    background-color:#F25266;
    border: #666561 solid 1em;
}

ul {
    list-style: none;
    column-count: 10;
}

a:link {
    color: #6DBF2E;
}

a:visited {
    color: #6DBF2E;
}

a:hover {
    color: yellow;
}
<!DOCTYPE html>

        <title>Edit User</title>

        <link href="https://fonts.googleapis.com/css?family=Anton" rel="stylesheet">

        <link type="text/css" rel="stylesheet" href="assets/stylesheets/style.css" />

User Information

        <form>
            E-Mail:
            <input>
            Password:
            <input>
            Confirm Password:
            <input>
            First Name:
            <input>
            Last Name:
            <input>

            <button type="submit" id="button_one">Submit</button>
        </form>

Create Haiku

Edit Haiku

Edit User

Signup User

List

One

Rules

Index

User List

User One

About the HTML

It’s typically a good idea to add a meta-charset element, which gives the character encoding of the page (e.g., "UTF-8"). This should be the first element in the head element:

  <meta charset="utf-8" />

The body element can’t have a background attribute (it’s obsolete).

You should associate each label with its input. This can be done with the for attribute or by nesting the input inside the label. You can easily test if it works correctly: click the label, and the corresponding input should get the focus.

You have the closing tag
, but miss the opening tag -- however, the br element has no closing tag to begin with, so you probably want to use
 or
. But it doesn’t seem to be correct to use br in this context anyway. br must only be used for meaningful line-breaks. If you just need some vertical space, use CSS instead.

The list seems to be navigation. If that’s the case, you could use the nav element:


HTML and jQuery audio player I had a task to create simple audio player. So far I managed to do functionalities and all that is needed. Other part of same task was to make it semantic and modular.

I read some blog posts and articles about semantic but I am not sure how to do it in right way.

player();

  function player(){

    music = new Audio('http://www.w3schools.com/tags/horse.mp3');

  }

  $('.button__play').click(function(event){
    event.preventDefault();
    music.play();
  });
  $('.button__pause').click(function(event){
    event.preventDefault();
    music.pause();
  });
  $('.button__stop').click(function(event){
    event.preventDefault();
    music.pause();
    music.currentTime = 0;
  });

  $('.button__repeat, .button__rate').click(function(event){
    event.preventDefault();
    $(this).toggleClass('button__active');
  });
.player{
  background: #eaeaea;
  width: 360px;
  font-family: 'Arial', sans-serif;
  font-size: 16px;
}
.cover{
  width: 100%;
  height: 160px;
  overflow:hidden;
}
.cover__img{
  width: 100%;
  height: auto;
  display:block;
}
.container{
  padding: 0.500em;
}
.info{
  margin: 1em 0;
}
.info:after{
  content: '';
  clear: both;
  display: table;
}
.song{
  list-style: none;
  margin:0;
  padding: 0;
  float: left;
  width: 75%;
}
.progress{
  height: 0.250em;
  width: 100%;
  background: orangered;
  margin: 1em 0;
  postion: relative;
}
.progress__bar{
  height: 0.250em;
  width: 20%;
  background: lightblue;

}
.button{
  display: inline-block;
  text-decoration: none;
  padding: 0.250em 0.500em;
}
.button__play{
  background: orangered;
  color: white;
}
.button__pause, .button__stop{
  font-size: 0.750em;
  background: gray;
  color: white;
}
.button__repeat, .button__rate{
  color: gray;
}
.button__active{
  color: red;
}

forest

rain

        rate

      play
      pause
      stop
      repeat

I don't know exactly what you mean about semantic and modular, but I usually like to use object oriented programming. You could create multiple and isolated players.

function AudioPlayer(src, $control) {

  var _this = this;

  this.src = src;
  this.music = new Audio(src);

  this.btnPlay = $control.find('.button__play');
  this.btnPause = $control.find('.button__pause');
  this.btnStop = $control.find('.button__stop');
  this.btnRepeat = $control.find('.button__repeat');
  this.btnRate = $control.find('.button__rate');

  this.btnPlay.click(function(event){
    event.preventDefault();
    _this.music.play();
  });

  this.btnPause.click(function(event){
    event.preventDefault();
    _this.music.pause();
  });

  this.btnStop.click(function(event){
    event.preventDefault();
    _this.music.pause();
    _this.music.currentTime = 0;
  });

  this.btnRepeat.click(function(event){
    event.preventDefault();
    $(this).toggleClass('button__active');
  });

  this.btnRate.click(function(event){
    event.preventDefault();
    $(this).toggleClass('button__active');
  });

}

AudioPlayer.prototype.play = function() {

  this.music.play();
}

AudioPlayer.prototype.pause = function() {
  this.music.pause();
}

$(document).ready(function() {

  //Here you could create more isolated audio controls and instantiate them

  var controlPlayer1 = $('.container');
  var player1 = new AudioPlayer('http://www.w3schools.com/tags/horse.mp3', controlPlayer1);
})


Pulling data from multiple excel files in Python I understand this code is running properly and is grabbing from each excel file individually, but I want to create a for loop for using this data across multiple excel files

kinematic_viscosity = []

kinematic_viscosity_1= []

kinematic_viscosity_2= []
#*empty arrays for creating after loop in completed*
for dynamic_viscosity in DV:
   kinematic_viscosities = dynamic_viscosity/Density_PE_298k
   kinematic_viscosity.append(kinematic_viscosities)

for dynamic_viscosity_1 in DV_1:

    kinematic_viscosities_1 = dynamic_viscosity_1/Density_PE_298k
    kinematic_viscosity_1.append(kinematic_viscosities_1)
#second loop
for dynamic_viscosity_2 in DV_2:

    kinematic_viscosities_2 = dynamic_viscosity_2/Density_PE_298k
    kinematic_viscosity_2.append(kinematic_viscosities_2)
#third loop

One possible solution is using zip(). This allows you to iterate, in your case, through multiple lists at a time. You can also condense creating the lists into one line. And, you can remove assigning dynamic_viscosity_#/Density_PE_298k to a variable, just append that to the lists. Consider the following:

kinematic_viscosity, kinematic_viscosity_1, kinematic_viscosity_2 = [], [], []

for dynamic_viscosity, dynamic_viscosity_1, dynamic_viscosity_2 in zip(DV, DV_1, DV_2):
    kinematic_viscosity.append(dynamic_viscosity/Density_PE_298k)
    kinematic_viscosity_1.append(dynamic_viscosity_1/Density_PE_298k)
    kinematic_viscosity_2.append(dynamic_viscosity_2/Density_PE_298k)

While it looks more blocky, it allows you to only loop through the lists once.

Another solution is to keep the three loops, but append the returns directly into the list. Consider the following:

kinematic_viscosity = [dynamic_viscosity/Density_PE_298k for dynamic_viscosity in DV]

kinematic_viscosity_1= [dynamic_viscosity_1/Density_PE_298k for dynamic_viscosity_1 in DV_1]

kinematic_viscosity_2= [dynamic_viscosity_2/Density_PE_298k for dynamic_viscosity_2 in DV_2]


library having two types of books I need to make a project related to a library catalog.

It should have following methods relevant to these books:

add
delete
list
There are two types of books: novels and poetry. Both types have number of pages and names. Novels have categories (SF...) and poetry have themes. Need to be able to list the books in the library.

How should I model the code for this (using inheritance, Lists, no hashmap)? I have thought to create a class BasicBook (fields pages, names), Novels (field category) and Poetry (field theme) to inherit from BasicBook. Create a class Library (fields List Novel, List Poetry) and methods to add, delete, print).
Is there a better way to tackle this problem? Thanks a lot!

package com.andy85;
public class Book {
private String name;
private int numOfPages;

  public Book(String name, int numOfPages) {
        this.name = name;
        this.numOfPages = numOfPages;
    }

    public String getName() {
        return name;
    }

    public void setName(String name) {
        this.name = name;
    }

    public int getNumOfPages() {
        return numOfPages;
    }

    public void setNumOfPages(int numOfPages) {
        this.numOfPages = numOfPages;
    }

    @Override
    public String toString() {
        return this.name + ", " + this.numOfPages;
    }
}

public class Novel extends Book {
    private String types;

    public Novel(String name, int numOfPages, String types) {
        super(name, numOfPages);
        this.types = types;
    }

    public String getTypes() {
        return types;
    }

    public void setTypes(String types) {
        this.types = types;
    }

    @Override
    public String toString() {
        return super.toString() + ": " + types;
    }
}

public class ArtAlbum extends Book {
    private String paperQuality;

    public ArtAlbum(String name, int numOfPages, String paperQuality) {
        super(name, numOfPages);
        this.paperQuality = paperQuality;
    }

    public String getPaperQuality() {
        return paperQuality;
    }

    public void setPaperQuality(String paperQuality) {
        this.paperQuality = paperQuality;
    }

    @Override
    public String toString() {
        return super.toString() + paperQuality;
    }

public class LibraryCatalog {
    private List<Novel> novel = new ArrayList<>();
    private List<ArtAlbum> artAlbum = new ArrayList<>();

    public LibraryCatalog(List<Novel> novel, List<ArtAlbum> artAlbum) {
        this.novel = new ArrayList<>(novel);
        this.artAlbum = new ArrayList<>(artAlbum);
    }

    public List<Novel> getNovel() {
        return novel;
    }

    public void setNovel(List<Novel> novel) {
        this.novel = novel;
    }

    public List<ArtAlbum> getArtAlbum() {
        return artAlbum;
    }

    public void setArtAlbum(List<ArtAlbum> artAlbum) {
        this.artAlbum = artAlbum;
    }

    public void addNovel (Novel newNovel) {
        this.novel.add(newNovel);
    }
}

Design

Since duplicates are allowed (no Set required) and you don't need to fetch books by name (no Map required), ArrayList will do just fine.

Overview Collection Types

Guard public entrypoints

Make sure to checks arguments on all publically available code that takes input from calling code.

if (novels == null) throw new IllegalArgumentException("novels must be set");

Use plural for variable names that represent collections

private List<Novel> novels = new ArrayList<>();
private List<ArtAlbum> artAlbums = new ArrayList<>();

Initialize object members only once

Don't create the ArrayList instances twice. You can use addAll to add a range of items to a list. Perhaps you also want to create public methods to perform these actions.

public class LibraryCatalog {
        private List<Novel> novels = new ArrayList<>();
        private List<ArtAlbum> artAlbums = new ArrayList<>();

        public LibraryCatalog(List<Novel> novels, List<ArtAlbum> artAlbums) {
            if (novels == null)
                throw new IllegalArgumentException("novels must be set");
            if (artAlbums == null)
              throw new IllegalArgumentException("argument must be set");
            this.novels.addAll(novels);
            this.artAlbums.addAll(artAlbums);
        }
    }


Matching square brackets I wrote this program to match square brackets as part of a Brainf**k interpreter. I'm aware that it gets caught in an infinite loop if brackets are entered like this: ][.

Are there any performance improvements I can make?
Should I change any variable names?
Are there other syntax errors that will break the code?
How can I fix said errors?
Anything else I did wrong?

//Matches an opening or closing bracket to a corresponding opening or closing bracket
int getMatchingBraceIndex(unsigned int braceIndex, char* str) {
    //Current nesting level
    int level = 0;
    //Matching brace's nesting level
    //(Matching braces have to be at the same level)
    int returnLvl = -1;

    /*******************************************************************************\
     * Note:                                                                       *
     * If brace given is open brace, iterate through string from beginning to end  *
     * If brace given is close brace, iterate through string from end to beginning *
    \*******************************************************************************/
    //If brace given is open brace
    if (str[braceIndex] == '[') {
        //Iterator
        int i = 0;

        //Iterate through given string
        for (i; i < strlen(str); i++) {
            //If current character of string is open brace
            if (str[i] == '[') {
                //Increment nesting level
                level++;
                //If current character is given brace, assume that matching brace is on the same nesting level
                if (i == braceIndex) {
                    returnLvl = level;
                }
            //If current character of string is close brace
            } else if (str[i] == ']') {
                //If currnet nesting level is nesting level of matching brace, return current index
                if (level == returnLvl) {
                    return i;
                //Otherwise, decrement nesting level and keep going through the string
                } else {
                    level--;
                }
            }
        }
    //If brace given is close brace
    } else if (str[braceIndex] == ']') {
        //Iterator
        int i = strlen(str) - 1;

        //Iterate through given string
        for (i; i >= 0; i--) {
            //If current character of string is close brace
            if (str[i] == ']') {
                //Increment nesting level
                level++;
                //If current character is given brace, assume that matching brace is on the same nesting level
                if (i == braceIndex) {
                    returnLvl = level;
                }
            //If current character of string is open brace
            } else if (str[i] == '[') {
                //If nesting level is nesting level of matching brace, return current index
                if (level == returnLvl) {
                    return i;
                //Otherwise, decrement nesting level and keep going through the string
                } else {
                    level--;
                }
            }
        }
    } else {
        //Error
        setColor(RED, BLACK);
        printf("Unable to match brace at index %i\n", braceIndex);
        setColor(DEFAULT, BLACK);
        return -1;
    }

    return -1;
}

Do not use strlen() in for loops. strlen() walks over the string every time you call it.


Small app to show variables declared in a bash script This code and updates are available on Github.

I've written a small utility to dump the variables that are assigned in a bash script.

Please note:

This script depends on the shfmt and jq apps.
I am not looking to make this script posix compliant, only bash specific.

Some possible areas for improvement:

Can the jq query be improved?
Are there other entries in the json output of shfmt that I can look for variable assignments.
Any gotchas I'm missing?

As I type out this question, it occurs to me that showing variables used in a script would be a good thing to allow as an option. I'll look into it, but if anyone has any ideas in this respect, I'm interested in hearing your input.

#!/bin/bash

#------------------------------------------------------------------------------

warn() { printf '%s\n' "$*" >&2; }

die() {
  (($#)) && warn "$*"
  exit 1
}

command_exists() { command -v "$1" &> /dev/null; }

#------------------------------------------------------------------------------
_showvars() {
  local filename=$1
  [[ -f $filename ]] || die "$filename is not a file or does not exist."
  [[ -r $filename ]] || die "$filename is not readable."

  jq_query='[ .. | select(.Assigns?) | .. | select(.Name?) | .Name.Value ] | unique[]'

  # shellcheck disable=SC2046
  printf '  %s\n' $(shfmt -tojson < "$filename" | jq "$jq_query" | tr -d '"')
}

#------------------------------------------------------------------------------
(($#)) || {
  cat << EOH

showvars is a simple script that shows
what variables are assigned in a bash
script

usage: showvars filename [filename ...]

EOH

  exit 1
}

for r in shfmt jq; do
  command_exists $r || die This script depends on $r and it is not found.
done

for f in "$@"; do
  printf '\n%s:\n' "$f"
  _showvars "$f"
  shift
done

echo

Edit: Fixed copy-paste error with (($#))... line
Edit: Removed code that removes lower case variables (an artifact from another code solution)

The difference between $* and $@

warn() { printf '%s\n' "$*" >&2; }

This is equivalent to the simpler:

warn() { echo "$*" >&2; }

The printf version is useful if you want to produce one line per parameter,
and in that case you must use "$@" instead of "$*".
Also in callers of warn.

Use -r for raw output of jq

Instead of jq "..." | tr -d '"' a better way is jq -r "...".

An alternative to printf and a sub-shell

Instead of this:

  # shellcheck disable=SC2046
  printf '  %s\n' $(shfmt -tojson < "$filename" | jq "$jq_query" | tr -d '"')

I recommend this way (and no need to disable shellcheck):

shfmt -tojson < "$filename" | jq -r "$jq_query" | sed -e 's/^/  /'

Use a bit more double-quotes

You did a good job of double-quoting the most important things.
I would double-quote here too:

command_exists $r || die This script depends on $r and it is not found.

To train good habits:

command_exists "$r" || die "This script depends on $r and it is not found."

"$@" is the default list for for

Instead of for f in "$@"; do, you can simply write for f; do.

The shebang

In some systems Bash is not in /bin/bash.
For that reason I prefer to use #!/usr/bin/env bash as the shebang, it makes the script more portable.

Simplify the readable file check?

  [[ -f $filename ]] || die "$filename is not a file or does not exist."
  [[ -r $filename ]] || die "$filename is not readable."

The -r implies -f. I would simplify this to one line:

[[ -r $filename ]] || die "$filename is not a readable file."

Use echo when it's good enough

Instead of printf '\n%s:\n' "$f" I would write:

echo
echo "$f:"

Here-documents

EOH is an unusual symbol for the here-document marker.
That's not a problem, but I think the less surprising elements in a script,
the better.
I don't see a good reason to not call this EOF as usual.

Your questions

  Some possible areas for improvement:

  Can the jq query be improved?
  Are there other entries in the json output of shfmt that I can look for variable assignments.

Unfortunately I'm not able to answer these.
You might want to wait for another reviewer who can!


Averaging lists of values with duplicate keys I have gene expression data that I represent as a list of genes and a list of lists of values. I average the expression data for any genes with the same name.

For example:

genes = ['A', 'C', 'C', 'B', 'A']
vals  = [[2.0, 2.0, 9.0, 9.0], # A: will be averaged with row=4
         [3.0, 3.0, 3.0, 3.0], # C: will be averaged with row=2
         [8.0, 8.0, 2.0, 2.0], # C: will be averaged with row=1
         [4.0, 4.0, 4.0, 3.0], # B: is fine
         [1.0, 1.0, 1.0, 1.0]] # A: will be averaged with row=0

is converted to

genes = ['A', 'B', 'C']
vals  = [[1.5, 1.5, 5.0, 5.0],
         [4.0, 4.0, 4.0, 3.0],
         [5.5, 5.5, 2.5, 2.5]]

Here is my function:

def avg_dups(genes, values):
    """Finds duplicate genes and averages their expression data.
    """
    unq_genes = np.unique(genes)
    out_values = np.zeros((unq_genes.shape[0], values.shape[1]))
    for i, gene in enumerate(unq_genes):
        dups = values[genes==gene]
        out_values[i] = np.mean(dups, axis=0)
    return (unq_genes, out_values)

This function is slower than any other part of my data pipeline, taking 5-10 seconds when other steps that also operate on the whole dataset take sub-seconds. Any thoughts on how I can improve this?

This seems to be the fastest so far:

import numpy
from numpy import newaxis

def avg_dups(genes, values):
    folded, indices, counts = np.unique(genes, return_inverse=True, return_counts=True)

    output = numpy.zeros((folded.shape[0], values.shape[1]))
    numpy.add.at(output, indices, values)
    output /= counts[:, newaxis]

    return folded, output

This finds the unique genes to fold the values into, along with the current index → new index mapping and the number of repeated values that map to the same index:

    folded, indices, counts = np.unique(genes, return_inverse=True, return_counts=True)

It adds the row from each current index to the new index in the new output:

    output = numpy.zeros((folded.shape[0], values.shape[1]))
    numpy.add.at(output, indices, values)

numpy.add.at(output, indices, values) is used over output[indices] += values because the buffering used in += breaks the code for repeated indices.

The mean is taken with a simple division of the number of repeated values that map to the same index:

    output /= counts[:, newaxis]

Using Ashwini Chaudhary's generate_test_data(2000) (giving a 10000x4 array), my rough timings are:

name             time/ms  Author
avg_dups           230    gwg
avg_dups_fast       33    Ashwini Chaudhary
avg_dups_python     45    Ashwini Chaudhary
avg_dups           430    Veedrac
avg_dups             5    Veedrac with Jaime's improvement


Basic random password generator I'm trying to pick up some Haskell skills so thought I'd write a random password generator to get to grips with using IO. It was far trickier than I expected and I employed rather more trial and error than I'd have liked.

What I've ended up with works, and while I'm happy with it generally I'd like to know if this is reasonable, idiomatic Haskell or if I've done anything peculiar.

import System.Random

-- Generate a random number in closed interval [lo, hi]
random_int :: Int -> Int -> IO Int
random_int lo hi = (randomRIO (lo,hi) :: IO Int)

-- Generate a random character from an internal alphabet
random_char :: IO Char
random_char = do
    index <- random_int 0 $ (length alphabet) - 1
    return $ alphabet !! index
    where alphabet = ['A'..'Z'] ++
                     ['a'..'z'] ++
                     ['0'..'9'] ++
                     "!\"£$%^&*()-_+={}[]:;@'~#|\\<,>.?/'"

-- Generate a random password of given length
random_password :: Int -> IO String
random_password length =
    sequence $ map (\x -> random_char) [1..length]

main :: IO ()
main = random_password 8 >>= putStrLn

The implementation of random_password feels odd and hacky, but I struggled to find another way to create an array of results from successive invocations of random_char or any way to avoid the need for sequence. Is it hacky?

hlint

You should run hlint on your program - it's a good way to become
familiar with techniques and practices used by expert Haskellers.

Running hlint on your code returned the following suggestions:

* use camelCase instead of underscores

./passwd.hs:7:20: Warning: Redundant bracket
Found:
  (randomRIO (lo, hi) :: IO Int)
Why not:
  randomRIO (lo, hi) :: IO Int

./passwd.hs:19:5: Error: Use mapM
Found:
  sequence $ map (\ x -> random_char) [1 .. length]
Why not:
  mapM (\ x -> random_char) [1 .. length]

./passwd.hs:19:21: Warning: Use const
Found:
  \ x -> random_char
Why not:
  const random_char

sequence . map

You're right that the code for random_passwd feels hacky since
the value x is ignored. The canonical way to write this is:

import Control.Monad (replicateM)

random_passwd = replicateM length random_char

replicateM repeats a monadic action a specified number of times collecting the
results in a list.

!!

You probably already know that using !! to index into a list
is inefficient. A better data structure to use for alphabet is
Text from Data.Text which gives you O(1) indexing.

import qualified Data.Text as T

alphabet = T.pack $ ['A'..'Z'] ++ ['a'..'z'] ++ ...

random_char = do
  index <- randomR (0, T.length alphabet - 1)
  return $ T.index alpha index


Binary search-like algorithm to find local peak Please help me improve my coding style.

/*
Author: Muhammad Awais
Blog: http://uetianblogger.blogspot.com/

Description:
Divide and Conquer Rule. Binary Search like algorithm
to find local peak.It may or may not find global peak
of the array.

Parameters:
A[]= Array of elements
i=starting point
j=ending point

Algorithms Class Analysis:
Theta(log(n))
BigO(log(n))
Omega(1)

*/

#include <stdio.h>
#include <stdlib.h>
int Peak1D(int A[],int i,int j)
{
    //Middle point
    int m=((i+j)/2);

    //If it comes to either corner of the array
    if (m==i || m==j)
    {
        return A[m];
    }
    //To find peak
    else
    {
        if (A[m+1]>A[m])
        {
            Peak1D(A,m+1,j);
        }
        else if (A[m-1]>A[m])
        {
            Peak1D(A,i,m-1);
        }
        else   return A[m];

    }

}

***UPDATE**

Following is my updated code bases on suggestions. Please review this too.*

/*
Author: Muhammad Awais
Blog: http://uetianblogger.blogspot.com/
Date: 16/11/2016
Description:
Divide and Conquer Rule. Binary Search like algorithm
to find local peak.It may or may not find global peak
of the array.

Parameters:
A[]= Array of elements
starting_pt=starting point
ending_pt=ending point

Algorithms Class Analysis:
Theta(log(n))
BigO(log(n))
Omega(1)

*/

#include <stdio.h>
#include <stdlib.h>
int Peak1D(int A[],size_t starting_pt,size_t ending_pt)
{
    //If length is zero
    if ((sizeof(A)==0))
    {
        return 0;
    }

    //Middle point
    int middle_pt=((starting_pt+ending_pt)/2);

    //If it comes to either end of the array
    if (middle_pt==starting_pt || middle_pt==ending_pt)
    {
        return A[middle_pt];
    }
    //To find local peak

    if (A[middle_pt+1]>A[middle_pt])
    {
        Peak1D(A,middle_pt+1,ending_pt);
    }

    else if (A[middle_pt-1]>A[middle_pt])
    {
        Peak1D(A,starting_pt,middle_pt-1);
    }

    return A[middle_pt];

}

Why else?  The if() branch never continues.

// Why `else`
if (m==i || m==j) {
    return A[m];
}
else {
   ....

// Alternative
if (m==i || m==j) {
    return A[m];
}
... rest of code

Does the above make a difference? Easier to see code is invalid as it is missing return paths

if (A[m+1]>A[m])
{
    Peak1D(A,m+1,j);
    // what happens here??? Looks like return missing
    // was return Peak1D(A,m+1,j); intended?
}
// Now do not need `else`
// else if (A[m-1]>A[m])
if (A[m-1]>A[m])
{
    // Peak1D(A,i,m-1);
    return Peak1D(A,i,m-1);
}
// Now do not need `else`
// else   return A[m];
return A[m];

int i,int j --> Why type int here?  Why not char or long long?  The best is to use size_t.  That is an unsigned type that is neither too narrow nor too wide for array indexing and size calculation.

// int Peak1D(int A[],int i,int j)
int Peak1D(int A[],size_t i,size_t j)

Consider more descriptive names. @Ron Beyer  How about "left" and "right"? or "starting_point" "ending_point"?

int Peak1D(int A[],size_t left,size_t right)

Code defensively.  Users of your useful code will try things not intended.  What happens when i>j?  Likely bad things.  Suggest adding some tests as assert() or other error handling code.

#include <assert.h>

...
assert(i <= j); // If assertion fails, code in debug mode exits with an error message

Use const for referenced data that is not modified.  This allows additional optimizations and allows execution with constant data.

int Peak1D(const int A[],int i,int j)

// Sample usage
const int sample[] = { 1,2,3,4,5};
printf("%d\n",  Peak1D(sample, 0,4));

Good top of file documentation.  Suggest adding month/year or at least year.
Above all, watch functionality.  This code seems odd as I would expect (A == {6,8}, i==0, j==1) to return 8, not 6.  OTOH, comment did say local peak - Oh, well.

// correct?
if (m==i || m==j) {
   return A[m];
}

Architecture style.:  A loop could have replaced the recursive function calls.  See little value to recursion here.
Indentation style.  It is consistent, which is most important.  I find it a bit too loose, but identation is a holy war.  IMO, best to follow an automated group's coding standards.


Reversing a string in-place - recursion or iteration? I am practicing my recursion skills as I think I am really weak in this area.

I have written the following Java class whose purpose is to reverse a string but to do it 'in-place' - i.e. no additional arrays. I have two implementations - one iterative and one recursive. Why would I choose one implementation over the other?

Also, does my recursive solution use tail call optimization?

public class TestRecursion {

    public static void reverseIter(char[] in) {
        int start = 0;
        int end = in.length - 1;

        while(start < end) {
            swap(start, end, in);
            start ++;
            end--;
        }
        System.out.println("iteration result: "+in);
    }

    public static char[] reverseRecur(char[] in, int start, int end) {
        if (end < start) return in;
        else {
            swap(start, end, in);
            return reverseRecur(in, ++start, --end);
        }
    }

    private static void swap(int start, int end, char[] input) {
        char c1 = input[start];
        char c2 = input[end];

        input[start] = c2;
        input[end] = c1;
    }

    public static void main(String[] args) {
        char[] input = "testy".toCharArray();
        reverseIter(input);
        char[] input2 = "testy".toCharArray();
        char[] out = reverseRecur(input2, 0, input2.length-1);
        System.out.println("recursive result: "+out);
    }
}

First some clean-up is needed on your code. The iterative method first:

do not call System.out.println("iteration result: "+in); as this will do a toString() on in and you will get something like iteration result [C@b312fca6. Use the following instead:  System.out.println("iteration result: " + Arrays.toString(in));
start and end are not great names because, after the first loop, they no longer represent the start and end positions. Consider left and right as alternatives.
In this case, a double-value for loop is a good candidate for readability:

for (int left = 0, right=in.length - 1; left < right; left++, right--) {
    swap(left, right, in);
}

alternatively, a single value iteration could be useful:

for (int i = (in.length - 1) / 2; i >= 0; i--) {
    swap(i, in.length - 1 - i, in);
}

in the recursive call, you have some other problems:

The character array is passed in, and modified in place. There is no reason to return it as well. The method should return void.
you should simplify the condtitional statement to make the tail-recursion obvious... since the true part of the if condition returns from the method, there is no need for an else block
you should not be modifying the actual start and end values 'in place' as this requires some extra work. In the recursive call you can simply do start + 1 instead of ++start.
I prefer using the final keyword on recursive components to make sure you only modify what you should be....

Consider the recursive method:

    public static void reverseRecur(final char[] in, final int left, final int right) {
        if (left >= right) {
            return;
        }
        swap(left, right, in);
        reverseRecur(in, left + 1, right - 1);
    }

Now the 'tail' part is obvious.

As for whether tail-call optimization is performed, I don't know. I expect that the compiler may completely re-write this recursive function as an iteration..... which leads on to the next question: Which one is better?

In this case, the iterative solution is best. It has the least impact on resources (stack space), and will be fastest. It is also the most intuitive (readable).

Your iterative solution would be preferable


Add two numbers stored as a list in F# I am solving the following problem:

  You are given two lists representing two non-negative numbers. The digits are stored in reverse order and each of their nodes contain a single digit. Add the two numbers and return it as a list.

I wrote the following code:

let addTwoNumbers (a: int list) (b: int list) =
    let rec loop (a: int list) (b: int list) (c: int list) (carry: int) =
        match a, b, carry with
        | (h1::t1), (h2::t2), _ ->
            let q, r = Math.DivRem(h1 + h2 + carry, 10)
            loop t1 t2 (List.append c [r]) q
        | (h1::t1), [], _ ->
            let q, r = Math.DivRem(h1 + carry, 10)
            loop t1 [] (List.append c [r]) q
        | [], (h2::t2), _ ->
            let q, r = Math.DivRem(h2 + carry, 10)
            loop [] t2 (List.append c [r]) q
        | [], [], 0 ->
            c
        | [], [], carry ->
            let q, r = Math.DivRem(carry, 10)
            loop [] [] (List.append c [r]) q
    loop a b [] 0

Is there a way to simplify and possibly to de-duplicate some of this code?

List.append is an expensive O(d) operation, making your addTwoNumbers O(d2).  To reverse the list, you would be better off calling List.rev once at the end.  (By the way, List.append c [r] would be better written as c @ [r].)

The loop helper function could use a better name.  My suggestions are either add or addTwoNumbers'.  I don't like the name c either, as it is somewhat mnemonically similar to carry.

Put the base cases first, followed by the recursive cases.

One of the cases can be simplified by taking advantage of commutativity.

When adding two numbers, carry should never be greater than 1.

let addTwoNumbers (a: int list) (b: int list) =
    let rec add (a: int list) (b: int list) (sum: int list) (carry: int) =
        match a, b, carry with
        | [], [], 0 ->
            sum
        | [], [], 1 ->
            1 :: sum
        | [], _, _ ->
            add b a sum carry
        | (h1::t1), [], _ ->
            let c, s = Math.DivRem(h1 + carry, 10)
            add t1 [] (s :: sum) c
        | (h1::t1), (h2::t2), _ ->
            let c, s = Math.DivRem(h1 + h2 + carry, 10)
            add t1 t2 (s :: sum) c
    add a b [] 0 |> List.rev


Jquery shopping cart code review I'd really appreciate if any of you could do a code review of my Sopping cart demo. I was wondering whether this could be improved? I'm honestly trying to improve myself too!

There's a small problem on the product list! looks like doest work if has more than 1 product. Demo: http://plnkr.co/edit/jLk0VdSgGYPwLhNKkcs7

var module =  {

    init: function () {
        this.shoppingCart();
    },

  shoppingCart: function () {
      numberIncrementer(this);
      updateQuantity(this);
      recalculateCart();

      /* Set rates + misc */
      var taxRate = 0.20;
      var fadeTime = 300;

      /* Assign actions */
      $('.product-quantity input').change( function() {
        updateQuantity(this);
      });

      $('.product-removal button').click( function() {
        removeItem(this);
      });

      /* Recalculate cart */
      function recalculateCart() {
        var subtotal = 0;

        /* Sum up row totals */
        $('.product').each(function () {
          subtotal += parseFloat($(this).children('.product-line-price').text());
        });

        /* Calculate totals */
        var tax = subtotal * taxRate;
        var total = subtotal + tax;

        /* Update totals display */
        $('.totals-value').fadeOut(fadeTime, function() {
        $('#cart-subtotal').html(subtotal.toFixed(2));
        $('#cart-tax').html(tax.toFixed(2));
        $('#cart-total').html(total.toFixed(2));
          if(total == 0){
            $('.checkout').fadeOut(fadeTime);
          }else{
            $('.checkout').fadeIn(fadeTime);
          }
          $('.totals-value').fadeIn(fadeTime);
        });
      }

      /* Update quantity */
      function updateQuantity(quantityInput) {
        /* Calculate line price */
        var quantityInput = $('.product-quantity input');
        var productRow = $(quantityInput).parent().parent();
        var price = parseFloat(productRow.children('.product-price').text());
        var quantity = $(quantityInput).val();
        var linePrice = price * quantity;

        /* Update line price display and recalc cart totals */
        productRow.children('.product-line-price').each(function () {
          $(this).fadeOut(fadeTime, function() {
            $(this).text(linePrice.toFixed(2));
            recalculateCart();
            $(this).fadeIn(fadeTime);
          });
        });
      }

      /* Remove item from cart */
      function removeItem(removeButton) {
        /* Remove row from DOM and recalc cart total */
        var productRow = $(removeButton).parent().parent();
        productRow.slideUp(fadeTime, function() {
        productRow.remove();
        recalculateCart();
        });
      }

    function numberIncrementer(){
          $("div.product-quantity").append('
+

-
');

          $(".button").on("click", function() {
            var $button = $(this);
            var oldValue = $button.parent().find("input").val();

            if ($button.text() == "+") {
              var newVal = parseFloat(oldValue) + 1;
            } else {
               // Don't allow decrementing below zero
              if (oldValue > 0) {
                var newVal = parseFloat(oldValue) - 1;
                } else {
                newVal = 0;
              }
              }
            $button.parent().find("input").val(newVal);
            updateQuantity();
          });

        }
    }
}

$(document).ready(function () {
    module.init();
});

This code style is not very good, becouse u declare functions inside a function. The problem with this is that you recreate all this functions every time u run "shoppingCart" function.

Instead try using this pattern;

(function(){

    //my private functions delare

    function recalculateCart(){
        ...
    }

    // my module public methods
    var myModule = {
        init: ...,
        shoppingCart: ...
    }

    return myModule;

})()


Performing calculations with updated integer How could I improve this code to make it shorter and more functional?

#!usr/bin/python
import time
integer = 0
print("The current integer is set at " + str(integer) + ".")
print("\n")
time.sleep(2)
prompt = raw_input("Would you like to change the integer? (Y/N) ")
print("\n")
if prompt == 'y':
    integer = int(raw_input("Insert the new integer here: "))
    print("\n")
    print("You have changed the integer to " + str(integer) + ".")
    print("\n")
    print("\n")
    time.sleep(1)
    print("1. Add / 2. Subtract / 3. Multiply / 4. Divide")
    print("\n")
    new_int = raw_input("What would you like to do with your new integer? (Choose a number) ")
    print("\n")
    if new_int == '1':
        added_int = int(raw_input("What number would you like to add to your integer (" + str(integer) + ") by?"))
        outcome1 = integer + added_int
        print("\n")
        print("The sum of " + str(integer) + " + " + str(added_int) + " is " + str(outcome1))
    if new_int == '2':
        subtracted_int = int(raw_input("What number would you like to subtract your integer (" + str(integer) + ") by?"))
        outcome2 = integer - subtracted_int
        print("\n")
        print("The difference of " + str(integer) + " - " + str(subtracted_int) + " is " + str(outcome2))
    if new_int == '3':
        multiplied_int = int(raw_input("What number would you like to multiply your integer (" + str(integer) + ") by?"))
        outcome3 = integer * multiplied_int
        print("\n")
        print("The product of " + str(integer) + " x " + str(multiplied_int) + " is " + str(outcome3))
    if new_int == '4':
        divided_int = int(raw_input("What number would you like to divide your integer (" + str(integer) + ") by?"))
        outcome4 = integer / divided_int
        print("\n")
        print("The quotient of " + str(integer) + " / " + str(divided_int) + " is " + str(outcome4))
elif prompt == "n":
    print("The integer will stay the same.")
    time.sleep(2)
    print("Press any key to exit...")
else:
    print("Invalid input.")
raw_input()

How about something like this?

import time

def add_num(x):
    added_int = int(raw_input("What number would you like to add to your integer (%s) by?" % x))
    outcome = x + added_int
    print("\nThe sum of %s + %s is %s" % (x, added_int, outcome))
    return outcome

def sub_num(x):
    sub_int = int(raw_input("What number would you like to subtract to your integer (%s) by?" % x))
    outcome = x - sub_int
    print("\nThe subtract of %s - %s is %s" % (x, added_int, outcome))
    return outcome

def mul_num(x):
    mul_int = int(raw_input("What number would you like to multiply your integer (%s) by?" % x))
    outcome = x * mul_int
    print("\nThe multiplication of %s * %s is %s" % (x, added_int, outcome))
    return outcome

def div_num(x):
    div_num = int(raw_input("What number would you like to divide your integer (%s) by?" % x))
    outcome = x / float(div_num)
    print("\nThe divider of %s / %s is %s" % (x, added_int, outcome))
    return outcome

def main():
    op_map = {"1":add_num, "2":sub_num, "3":mul_num, "4":div_num}
    number = 0
    print("The current integer is set at %s ." % number)
    while True:
        prompt = raw_input("Would you like to change the number? (Y/N)").lower()
        if prompt == "y":
            number = int(raw_input("Insert the new integer here: "))
            print("\nYou have changed the integer to %s .\n\n" % number)
            time.sleep(1)
            print("1. Add / 2. Subtract / 3. Multiply / 4. Divide\n")

            op = raw_input("What would you like to do with your new integer? (Choose a number) \n")
            if op is not None:
                operation = op_map.get(op)
                number = operation(number)

        elif prompt == "n":
            print("The integer will stay the same.")
            time.sleep(2)
            raw_input("Press enter key to exit...")
            break
        else:
            print("Invalid response")

if __name__ == '__main__':
    main()


Find whether a given number is a perfect square or not I am trying to find whether a given number is a perfect square or not by using just addition and subtraction.

Please review my code.

#include<stdio.h>
int check(int);
main()
{
    int N;

    printf("\n Enter the N:");
    scanf("%d",&N);
    if(check(N))
    {
        printf("\n[%d] Perfect Square:\n",N);

    }

    else
    {

        printf("\nNot perfect Square\n");

    }
}
int check(int n)
{
    int i=1;

    while(n>0)
    {
        n-=i;
        printf("[%d]",n);
        i+=2;
    }
    if(n==0)
        return 1;

    return 0;

}

Presentation

I have re-indented your code, I do not know how your code is on your side but you should definitly indent it properly.
Also, you should remove useless lines.

Naming

check is not a good function name. isPerfectSquare would probably be easier for everyone to understand.

Documentation

Is it a good habit to add some documentation telling what your code is supposed to do. In your case, it could be nice to also tell the reader how it works.

  The expression for the nth square number is n2. This is also equal to the sum of the first n odd numbers.

Types

You can use the bool type in C++.


Dynamic PHP PDO insert method This is my attempt at making a class for dynamic PDO queries in PHP.

The code works, but I would like the hear what I can improve to make it more secure and readable.

class DB

{

    private static $connection;

    public static function setConnection ($connection)

    {

        static::$connection = $connection;

    }

    public static function insert($table, $data = [])

    {

        try {

            $columnsArray = array_keys($data);
            $columnsString = implode(',', $columnsArray);

            $valuesArray = array_values($data);
            $valuesCount = count($valuesArray);

            $valuesPlaceholder = '';
            for ($i=0; $i < $valuesCount; $i++) {
                $valuesPlaceholder .= '?,';
            }
            $valuesPlaceholder = rtrim($valuesPlaceholder, ',');

            $query = "INSERT INTO $table ($columnsString) VALUES ($valuesPlaceholder)";

            $statement = static::$connection->prepare($query);

            $statement->execute($valuesArray);

        } catch (\PDOException $e) {

            die("Insert failed: " . $e->getMessage());

        }

    }

}

Usage example

//setting the connection in app entry point
$config = require_once "config.php";
$dbConnection = Connection::make($config['database']);

DB::setConnection($dbConnection);

//using DB class in a controller
class PostController

{
    //inserting a post to posts table with name and content columns
    public function create()

    {

        $name = $_POST['postName'];
        $content = $_POST['postContent'];

        DB::insert('posts', [

            'name' => $name,
            'content' => $content

        ]);

        return redirect('somewhere');

    }

}

You are using the prepared statement with placeholders properly, so regarding security, I would only advise you to not display the actual error details to the end user -- you don't want naughty people getting any funny ideas.

As for readability, I try not to declare single-use variables, though it can be good to declare a single-use variable to clarify the data being processed.

I don't prefer the over-delimit, then rtrim() technique.  Building a temporary array of ? then imploding it with commas seems cleaner to me.

$columns = implode(',', array_keys($data));

$placeholders = implode(',', array_fill(0, count($data), '?'));

$values = array_values($data);


A simple bubble sort implementation in Java I've written a Bubble Sort algorithm in Java and I want a review on these:

Performance improvements.
Code conventions.
Algorithm design improvements.
Cleaner approaches.

I would highly appreciate if you can review based on the points above, and I would prefer if you can add more points on top if it if you find necessary.

Here's the code:

package com.hassanalthaf.sortingalgorithms;

public class BubbleSort {

    private int temporary;

    public int[] sort(int[] numbers) {
        for (int mainIterations = 0; mainIterations < numbers.length; mainIterations++) {
            int comparisonsCount = (numbers.length - mainIterations) - 1;

            for (int comparisons = 1; comparisons < comparisonsCount; comparisons++) {
                if (numbers[comparisons] > numbers[comparisons + 1]) {
                    this.temporary = numbers[comparisons];
                    numbers[comparisons] = numbers[comparisons + 1];
                    numbers[comparisons + 1] = this.temporary;
                }
            }
        }

        return numbers;
    }

}

Here's my main class calling the BubbleSort sort method:

package com.hassanalthaf.sortingalgorithms;

public class Main {

    public static void main(String[] args) {
        int[] numbers = {1, 20, 51, 12, 43, 2, 20};

        BubbleSort bubbleSort = new BubbleSort();

        numbers = bubbleSort.sort(numbers);

        for (int iterations = 0; iterations < numbers.length; iterations++) {
            System.out.println(numbers[iterations]);
        }
    }

}

This produces an output:

1
2
12
20
20
43
51

Bug

On the array { 3, 1, 4, 5, 2 }, the result will be { 3, 1, 2, 4, 5 }. In order to fix this, in the inner for loop change

int comparisons = 1;

to

int comparisons = 0;

API

The Java coding conventions dictate that sorting routines are declared static, return nothing (have type void), and provide the version of the routine that can sort a particular subarray within an array.

Also, you might prefer to declare the constructor of BubbleSort as private, since that object does not do anything of interesting on its own.

Algorithm

Your algorithm runs always in \$\Theta(N^2)\$, whereas the actual bubble sort runs in \$\Theta(N)\$ on almost sorted input. Refer to the Wikipedia for pseudocode that is more adaptive.

Printing an integer array

Instead of

for (int iterations = 0; iterations < numbers.length; iterations++) {
    System.out.println(numbers[iterations]);
}

you can easily write

System.out.println(Arrays.toString(yourArray));

Hope that helps.


VBA function that returns color RBG code of a referenced cell I have this function I'm building to return the RBG code of a cell I input.  The code "works" but something funny is going on.  If I write multiple functions each referencing different cells, then all the functions simultaneously show the last cell I run through the function.

I'm probably doing something trivial here, but I'm also new to VBA.

 Public Function ColorCode(Reference As Range) As Variant

' Function to return RGB code of a single cell when referenced

Dim RGBColor As Long

Dim Red As Long, Green As Long, Blue As Long

Range(Reference.Address).Activate
RGBColor = Selection.Interior.Color

    Red = &HFF& And RGBColor

    Green = (&HFF00& And RGBColor) \ 256

    Blue = (&HFF0000 And RGBColor) \ 65536

ColorCode = Red & "/" & Green & "/" & Blue

End Function

I don't think you need to activate the cell, or use the selection, just inspect the range directly.

I've also made the function volatile so it will update when you calculate.

But, for Code Review purposes, you're only getting the interior color, but the cell might not actually appear in that color, for example:

If there is conditional formatting on the cell, the interior color will not be accurate.
If there is a style on the cell, and the style applies a fill, then the interior color will not be accurate.
If the cell's interior has a 2-color fill, then then the interior color doesn't necessarily reflect either color of the 2-color fill.

Also, you're using some nice concise bit-shifting, and you'll have noticed that VBA doesn't have any shifting operators, but you might consider using some bit-shifting functions to make the code slightly clearer (and you can re-use them everywhere else that you need to shift bits.

And finally, unless you're certain that reference will always be a single celled reference, the color will default to 0/0/0 if there are more than 2 cells in the range and they have different colors.

 Public Function ColorCode(Reference As Range) As Variant
   Application.Volatile
' Function to return RGB code of a single cell when referenced

Dim RGBColor As Long

Dim Red As Long, Green As Long, Blue As Long

RGBColor = Reference.Interior.Color

    Red = &HFF& And RGBColor

    Green = (&HFF00& And RGBColor) \ 256

    Blue = (&HFF0000 And RGBColor) \ 65536

ColorCode = Red & "/" & Green & "/" & Blue

End Function


Simple ring/circular buffer C++ class I've got this simple ring/circular buffer class:

template<class T, size_t sz>
class CircularBuffer {

    std::array<T, sz> buffer;
    size_t head;
    size_t tail;
    bool isFull;

public:
    CircularBuffer() :
        head{0},
        tail{0},
        isFull{false} {
    }

    void put(T item) {
        buffer[head] = item;
        head = (head + 1) % sz;
        if (isFull) {
            tail = (tail + 1) % sz;
        }
        isFull = head == tail;
    }

    T get() {
        auto result = buffer[tail];
        tail = (tail + 1) % sz;
        isFull = false;
        return result;
    }

    bool empty() const {
        return tail == head;
    }

    size_t capacity() const {
        return sz;
    }

    size_t size() const {
        if (isFull)
            return sz;

        if (head >= tail)
            return head - tail;

        return sz + head - tail;
    }
};

And I was looking for clarification on a few things, to take advantage of C++ features.

First, the new constexpr keyword, what here, if anything should I apply it to? (I'm assuming the size_t size() const member function could use it? Anything else?)

Second, all of these member functions are quite small, should they be inlined?

Third, in the T get() member function, I do auto result = buffer[tail];, should I use auto& instead, or any other versions? (or even just T/T&?) Should that be a const as it's not modified within the function, and only potentially modified once a copy is returned via the functions return parameter.

Any other feedback is welcome!

Interface
Naming
Functions returning a bool should be phrased as a question. empty should be is_empty instead. Yes, the standard library does it wrong too, leading to confusion like "I used vector.empty();, but it didn't empty my vector. Why?"
get should be pop or pop_get. Getters are not supposed to change the object.
Note that it is impossible to write get with the strong exception guarantee, which is the reason why std::vector::pop_back returns void instead of the element.
constexpr
Currently you can mark all your functions constexpr. Sometimes it is possible to evaluate the result of your CircularBuffer at compile time. That probably rarely comes up, but there is no good reason not to do it (yet).
Generality
Type restrictions
There are limits for what Ts I can use your CircularBuffer with. T must be copyable and default constructible. That means I cannot use a struct Foo{ Foo(int); }; or a std::unique_ptr<int>. Arguably those should be allowed.
Move-Only
Supporting move-only types is possible by using std::move in the appropriate spots, mainly buffer[head] = std::move(item); and auto result = std::move(buffer[tail]);. Just try to use a CircularBuffer<std::unique_ptr<int>> and the compiler will tell you about each spot.
Non-Default-Constructible
To be able to use CircularBuffer<Foo> you would need to delay constructing objects until the user uses put. You can achieve that by changing std::array<T, sz> buffer; to alignas(alignof(T)) std::array<char, sz * sizeof(T)> buffer;. That way no Ts are default constructed. When you add an element in put you have to placement new the element: new (&buffer[head * sizeof(T)]) T(std::move(item));. get then has to call std::destroy_at(reinterpret_cast<T*>(&buffer[tail * sizeof(T)])); (or just call the destructor). This makes things more complicated and also reinterpret_cast and new are not constexpr.
Brick Types
Some types like std::mutex cannot be copied or moved, but you could still support them. To do that, offer an emplace function similar to std::vector::emplace_back that constructs the  T in place from a given list of arguments.
get Return Type
Returning a T by value seems reasonable. You are taking out the element. Returning a T & instead seems dangerous, because usage of the buffer will eventually change the value you got. Maybe add 2 peek functions instead that return a reference to the current object without removing it. One of the functions would be T &peek() and the other const T &peek() const.
Bugs
empty When Full
CircularBuffer<int, 3> b;
b.put(1);
b.put(2);
b.put(3);
std::cout << std::boolalpha << b.empty();

That should really not print true.
Over- and Underflow
If I put more items into the buffer than it has space it silently overwrites objects. If I try to get items without putting items in, it simply returns uninitialized objects which is undefined behavior for builtins. This is my fault for using your container incorrectly, but you could be nice and add an assert so that I can find my bug easier.
inline
Your functions are already implicitly marked inline which changes the linkage and has nothing to do with inlining. Whether inlining is the right choice is a complicated case-by-case question that you should leave to your compiler. Only use inline to mean "I want internal linkage", which you can also do for variables since C++17.


Cross validation of gradient boosting machines I am fairly new to Python. I implemented a short cross-validation tool for gradient boosting methods.

import numpy as np

from sklearn.metrics import roc_auc_score as auc
from sklearn import cross_validation
from time import time

def heldout_auc(model, X_test, y_test):
    score = np.zeros((model.get_params()["n_estimators"],), dtype=np.float64)
    for i, y_pred in enumerate(model.staged_decision_function(X_test)):
        score[i] = auc(y_test, y_pred)
    return score

def cv_boost_estimate(X,y,model,n_folds=3):
    cv = cross_validation.StratifiedKFold(y, n_folds=n_folds, shuffle=True, random_state=11)
    val_scores = np.zeros((model.get_params()["n_estimators"],), dtype=np.float64)
    t = time()
    i = 0
    for train, test in cv:
        i = i + 1
        print('FOLD : ' + str(i) + '-' + str(n_folds))
        model.fit(X.iloc[train,], y.iloc[train])
        val_scores += heldout_auc(model, X.iloc[test,], y.iloc[test])
    val_scores /= n_folds
    return val_scores,(time()-t)

Then I can look for the optimal number of trees with the following:

print('AUC : ' + str(max(auc)) + ' - index : ' + str(auc.tolist().index(max(auc))))

Everything is working, but the syntax feels uneasy and "not Pythonic". Does someone have improvements to suggest?

You didn't use enumerate for your cv loop, I assume you tried this and found it didn't work:

for i, train, test in enumerate(cv):

To understand this, you need to know what enumerate is actually doing. It's wrapping each element of cv in a 2 item tuple, with index as the first item and the element from the iterable in the other. Basically it's using this structure: (i, (train, test)).

Fortunately this means you can get the result you need with just one modification:

for i, (train, test) in enumerate(cv):

Now it can extract all 3 values correctly without error. Even though you want i to start at 1, I find this to be more clear and readable than a manually incrementing value. You can just make the allowance to push the i value up by 1.

Also you're manually concatenating strings when you should use str.format. It allows you to insert values into strings, this is how you'd use it:

    print('FOLD : {}-{}'.format(i, n_folds))

format will replace any {} with the parameters passed to it. It will also automatically try to call str on any objects that can be turned into strings so you no longer need those manual calls.

Lastly, you don't need to wrap your time expression in brackets, it returns fine without it and it's more Pythonic to leave out the brackets.

return val_scores, time() - t

You should read the Python style guide, it has a lot of info about Pythonic style coding. You have it mostly correct, but some lines are a bit long and sometimes you should put more whitespace in (eg. time()-t -> time() - t).


JavaScript get total function I have two JS functions that return a total number for me based on some properties. It works fine but feels a bit 'clunky'. Would there be a cleaner way of doing this?

getTotalTrucks: function (data) {
    var noOfTrucks = 0;

    if (data.truck1 == "1") {
        noOfTrucks += 1;
    }
    if (data.truck2 == "1") {
        noOfTrucks += 1;
    }
    if (data.truck3 == "1") {
        noOfTrucks += 1;
    }
    if (data.truck4 == "1") {
        noOfTrucks += 1;
    }
    if (data.truck5 == "1") {
        noOfTrucks += 1;
    }

    return noOfTrucks ;
},

getTotalCars: function (data) {
    var noOfCars = 0;

    if (data.car1 == "2") {
        noOfCars += 1;
    }
    if (data.car2 == "2") {
        noOfCars += 1;
    }
    if (data.car3 == "2") {
        noOfCars += 1;
    }
    if (data.car4 == "2") {
        noOfCars += 1;
    }
    if (data.car5 == "2") {
        noOfCars += 1;
    }

    return noOfCars ;
}

Magic values
Your code compares to "1" and "2". What are these values supposed to convey? Use constants to signify what they mean.
Data structure
Your data structure is not great for the task you are trying to accomplish. Why not use a list containing car types? Why not at least split the data based on cars and trucks, so you can get away with just using Object.keys(data.cars).length or even a simple reduce?
{
    "cars" : {
         "car1": "2",
         "car2": "0"
    },
    "trucks" : {
         "truck1": "0",
         "truck2": "1"
    }
}

or
{
    "cars": [
        {
            "type": "car1",
            "status": "broken"
        },
        {
            "type": "car2",
            "status": "contains 4 wheels"
        }
    ],
    "trucks": [
        //you get the point
    ]
}


Cancel Long Running Query using Cancellation Token AND SqlCommand.Cancel() I need to be able to cancel a long running query programmatically through our application.  The code below will kick off a long running query and give control back to the main thread.  At any arbitrary time, a user could choose to cancel the query.  It both cancels the Task as well as cancels the query in SQL Server. I can check the status of the query in SQL Server with select * from sys.query_store_runtime_stats to verify that the query was in fact aborted.  This is important as I need to make sure it's not just canceled in the app but in the database as well.

The code below does this, but I'm hung up on the line where I Register the cmd.Cancel method with the cancellationToken, cancellationToken.Register(cmd.Cancel); Is there a potential issue since I'm referencing a variable created inside the scope of the Task.Run from the Main method(outside the Task.Run scope)?

Also, I'm including the cmd.Cancel bit so that the SQL query actually gets cancelled and not just the task that kicked it off.  Is there a better way to ensure that the long running SQL query currently being executed in the database is aborted? (again, the code below does this, just wondering if there is a better solution for cancelling a SQL query from the c# code that initiated the SQL query)

I'm using .NET Core 3.1 preview 3 and SQL Server 2017 with the code posted below.

using Microsoft.Data.SqlClient;
using System;
using System.Threading;
using System.Threading.Tasks;

public class Program
{
    public static void Main(string[] args)
    {
        var tokenSource = new CancellationTokenSource();
        var cancellationToken = tokenSource.Token;

        Task.Run(async () =>
        {
            await using var cn = new SqlConnection(CONNECTION_STRING);
            await using var cmd = new SqlCommand(LONG_RUNNING_SQL, cn);
            cancellationToken.Register(cmd.Cancel); // is this bad??
            await cn.OpenAsync(cancellationToken);
            await using var reader = await cmd.ExecuteReaderAsync(cancellationToken);
            while (await reader.ReadAsync())
            {
            }
        }, cancellationToken);

        Console.WriteLine("Press z+<Enter> to Stop");
        while (true)
        {
            if (Console.Read() == 'z')
            {
                tokenSource.Cancel();
                break;
            }

        }
    }

    private const string CONNECTION_STRING = "server=.;database=scratch;trusted_connection=true;";
    private const string LONG_RUNNING_SQL = @"
Declare @start bigint, @end bigint
Select @start=1, @end=999999
;With NumberSequence( Number ) as
(
    Select @start as Number
        union all
    Select Number + 1
        from NumberSequence
        where Number < @end
)
Select * From NumberSequence Option (MaxRecursion 0)
";
}

It both cancels the Task as well as cancels the query in SQL Server. I can check the status of the query in SQL Server with select * from sys.query_store_runtime_stats to verify that the query was in fact aborted. This is important as I need to make sure it's not just canceled in the app but in the database as well.

Baaaack up a few steps. Your strategy to rely on proof of query completion by looking in a secondary table seems strange to me. If your long-running query were an update, insert etc., you would want to consider

Running your query in a transaction that does not have any kind of auto-commit enabled
Only committing at the end, once you're sure that there will be no desire for cancellation
If cancellation occurs before the commit, then you can be sure that the query was cancelled with no effect on the database

But that's not required, because your long-running query is only a select. Is there a specific reason that you don't trust Cancel to do what it's designed to do? Doing the query_store_runtime_stats check manually as a part of development testing is fine, as long as you aren't baking it in programmatically.


Find the shortest whole repetitive substring (part 3) This is a new discussion from this post (Find the shortest whole repetitive substring) and since it is totally new code, I post as a new thread here.

The major initiative of posting a new thread is, RobAu has posted a smarter idea, and there is no implementation, and I implemented the ideas and has simple prove the ideas is correct. Post my code and simple prove here for advice.

The problem,

I'm working on a problem to find wholly repeated shortest substring of a given string, and if no match, return the whole original string.

Input and output example

catcatcat => cat
catcatcatdog=>catcatcatdog
aaaaaa = > a

Major idea of the algorithm,

Try to match the first shortest repetitive candidate as the first character of original string, if there is no match, treat the whole non-matched string as next candidate.

I have a simple prove why if there is no match, treat the whole non-matched string as next candidate is correct,

Let us say, previous candidate length is N and is not satisfied during current comparison, previous matched length is N*k (previous candidate matched for k times)
Suppose and there could be another satisfied candidate, whose length is N+x (1<x<N) and this candidate can match p times for previous matched string. Then,
N*k = (N+x)*p, in other words N*k/(N+x) = p, since N/(N+x) is not integer, but k and p are both integer, it could not be satisfied.
(At the same time, we know if N length string does not match, 2N, 3N, etc. does not match, it is why I choose x as as value between 1 and N in previous analysis.)
So, we have to treat the whole mis-matched string as next whole repetitive string candidate.

My code

def check_shortest(original_string):
    current_candidate = original_string[0]
    j = 0
    for i in range(len(original_string)):
        if original_string[i] == current_candidate[j]:
            j+=1
            if j == len(current_candidate):
                j = 0
        else:
            current_candidate= original_string[:i+1]

    return current_candidate if j == 0 else original_string

if __name__ == "__main__":
    print check_shortest('catcatcat') #cat
    print check_shortest('catcatcatdog') #catcatcatdog
    print check_shortest('aaaaaa') #a
    print check_shortest('aba')  # aba

You currently write:

for i in range(len(original_string)):

Consider using enumerate:

for i, char in enumerate(original_string):
    if char == current_candidate[j]:

enumerate allows you to reference the current object in a nice manner, while also giving you the index that the item belongs to.


Job shop scheduling with Clojure's core.logic Here's a job shop scheduler using core.logic. I had a lot of fun coding it. I'd appreciate ideas on how to make the code faster or more readable.

Setup

(ns schejule.no-db
  (:require [clojure.core.logic :refer [== all everyg lvar run run* fresh succeed] :as l]
            [clojure.core.logic.fd :as fd])
  (:refer-clojure :exclude [==]))

(def test-data
  [{:id 1 :job :cooljob :duration 180 :machine 1}
   {:id 2 :job :cooljob :duration 120 :machine 2}
   {:id 3 :job :cooljob :duration 100 :machine 3}
   {:id 4 :job :cooljob :duration 150 :machine 4}
   {:id 5 :job :ajob :duration 180 :machine 1}
   {:id 6 :job :ajob :duration 120 :machine 2}
   {:id 7 :job :ajob :duration 100 :machine 3}
   {:id 8 :job :ajob :duration 150 :machine 4 :startby 300}
   {:id 9 :job :another :duration 180 :machine 1}
   {:id 10 :job :another :duration 120 :machine 2}
   {:id 11 :job :another :duration 100 :machine 3}
   {:id 12 :job :another :duration 150 :machine 3 :startby 400}])

Ratiocinate!

(defn init-jobs
  "Binds tasks in a job to occur in the correct order.
  `lengths` must be a seq of integers defining no. of tasks in nth job."
  [tasks lengths]
  (if (seq lengths)
    (let [l (first lengths)]
      (all
      (everyg (fn [[x y]] (fd/< x y)) (partition 2 1 (take l tasks)))
      (init-jobs (drop l tasks) (next lengths))))
    succeed))

(defn sort-tasks
  "Sorts tasks by `:job`, then by `:id` to prepare for `solvo`"
  [tasks]
  (->> tasks (sort-by :job) (sort-by :id)))

(defn solvo*
  "Find a feasible ordering for tasks.
  tasks **MUST** be sorted by `:job`, then by `:id`."
  [tasks]
  (let [n (count tasks)
        lengths (->> tasks (partition-by :job) (map count))
        vars (repeatedly n lvar)]
    (run* [q]
          (== q vars)
          (init-jobs vars lengths)
          (everyg #(fd/in % (fd/interval (- n 1))) vars)
          (fd/distinct vars))))

(defn constructo*
  "Calls solvo on some tasks, then associates ords with tasks.
  Returns a lazy (possibly huge) sequence."
  [tasks]
  (let [ts (sort-tasks tasks)
        feasible* (solvo* ts)]
    (map (fn [tasks ords]
          (map (fn [t ord]
                  (assoc t :ord ord))
                tasks
                ords))
        (repeat ts)
        feasible*)))

Filter

I'm a bit worried about the (lack of) speed of this gargantuan filter expression!

(some (fn [tasks]
        (reduce (fn [solution task]
                  (let [{:keys [machine duration job id startby]}
                        task

                        prev-machine (get-in solution [:prev-task job])

                        prev-end (max
                                  (or (:end-time (last (get-in solution [:tasks machine]))) 0)
                                  (or (:end-time (get-in solution [:tasks prev-machine])) 0))

                        task-w-end (assoc task :end-time (+ prev-end duration))]

                    (when (and startby (< startby prev-end))
                      (reduced nil))
                    (-> solution
                        (update-in [:tasks machine] #(conj % task-w-end))
                        (assoc-in [:prev-task job] machine))))
                {}
                tasks))
      (map (partial sort-by :ord) (constructo* test-data-2)))

A terser way to write sort-tasks would be:

(sort-by (juxt :job :id) tasks)


Swift protocol with lazy property requirement First question here on CodeReview. I was directed here from a question I asked on SO: Swift protocol with lazy property - Cannot use mutating getter on immutable value: '$0' is immutable.

Goal: Create a protocol that allows lazy computation of a property for structs that conform to the protocol. The computation is intensive and should only be executed once, hence the lazy requirement.

So, after lots of reading (eg Swift Struct with Lazy, private property conforming to Protocol, where I based this code on) and trial and error, I came up with something that works:

import Foundation

protocol Foo {
    var footype: Double { mutating get }

    func calculateFoo() -> Double
}

struct Bar: Foo {
    private lazy var _footype: Double = {
        let value = calculateFoo()

        return value
    }()

    var footype: Double {
        mutating get {
            return _footype
        }
    }

    func calculateFoo() -> Double {
        print("calc")
        return 3.453
    }
}

Testing this in a Playground:

var bar = Bar()
print(bar.footype)
print(bar.footype)

And the output is:

calc
3.453
3.453

So it is working. But it feels like a hack using _footype

Also in the comments, it was suggested that

  However, I would not put calculateFoo in the protocol because it sounds like an implementation detail.

Appreciate any comments and/or improvements.

You don't need the additional _footype variable and a computer “wrapper” property. The protocol requirement can be satisfied with a lazy stored property:

struct Bar: Foo {
    lazy var footype = calculateFoo()

    func calculateFoo() -> Double {
        print("calc")
        return 3.453
    }
}

Now footype is read-only for instances of type Foo

var foo: Foo = Bar()
print(foo.footype)
foo.footype = 12.3 // Error: Cannot assign to property: 'footype' is a get-only property

but as a property of Bar it is read-write:

var bar = Bar()
print(bar.footype)
bar.footype = 12.3 // OK

If assigning to the footype property should be inhibited then you can mark it with a private(set) access modifier:

struct Bar: Foo {
    private(set) lazy var footype = calculateFoo()

    func calculateFoo() -> Double {
        print("calc")
        return 3.453
    }
}

var bar = Bar()
print(bar.footype)
bar.footype = 12.3 // Cannot assign to property: 'footype' setter is inaccessible

With respect to

  However, I would not put calculateFoo in the protocol because it sounds like an implementation detail.

Yes, in your current code it is an implementation detail of the Bar class. The only use would be that a caller can “enforce” the evaluation:

var foo: Foo = Bar()
foo.calculateFoo()

The situation would be different if there were a way to provide a default implementation in an extension method:

protocol Foo {
    var footype: Double { mutating get }

    func calculateFoo() -> Double
}

extension Foo {
    var footype: Double {
        // Call calculateFoo() on first call only ...
    }
}

so that conforming to the protocol is sufficient, i.e. Bar only has to implement the calculateFoo() method, but not the footype property.

But I am not aware of a way to do this lazily so that the function is called only once. The reason is that extensions cannot add stored properties to a type.

For subclasses of NSObject you can come around this problem with “associated objects,” but not for structs.


Find all k points which are closest to origin We have a list of points on the plane.  Find the K closest points to
  the origin (0, 0).

  (Here, the distance between two points on a plane is the Euclidean
  distance.)

  You may return the answer in any order.  The answer is guaranteed to
  be unique (except for the order that it is in.)

  Example 1:

  Input: points = [[1,3],[-2,2]], K = 1

  Output: [[-2,2]]

  Explanation: The distance between (1, 3) and the origin is
  \$\sqrt{10}\$.  The distance between (-2, 2) and the origin is
  \$\sqrt{8}\$.  Since \$\sqrt{8} < \sqrt{10}\$, (-2, 2) is closer to
  the origin.  We only want the closest K = 1 points from the origin, so
  the answer is just [[-2,2]].

  Example 2:

  Input: points = [[3,3],[5,-1],[-2,4]], K = 2

  Output: [[3,3],[-2,4]]

  (The answer [[-2,4],[3,3]] would also be accepted.)

  Note:

  1 <= K <= points.length <= 10000
  -10000 < points[i][0] < 10000
  -10000 < points[i][1] < 10000

  Also note that there can be a situation where distance of 2 nodes are
  equal and hence can print any of the node.

Here is my approach.

Took tree map (So that I get all sorted distance)
Start filling tree map for all points
Finally took first k element.

public class KClosestPointsToOrigin {

    public int[][] kClosest(int[][] points, int K) {

        int rows = points.length;

        SortedMap<Double, List<CoordinatePoint>> distanceToPoint = new TreeMap<>();

        for(int i =0; i<rows; i++) {
            double distance = getDistance(points[i]);
            if(Objects.nonNull(distanceToPoint.get(distance))) {
                List<CoordinatePoint> coordinatePointList = distanceToPoint.get(distance);
                coordinatePointList.add(new CoordinatePoint(points[i][0], points[i][1]));
            } else {
                List<CoordinatePoint> coordinatePoints = new ArrayList<>();
                coordinatePoints.add(new CoordinatePoint(points[i][0], points[i][1]));
                distanceToPoint.put(distance, coordinatePoints);// x and y coordinates.
            }
        }

        int[][] arrayToReturn = new int[K][2];
        int counter = 0;
        for (Double key : distanceToPoint.keySet()) {
            List<CoordinatePoint> coordinatePoints = distanceToPoint.get(key);
            Iterator iterator1 = coordinatePoints.iterator();
            while (iterator1.hasNext() && counter < K) {
                CoordinatePoint coordinatePoint = (CoordinatePoint) iterator1.next();
                arrayToReturn[counter][0] = coordinatePoint.x;
                arrayToReturn[counter][1] = coordinatePoint.y;
                counter++;
            }
        }

        return arrayToReturn;
    }

    private double getDistance(int[] point) {
        int x = point[0];
        int y = point[1];

        int x2 = Math.abs(x) * Math.abs(x);
        int y2 = Math.abs(y) * Math.abs(y);

        return Math.sqrt(x2+y2);
    }
}

class CoordinatePoint {
    int x;
    int y;

    public CoordinatePoint(int x, int y) {
        this.x = x;
        this.y = y;
    }
}

However, this solution is not efficient as runtime and memory usage is high. Can you please help me to optimize the solution.

This task sounds as if it came directly from an advertisement for the Java 8 streams API:

Sort the points by their distance
Take the minimum k points

This boils down to the following code:

static int[][] kClosest(int[][] points, int k) {
    return Arrays.stream(points)
        .sorted(Comparator.comparing((int[] point) -> point[0] * point[0] + point[1] * point[1]))
        .limit(k)
        .toArray(int[][]::new);
}

That code is written from the top of my head. Since the Java streams API is general-purpose and intended to be highly optimized, it should notice that it only ever needs to remember the \$k\$ smallest elements. You should check this by counting how often the distance function is called. To do that you should extract it to a local method, which is something that your IDE can do for you.

Compared to your code:

I left out the Math.sqrt since the point coordinates are restricted to 10000, which means that the square of the distance can become at most 2e8, which luckily is a bit smaller than Integer.MAX_VALUE.
Calling Math.abs is unnecessary as well.
Making a map of lists may help in pathological cases when the given points all have the same distance. In other cases it can be left out.