Node.js Pun Generator Longtime lurker here. I'm making a pun generator in JavaScript, and I was looking for some feedback/guidance. I don't have much experience building applications with Node, and so a lot of my code feels a bit like rambling. This class is meant to be used like so:

let p = new Phrase("The dog likes cheese.")
p.generatePun().then((response) => {
    console.log(response)
})

Specifically, I think the web api call could be structured better, but I don't quite know how. I'd appreciate any feedback!

Phrase.js

const util = require("./util.js");
const wordsApi = require("./rhymeApi.js");

const letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";

class Phrase {
    constructor(string, badwords = ["the", "on"]) {
        this.tokens = string.split(" ");
        this.badwords = badwords;
    }

    generatePun() {

        let nonBadWordsIndices = [];
        for (let i = 0; i < this.tokens.length; i++) {
            if (!this.badwords.includes(this.tokens[i].toLowerCase())) {
                nonBadWordsIndices.push(i)
            }
        }

        let replaceIndex = util.getRandomElement(nonBadWordsIndices);
        let wordToBePunned = this.tokens[replaceIndex];

        const isCapitalized = wordToBePunned[0] === wordToBePunned[0].toUpperCase();
        const isAllCaps = wordToBePunned === wordToBePunned.toUpperCase();
        const punctuation= wordToBePunned[wordToBePunned.length - 1];
        const isPunctuated = !letters.includes(punctuation);

        if (isPunctuated) {
            wordToBePunned = wordToBePunned.slice(0, wordToBePunned.length - 1)
        }

        return wordsApi.getRhyme(wordToBePunned).then((rhymes) => {
            let rhyme = util.getRandomElement(rhymes).word;
            if (isCapitalized) {
                rhyme = rhyme.replace(/\b\w/g, l => l.toUpperCase())
            }
            if (isAllCaps) {
                rhyme = rhyme.toUpperCase();
            }
            if (isPunctuated) {

                rhyme = rhyme + punctuation
            }

            let punnedPhrase = this.tokens.slice();
            punnedPhrase.splice(replaceIndex, 1, rhyme);
            return punnedPhrase.join(" ")

        }).catch((err) => {
            return "Error in generating pun: " + err;
        });

    }

    toString() {
        return this.tokens.join(" ");
    }
}

module.exports = Phrase;

util.js

let getRandomElement = (array) => {
    return array[Math.floor(Math.random()*array.length)];
}

//returns random int in [0, stop)
let getRandomIntInRange = (stop) => {
    return Math.floor(Math.random()*stop)
}

module.exports = {
    getRandomElement,
    getRandomIntInRange
}

rhymeApi.js

const rp = require('request-promise');

const options = {
    method: 'GET',
    uri: 'https://api.datamuse.com',
    json: true
}

let getRhyme = (word) => {
    let rhymeOptions = Object.assign({}, options);
    rhymeOptions.uri += '/words?rel_rhy=' + word;
    return rp(rhymeOptions)
}

module.exports = {
    getRhyme
}

This is mostly nitpicks, there really isn't anything blatantly wrong with your code. Good work!

In util.js, getRandomElement could use getRandomIntInRange - since it isn't used anywhere else, if getRandomElement doesn't use it, it should be dropped.
Avoid unnecessary mixing of let and const. Both nonBadWordsIndices and replaceIndex are not reassigned but for some reason let is used instead of const while you use const elsewhere. (Also in util.js and rhymeApi.js)
What should happen if the empty string is passed as your phrase? I suspect current behavior isn't desired.
Swallowing errors is bad 99% of the time. Libraries that swallow errors make logging errors to be able to quickly discover the problem and fix it much more difficult.
It may not be required for this use case, but when building URLs, you should always use encodeURI or encodeURIComponent as required to avoid problems like trying to get rhymes for words containing &. This is especially important if it is to be used in a web application.

The following points are more my personal preference than objectively wrong.

If supported by your environment, prefer destructuring to Object.assign. (Also, why is the /words?rel_rhy= appended here instead of being stored in the options object? There might be a good reason, but it's something to consider)

let getRhyme = (word) => {
    return rp({
        ...options,
        uri: options.uri + '/words?rel_rhy=' + encodeURIComponent(word)
    })
}

Async functions are great! Use them instead of explicitly using promises if possible.
isPunctuated would be more clearly defined with /[a-z]$/i.test(wordToBePunned), similarly, isCapitalized could be defined as /^[A-Z]/.test(wordToBePunned) These also seem like great utility methods to me, I would likely move them to a utils file if they are used anywhere else in your codebase - and maybe even if they aren't to ensure that if you need them anywhere else you don't end up forgetting you already did the work and rewriting them.
If isAllCaps is true, then isCapitalized must also be true, I would prefer to check isCapitalized only if isAllCaps is false when modifying the returned word.
Avoid double negatives, it makes code not non-harder to read. nonBadWordsIndices is equivalent to goodWordsIndices


Fetch a department's total, Its percentage of earnings for that site and its percentage of earnings across the company I am trying to use the data from one table to produce the result set
| Dept | TotalValue | PercentageOfSite | PercentageOfCompany |

Any improvements or advice on the below would be helpful.

DECLARE @SiteNo INT = 1
DECLARE @SessionDateFrom DATETIME = '2017/02/01'
DECLARE @SessionDateTo DATETIME = '2017/03/01'

    CREATE TABLE #DepartmentSales
(
    DeptName VARCHAR(50),
    DeptNo SMALLINT,
    Value MONEY,
    SiteNo SMALLINT
)

INSERT INTO #DepartmentSales
SELECT DeptName
    ,DeptNo
    ,SUM(Value) AS SalesValue
    ,SiteNo
FROM PLUSalesDetailExtended
WHERE DATE BETWEEN @SessionDateFrom AND @SessionDateTo
AND TermNo = 0
GROUP BY DeptName, DeptNo, SiteNo

SELECT DeptName
    ,DeptNo
    ,Value AS SalesValue
    ,Value / (SELECT SUM(Value) FROM #DepartmentSales WHERE SiteNo = @SiteNo) AS SitePercent
    ,Value / (SELECT SUM(Value) FROM #DepartmentSales) AS CompanyPercent

FROM #DepartmentSales
WHERE SiteNo = @SiteNo
GROUP BY DeptName
    ,DeptNo
    ,Value

If(OBJECT_ID('tempdb..#DepartmentSales') Is Not Null)
Begin
    Drop Table #DepartmentSales
End

Any further questions please ask and I shall try to answer them.

Adding the same GROUP BY in the 2nd Select is unneccessary, the data is already unique.

I prefer writing this kind of logic (where you need multiple aggregation levels) as a single Select using Analytical Functions, i.e. Group Sums:

WITH AllSites AS
 (  -- calculate the percentages for all sites
    SELECT DeptName
        ,DeptNo
        ,SUM(Value) AS SalesValue
        ,SiteNo
    -- Analytical Functions are calculated after GROUP BY/HAVING,
    -- thus you can nest the aggregated Sum in the Group Sum
        ,SUM(Value) / SUM(SUM(Value)) OVER (PARTITION BY SiteNo) AS SitePercent
        ,SUM(Value) / SUM(SUM(Value)) OVER () AS CompanyPercent
    FROM PLUSalesDetailExtended
    WHERE DATE BETWEEN @SessionDateFrom AND @SessionDateTo
    AND TermNo = 0

    GROUP BY DeptName, DeptNo, SiteNo
 )
-- return only one site
SELECT * FROM AllSites
WHERE SiteNo = @SiteNo

If this is more or less efficient than your current approach depends mainly on he number of sites, at least it's simpler (for me) and easy to extend to multiple/all sites.


Implementing Haskell#lines Learn You a Haskell shows the lines function:

  It takes a string and returns every line of that string in a separate list.

Example:

>lines' "HELLO\nWORLD\nHOWAREYOU"
["HELLO","WORLD","HOWAREYOU"]

Here's my implementation:

lines' :: String -> [String]
lines' [] = []
lines' xs = lines'' xs []
             where lines'' [] ys = ys : []
                   lines'' (x:xs) ys
                     |  x == '\n' = ys : lines'' xs []
                     | otherwise  = lines'' xs (ys ++ [x])

Please critique it. Also, when using an accumulator value (such as ys in lines''), I don't know how to use the : function instead of ++.

What happens when a string ends in a newline? lines "\n" ⇒ [""], but lines' "\n" ⇒ ["", ""].
ys could have a more informative name. line?
When you find yourself wanting to add new items to the end of a list, consider keeping the list in reverse order, adding items to the front and reversing it when it's done. This avoids needing to use ++. ("Done" is a fuzzy concept when working lazily with infinite lists.)
...but when reinventing the wheel, you needn't reinvent the axle and bearings as well. There is a Prelude function that will parse one line from a string: break. If you use that, you need only handle recursing on the remainder. This is how lines is really implemented.

lines s         =  let (l, s') = break (== '\n') s
                   in  l : case s' of
                                []      -> []
                                (_:s'') -> lines s''


Motor degree movement function I want to control a motor which rotates a device over a gear.

Input values for the device are from 0 to 360 degrees. The motor should always rotate the device the shorter way around. For example, if the position is 30° and target position will be 340°, it should rotate over 0° counter clockwise.

The motor controller internally has so called qc unit system. One turnaround of the the device for example constitutes to 2938500, so there is a translation of values necessary. The internal position (in qc) could become negative or positive values like an integer.

I ended up with that code and some tests to be sure it should work. Did I catch all cases? Can I still simplify the move function?

import unittest

qc360 = 2938500

def qc2deg(qc):
    return (qc / float(qc360)) * 360.0

def deg2qc(deg):
    return int(round((deg/360.0) * qc360 ))

def move(is_qc, target_deg):
    is_deg = qc2deg(is_qc)
    diff = is_deg - target_deg

    if abs(diff) <= 180.0:
        if diff < 0:
            target_qc = is_qc + deg2qc(abs(diff))
        else:
            target_qc = is_qc + deg2qc(-diff)
    else:
        if diff < 0:
            target_qc = is_qc + deg2qc(abs(diff)-360)
        else:
            target_qc = is_qc + deg2qc(360-diff)

    return target_qc

class TestMovement(unittest.TestCase):

    def test_1qc2deg(self):
        self.assertEqual(qc2deg(qc360/2), 180)
        self.assertEqual(qc2deg(qc360+qc360/2), 360+180)
        self.assertEqual(qc2deg(-qc360/2), -180)
        self.assertEqual(qc2deg(-qc360-qc360/2), -360-180)

    def test_2deg2qc(self):
        self.assertEqual(deg2qc(180), qc360/2)
        self.assertEqual(deg2qc(360+180), qc360+qc360/2)
        self.assertEqual(deg2qc(-180), -qc360/2)
        self.assertEqual(deg2qc(-360-180), -qc360-qc360/2)

    def test_3simpleMovePositive(self):
        # Move within 180 degrees, without zero crossing
        self.assertEqual(move(0, 170), deg2qc(170))
        self.assertEqual(move(20, 90), deg2qc(90))

    def test_4simpleMoveNegative(self):
        self.assertEqual(move(20, 5), deg2qc(5))
        self.assertEqual(move(210, 170), deg2qc(170))

    def test_5wideMovePositive(self):
        # positive direction
        self.assertEqual(move(0, 190), deg2qc(-170))
        self.assertEqual(move(deg2qc(60), 350), deg2qc(-10))

    def test_6wideMoveNegative(self):
        # negative direction
        self.assertEqual(move(deg2qc(190), 0), deg2qc(360))
        self.assertEqual(move(deg2qc(350), 60), deg2qc(360+60))

    def test_7variousMoves(self):
        self.assertEqual(move(deg2qc(260), 50), deg2qc(360+50))
        self.assertEqual(move(deg2qc(260), 60), deg2qc(360+60))
        self.assertEqual(move(deg2qc(260), 70), deg2qc(360+70))
        self.assertEqual(move(deg2qc(260), 80), deg2qc(80))
        self.assertEqual(move(deg2qc(260), 90), deg2qc(90))
        self.assertEqual(move(deg2qc(260), 80), deg2qc(80))
        self.assertEqual(move(deg2qc(260), 120), deg2qc(120))

if __name__ == '__main__':
    unittest.main()

if diff < 0:
        target_qc = is_qc + deg2qc(abs(diff))
    else:
        target_qc = is_qc + deg2qc(-diff)

If diff < 0, then abs(diff) == -diff, just the same as the else case. The

    target_qc = is_qc + deg2qc(-diff)

(no if's and else's) should suffice. The else case (that is, abs(diff) > 180.0) also can be rewritten as

    if diff < 0:
        target_qc = is_qc + deg2qc(-diff - 360)
    else:
        target_qc = is_qc + deg2qc(-diff + 360)

where the cases only differ in signs of 360, which suggests that you are looking for the remainder -diff % 360; notice that python supports the remainder of floats.

To get rid of ugly leading -, you may want to calculate diff as

    diff = target_deg - is_deg


How to simplify a discord bot remote tool After a bit of struggling I've come up with the code

import discord
import os
import subprocess as sub
import psutil

client = discord.Client()

my_path = os.getcwd()
program_instance = False
currently_installed = os.path.isdir("web-application")

def run_command(command):
    value = sub.Popen(command, stdout = sub.PIPE, stderr = sub.PIPE)
    return value.communicate()

def kill():
    global program_instance
    if not program_instance:
        return "Program instance not initiated";
    process = psutil.Process(program_instance.pid)
    for proc in process.children(recursive = True):
        proc.kill()
    process.kill()
    program_instance = False
    return "Instance Killed"

def delete_if_exists():
    global currently_installed
    kill()
    if currently_installed:
        os.system("rmdir /S /Q web-application")
        while currently_installed:
            currently_installed = os.path.isdir("web-application")

def download_from_git():
    #pull from git
    global currently_installed
    kill()
    delete_if_exists()
    instance = sub.Popen("git clone LINKTOWEBAPPLICATION")
    instance.communicate()
    currently_installed = True
    return "Successfully downloaded"

def run(download = False):
    global program_instance
    folders = run_command("dir")
    if not currently_installed or download:
        download_from_git()
    kill()
    path = my_path+"\\web-application"
    #installing values in pipenv
    install_command_instance = sub.Popen("pipenv install", cwd = path)
    install_command_instance.communicate()

    program_instance = sub.Popen("pipenv run app.py", cwd = path)
    return "App Running"

@client.event
async def on_ready():
    print("Gitty woke")

@client.event
async def on_message(message):
    if message.author.bot or not message.channel.id == MYDISCORDUSERID:
        return;
    command = message.content

    checker = lambda command_name: command == command_name

    if checker("run"):
        await message.channel.send(run())

    if checker("kill"):
        await message.channel.send(kill())

    if checker("download"):
        await message.channel.send(download_from_git())

    if checker("run download"):
        await message.channel.send(run(download = True))

    if checker("ping"):
        currently_installed = os.path.isdir("web-application")
        await message.channel.send("Running:" + f"\n{program_instance=}, {currently_installed=}")

with open(".key", "r") as key:
    client.run(key.read())

The biggest problem with this code was killing running processes. I just wanted to know if there are any easier ways to do this. And if I made any choices which are over complicated.
Edit:
This bot is built to pull a repo from GitHub which contains a web-application that needs to be run. The reason I have to kill the application is so I can download a new version and rerun the code (as multiple instances of the website would be bad). The issue is that when the website is running I also have to be able to send the bot commands and thus have to run it as a separate instance so that the bot doesn't get "clogged" up running the site and can still take inputs

This depends on the nature and purpose of the process.
For example, you might schedule your process as a coroutine, and cancel its execution with Future.cancel: https://docs.python.org/3/library/asyncio-future.html#asyncio.Future.cancel
Your process killing method is aggressive and quite risky, some more context around why you're cancelling processes and the nature of the processes would be useful for providing answers.


Existential quantification versus explicit types I developed a card game engine for a german three player card game called Skat. The operating code lives in a StateT SkatEnv (WriterT [Trick] IO) Monad.

data SkatEnv = SkatEnv { piles :: Piles
                       , turnColour :: Maybe TurnColour
                       , skatGame :: Game
                       , players :: Players
                       , currentHand :: Hand
                       , skatSinglePlayer :: Hand }
               deriving Show

type Skat = StateT SkatEnv (WriterT [Trick] IO)

To force the three player character on the typelevel, I decided to use a special Players datatype instead of a list, that uses an existentially quantified type PL that can hold any kind of player, that implements the Player typeclass. To make match specific information available to the Player, the chooseCard etc. functions live in a MonadPlayer Monad.

class (Monad m, MonadIO m) => MonadPlayer m where
    trump :: m Trump
    turnColour :: m (Maybe TurnColour)
    singlePlayer :: m Hand
    game :: m Game

class (Monad m, MonadIO m, MonadPlayer m) => MonadPlayerOpen m where
    showPiles :: m (Piles)

class Player p where
    team :: p -> Team
    hand :: p -> Hand
    chooseCard :: (HasCard d, HasCard c, MonadPlayer m)
               => p
               -> [CardS Played]
               -> [CardS Played]
               -> Maybe [d]
               -> [c]
               -> m (Card, p)
    onCardPlayed :: MonadPlayer m
                 => p
                 -> CardS Played
                 -> m p
    chooseCardOpen :: MonadPlayerOpen m
                   => p
                   -> m Card

data PL = forall p. (Show p, Player p) => PL p

data Players = Players PL PL PL
               deriving Show

By using the MonadPlayer interface I can use my Skat monad for calling the Player's functions without exposing unallowed information, e.g. the other players' cards.

instance MonadPlayer Skat where
    trump = getTrump <$> P.game
    turnColour = gets turnColour
    singlePlayer = gets skatSinglePlayer
    game = gets skatGame

I used the PL wrapper, to be able to implement different players in seperate modules, to keep the code clean and extendible, since I have many different players, e.g. an online player that communicates via a socket or a bot, that heuristically decides which card to play.

Another concern for me is the need of the MonadIO restriction for the
MonadPlayer typeclass. This is needed, because my online player for example needs to send messages via a socket, which obviously needs to live in IO, although the bot player could perfectly deal without it. But since I have one common Player interface, I have to make IO available to every player.

Is it better style, not to use the PL wrapper but implement the Players type as

data Players a b c = Players a b c

This would lead to type variables all over the place, especially for everything SkatEnv related.

I left out many implementation details, e.g. many small type definitions. Please let me know, if you need more details about a specific type.

Can you make Player a datatype instead of a typeclass? The datatype can have exactly the same methods but with the first argument elided. Make Player an instance of Show, and voilá, data Players = Players Player Player Player. One implications is that a bot and a human player are now both just values of the same type, Player. Thus the chooseCard, chooseCardOpen and onCardPlayed of a bot can yield a human with a hand and cards on the table mid-game, and vice versa. I'd love to have the freedom to have a bot play the first few rounds for me, yield to me when the game starts going, and play out the last couple of rounds when I feel victory is secure! And as you note, you already opted to give every player type MonadIO, anyway, so these types are not really looser.


jQuery event looping I'm looking for a way to shrink this down. It'not anything special or unique but I feel the repetitive nature of the code could lend itself to a sleeker and more functional refactoring.

$.e = {
    doc: { ready:[], custom:[] },
    win: { scroll:[], scrollstart:[], scrollstop:[], load:[], resize:[], resizestart:[], resizestop:[], orientationchange:[] }
};

$(window).on({
    load:function(){for(var i = 0; i < $.e.win.load.length; i++){ $.e.win.load[i](); }},
    scroll:function(){for(var i = 0; i < $.e.win.scroll.length; i++){ $.e.win.scroll[i](); }},
    scrollstart:function(){for(var i = 0; i < $.e.win.scrollstart.length; i++){ $.e.win.scrollstart[i](); }},
    scrollstop:function(){for(var i = 0; i < $.e.win.scrollstop.length; i++){ $.e.win.scrollstop[i](); }},
    resize:function(){for(var i = 0; i < $.e.win.resize.length; i++){ $.e.win.resize[i](); }},
    resizestart:function(){for(var i = 0; i < $.e.win.resizestart.length; i++){ $.e.win.resizestart[i](); }},
    resizestop:function(){for(var i = 0; i < $.e.win.resizestop.length; i++){ $.e.win.resizestop[i](); }},
    orientationchange:function(){for(var i = 0; i < $.e.win.orientationchange.length; i++){ $.e.win.orientationchange[i](); }}
});

Use one event listener that propagates the event.
The correct collection of listeners in $.e.win is selected with event.type.

$.each($.e.win, function(eventName) {
  $(window).on(eventName, propagateEvent);
});

function propagateEvent(event) {
  var listeners = $.e.win[event.type];
  for (var i = 0; i < listeners.length; i++) {
    listeners[i].call(this, event);
  }
}


Using the ConcurrentBag as an object pool for an ASP.NET application Is there anything wrong with this implementation of an object pool that will be called from an ASP.NET application?

The object I want to pool has extremely expensive initialization, so currently I am storing one instance of it in a static variable and using C# lock() to avoid race conditions.  I want to change to an object pool so that more that one asp.net thread can make use of it simultaneously.

The object pool must have a maximum size restriction, so that callers of GetObject wait in line to get an instance of the pooled object.  For example, if I have a maximum size of 10, and suddenly 20 asp.net threads need an instance, 10 threads should get an instance immediately, and the other 10 threads should wait patiently (and in turn) for one of the 10 instances to be freed up.

The object pool class below is stolen nearly in-toto from this MSDN article, except that it incorporates Scott Chamberlain's suggestion to use a BlockingCollection:

public class ObjectPool<T>
{
    private ConcurrentBag<T> _objects;
    private Func<T> _objectGenerator;

    public ObjectPool(Func<T> objectGenerator,
        int boundedCapacity,
        int initializeCount = 0)
    {
        if (objectGenerator == null) throw new ArgumentNullException("objectGenerator");
        _objects = new BlockingCollection<T>(new ConcurrentBag<T>(), boundedCapacity);
        _objectGenerator = objectGenerator;
        if (initializeCount > 0)
        {
            int counter = 0;
            while (counter++ < initializeCount)
            {
                _objects.Add(_objectGenerator());
            }
        }
    }

    public T GetObject()
    {
        T item;
        if (_objects.TryTake(out item)) return item;
        return _objectGenerator();
    }

    public void PutObject(T item)
    {
        _objects.Add(item);
    }

}

From my ASP.NET code, I'm instantiating the pool as a static variable:

static ObjectPool<MyClass> _pool = new ObjectPool<MyClass>(() => {
            var myClass = new MyClass();
            myClass.DataSource = something;
            return myClass;
        }, 10, 5);

And then using it like this, in a try-finally block so that the PutObject method is always called.  The ConcurrentBag is called "thread safe" so I am not locking access to the GetObject or PutObject methods.

MyClass myClass = _pool.GetObject();
try
    {
        myClass.Input = "123 Main, Anytown, MI";
        var result = myClass.ValidateAddress();
    }
finally
    {
        _pool.PutObject(myClass);
    }

Instead of

    if (initializeCount > 0)
{
    int counter = 0;
    while (counter++ < initializeCount)
    {
        _objects.Add(_objectGenerator());
    }
}

I would prefer a for loop for initial initialization:

for (int i = 0; i < initializeCount; ++i)
{
    _object.Add(_objectGenerator());
}

The biggest problem however is that GetObject will return a new object anyway - even if the pool is empty. This violates your requirement:

  The object pool must have a maximum size restriction, so that callers of GetObject wait in line to get an instance of the pooled object.

As mentioned by Scott in his comment to your question you should use Take to retrieve an item as it will block until one is available.
You might want to consider providing some sort of "object resetter" action which gets called whenever you have retrieved an item from the pool which puts the item into a clean initial state automatically.


Functional organisation of JavaScript functions In one of the web apps I contribute to, the main.js file has multiple interacting "objects". Unfortunately, I'm so bad at OOP in JavaScript, despite studying other questions, that I just have a bunch of functions divided by a comment block as follows for the editor "object":

//*****************
// Setup the editor
//*****************
//Functions for interaction with editor
var aceRange = ace.require('ace/range').Range;
var editor = null;
var marker = null;
var gui_updating = false;

function removeMarker() {
    if (marker != null) {
        editor.getSession().removeMarker(marker);
        marker = null;
    }
}

function annotateLine(d) { //Called on mouseover in graph
    removeMarker();
    marker = editor.getSession()
        .addMarker(new aceRange(d.line, 0, d.line, 10),
            'highlight', 'fullLine', true);

    editor.getSession().setAnnotations([{row: d.line, type: 'info'}]);
}

function clearAnnotation(d) { //Called on mouseout in graph
    removeMarker();
    editor.getSession().clearAnnotations();
}

The initialization of the editor "object" and it's bonding to events is called later, but isn't included here as I don't think it will help.

This seems bad, but I can't figure out how to improve it other than dividing the objects into separate files. I also think it's important to divide the code functionally, but I'm not sure how to do that. Should I wrap the whole thing up in a function?

Like so:

var app_editor = function(ace, ace_editor){
    self.aceRange = ace.require('ace/range').Range;
    self.editor = ace_editor;
    self.marker = null;

    removeMarker = function() {
        if (self.marker != null) {
            self.editor.getSession().removeMarker(self.marker);
            self.marker = null;
        }
    }

    annotateLine = function(d) { //Called on mouseover in graph
        removeMarker();
        self.marker = self.editor.getSession()
            .addMarker(new self.aceRange(d.line, 0, d.line, 10),
                'highlight', 'fullLine', true);

        self.editor.getSession().setAnnotations([{row: d.line, type: 'info'}]);
    }

    clearAnnotation = function(d) { //Called on mouseout in graph
        removeMarker();
        self.editor.getSession().clearAnnotations();
    }
};

Should I be using prototypes as well, even if there will only be one function ever? I understand what a closure is, but I still don't know how to apply it and if it's appropriate here. Alternatively, since my code has gotten so big, should I look into using a library?

This project is still in its early stages, so I'm open to radical changes.

Should I be using prototypes as well, even if there will only be one function ever?

No, you will only benefit from using the prototype when you create a lot of instances of a constructor function. I won't go in to the details of that, since it has already been discussed a lot in various SO posts.

Without more context, I suggest that you create a revealing module:

var app_editor = (function(window, ace, ace_editor, undefined){

    var aceRange, editor, marker, session;

    function init(){
        aceRange = ace.require('ace/range').Range;
        editor = ace_editor;
        marker = null;
        session = editor && editor.getSession();
    }

    function removeMarker () {
        if (marker && session) {
            session.removeMarker(marker);
            marker = null;
        }
    }

    return {
        init: init,
        removeMarker: removeMarker
    }

})(window, ace, ace_editor);

app_editor.init();

Now you can keep variables and methods private within your app_editor and only expose what you need to access from the outside. This is just a small sample to get you started.

Are ace and ace_editor already defined in the global namespace? If not, you probably want to pass them as arguments to the init methods. It's hard to say from your two examples.

Some other comments:

annotateLine = function(d) { - What's d? Use a more descriptive name.
You're chaining methods a lot, e.g. editor.getSession().clearAnnotations();. What if getSession() returns undefined? Do more checks rather than making assumptions.

Let me know if you want me to elaborate anything.

Update

Simple object literal example:

var app_editor = {

    aceRange: null,
    editor: null,

    init: function(aceRange, editor){
        this.aceRange = aceRange;
        this.editor = editor;
    },

    removeMarker: function(marker){
        if(!this.editor || !this.editor.session) return;
        this.editor.session.removeMarker(marker);
    }
};

You get the idea...


Classifying top and bottom navigation links The current code I have runs fine, but I'm curious about some other approaches to making this more DRY.

var topLinks = document.querySelectorAll('.navUtility-listItem a');

for(var i=0; i<topLinks.length; i++){
    if((topLinks[i].getAttribute('href').indexOf('a') > -1)){
        topLinks[i].className += ' top-nav-a';
    } else if (topLinks[i].getAttribute('href').indexOf('b') > -1){
        topLinks[i].className += ' top-nav-b';
    }
}

var bottomLinks = document.querySelectorAll('.footerMain-listPlatformItem a');

for(var i=0; i<bottomLinks.length; i++){
    if((bottomLinks[i].getAttribute('href').indexOf('a') > -1)){
        bottomLinks[i].className += ' bottom-nav-a';
    } else if (bottomLinks[i].getAttribute('href').indexOf('b') > -1){
        bottomLinks[i].className += ' bottom-nav-b';
    }
}

You can have a function to which you will pass your links as an object (will be passed by reference) and classes. In the function, if you will pass the top or bottom links, they will be passed by reference and every change in the function will affect the original object.

function setClasses(links, classes){
    for(var i = 0; i < links.length; i++){
       if( (links[i].getAttribute('href').indexOf('a') > -1) ){
           links[i].className += classes[0];
       } else if ( links[i].getAttribute('href').indexOf('b') > -1 ){
           links[i].className += classes[1];
       }
     }
}

Used like this:

var topLinks = document.querySelectorAll('.navUtility-listItem a');
var bottomLinks = document.querySelectorAll('.footerMain-listPlatformItem a');

setClasses(topLinks, [' top-nav-a', ' top-nav-b']);
setClasses(bottomLinks , [' bottom-nav-a', ' bottom-nav-b']);


Returning postal codes between the ones that are given I have a function that returns postal codes between the ones given. It works but I feel like I made this over-complicated and there's easy way to make it more simple but I have no idea how.

Inputs are two strings that represents postal codes of my country. They are in the format 'XX-XXX' where X is [0-9]. The task is to return postal codes between them.

Example:

Input: '80-998' '81-003'

Output: '80-998' '80-999' '81-000' '81-001' '81-002' '81-003'

def codes():
    x = '79-900'
    y = '80-155'

    x1 = int(x.split('-')[0])
    x2 = int(x.split('-')[1])
    y1 = int(y.split('-')[0])
    y2 = int(y.split('-')[1])

    while x1 <= y1:
        if x1 == y1:
           while x2 <= y2:
                print ('{}-{:03d}'.format(x1,x2))
                x2 += 1
        else:
            while x2 < 1000:
                print ('{}-{:03d}'.format(x1,x2))
                x2 += 1
        x1 += 1
        x2 = 0

Reusability
As it stand your code compute zip codes between '79-900' and '80-155'. In case you want to change these bounds, you have to modify the code itself. Which means you can even call this function twice with different bounds within the same program.
You should instead make x and y parameters of the function:
def codes(x, y):
    # Rest of the code

And then you can call it like:
codes('79-900', '80-155')
codes('80-998', '81-003')

You can also benefit from using more descriptive variable names here; like begin and end or start and stop. Same applies for the function name, something like zip_codes_range is more meaningful.
Range
The name zip_codes_range is not chosen arbitrarily. It is because it expand over the name of the range function. And this is because, in the end, zip codes can be viewed as integers and the range function is meant to generate "integers between the ones that are given" (note how this is close to your title).
So the idea is to just parse the zip codes into integers and then feed them to range so it can generate intermediate values:
def int_from_zip(zip_code):
    parts = zip_code.split('-')
    value = ''.join(parts)
    return int(value)

def zip_codes_range(start, stop, step=1):  # Mimic `range` signature
    start = int_from_zip(start)
    stop = int_from_zip(stop)
    for zip_code in range(start, stop+1, step):
        begin = zip_code // 1000
        end = zip_code % 1000
        print('{:02}-{:03}'.format(begin, end))

You could also use begin, end = divmod(zip_code, 1000) instead of the 2 lines process.
Print vs return
To make your function more useful, you should separate concerns. This function should only serves to compute the needed zip codes and return them somehow; the printing of the values being done in another part of the code.
This allow any program using this function to use the value as they intend to, and not necessarily print them.
The first thing you can do is return a list of all the requested values:
def int_from_zip(zip_code):
    return int(''.join(zip_code.split('-')))

def zip_codes_range(start, stop, step=1):
    start = int_from_zip(start)
    stop = int_from_zip(stop)
    return [
        '{:02}-{:03}'.format(*divmod(zip_code, 1000))
        for zip_code in range(start, stop+1, step)
    ]

if __name__ == '__main__':
    for code in zip_codes_range('79-900', '80-155'):
        print(code)

But you could consider using generators, the same way range changed from returning a list in Python 2 to being a generator in Python 3:
def zip_codes_range(start, stop, step=1):
    start = int_from_zip(start)
    stop = int_from_zip(stop)
    for zip_code in range(start, stop+1, step):
        yield '{:02}-{:03}'.format(*divmod(zip_code, 1000))


Division without using addition, multiplication or division Could you please tell me if there is better way of performing division without +, * or /?

I've tested this code for different values of a and b.  Could you please tell me if there is a cleaner solution?

   private static int divide(int a , int b){

        if(a<b) return a;
        boolean isNeg=false,sign=true,bothNegative=false;
        int quotient=1;

        if( a<0 && b<0 ){
            bothNegative=true;
        }else if(a<0||b<0) {

            isNeg=true;
            sign=true;

        }

            a=Math.abs(a);
            b=Math.abs(b);
            int s=add(a,-b);

            while(Math.abs(s)>=Math.abs(b)){
                b=add(b,b);
                s=add(a,-b);
                quotient=add(quotient,1);
                 if(isNeg){
                    sign=!sign;
                }
            }

            if(bothNegative){
                return quotient;
            }else if(isNeg && !sign){
                quotient=add(0,-quotient);
             }

            return quotient;

    }
    private static int add(int a , int b){
            do{
                a=a^b;
                b=(a^b)&b;
                b=b<<1;
            }while(Math.abs(b)!=0);

            return a;
        }

First of all, please keep your indentation consistent; it's all over the place here.  Java code is supposed to be indented by four spaces.  Also, try not to put whitespace in random places.  Keep things consistent.

Also, here are some additional cleanliness tips:

Don't declare or initialize multiple variables on a single line:

boolean isNeg=false,sign=true,bothNegative=false;

Have each variable on separate lines, which helps with maintenance and readability:

boolean isNeg = false;
boolean sign = true;
boolean bothNegative = false;

If you ever need additional boolean variables, having more lines would be better than having a longer single line across the screen.
Your bitwise operations can be simplified a bit:

a=a^b;
b=(a^b)&b;
b=b<<1;

If you're doing a bitwise (or a mathematical) operation where the same variable is on both sides of the = sign, you can simply them as such:

a ^= b;
b &= (a^b);
b <<= 1;


Check style found code duplication in factory I have some methods in my mapper:

public EventDto mapToDto(Event googleEvent, EventDto baseEventDto) {
    EventDto eventDto = mapToDto(googleEvent, baseEventDto.getOriginalCalendarId());
    eventDto.setCalendarId(baseEventDto.getCalendarId());
    eventDto.setCollectiveId(baseEventDto.getCollectiveId());
    eventDto.setId(baseEventDto.getId());
    return eventDto;
}

and

public EventDto mapToDto(Event googleEvent, String originalCalendarId) {
    ///...
}

difference is in few rows here because of different clients. Right now, my mapper should be improved with abstraction and another child. To use improved mappers I need a factory that contains:

@Autowired
private GoogleEventDtoMapperConfirmed googleEventDtoMapperConfirmed;
@Autowired
private GoogleEventDtoMapperCanceled googleEventDtoMapperCanceled;

public EventDto mapToDto(Event googleEvent, EventDto baseEventDto) {
    switch (googleEvent.getStatus()) {
        case GoogleEventStatus.CANCELLED:
            return googleEventDtoMapperCanceled.mapToDto(googleEvent, baseEventDto);
        case GoogleEventStatus.CONFIRMED:
        case GoogleEventStatus.TENTATIVE:
        default:
            return googleEventDtoMapperConfirmed.mapToDto(googleEvent, baseEventDto);
    }
}

public EventDto mapToDto(Event googleEvent, String originalCalendarId) {
    switch (googleEvent.getStatus()) {
        case GoogleEventStatus.CANCELLED:
            return googleEventDtoMapperCanceled.mapToDto(googleEvent, originalCalendarId);
        case GoogleEventStatus.CONFIRMED:
        case GoogleEventStatus.TENTATIVE:
        default:
            return googleEventDtoMapperConfirmed.mapToDto(googleEvent, originalCalendarId);
    }
}

I believe factory is a good choice, but as you see here, there is some duplication that should be reduced. I can't move part mapper logic to the factory because of semantical reason (if I move duplication part will be fixed), and on the other hand, I have sonar duplicate issues.

Any ideas on how to reduce duplication part? Or, if you think some mapper part can be moved into the factory, please explain concepts why mapper rules should be moved into the factory part.

I've just got idea how solution should be implemented. It seems stayed clear for me when question was published (this implementation is not best, but idea is pretty good for me). My abstract factory should be replaced with strategy pattern who returns builder that I need.

public EventDto mapToDto(Event googleEvent, EventDto baseEventDto) {
    return getStrategy(googleEvent.getStatus()).mapToDto(googleEvent, baseEventDto);
}

public EventDto mapToDto(Event googleEvent, String originalCalendarId) {
    return getStrategy(googleEvent.getStatus()).mapToDto(googleEvent, originalCalendarId);
}

public GoogleEventDtoMapperAbstract getMappingStrategy(String status) {
    switch (status) {
        case GoogleEventStatus.CANCELLED:
            return googleEventDtoMapperCanceled;
        case GoogleEventStatus.CONFIRMED:
            // will be implemented by needs
        case GoogleEventStatus.TENTATIVE:
            // will be implemented by needs
        default:
            return googleEventDtoMapperConfirmed;
    }
}


"Pivoting" data with VBA I've attempted to erite some VBA for this this question.  The output is OK

but the code is not very elegant.  I'm happy-ish with creating unique list of names and certificates, but the rest seems rather ugly.  I'd love to learn how to make it more elegant and programmer-like (and less amateur-like-crap).

Sub PivotData()

    Dim rng As Range, cll As Range
    Dim arr As New Collection, a
    Dim var() As Variant
    Dim l As Long
    Dim lRow As Long, lCol As Long

    l = 1

    Set rng = Range("A2:C7")

    ' Create unique list of names
    var = Range("A2:A7")
    On Error Resume Next
    For Each a In var
        arr.Add a, a

    Next
    For l = 1 To arr.Count
        Cells(l + 1, 5) = arr(l)
    Next
    Set arr = Nothing

    ' Create unique list of certificates
    var = Range("B2:B7")
    For Each a In var
        arr.Add a, a
    Next
    For l = 1 To arr.Count
        Cells(1, 5 + l) = arr(l)
    Next
    Set arr = Nothing
    On Error GoTo 0

    ' Ugly code, how to make it more elegant?
    Range("F2").FormulaArray = _
        "=IFERROR(INDEX(R2C3:R7C3,MATCH(1,((R2C1:R7C1=RC5)*(R2C2:R7C2=R1C)),0)),"""")"

    With Range("F2")
        lRow = .CurrentRegion.Rows.Count
        lCol = .CurrentRegion.Columns.Count + 4
    End With

    Range("F2:F" & lRow).FillDown
    Range(Cells(2, 6), Cells(lRow, lCol)).FillRight

End Sub

Fully Qualify Ranges

Your code assumes that the correct worksheet will be active when the code is ran.  You should get in the habit of Fully Qualifying all range references.  This will ensure that your code will work as intended no matter what worksheet is activated.

Technically, to be fully Fully Qualified a range needs to be referenced by workbook and worksheet.

  ThisWorkbook.Worksheets("Sheet1").Range ("A2:C7")

Or

  Workbooks("Some Book").Worksheets("Sheet1").Range ("A2:C7")

But in general it is acceptable to exclude the workbook if you are not going to be working with multiple workbooks.

  Worksheets("Sheet1").Range ("A2:C7")

With statement blocks should be used so that you don't have to repeatedly requalify your ranges.

  With ThisWorkbook.Worksheets("Sheet1")

  For Each a in .Range ("A2:C7")

Dynamic Ranges and Relative References

Dynamic Ranges should be used when working with records.  In this way, you will not have to rewrite you code every time a recorded is added or deleted.

This applies to both ranges

With ThisWorkbook.Worksheets("Sheet1")
    With .Range("A2", .Range("A" & .Rows.Count).End(xlUp))
        For Each a In .Cells
            arr.Add a, a
        Next
    End With
End With

and formulas and FormulaArray

Range("F2").FormulaArray = "=IFERROR(INDEX(OFFSET($C1,1,0,COUNTA($A:$A)-1,1),MATCH(1,((OFFSET($A1,1,0,COUNTA($A:$A)-1,1)=$E2)*(OFFSET($B1,1,0,COUNTA($A:$A)-1,1)=F$1)),0)),"""")"

Having the macro hard the ranges for the FormulaArray is an acceptable middle ground.  I would do this to prevent the formulas from slowing up the workbook.

Error Handling

It is best to reduce the scope of On Error Resume Next as much as possible.  This will give you better information when something goes wrong.

On Error Resume Next
For Each a In var
    arr.Add a, a
Next
On Error GoTo 0

Or

For Each a In var
    On Error Resume Next
    arr.Add a, a
    On Error GoTo 0
Next

Variable Naming

Don't not use l as a variable name.  It is too hard to distinguish from 1.

l = 1 does nothing.  The For l = 1 initiates l to 1.

arr should only be used to name arrays variables.  Personally, I default to data, result or results.

Although there is nothing wrong with a, I prefer v, key, or item.

Alternate Methods

Here are some other ways of pivoting the data without using worksheet formulas.

Indexing Array Using ArrayList to Sort Headers

This uses ArrayList to store and sort the headers.  The position of the headers in the ArrayList is used as indices for a 2 dimensional array.

Sub PivotDataIndexedArray()
    Dim key As Variant, data() As Variant
    Dim rowHeaders As Object, columnHeaders As Object
    Set rowHeaders = CreateObject("System.Collections.ArrayList")
    Set columnHeaders = CreateObject("System.Collections.ArrayList")

    With ThisWorkbook.Worksheets("Sheet1")
        With .Range("A2", .Range("A" & .Rows.Count).End(xlUp))
            For Each key In .Value
                If Not rowHeaders.Contains(key) Then rowHeaders.Add key
            Next
            For Each key In .Offset(0, 1).Value
                If Not columnHeaders.Contains(key) Then columnHeaders.Add key
            Next

            data = .Cells.Resize(, 3).Value
        End With

        Dim results() As Variant
        Dim n As Long
        ReDim results(1 To rowHeaders.Count + 2, 1 To columnHeaders.Count + 2)

        'Add names
        rowHeaders.Sort
        For n = 0 To rowHeaders.Count - 1
            results(n + 2, 1) = rowHeaders(n)
        Next

        'Add categories
        columnHeaders.Sort
        For n = 0 To columnHeaders.Count - 1
            results(1, n + 2) = columnHeaders(n)
        Next

        Dim r As Long, c As Long

        'Add dates
        For n = 1 To UBound(data)
            r = rowHeaders.IndexOf(data(n, 1), 0)
            c = columnHeaders.IndexOf(data(n, 2), 0)
            results(r + 2, c + 2) = data(n, 3)
        Next

        .Range("F1").Resize(UBound(results), UBound(results, 2)).Value = results

    End With

End Sub

ADODB CrossTab Query

An ADODB.Connection query can be used to pivot the data.  The source data should be the only data on the worksheet, otherwise, you would have to specify the datas' range on the worksheet in the query (e.g [Sheet1$A1:C7]).

Sub PivotDataADODBQuery()
    Const SQL As String = "TRANSFORM First([Date])" & vbNewLine & _
          "SELECT [Name]" & vbNewLine & _
          "FROM [Sheet1$]" & vbNewLine & _
          "GROUP BY [Name]" & vbNewLine & _
          "PIVOT [Certificate];"

    Dim conn As Object
    Dim rs As Object
    Set conn = CreateObject("ADODB.Connection")

    conn.ConnectionString = "Provider=Microsoft.ACE.OLEDB.12.0;Data Source='" & ThisWorkbook.FullName & "';Extended Properties='Excel 12.0;HDR=YES;IMEX=1';"
    conn.Open

    Set rs = conn.Execute(SQL)

    With ThisWorkbook.Worksheets("Sheet2")
        .Cells.ClearContents

        Dim c As Long

        For c = 0 To rs.Fields.Count - 1
            .Cells(1, c + 1).Value = rs.Fields(c).Name
        Next

        .Range("A2").CopyFromRecordset rs
    End With

    rs.Close
    conn.Close

End Sub


Maximum of all subarrays of size k Given an array and an integer k, find the maximum for each and every contiguous subarray of size k.
Examples:
Input:
arr[] = {1, 2, 3, 1, 4, 5, 2, 3, 6}
k = 3

Output:
3 3 4 5 5 5 6

Input:
arr[] = {8, 5, 10, 7, 9, 4, 15, 12, 90, 13}
k = 4

Output:
10 10 10 15 15 90 90

Looking for code-review, best practices, and optimizations.
public class MaxOfSubArrays {

    private MaxOfSubArrays() { }

    public static List<Integer> maxList(int[] a, int k)   {
        if (k <= 0) {
            throw new IllegalArgumentException("The value of k should atleast be 1.");
        }

        final Deque<Integer> deque = new ArrayDeque<Integer>();
        final List<Integer> result = new ArrayList<Integer>();

        int i;
        for (i = 0; i < k; i++) {
            while (!deque.isEmpty() && a[deque.peek()] <= a[i]) {
                deque.removeFirst();
            }
            deque.addLast(i);
        }

        for (; i < a.length; i++) {
            result.add(a[deque.peek()]);

            while (!deque.isEmpty() && deque.peek() <= i - k) {
                deque.removeFirst();
            }

            while (!deque.isEmpty() && a[deque.peek()] <= a[i]) {
                deque.removeFirst();
            }

            deque.addLast(i);
        }

        result.add(a[deque.peek()]);
        return result;
    }
}

public class MaxOfSubArraysTest {

    @Test
    public void test1() {
        int[] a1 = {1, 2, 3, 4, 5, 6};
        List<Integer> list1 = new ArrayList<Integer>(Arrays.asList(3, 4, 5, 6));
        assertTrue(list1.equals(MaxOfSubArrays.maxList(a1, 3)));
    }

    @Test
    public void test2() {
        int[] a2 = {6, 5, 4, 3, 2, 1};
        List<Integer> list2 = new ArrayList<Integer>(Arrays.asList(6, 5, 4, 3));
        assertTrue(list2.equals(MaxOfSubArrays.maxList(a2, 3)));
    }

}

Here you are using a few structures which I feel are not the right tool for the job.

Firstly, you are given an array of int, but you are returning a List<Integer>. I dislike List<Integer>, but even worse than using a List for primitives, is using a mixture of them. Be consistent!

The dueue is a confusing concept. It is worse because you use a for-loop in a non-traditional way, with the loop-variable declared outside the loop, and you reuse it later. You don't have any comments explaining how you use it.

At this point, I have looked through the requirements, I have figured out how I would do it. I can see that your strategy may have some advantages, if it does what I think it does, but it is complicated, and hard to read. So, I am giving up....

Code Review not successful, not commented enough, not enough background information.


Retrieving doubles from string - simple problem The problem is not very complicated (I mean it is easy to solve it), but the question is how to make it in the easiest way (Java)

Input String:

String inputString = "[value=20][param=304.2][indicator=1500]";

Expected output: array of doubles : [20.0][304.2][1500.0]

Sample solution:

String[] chunks = inputString.split("=");
String valueStr = chunks[1].substring(0, chunks[1].indexOf(']'));
String paramStr = chunks[2].substring(0, chunks[2].indexOf(']'));
String indicatorStr = chunks[3].substring(0, chunks[3].indexOf(']'));
// parsing by Double.parseDouble forming an array and returning it as a result.

What other (smarter) way would you propose using Java?

Don't know if using regex is overkill, but it does open up for a fairly concise and (in my opinion) readable solution.

public class ArgumentParser {
    public static final Pattern KEY_VALUE_PATTERN = Pattern.compile("\\[([a-zA-Z]+)=(-?\\d+\\.?\\d*)\\]");

    public Map<String, Double> parse(String str) {
        Map<String, Double> values = new HashMap<>();

        Matcher matcher = KEY_VALUE_PATTERN.matcher(str);
        while (matcher.find()) {
            String key = matcher.group(1);
            Double value = Double.parseDouble(matcher.group(2));
            values.put(key, value);
        }

        return values;
    }
}

Edit: The question did ask for an array of doubles to be returned, but that should be trivial to obtain once the parsing is done. Alternatively the parse-method could be modified to only extract the values.


Variable integer encoding There is a varint encoding, which I call LEP64 (Little Endian Prefix Max 64-bit).

Basically, the first byte of the encoded value tells us, how many bytes follow (needed bytes encoded in unary coding, then the value encoded simply in binary):

First Byte   following   can encode values
               bytes        between
  xxxxxxx1       -       [ 0; 2^ 7-1 ]  // xxxx contains low bits of value
  xxxxxx10   [1  byte]   [ 0; 2^14-1 ]
  xxxxx100   [2 bytes]   [ 0; 2^21-1 ]
      .
      .
  x1000000   [6 bytes]   [ 0; 2^49-1 ]
  10000000   [7 bytes]   [ 0; 2^56-1 ]
  00000000   [8 bytes]   [ 0; 2^64-1 ]

There is a need for a function, which can write a LEP64 into a buffer. Here's my current version (for gcc, can be ported to any current compiler easily):

int enc(unsigned char *result, unsigned long long int value) {
    int nUsedBits = 64 - __builtin_clzll(value|1);
    int nBytesNeeded = (nUsedBits+6)/7;
    if (nBytesNeeded<9) {
        value = ((value<<1)|1)<<(nBytesNeeded-1);
    } else {
        *result++ = 0;
        nBytesNeeded = 9;
    }
    __builtin_memcpy(result, &value, 8);
    return nBytesNeeded;
}

The question is, is it possible to improve on this function? Algorithm-wise (I don't really like the division by 7, even though it is optimized to a mul), or implementation-wise? Some clever tricks to speed up this routine? Or maybe shorten its asm code, while retaining its speed?

Note: I interested only in x86_64 platform, and it's fine that this routine always writes at least 8 bytes.

Note2: The distribution of values -- as almost always -- skewed towards small numbers (small numbers are more probable than 8-byte numbers)

I actually take issue with functions written like this in general. It screams "I am smarter than a compiler", and flies in the face of most of what C++ is about at a philosophical level.

It's not portable, it's not future-proof, it's severely hamstringing the compiler, so optimizations implemented in future versions of the compiler will likely not be able to be applied.

All this to say: for me, there needs to be a crazy strong rationale to motivate the existence of something written like this in a non-toy codebase.

The biggest culprit of this in your code is the "writing more data than the size returned" part of it. The chances of this eventually stomping out-of-bounds memory is simply not worth the marginal performance difference. Considering the places in code where such encodings tend to be used, I find it highly unlikely that it has any measurable performance impact in anything that benchmarks this in context with some other code around it.

So, now that that's out of the way, here's my feedback of the function as written:

Don't use builtins where you don't have to.

I get using __builtin_clzll, but there is no valid reason to use __builtin_memcpy over std::memcpy.

Don't use magic values where unnecessary

That 64 coould easily be 8 * sizeof(value). Same goes for the 9, it could be sizeof(value) + 1.

I know you only care about X86_64, and unsigned long long int is always 8 bytes on that platform, but if being safe does not cost you anything (which is the case here), then there is no excuse not to be.

Alternatively, if you really want 64 bits exactly, just use std::uint64_t instead.

Separation of concerns

The main issue I see with your function is that it conflates the encoding and the storage. You would be better off separating them, and letting the compiler reconciliate.

In the following, encode_and_store() compiles to effectively the same thing as your enc(). On top of that, since enc() is now a pure function, and does not involve any side effect anymore, the compiler is a lot more free to mix and merge it with the calling context, so odds are it'll end up being faster in actual use.

std::tuple<unsigned long long int, int> enc(unsigned long long int value) {
    int nUsedBits = 64 - __builtin_clzll(value|1);
    int nBytesNeeded = (nUsedBits+6)/7;

    if (nBytesNeeded<9) {
        value = ((value<<1)|1)<<(nBytesNeeded-1);
    }
    return std::make_tuple(value, nBytesNeeded);
}

int enc_and_store(unsigned char *result, unsigned long long int value) {
   auto [data, bytes] = enc(value);
   if(bytes >= 9) {
       *result++ = 0;
   }

   __builtin_memcpy(result, &data, 8);
   return bytes;
}


Loading a database table using a promise chain I am creating a util module that I'm using to communicate with a MS-SQL database. I want each public method to return a promise. I started with a private function that executes a DB query and returns a promise. The function establishes a connection to the DB then it executes the query. My current code just lets all errors fall through to a single catch, and I'm not sure if this is a best practice or not.

I was hoping someone could answer that for me and any other suggestions about code read ability or anything.

var config = require('./mainConfig')();
var sql = require('mssql');
var connection =  new sql.Connection(config.db.sql);
var Promise = require("bluebird");

exports.getAll = function(table){

    return executeQueryStatment("select * from " + table);

};

function executeQueryStatment(query, newconnection) {

    var conn =  newconnection || connection;
    var request = new sql.Request(conn);

    return new Promise(function(res, rej){

     conn.connect().then(function(){

       return request.query(query);

     }).then(function (data) {

         res(data);
         conn.close();
     })
     .catch(function(err) {
        // plan on handling all errors here
        // is it better to handle each error individually upstream?
        console.log(err);
        conn.close();

    })

  });

};

Having a catch at the very end is fine. This assumes that your errors weren't recovered or handled midway.

Promise.reject('foo').then(v => console.log(v), e => console.log(e)).catch(e => console.log(e));
> foo // from `then`

In the code above, the Promise failed. However, it had a chance to recover via then's reject handler. Thus, the promise returned after that is actually resolved rather than rejected, causing catch to not execute. To log those recoveries, do a console.log at the recovery site instead, and not rely on catch.

As for the rest of your code

I suggest you generate your query using template strings. That way, you avoid ugly string concatenation. Note that template strings use backticks (``).

I also suggest you just create a connection and leave it open. There is some overhead in creating a connection, you'd want to avoid that especially when you hit the DB hard.

Your MySQL driver appears to already use Promises. No need to wrap the entire thing in a Promise. Simply return the object created by the API.

// Assuming connection is a promise from a connect()
return connection.then(() => request.query(query)).catch(err => {
    console.log(err);
    throw err;
});


Multiple random falling objects animation in Java I am new to programming but I tried to make my code as readable as possible! Hopefully you can see what it does before you run the program. I did not implement a ups or fps controller, the animation may be fast for some computers and slower for others. I also tried to use the information provided to me from my first code review. Eventually I will be making this concept into a dodge the meteors type game.

Game class:

import javax.swing.*;
import java.awt.*;

public class Game extends JPanel {

    //changing these values will change the size of the game, while still remaining functional
    //within the size limit specified.
    public static final int WINDOW_WIDTH = 600;
    public static final int WINDOW_HEIGHT = 400;

    //Creates a Square object Array
    Square[] squareArray = new Square[20];

    public Game() {

        //initializes square objects
        for (int i = 0; i < squareArray.length; i++)
            squareArray[i] = new Square();
    }

    public void paint(Graphics graphics) {

        //makes background black
        graphics.setColor(Color.black);
        graphics.fillRect(0, 0, WINDOW_WIDTH, WINDOW_HEIGHT);

        //paints square objects to the screen
        for (Square aSquareArray : squareArray) {
            aSquareArray.paint(graphics);
        }
    }

    public void update() {

        //calls the Square class update method on the square objects
        for (Square aSquareArray : squareArray) aSquareArray.update();
    }

    public static void main(String[] args) throws InterruptedException {

        Game game = new Game();
        JFrame frame = new JFrame();
        frame.add(game);
        frame.setVisible(true);
        frame.setSize(WINDOW_WIDTH, WINDOW_HEIGHT);
        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        frame.setTitle("Raining Multiple Squares");
        frame.setResizable(false);
        frame.setLocationRelativeTo(null);

        while (true) {
            game.update();
            game.repaint();
            Thread.sleep(10);
        }
    }
}

Square class:

import javax.swing.*;
import java.awt.*;
import java.util.Random;

public class Square extends JPanel {

    private int squareXLocation;
    private int squareSize;
    private int squareYLocation = -squareSize;
    private int fallSpeed = 1;
    Random rand = new Random();

    /*
    //creates a random value inside the window and stores it in squareXLocation
    */
    public int generateRandomXLocation(){
        return squareXLocation = rand.nextInt(Game.WINDOW_WIDTH - squareSize);
    }

    /*
    //creates a random value between 1-50 and stores it in squareWidth
    */
    public int generateRandomSquareSize(){
        return squareSize = rand.nextInt(50);
    }

    /*
    //creates a random value that is not zero and stores it in fallSpeed(so squares do not get stuck at 0 speed)
    */
    public int generateRandomFallSpeed(){
        return fallSpeed = rand.ints(1, 1, 10).findFirst().getAsInt();
    }

    /*
    //paints the square with the variables generated in the random methods
    */
    public void paint(Graphics g){
        g.setColor(Color.CYAN);
        g.fillRect(squareXLocation,squareYLocation,squareSize,squareSize);
    }

    /*
    //sets the squareWidth and square fallSpeed to a random value for every square created
    */
    public Square(){
        generateRandomXLocation();
        generateRandomSquareSize();
        generateRandomFallSpeed();
    }

    public void update(){

        //changes the squares xLocation and fallSpeed if the created square reaches the bottom of the screen
        if(squareYLocation >= Game.WINDOW_HEIGHT){
            generateRandomXLocation();
            generateRandomFallSpeed();
            generateRandomSquareSize();
            squareYLocation = -squareSize;
        }

        //moves the square down if the square is inside the window
        if(squareYLocation <= Game.WINDOW_HEIGHT){
            squareYLocation += fallSpeed;
        }
    }
}

Well done, you're making good progress.

Structural changes:

You should not be reusing the squares. Reusing objects is more efficient, and is great for things like particle systems (where tons of particles are created and destroyed every second). But your program is nowhere near resource intensive enough that reusing objects is necessary.

Reusing objects is unclear. It's not always easy to reset an object to it's original state and is not always clear when an object is reset.

You should create a new square whenever you need one, and remove them from your list when they reach the bottom.

Below is a review of the code as it currently is. Some of it will be obsolete when you stop reusing squares, so keep that in mind.

The game class:

import javax.swing.*;
import java.awt.*;

Be careful with importing .* you're importing a lot of stuff you don't need, this makes reading the code harder.

When reading code you're always wondering about the intent of the author. "Did they really mean to use a string there", and other questions like them. So as an author, you have to try your hardest to be clear about your intent. This involves not importing everything, but only the things you need.

As a side note, I talk a lot about the reader, and other people reading your code, which you might not be too worried about if you're working alone. In that case please remember that you will have to read your code as well. If you come back to your code in six months, it will be like you've never seen it before. So please make it easy on yourself, and show your intent as clearly as you can.

public class Game extends JPanel {

Game is an ok name, but you could be a bit more precise, what kind of game is it?

    //changing these values will change the size of the game, while still remaining functional
    //within the size limit specified.
    public static final int WINDOW_WIDTH = 600;
    public static final int WINDOW_HEIGHT = 400;

    //Creates a Square object Array
    Square[] squareArray = new Square[20];

While arrays are the most efficient, in this case I think it's better to use a list instead. Using a list will make it much easier to add and remove squares.

The 20 you use in the array can be a constant (TOTAL_AMOUNT_OF_SQUARES).

    public Game() {

        //initializes square objects
        for (int i = 0; i < squareArray.length; i++)
            squareArray[i] = new Square();
    }

I noticed that every method in this class starts with an empty line. This is not necessary and looks a bit sloppy.

    public void paint(Graphics graphics) {

        //makes background black

This comment is not really nessesary. The code it describes is self explanatory. It can create some annoying mistakes, because it is easy to forget to change the comment when you change the code. Here is some nice discussion on when to use comments. Be sure to look around the related questions as well.

        graphics.setColor(Color.black);

The background can be made into a constant as well (BACKGROUND_COLOR).

        graphics.fillRect(0, 0, WINDOW_WIDTH, WINDOW_HEIGHT);

        //paints square objects to the screen
        for (Square aSquareArray : squareArray) {
            aSquareArray.paint(graphics);
        }

The name of the square in the loop does not have to be aSquareArray, but can simply be square. so the line can read "for (Square square: squareArray)".

    }

    public void update() {

        //calls the Square class update method on the square objects
        for (Square aSquareArray : squareArray) aSquareArray.update();
    }

Always use brackets for loops. I know, I know, they take up a lot of space, but if you don't use them, you will run into some nasty problems. (to bracket or not to bracket, has by this point turned into somewhat of a holy war, but especially for beginners I recommend using them, because even though they take up space, they promote clarity).

    public static void main(String[] args) throws InterruptedException {

        Game game = new Game();
        JFrame frame = new JFrame();
        frame.add(game);
        frame.setVisible(true);
        frame.setSize(WINDOW_WIDTH, WINDOW_HEIGHT);
        frame.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
        frame.setTitle("Raining Multiple Squares");
        frame.setResizable(false);
        frame.setLocationRelativeTo(null);

This whole frame setup is pretty long, you could consider moving it to it's own method, just to keep it out of the way.

        while (true) {
            game.update();
            game.repaint();
            Thread.sleep(10);
        }
    }
}

The Square class:

import javax.swing.*;
import java.awt.*;
import java.util.Random;

public class Square extends JPanel {

Square does not have to extend JPanel, and actually it shouldn't. Because it's not a JPanel.

    private int squareXLocation;
    private int squareSize;
    private int squareYLocation = -squareSize;

The square size is still 0 at this point, so you're setting the location at 0 again.

    private int fallSpeed = 1;

Setting fallspeed is not necessary, since you're changing it later anyways.

    Random rand = new Random();

    /*
    //creates a random value inside the window and stores it in squareXLocation
    */
    public int generateRandomXLocation(){
        return squareXLocation = rand.nextInt(Game.WINDOW_WIDTH - squareSize);
    }

This method returns an int that isn't used anywhere. Either use that int to set the location, or don't return anything.

The generate methods should not be public. They should only be used by the square, and not outside of it. Make them private. Always make methods as private as possible.

    /*
    //creates a random value between 1-50 and stores it in squareWidth
    */
    public int generateRandomSquareSize(){
        return squareSize = rand.nextInt(50);
    }

    /*
    //creates a random value that is not zero and stores it in fallSpeed(so squares do not get stuck at 0 speed)
    */
    public int generateRandomFallSpeed(){
        return fallSpeed = rand.ints(1, 1, 10).findFirst().getAsInt();
    }

You can replace rand.ints(1, 1, 10).findFirst().getAsInt() with rand.nextInt(9)+1; to simplify.

    /*
    //paints the square with the variables generated in the random methods
    */
    public void paint(Graphics g){
        g.setColor(Color.CYAN);
        g.fillRect(squareXLocation,squareYLocation,squareSize,squareSize);
    }

The color can be made into a constant or a variable (SQUARE_COLOR) to allow for easier changing later on.

    /*
    //sets the squareWidth and square fallSpeed to a random value for every square created
    */
    public Square(){
        generateRandomXLocation();
        generateRandomSquareSize();
        generateRandomFallSpeed();
    }

You're forgetting to reset the y location at this point. You'll also notice that this code is exactly the same as the code in the if(squareYLocation >= Game.WINDOW_HEIGHT){ block. This is a good moment to abstract this code. You can create a "reset()" method here and call all the generate code from there. Then you can reuse the reset method in both places.

    public void update(){

        //changes the squares xLocation and fallSpeed if the created square reaches the bottom of the screen
        if(squareYLocation >= Game.WINDOW_HEIGHT){
            generateRandomXLocation();
            generateRandomFallSpeed();
            generateRandomSquareSize();
            squareYLocation = -squareSize;
        }

        //moves the square down if the square is inside the window
        if(squareYLocation <= Game.WINDOW_HEIGHT){
            squareYLocation += fallSpeed;
        }
    }
}


Remove duplicates from a linked list Given a sorted linked list, delete all duplicates such that each element appears only once.

  For example:

  Given 1->1->2, return 1->2.
  Given 1->1->2->3->3, return 1->2->3.

The following is my code:

struct ListNode {
    int data;
    ListNode *next;
    ListNode(int x) : data(x), next(NULL) {}
};

ListNode *getNextElement(ListNode *head){
    while ((head != NULL) && (head->next != NULL)
          && (head->data == head->next->data)){
        head = head->next;
    }

    return head->next;
}

ListNode *deleteDuplicates(ListNode *head) {
        // Start typing your C/C++ solution below
        // DO NOT write int main() function
    ListNode *cur = head;
    ListNode *next = NULL;
    while (cur != NULL){
        next = getNextElement(cur);
        cur->next = next;
        cur = next;
    }
    return head;
}

head can never be NULL

ListNode *getNextElement(ListNode *head){
    while ((head != NULL)                      // This can't be NULL
                                               // because you test before calling.
          && (head->next != NULL)
          && (head->data == head->next->data)){
        head = head->next;
    }

    // If head can be NULL then you should also be checking here.
    // Otherwise this may fail.
    return head->next;
}

Small notes:

ListNode *deleteDuplicates(ListNode *head) {
        // Start typing your C/C++ solution below
        // DO NOT write int main() function

    // Replace with for(;;)
    for(ListNode* cur = head; cur != NULL; cur = cur->next){
        // Move declaration of next here.
        // It is not used outside the loop so why pollute
        ListNode *next = getNextElement(cur);

        // May want to make sure you don't leak.
        ListNode *old  = cur->next;
        while(old != next)
        {
            ListNode* toFree = old;  // you can probably put this logic in getNext()
            old = old->next;
            free(toFree);
        }
        cur->next = next;
    }
    return head;
}


Discount curve from instant rate This my first foray in F#, and I cannot think (yet) functional.  I have implemented an interface for a discount curve and the implementation for a piecewise constant instant rate implementation.

I would like to have your opinion on how to make this more F#.  Should I keep the second input of the constructor as an array or a sequence?

Also, as I pre-compute all possible values between the first point and the end_date, I could have done a tail recursion and lazily populate the cache, but I was wondering about efficiency: I did not have time to write the tail-recursion and do a timing comparison.

module public DiscountCurve =
    type IDiscountCurve =
       abstract member Df : System.DateTime -> double

    type DfPieceWiseInstant(end_date:System.DateTime, points:(System.DateTime * double)[]) =
        let start_date = fst(points.[0])
        let end_date = end_date
        let cache : double array = Array.zeroCreate ((end_date - start_date).Days + 1)
        let points = points
        do
            cache.[0] <- 1.
            for idx in { 0 .. (points.Length - 1) } do
                let cache_idx_start = max 1 (fst(points.[idx]) - start_date).Days
                let cache_idx_end =
                    if idx+1 < points.Length then
                        (fst(points.[idx+1]) - start_date).Days - 1
                    else
                        (end_date - start_date).Days - 1
                let cache_idx_df = exp (-snd(points.[idx])/365.)
                for cache_date in seq { cache_idx_start .. cache_idx_end-1 } do
                    cache.[cache_date] <- cache.[cache_date-1] * cache_idx_df

        interface IDiscountCurve with
            member this.Df(date) =
                cache.[(date - start_date).Days]

I can't see how tail recursion comes into the picture exactly, but I would start with putting a stop watch around the do that populates the cache and checking how much work it actually needs to do, just to see whether you couldn't get away with what you have now. There's certainly some value in computing only the part of the curve you need, but I'd check how much that would gain you before making that code more complicated.

For starters, you certainly don't need let bindings for end_date and points. You already have them accessible anywhere within your type as they're constructor arguments. You can also drop the {} and seq {} in you for ... in ... do expressions.

A quick and simple way to delay populating of the cache until it's needed is to make it lazy.

let cache =
     Lazy.Create <| fun () ->
        let cache = Array.zeroCreate ((end_date - start_date).Days + 1)
        (* the entirety of your 'do' goes here *)
        cache

interface IDiscountCurve with
    member this.Df(date) =
        cache.Value.[(date - start_date).Days]

This way it will only be populated once Df is called, and no work would be done on object's construction.


Grouping into more groups in one iteration I had a need to group the same dataset in several groups. So instead of repeatedly query the dataset, I made an extension that could do it once. The caveat is, that the result is materialized in dictionaries, because I didn't managed to find an way to avoid that. Maybe you can?

public static IDictionary<string, Dictionary<object, HashSet<T>>> MultiGroupBy<T>(this IEnumerable<T> source, params (string Label, Func<T, object> Getter)[] groupers)
{
  if (source == null) throw new ArgumentNullException(nameof(source));
  if (groupers == null) throw new ArgumentNullException(nameof(groupers));

  IDictionary<string, Dictionary<object, HashSet<T>>> results = new Dictionary<string, Dictionary<object, HashSet<T>>>();

  using (var enumer = source.GetEnumerator())
  {
    while (enumer.MoveNext())
    {
      foreach ((var label, var func) in groupers)
      {
        if (!results.TryGetValue(label, out var dict))
        {
          dict = new Dictionary<object, HashSet<T>>();
          results[label] = dict;
        }

        var key = func(enumer.Current);
        if (!dict.TryGetValue(key, out var set))
        {
          set = new HashSet<T>();
          dict[key] = set;
        }

        set.Add(enumer.Current);
      }
    }

  }

  return results;
}

Use case:

static void TestMultiGrouping()
{
  string[] data =
  {
    "Black",
    "White",
    "Yellow",
    "green",
    "Red",
    "blue",
    "cyan",
    "Magenta",
    "Orange"
  };

  foreach (var result in data.MultiGroupBy(
    ("First UCase", s => s.Length > 0 && char.IsUpper(s[0])),
    ("Length", s => s.Length),
    ("Length Four", s => s.Length == 4),
    ("Contains 'e'", s => s.Contains('e')),
    ("Num n's", s => s.Count(c => c == 'n'))))
  {
    Console.WriteLine($"Results for {result.Key}:");
    foreach (var dict in result.Value)
    {
      Console.WriteLine($"{dict.Key}: {dict.Value.Count} [{(string.Join(", ", dict.Value))}]");
    }
    Console.WriteLine();
  }
}

public static IDictionary<string, Dictionary<object, HashSet<T>>> MultiGroupBy<T>(this IEnumerable<T> source, params (string Label, Func<T, object> Getter)[] groupers)

I don't understand the mixture of interfaces (IDictionary) and implementations (Dictionary, HashSet), nor the mixture of generics (<T>) and non-generics (object). Why is it not

public static IDictionary<string, IDictionary<K, ISet<T>>> MultiGroupBy<T, K>(this IEnumerable<T> source, params (string Label, Func<T, K> Getter)[] groupers)

?

  IDictionary<string, Dictionary<object, HashSet<T>>> results = new Dictionary<string, Dictionary<object, HashSet<T>>>();

  using (var enumer = source.GetEnumerator())
  {
    while (enumer.MoveNext())
    {
      foreach ((var label, var func) in groupers)
      {
        if (!results.TryGetValue(label, out var dict))
        {
          dict = new Dictionary<object, HashSet<T>>();
          results[label] = dict;
        }

        ...

I'm not quite sure why you want to return an empty dictionary if the source is empty. As a caller of your library, I'd probably rather get a dictionary mapping the grouper names to empty dictionaries.

That also simplifies the initialisation:

  var results = groupers.ToDictionary(grouper => grouper.Item1, _ => new Dictionary<object, HashSet<T>>());

  using (var enumer = source.GetEnumerator())
  {
    while (enumer.MoveNext())
    {
       ...
    }

  }

KISS. foreach is much kinder on the maintenance programmer, who doesn't have to check for correct usage patterns of the unsugared API. Using MoveNext() / Current for speed is the epitome of premature optimisation unless benchmarking shows that it's a bottleneck, in which case there should be a comment explaining the bottleneck to justify the more complex code.

Moreover, if this is a bottleneck then it seems likely that the dictionary lookups in results for every single element in the source will be slower than the overhead of foreach, so you could start by replacing results with a List<(string Label, Func<T, K> Getter, IDictionary<K, ISet<T>> Groups)> and just convert it to a dictionary after the loop.

      foreach ((var label, var func) in groupers)

var (label, func) saves the repetition.

After my proposed refactors and some minor tidying of whitespace, I get

public static IDictionary<string, IDictionary<K, ISet<T>>> MultiGroupBy<T, K>(this IEnumerable<T> source, params (string Label, Func<T, K> Getter)[] groupers)
{
  if (source == null) throw new ArgumentNullException(nameof(source));
  if (groupers == null) throw new ArgumentNullException(nameof(groupers));

  var results = groupers.ToDictionary(grouper => grouper.Item1, _ => (IDictionary<K, ISet<T>>)new Dictionary<K, ISet<T>>());

  foreach (var elt in source)
  {
    foreach (var (label, func) in groupers)
    {
      var dict = results[label];
      var key = func(elt);
      if (!dict.TryGetValue(key, out var set))
      {
        set = new HashSet<T>();
        dict[key] = set;
      }

      set.Add(elt);
    }
  }

  return results;
}


Return the state of collection based on individual object states The following code is a simplified version of what I want to achieve. Each object may have one of several states, and the overall collection state depends on the state of each of these objects. Currently, the code is written in the form of several if-else blocks, however, it seems ugly and non-extensible. By that, I mean, tomorrow if additional object states are added, I would end up adding several more flags and if-conditions, making the code difficult to read and maintain. Can this be done in a better way?

internal enum State
{
    Error,
    Warning,
    Okay
}

internal enum CollectionState
{
    Error,
    AllWarnings,
    AllOkay,
    OkayAndWarnings
}

internal class MyClass
{
    //other properties...

    internal State State { get; set; }
}

internal class MyBusinessLogic
{
    /// <summary>
    /// 1. If any object is in Error state, CollectionState is Error.
    /// 2. If all objects are in Warning state, CollectionState is AllWarnings.
    /// 3. If all objects are in Okay state, CollectionState is AllOkay.
    /// 4. If some objects are in Okay state and some in Warning state, CollectionState is OkayAndWarnings
    /// </summary>
    internal CollectionState GetCollectionState(IList<MyClass> objectList)
    {
        bool isError, isOkay, isWarning;
        isError = isOkay = isWarning = false;
        foreach (var myObject in objectList)
        {
            if (myObject.State == State.Error)
            {
                isError = true;
                break;
            }
            if(myObject.State == State.Okay)
            {
                isOkay = true;
            }
            else if (myObject.State == State.Warning)
            {
                isWarning = true;
            }
        }

        if (isError)
        {
            return CollectionState.Error;
        }
        else if(!isOkay && isWarning)
        {
            return CollectionState.AllWarnings;
        }
        else if (isOkay && !isWarning)
        {
            return CollectionState.AllOkay;
        }
        else //if(isOkay && isWarning)
        {
            return CollectionState.OkayAndWarnings;
        }
    }
}

You can also use bitwise flags for that:

[Flags]
internal enum State
{
    None = 0,
    Error = 1,
    Warning = 2,
    Okay = 4,

    OkayAndWarning = Okay | Warning,
}

internal State GetCollectionState(IList<MyClass> objectList)
{
    return objectList.Select(o => o.State)
                     .Aggregate((result, next) => result |= next);
}

You can then check the aggregated state using HasFlag method or equality operator:

var state = GetCollectionState(...);
var error = state.HasFlag(State.Error);
var allWarnings = state == State.Warning;
var allOkay = state == State.Okay;
var okayAndWarnings = state == State.OkayAndWarning;
//etc.

On the plus side this approach is easy to implement and State enum is easy to extend when needed. Combining flags is a trivial process of applying | (bitwise OR) operator.

On the minus side you give up some of the type safety. With this approach any flag combination is a valid state, (including no flags at all). Also there is no longer a distinction between individual state and collection state (both use State enum), compiler will not warn you if you accidentally mix them. So in general, you'll have to be more careful, when checking states.


Exercism assignment for word-count in Clojure I would appreciate some insights / comments from Clojure regulars out there about my submission here.

(ns phrase)
(require '[clojure.string :as s])

(defn word-array
  [phrase]
  (-> (s/lower-case phrase)
      (s/split #"\W+")))

(defn word-count
  [phrase]
  (-> (word-array phrase)
      (frequencies)))

I would format it like this:

(ns phrase
  (:require [clojure.string :as s]))

(defn word-array [phrase]
  (s/split (s/lower-case phrase) #"\W+"))

(defn word-count [phrase]
  (frequencies (word-array phrase)))

Notice that I included the require statement as part of the ns definition.

Whether or not you use threading macros (->, ->>) is generally a matter of personal preference, and there's nothing wrong with using them here, but I think in this case since you're only using 2 functions, I find the above easier to read. You might also consider using comp:

(def word-array (comp #(s/split % #"\W+") s/lower-case)
(def word-count (comp frequencies word-array))


'Zero out' a matrix using Python Below is my code for an attempted solution to cracking the coding interview exercise 1.8 written in python 3.5.  The problem statement is:

  Write an algorithm such that if an element in an MxN matrix is 0, its entire row and column are set to 0.

I looked at the hints for this problem so I do know that my solution is not the most space efficient possible.  I believe the time complexity of the code below is \$O(M*N)\$ and the space complexity is \$O(M + N)\$.  I wrote small unit tests to test my code.  Looking for feedback on what needs to be improved, especially in terms of readability.

import unittest

def locate_zero_rows(matrix: list) -> list:
    """Given an NxM matrix find the rows that contain a zero."""
    zero_rows = [False for _ in range(len(matrix))]
    for row_num, row in enumerate(matrix):
        for col_num, element in enumerate(row):
            if element == 0:
                zero_rows[row_num] = True
    return zero_rows

def locate_zero_cols(matrix: list) -> list:
    """Given an NxM matrix find the columns that contain a zero."""
    zero_cols = [False for _ in range(len(matrix[0]))]
    for row_num, row in enumerate(matrix):
        for col_num, element in enumerate(row):
            if element == 0:
                zero_cols[col_num] = True
    return zero_cols

def zero_out(matrix: list) -> list:
    """Given an NxM matrix zero out all rows and columns that contain at least one zero."""
    zero_rows, zero_cols = locate_zero_rows(matrix), locate_zero_cols(matrix)

    for row_num, row in enumerate(matrix):
        for col_num, element in enumerate(row):
            if zero_rows[row_num] or zero_cols[col_num]:
                matrix[row_num][col_num] = 0
    return matrix

class MyTest(unittest.TestCase):
    def test_locate_zero_rows(self):
        matrix = [[5, 3, 2, 1],
                  [-3, 0, 5, 0],
                  [0, -1, 2, 6]]
        zero_rows = [False, True, True]
        self.assertSequenceEqual(locate_zero_rows(matrix), zero_rows)

    def test_locate_zero_cols(self):
        matrix = [[5, 3, 2, 1],
                  [-3, 0, 5, 0],
                  [0, -1, 2, 6]]
        zero_cols = [True, True, False, True]
        self.assertSequenceEqual(locate_zero_cols(matrix), zero_cols)

    def test_zero_out(self):
        matrix = [[5, 3, 2, 1],
                  [-3, 0, 5, 0],
                  [0, -1, 2, 6]]
        zeroed_out_matrix = [[0, 0, 2, 0],
                             [0, 0, 0, 0],
                             [0, 0, 0, 0]]
        self.assertSequenceEqual(zero_out(matrix), zeroed_out_matrix)

if __name__ == '__main__':
    unittest.main()

Repetition

I would write only one function to locate zeros:

def locate_zeros(matrix: IntegerMatrix) -> Iterable[Position]:
    """Given an NxM matrix find the positions that contain a zero."""
    for row_num, row in enumerate(matrix):
        for col_num, element in enumerate(row):
            if element == 0:
                yield (col_num, row_num)

And use it in zero_out like this:

    if row_num in (x[1] for x in zeros_positions) or col_num in (x[0] for x in zeros_positions):
        matrix[row_num][col_num] = 0

Type hints

Given that you specifically mentioned Python 3.5 and that you already have something like type hints on your functions, I suggest you go all the way with mypy compatible type hints.

from typing import List, Any,  Iterable, Tuple

Position = Tuple(int, int)
IntegerMatrix = List[List[int]]

def locate_zeros(matrix: IntegerMatrix) -> Iterable[Position]:

def zero_out(matrix: IntegerMatrix) -> IntegerMatrix:

This way you can statically check your code has the correct types like in natively statically types languages and give the user much more detailed information on the types.


Effective usage of multiple joins in Eloquent dependent on variable I have an Eloquent model that has multiple types of joins that is used to filter out different request to grab the data of movies e.g.: Actors, Directors, Writers, etc...

The joins are necessary because they use unconventional pivot tables. All the queries and eloquent here works and gives the desired returned data - however I'm breaking DRY (Don't Repeat Yourself) principle for each join filters I am creating within a controller.

Here are the code segments that I would like to tweak:

This is a call for all movies by actors:

public function getByActor($where, $limit, $offset)
{
    $movieData = array();

    $restrictedIdList = $this->getRestrictedIds();
    if (empty($restrictedIdList)) {
        $restrictedIdList = array(0);
    }

    $movieData = Movie::
        FilterByActorWithInnerJoin(urldecode($where['actorName']))
        ->selectRaw('movie.*')
        ->where('movie.status','=','active')
        ->whereNotIn('movie.id', $restrictedIdList)
        ->take($limit)
        ->skip($offset)
        ->get()
        ->toArray();
    return $movieData;
}

This is for Writers:

public function getByWriter($where, $limit, $offset)
{
    $movieData = array();

    $restrictedIdList = $this->getRestrictedIds();
    if (empty($restrictedIdList)) {
        $restrictedIdList = array(0);
    }

    $movieData = Movie::
        FilterByWriterWithInnerJoin(urldecode($where['writerName']))
        ->selectRaw('movie.*')
        ->where('movie.status','=','active')
        ->whereNotIn('movie.id', $restrictedIdList)
        ->take($limit)
        ->skip($offset)
        ->get()
        ->toArray();
    return $movieData;
}

As you can see the only difference is the FilterBy<filter type join>WithInnerJoin that I have created within my model.

Here are the joins of the model:

public function scopeFilterByWriterWithInnerJoin($query,$writerName)
{
    $query->join('movie_writers as mw', 'mw.movie_id', '=', 'movie.id')
        ->join('writers as w',function($join) use ($writerName){
            $join->on('w.id', '=', 'mw.writer_id')
                ->where('w.name','like','%'.$writerName.'%');
        });
}

public function scopeFilterByActorWithInnerJoin($query,$actorName)
{
    $query->join('movie_actors as ma', 'ma.movie_id', '=', 'movie.id')
        ->join('actors as a',function($join) use ($actorName){
            $join->on('a.id', '=', 'ma.actor_id')
                ->where('a.name','like','%'.$actorName.'%');
        });
}

I will have to do the same for producers, directors, etc.

Is there a more efficient way to do this?

I don't see a way to do it with eager loads. I wonder if you could reduce it to getBy($k, $v) using distinct though?

class Movie extends Eloquent
{

    function writers()
    {
        return $this->belongsToMany('Writer');
    }

    function actors()
    {
        return $this->belongsToMany('Actor');
    }

    /**
     * Get a list of Movies with the matching the key-value.
     *
     * @param string $k Key (movie attribute) name
     * @param string $v Value of the movie attribute
     *
     * @return \Illuminate\Database\Query\Builder
     */
    function getBy($k, $v) {
        return Movie::join('movie_writers as mw', 'mw.movie_id', '=', 'movie.id')
            ->join('writers as w', 'w.id', '=', 'mw.writer_id')
            ->join('movie_actors as ma', 'ma.movie_id', '=', 'movie.id')
            ->join('actors as a', 'a.id', '=', 'mw.actor_id')
            ->where('movie.status','=','active')
            ->select('movie.id', 'movie.name')
            ->where($k, 'like', "%$v%")
            ->distinct();
    }
}

With this in place, getBy will return a Query Builder object that you can filter by restricted and limit.

$movie = new Movie();
$movies = $movie->getBy('writers.name', 'Matthew Weiner')
                ->whereNotIn('movie.id', $this->restricted())
                ->take($limit)
                ->skip($offset)
                ->get();

Then you can take the a step further by pulling this method out into a MovieRepository that implements MovieRepositoryInterface. Check out this Laracasts episode for more on that. It could have a MovieRepository->restrictedBy(Builder $builder, $restrictedList) that also returns a Builder object.


Unisex Bathroom problem with semaphore An office has a bathroom that can be used by both men and women, but not both at the same time. If a man is in the bathroom, other men may enter, but any women wishing to use the bathroom should wait for it to be empty. If a woman is in the bathroom, other women may enter, but any men wishing to use the bathroom should wait it to be empty. Each person (man or woman) will spend some time using the bathroom.

package basics.problems;

    //BATHROOM_SIZE - 5
    //2 types of threads - MEN and WOMEN
    import java.util.concurrent.Semaphore;
    import java.util.concurrent.atomic.AtomicInteger;

    public class UniSexBathroom {
        AtomicInteger currentSize;
        int capacity;
        Semaphore noOfEntriesSemaphore;
        Semaphore currentGenderSemaphore = new Semaphore(1);
        volatile Person currentGender;

        public UniSexBathroom(int capacity) {
            this.capacity = capacity;
            this.noOfEntriesSemaphore = new Semaphore(capacity);
            this.currentSize =  new AtomicInteger(0);
        }

        public void occupy(Person personSex) throws InterruptedException {
            acquireGenderSemaphore(personSex);
            useBathroomAndExit(personSex);
            releaseGenderSemaphore(personSex);
        }

        private void releaseGenderSemaphore(Person personSex) {
            if(currentSize.get()==0){
                currentGenderSemaphore.release();
                currentGender = null;
                System.out.println(personSex.type+ Thread.currentThread().getName()+" releasing gender semaphore");
            }
        }

        private void useBathroomAndExit(Person personSex) throws InterruptedException {
            noOfEntriesSemaphore.acquire();
            currentSize.incrementAndGet();
            System.out.println(personSex.type +  Thread.currentThread().getName()+" is stepping in, current capacity is : "+currentSize.get());
            Thread.sleep(100);
            noOfEntriesSemaphore.release();
            currentSize.decrementAndGet();
            System.out.println(personSex.type +  Thread.currentThread().getName()+" is stepping out, current capacity is : "+ currentSize.get() +"-----------------");
        }

        private void acquireGenderSemaphore(Person personSex) throws InterruptedException {
            if(currentSize.get() == 0){
                currentGenderSemaphore.acquire();
                currentGender = personSex;
            }
            if(!personSex.type.equals(currentGender.type)){
                currentGenderSemaphore.acquire();
                currentGender = personSex;
            }
        }

        public static void main(String[] args){
            UniSexBathroom uniSexBathroom = new UniSexBathroom(2);
            new Thread(new PersonThread(uniSexBathroom, Person.MALE)).start();
            new Thread(new PersonThread(uniSexBathroom, Person.MALE)).start();
            new Thread(new PersonThread(uniSexBathroom, Person.MALE)).start();
            new Thread(new PersonThread(uniSexBathroom, Person.MALE)).start();

            new Thread(new PersonThread(uniSexBathroom, Person.FEMALE)).start();
            new Thread(new PersonThread(uniSexBathroom, Person.MALE)).start();
            new Thread(new PersonThread(uniSexBathroom, Person.FEMALE)).start();
        }
    }

    enum Person {
        MALE("Male"), FEMALE("Female");
        String type;

        Person(String type) {
            this.type = type;
        }
    }

    class PersonThread implements Runnable {
        public UniSexBathroom uniSexBathroom;
        public Person person;
        public PersonThread(UniSexBathroom uniSexBathroom, Person person){
            this.uniSexBathroom = uniSexBathroom;
            this.person = person;
        }

        @Override
        public void run() {
            try {
                uniSexBathroom.occupy(person);
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
    }

Your code has a bug.  I can reproduce it fairly consistently by changing your run method to:

@Override
public void run() {
    try {
        for(int i=0;i<100;i++) {
            uniSexBathroom.occupy(person);
        }
    } catch (InterruptedException e) {
        e.printStackTrace();
    }
}

Notice, that I've increased the number of times that each thread tries to occupy the bathroom.  Starting/Stopping threads is a fairly slow process so in order to encourage more contention I used a loop so more time is spent trying to get into the bathroom.  It results in a null pointer in the acquireGenderSemaphore method on this line:

if (!personSex.type.equals(currentGender.type)) {

It's happening because currentGender is null.  We can be confident about this, because personSex is the same for every call for a given thread.  How is it null?  Well, because this isn't safe:

if (currentSize.get() == 0) {
    currentGenderSemaphore.acquire();
    currentGender = personSex;
}

The problem is that context switches can happen at any time, so just because currentSize wasn't 0 for the if evaluation, it doesn't mean that it's still zero during the execution of the if block.

So:

Thread2 is currently in the bathroom (Size 1)

Thread1 comes in to acquire the lock, checks the value of current size and finds it's 1, skip the if block

if (currentSize.get() == 0) {  // Size still 1

Thread2, which is currently using the bathroom leaves:

noOfEntriesSemaphore.release();
currentSize.decrementAndGet(); // Size becomes 0

Thread2, then checks if they were the last one out, which they were:

if (currentSize.get() == 0) {
    currentGenderSemaphore.release();
    currentGender = null;  // Gender becomes null
}

Thread1 then proceeds, assuming there is still somebody in the bathroom:

if (!personSex.type.equals(currentGender.type)) {

But since currentGender has been set to null by Thread 2 when it left.... Bang.