Edited simple Blackjack game in Python 3.4 I'm a beginning Python programmer, and earlier I made a simple Blackjack game in Python. The original version (which can be found here) got a lot of great feedback, so I rewrote the entire program to reflect the feedback I received. Again, any tips are appreciated.

import random
import itertools

# generates the deck and card values
SUIT = 'scdh'
RANK = '23456789TJQKA'
DECK = tuple(''.join(card) for card in itertools.product(RANK, SUIT))
VAL = []
for a in range(9):
    VAL += [a+2] * 4
for _ in range(3):
    VAL += [10] * 4
VAL += [1] * 4
DECKVAL = dict(zip(DECK, VAL))
counter = 0

def shuffle():
    # shuffles the deck into random order
    global s_deck
    s_deck = random.sample(DECK, 52)

class Hand:

    global counter, s_deck

    def __init__(self):
        # deals 2 cards
        # sets up variables hand_sum and init_sum for totaling
        global counter
        self.cards = list(s_deck[counter:counter + 2])
        counter += 2
        self.hand_sum = 0
        self.init_sum = 0

    def sum_hand(self):
        # totals the hand
        self.hand_sum = 0
        self.init_sum = 0
        for b in range(len(self.cards)):
            self.init_sum += DECKVAL[self.cards[b]]
        if ('As' in self.cards or 'Ac' in self.cards or 'Ad' in self.cards or 'Ah' in self.cards)\
                and self.cards and self.init_sum + 10 <= 21:    # special ace rule conditions
            self.hand_sum = self.init_sum + 10
        else:
            self.hand_sum = self.init_sum

    def draw(self):
        # draws a card from the deck
        global counter
        self.cards += list(s_deck[counter:counter + 1])
        counter += 1

    def hit_or_stay(self, dealer):
        # asks the player if they want to hit or stay
        self.bust(dealer)
        print("\n\nDealer's Hand:", dealer.cards[0], "--")
        print("Your hand is:", self.cards, "   Your sum is:", self.hand_sum, "\n")
        choice = input("Hit or Stay? ").lower()
        if choice == 'hit':
            self.draw()
            self.hit_or_stay(dealer)
        elif choice == 'stay':
            self.total(dealer)
        else:
            print("Please enter hit or stay")
            self.hit_or_stay(dealer)

    def total(self, dealer):
        # determines winner
        if self.hand_sum > dealer.hand_sum:
            print("You won the hand")
        elif self.hand_sum < dealer.hand_sum:
            if dealer.hand_sum <= 21:
                print("You lost the hand")
            else:
                print("Dealer busted")
        else:
            print("You tied")
        print("Dealer's hand:", dealer.cards, "   Dealer's sum:", dealer.hand_sum)
        print("Your hand:", self.cards, "Your sum:", self.hand_sum)
        print("\n*******************\n")

    def bust(self, dealer):
        # checks for player busting
        self.sum_hand()
        if self.hand_sum > 21:
            print("YOU BUSTED\n")
            self.hand_sum = 0
            self.total(dealer)
        else:
            pass

class Dealer(Hand):

    def __init__(self):
        Hand.__init__(self)
        self.logic()

    def logic(self):
        # makes sure the dealer hits "soft 17"
        self.sum_hand()
        while self.hand_sum < 17:
            self.draw()
            self.sum_hand()

class Game:

    def __init__(self):
        # starts the game
        x = input("Would you like to play?").lower()
        if x == "yes":
            shuffle()
            player = Hand()
            dealer = Dealer()
            player.hit_or_stay(dealer)
        else:
            pass

if __name__ == '__main__':
    game = Game()

My first tip would be to get rid of the Game class. It serves no value. Rather, it should be a method, appropriately named main. If you want a few tips for determining whether to write a class or not, here are a some instances where you should avoid classes:

If the class only contains two methods, __init__, and another.
If the class never refers to self. (Credit to @jonrsharpe)

Secondly, those comments underneath your function declarations should be docstrings. A typical function docstring would look something like this,

def my_func(args):
    """
    A detailed description of the function
    and its arguments.
    """

The below statement in Game.__init__ is not needed, and does nothing. It can be removed.

else:
    pass

You have some odd naming in places. For example, logic could be renamed to check_for_soft_17 or something along those lines, and total could be renamed to determine_winner.
The two for loops underneath the VAL = [] could be shortened to generator expressions. Here's how that would be done.

VAL = [[a + 2] * 4 for a in range(9)] + [[10] * 4 for _ in range(3)]

Preferably, the variable a in the above example should also be renamed to something a little more descriptive, and you should apply some of those "magic numbers" to variables as well.
As mentioned by @Josay, you're using globals in a lot of your functions. Rather than assigning a value to a global, try returning a value instead. It makes things a whole lot easier to manage.

Anyways, that's about all I can think of. If there's anything else that you want me to cover, just mention it in the comments, and I'll see what I can do. I hope this helps!


Small improved std::list to manage doubloons and pointers I am extending std::list in order to have access to convenience member functions that simplify my everyday developments (e.g. ways to deal with doubloons, delete all the stored pointers, etc.). I removed some of these member functions, but I kept the essential ones.

#ifndef __PJ_LIST_H__
#define __PJ_LIST_H__

#include <list>
#include <iostream>

// This class is a wrapper of std::list to provide some smarter functionalities
template<class T>
class PjList: public std::list<T>
{
    public:

        typedef typename PjList<T>::iterator PjIterator;

        PjList() {};
        virtual ~PjList() {};

        // Helper functions to deal with doubloons
        bool contains(const T& t);
        bool containsDoubloons();
        PjList<T> removeDoubloons();

        // Calls destructor of all list item and empty the list
        void destroyContents();

        // Accessing elements at index
        T& at(int index);

        // Inserting element around "ref"
        bool insertItem(const T& item, const T& ref, bool before = true);

};

template<class T>
bool PjList<T>::contains(const T& t)
{
    PjIterator it;

    for( it = std::list<T>::begin(); it != std::list<T>::end(); it++ )
    {
        if ( *it == t )
            return true;
    }
    return false;
}

template<class T>
bool PjList<T>::containsDoubloons()
{
    PjIterator it;
    PjIterator jt;
    int nb_occurence;

    for( it = std::list<T>::begin(); it != std::list<T>::end(); it++ )
    {
        nb_occurence = 0;
        for( jt = std::list<T>::begin(); jt != std::list<T>::end(); jt++ )
        {
            if ( *it == *jt )
                nb_occurence += 1;
        }
        if ( nb_occurence > 1 )
            return true;
    }
    return false;
}

template<class T>
PjList<T> PjList<T>::removeDoubloons()
{
    PjList<T> output;

    PjIterator it;
    for( it = std::list<T>::begin(); it != std::list<T>::end(); it++ )
    {
        T& t = *it;
        if ( !output.contains(t) )
            output.push_back(t);
    }

    return output;
}

template<class T>
void PjList<T>::destroyContents()
{
    PjIterator it;

    for( it = std::list<T>::begin(); it != std::list<T>::end(); it++ )
        delete *it;

    std::list<T>::clear();
}

template<class T>
bool PjList<T>::insertItem(const T& t, const T& ref, bool before)
{
    bool ref_found = false;

    PjIterator it;

    for( it = std::list<T>::begin(); it != std::list<T>::end(); it++ )
    {
        if ( *it == ref )
        {
            ref_found = true;
            break;
        }
    }

    if ( !ref_found )
    {
        std::cerr << "Insertion in list failed since ref item is not found" << std::endl ;
        return false;
    }

    insert(it, t);
    return true;
}

template<class T>
T& PjList<T>::at(int index)
{
    static T dummy;
    int i = 0;
    PjIterator it;

    for( it = std::list<T>::begin(); it != std::list<T>::end(); it++ )
    {
        if ( index == i++ )
            return *it;
    }
    std::cerr << "Bad index " << index << std::endl ;
    return dummy;
}

#endif

I see no reason to have this be an entirely new data structure that inherits from std::list<T>. All of the new member functions you've defined are really just wrapper functions that could just be delegated to std::list<T> functions or functions that could be implemented as regular procedural functions.

For example, the contains() and at() and insertItem() (which should really just be named insertItemBefore()) functions could simply be:

namespace list_util {

template <class T>
bool contains(const std::list<T>& l, const T& elem)
{
    return std::find(l.begin(), l.end(), elem) != l.end();
}

template <class T>
T& at(const std::list<T>& l, std::size_t index)
{
    assert(index < l.size());
    auto begin = l.begin();
    std::advance(begin, index);
    return *begin;
}

template <class T>
bool insertItemBefore(std::list<T>& l, const T& ref_item, const T& elem)
{
    auto itemItr = std::find(l.begin(), l.end(), ref_item);

    // if we found the item, itemItr != l.end()
    if (itemItr != l.end()) {
        l.insert(itemItr, elem);
        return true;
    }
    return false;
}

}

You'd have to #include<algorithm> and #include<iterator> for this to work. Notice that the index parameter is now std::size_t to avoid having to check for negative indexes. They could then simply be called as: list_util::contains(the_list, elem); and list_util::at(the_list, the_index);.

Moreover, containsDoubloons() could be implemented using std::find() and destroyContents() is really just a call to std::list<T>::clear():

namespace list_util {

template<class T>
bool containsDoubloons(const std::list<T>& l)
{
    for (auto it = l.begin(); it != l.end(); ++it) {
        const auto beginItr = std::next(it);
        if (beginItr != l.end() &&
            std::find(beginItr, l.end(), *it) != l.end()) {
            return true;
        }
    }
    return false;
}

template<class T>
void destroyContents(std::list<T>& l)
{
    l.clear();
}

}

Moreover, since the new functions we've written are completely generic and use the STL iterators for all operations, there's no reason to limit them only to std::list<T>. We could allow any container of containing T to be a parameter. An example is the contains() function:

template < template <class> class C, class T>
bool contains(const C<T>& cont, const T& elem)
{
    return std::find(cont.begin(), cont.end(), elem) != cont.end();
}


Binding a DataGridView to a List I'm new to Databinding in MVP and I want to bind my DataGridView to a object list. This grid may be populated in two ways.

There are TextBoxes in the form and users can enter text, and when they press Add button, a new object is instantiated with the provided fields and then it is added to the list.

Other way is, when a user searches for objects, matching objects are loaded from database to the list. Also after searching users should be able to add new items to this list as in the first method.

Finally this list is binded to the DataGridView and details are shown in the grid. Now the user could do any changes to the items shown in the grid and if it's all right, the user could save them.

So in my presenter I have three methods for the above requirements.

    Class AttendancePresenter : BasePresenter
    {
        private IAttendance _Model;
        private readonly IAttendanceView _View;
        private readonly IDataService _DataService;
        private readonly IMessageService _MessageService;
        List<IAttendance> AttendanceList = new List<IAttendance>();

        public AttendancePresenter( IAttendance model, IAttendanceView view, IDataService dataService, IMessageService messageService ) : base(messageService)
        {
            _Model = model;
            _View = view;
            _DataService = dataService;
            _MessageService = messageService;

            SetModelPropertiesFromView(_Model, _View);
            WireUpViewEvents();
        }

        //Add new attendance to the list and the the list is binded to the grid
        private void _View_OnAddAttendance(object sender, EventArgs e)
        {

            SetModelPropertiesFromView(_Model, _View); // call base class method to update the Mode with uptodate data from the view
            IAttendance attendanceModel = new Attendance(_Model); // Use Model's copy constructor to get a clone
            AttendanceList.Add(attendanceModel);
            var bindingList = new BindingList<IAttendance>(AttendanceList);
            var source = new BindingSource(bindingList, null);
            _View.AttendanceGrid = source;
        }

        //Get attendance from the database and show on the grid
        void _View_OnSearchForAttendance(object sender, EventArgs e)
        {
            AttendanceList = _DataService.GetAttendance(_View.InTime, _View.PointID, AttendanceList);
            var bindingList = new System.ComponentModel.BindingList<IAttendance>(AttendanceList);
            var source = new BindingSource(bindingList, null);
            _View.AttendanceGrid = source;
        }

        //Pass attendance list to DAL to insert into the database
        private void _View_OnSaveAttendance(object sender, EventArgs e)
        {
             var result = _DataService.InsertAttendance(AttendanceList);
             AttendanceList.Clear();

            _MessageService.ShowResult(result);
        }

    }

I'm not sure the way I've binded things is right. And also I wonder if I could use a single binding list here as I'm binding same list to the same DataGridView! Please review my code and give your advice.

First some nitpicks:

Variables should be camelCased, not PascalCased. The underscore prefixes are okay, but _Model is not. Let the underscore be your visual indicator. What you've done is confusing on the eyes.
Don't fully qualify names unless you have to.

var bindingList = new System.ComponentModel.BindingList<IAttendance>(AttendanceList);

Else where you directly use BindingList, so System.ComponentModel must be in your usings somewhere. Using the fully qualified call clutters the code.
You could extract this duplicated code into its own method.

var bindingList = new BindingList<IAttendance>(AttendanceList);
var source = new BindingSource(bindingList, null);
_View.AttendanceGrid = source;

I don't see anything particularly bad with what you've done, but perhaps I'm inexperienced or lacking larger context. The only thing I might recommend, would be to keep a bindingList around and just call the clear method instead of creating a new instance from scratch all the time. Let me be clear though, I'm not 100% sure that's good advice or any better than "newing it up".


Downloading multiple log files This code succeeds in downloading log text files from multiple URLs. Can this be made more efficient?
let private downloadLogs toDirectory =

    let createFileDirectories (url : string) =
        let directory = $@"{toDirectory}\{url.Substring(13, 3)}"

        try
            if not (Directory.Exists(directory))
            then
                Directory.CreateDirectory(directory) |> ignore
        with
            | ex -> failwith ex.Message

    let year = DateTime.Now.Year
    let month = DateTime.Now.Month.ToString("d2")
    let day = DateTime.Now.Day

    let d01LogUrlWithDate = d01LogPartialUrl + $"{year}-{month}-{day}"
    let d02LogUrlWithDate = d02LogPartialUrl + $"{year}-{month}-{day}"
    let s01LogUrlWithDate = s01LogPartialUrl + $"{year}-{month}-{day}"
    let s02LogUrlWithDate = s02LogPartialUrl + $"{year}-{month}-{day}"

    let urlWithDateList = [d01LogUrlWithDate; d02LogUrlWithDate; s01LogUrlWithDate; s02LogUrlWithDate]

    // create folder for each url file
    urlWithDateList |> List.iter createFileDirectories

    let buildFinalUrlAndRequest number url =
        try
            let finalUrl =  if number = 0 then $"%s{url}-1.log" else $"%s{url}-{number}.log"
            printfn $"Searching for URL: {finalUrl}"
            let response = Http.AsyncRequestStream(finalUrl) |> Async.RunSynchronously
            if response.StatusCode >= 200 && response.StatusCode <= 299
            then
                printfn $"URL Ready For Download: {url}"
                Ok response
            else // this is never triggered
                printfn $"Error. Status Code: {response.StatusCode}"
                Error $"Status Code: {response.StatusCode}"
        with
            | ex -> Error $"Request Error: {ex.Message}"

    let buildRequestList (state : seq<Result<HttpResponseWithStream, string>>) t =
        let range = seq{0..11}

        let responseSeq =
            seq{
                for num in range do
                    let numberUpdated = num + 1
                    let response = buildFinalUrlAndRequest numberUpdated t
                    response
            }
        responseSeq |> Seq.filter (fun x -> x |> function | Ok _ -> true | Error _ -> false) |> Seq.append state

    let getResponse urls =
        urls
        |> Seq.fold buildRequestList Seq.empty
        |> Seq.choose (fun x ->
            x |> function
                | Ok response ->
                    Some (async{
                        try
                            let fileName = Path.GetFileName(response.ResponseUrl.Replace(@"/", @"\"))
                            let subDir = response.ResponseUrl.Substring(13,3)
                            let pathName = $"""{toDirectory}\{subDir}\{fileName}"""
                            use outputFile = new FileStream(pathName, FileMode.Create)
                            do! response.ResponseStream.CopyToAsync(outputFile) |> Async.AwaitTask
                            printfn $"File Downloaded: {pathName}"
                            return pathName
                        with
                            | ex ->
                                printfn $"Download error {ex.Message}"
                                return ""
                    }) | _ -> None
        )

    urlWithDateList
    |> getResponse
    |> Async.Parallel
    |> Async.RunSynchronously |> Array.toList

There appears to be more options for parallelization and async here, and I think the fold overcomplicated what you were trying to do.
#if false
#r $"FSharp.Data"
#endif
open System.Net
open System.IO
open FSharp.Data

let  d01LogUrlWithDate, d02LogUrlWithDate, s01LogUrlWithDate, s02LogUrlWithDate = $"","","",""

module Async =
    let map f x =
        async {
            let! x' = x
            return f x'
        }
    let bind f x =
        async{
            let! x' = x
            return! f x'
        }

let private downloadLogs toDirectory =

    let createFileDirectories (url : string) =
        let directory = @"{toDirectory}\{url.Substring(13, 3)}"

        // if it is going to blow up, let it blow up keeping the entire stack trace
        if not <| Directory.Exists directory
        // I prefer strongly typed ignore, less prone to refactoring issues
        then  Directory.CreateDirectory directory |> ignore<DirectoryInfo>

    let urlWithDateList = // less clutter in scope
        let year = DateTime.Now.Year
        let month = DateTime.Now.Month.ToString("d2")
        let day = DateTime.Now.Day
        [
            d01LogUrlWithDate
            d02LogUrlWithDate
            s01LogUrlWithDate
            s02LogUrlWithDate
        ] |> List.map (fun x -> x + $"{year}-{month}-{day}")

    // create folder for each url file
    urlWithDateList |> List.iter createFileDirectories

    let buildFinalUrlAndRequest number url =
        async {

            try // this area wasn't as complex, so I left the try alone
                // I'd think this should be $"%s{url}-{number+1}.log" or else 1 would stop on zero's output
                let finalUrl = if number = 0 then $"%s{url}-1.log" else $"%s{url}-{number}.log"
                printfn $"Searching for URL: {finalUrl}"
                let! response = FSharp.Data.Http.AsyncRequestStream finalUrl
                if response.StatusCode >= 200 && response.StatusCode <= 299
                then
                    printfn $"URL Ready For Download: {url}"
                    return Ok response
                else // this is never triggered
                    printfn $"Error. Status Code: {response.StatusCode}"
                    return Error $"Status Code: {response.StatusCode}"
            with
                | ex -> return Error $"Request Error: {ex.Message}"
        }

    // if you are just throwing away the Result.Error options, then you don't need to use Results
    let buildRequestList t : seq<Async<Result<HttpResponseWithStream,string>>> =
        seq{0..11}
        |> Seq.map(fun i ->
            async {
                let numberUpdated = i + 1
                let! response = buildFinalUrlAndRequest numberUpdated t
                return response
            }
        )

    let getResponse urls =
        urls
        |> Seq.collect buildRequestList
        |> Async.Parallel
        // these two Async.map calls could be composed together
        |> Async.map(Seq.choose(function | Ok x -> Some x | _ -> None))
        |> Async.map (Seq.map (fun response ->
            async{
                let fileName = Path.GetFileName(response.ResponseUrl.Replace(@"/", "\"")) // fix syntax highlighting here on stack
                let subDir = response.ResponseUrl.Substring(13,3)
                let pathName = $@"{toDirectory}\{subDir}\{fileName}" // fix syntax highlighting here on Stack
                use outputFile = new FileStream(pathName, FileMode.Create)
                do! response.ResponseStream.CopyToAsync(outputFile) |> Async.AwaitTask
                printfn $"File Downloaded: {pathName}"
                return pathName
            }
            |> Async.Catch // this way, no try needed above
            |> Async.map( // map the catch to option, and log the errors
                function
                | Choice1Of2 x -> Some x
                | Choice2Of2 ex -> printfn $"Download error {ex.Message}"; None
            )
        ))
        |> Async.bind Async.Parallel

    urlWithDateList
    |> getResponse
    |> Async.RunSynchronously
    |> Seq.choose id // discard empty values
    |> Seq.toList


ToastUtils - helper class for Android SDK Toast Using:

ToastUtils.LONG.show(context, R.string.greeting);

Code:

public class ToastUtils {
    public static ToastUtils LONG = new ToastUtils(Toast.LENGTH_LONG);
    public static ToastUtils SHORT = new ToastUtils(Toast.LENGTH_SHORT);

    private final int duration;

    private ToastUtils(int duration) {
        this.duration = duration;
    }

    public Toast makeText(Context context, String text) {
        return Toast.makeText(context, text, duration);
    }

    public void show(Context context, String text) {
        makeText(context, text).show();
    }

    public Toast makeText(Context context, int resId) {
        return Toast.makeText(context, resId, duration);
    }

    public void show(Context context, int resId) {
        makeText(context, resId).show();
    }

}

In my opinion, there are some disadvantages of using Toast class directly:

In Toast class I often miss the call to show()
Parameter length has type int. So a compiler couldn't detect invalid argument value

What do you think? Maybe this class is useless?

Can you suggest any improvements?

IMO this class isn't very useful (at least in a current shape).

  In Toast class I often miss the call to show()

Well, it's not a bug, it's a feature. There are many situations when you only want to prepare your Toast using makeText() and show it later with show().  Moreover, you don't fix this "bug" with your class - there is still a makeText() method which does exactly what Toast used to do. And you still can forget to call show().

  Parameter length has type int. So a compiler couldn't detect invalid argument value

I wouldn't say that this is a bug, too. And you're fixing it in a bad way, again:

   public static ToastUtils LONG = new ToastUtils(Toast.LENGTH_LONG);
   public static ToastUtils SHORT = new ToastUtils(Toast.LENGTH_SHORT);

So there can only be single instances of LONG and SHORT at the time. This will make many things complicated.

To sum up, this is usage example of your class:

ToastUtils.LONG.show(context, R.string.greeting);

And this is the same without using your class:

Toast.makeText(context, R.string.greeting, Toast.LENGTH_LONG).show();

It's not a big improvement, is it?


Deleting three consecutive numbers in an array I am a beginner in programming, started about 2 months ago with Java, changing careers from law.

I have recently applied for an internship but was denied because I failed to do the pre-interview test right.

One of the tasks was to keep deleting 3 consecutive same integers from the array until the list is either empty or has no 3 consecutive same numbers. The assignment can be found here.

Below a description of the task with examples:

  Three brothers walk into a bar. All the beverages are placed in one
  line at the long bar table. The size of each glass is represented in
  an array of integers, glasses. The brothers will drink a round if they
  can find 3 consecutive glasses of the same size. The barman removes
  the empty glasses from the table immediately after each round. Find
  the maximum number of rounds the three brothers can drink.

  For glasses = [1, 1, 2, 3, 3, 3, 2, 2, 1, 1], the output should be
  brothersInTheBar(glasses) = 3. The brothers can start with a round of
  size 3, then after the glasses are cleared, a round of size 2 can be
  formed, followed by a round of size 1. One glass will be left at the
  table.

  For glasses = [1, 1, 2, 1, 2, 2, 1, 1], the output should be
  brothersInTheBar(glasses) = 0. There are no 3 consecutive glasses of
  the same size.

This is my code:

    public int brothersInTheBar(int[] glasses){

    if(glasses.length<=2)
        return 0;

    ArrayList<Integer> glassesAsList = new ArrayList<Integer>();
    Arrays.stream(glasses).forEach(i -> glassesAsList.add(i));

    int counter=0;
    for (int x=0;x<glassesAsList.size()-2;++x){

        if(glassesAsList.get(x) == glassesAsList.get(x+1) && glassesAsList.get(x) == glassesAsList.get(x+2)){
            glassesAsList.remove(x);
            glassesAsList.remove(x);
            glassesAsList.remove(x);
            counter++;
            x=-1;
        }
        if(glassesAsList.isEmpty())
            break;
    }
 return counter;
 }

I sent the email back asking what I did wrong, so I can improve in the future. The response from the company only said that I have to look more into algorithm complexity and that my code is inefficient.

So my question is how to improve my algorithm and what is wrong with it?

Remove(x) deletes the element and then shifts every element up so it alone is an O(n) operation making your entire algorithm O(n^2). I actually wrote a removeAtFast(x) which swaps the item being removed and the last element then removes the last element normally. But you can't do this because it messes up the ordering.

I'd create a new array like what Pure Evil suggested. Just go through the elements and if you find 3 consecutive numbers skip 3 ahead and continue copying.

Edit: Also it would appear your code would fail the first example given in the assignment.


random_iterator in C++ This is a small utility that permutes a C++ standard container without modifying the container being iterated:

random_iterator.hpp

#ifndef RANDOM_ITERATOR_HPP
#define RANDOM_ITERATOR_HPP

#include <initializer_list>
#include <random>
#include <vector>

namespace net {
namespace coderodde {
namespace utilities {

template<typename T>
class random_iterator {
private:
    std::vector<T> vec;

public:

    template<typename Iterable>
    explicit random_iterator(Iterable iterable)
    {
        for (auto& x : iterable)
        {
            vec.push_back(x);
        }

        std::random_device rd;
        std::mt19937 g(rd());
        std::shuffle(vec.begin(), vec.end(), g);
    }

    template<typename Iterable>
    explicit random_iterator(Iterable iterable, std::random_device& rd)
    {
        for (auto& x : iterable)
        {
            vec.push_back(x);
        }

        std::mt19937 g(rd());
        std::shuffle(vec.begin(), vec.end(), g);
    }

    explicit random_iterator(std::initializer_list<T>& lst)
    {
        for (auto& x : lst)
        {
            vec.push_back(x);
        }

        std::random_device rd;
        std::mt19937 g(rd());
        std::shuffle(vec.begin(), vec.end(), g);
    }

    explicit random_iterator(std::initializer_list<T>& lst,
                             std::random_device& rd)
    {
        for (auto& x : lst)
        {
            vec.push_back(x);
        }

        std::mt19937 g(rd());
        std::shuffle(vec.begin(), vec.end(), g);
    }

    typename std::vector<T>::iterator begin()
    {
        return vec.begin();
    }

    typename std::vector<T>::iterator end()
    {
        return vec.end();
    }

    typename std::vector<T>::const_iterator begin() const
    {
        return vec.cbegin();
    }

    typename std::vector<T>::const_iterator end() const
    {
        return vec.cend();
    }
};

} // End of net::coderodde::utilities
} // End of net::coderodde
} // End of net
#endif // RANDOM_ITERATOR_HPP

main.cpp

#include "random_iterator.hpp"
#include <iostream>
#include <set>
#include <vector>

using namespace std;
using net::coderodde::utilities::random_iterator;

int main(int argc, const char * argv[]) {
    vector<int> v1 {1, 2, 3, 4, 5, 6, 7, 8, 9, 10};

    for (auto& x : random_iterator<int>(v1))
        cout << x << " ";

    cout << "\n";

    set<int> s1 {1, 2, 3, 4, 5};

    for (auto& x : random_iterator<int>(s1))
        cout << x << " ";

    cout << "\n";
}

Critique request

I would like to know, how a professional C++ programmer would implement this.

Naming

First of, what you have created is not an iterator so naming it as such is misleading. I would recommend a name but I actually think that you should redesign your class and the name I would recommend wouldn't be fit for the redesigned class so I'll wait with that until the end.

Design

In C++ as opposed to Java, when you assign an object you get a copy of the object which can be slow and it occupies extra memory for the copy.

I believe that there are two ways that you can improve this design:

Design a proper iterator that fulfills the contract of being an Iterator. It takes some doing but when you're done you it will not copy the contents and will be generally usable. One pitfall is how to define the end of the iterator.
Design a "view" over an existing container. This may or may not be more convenient depending on the use case.

The two ideas are kind of related but different concepts. You can store a view as a member and it makes sense but storing an iterator as a member is kind of dubious. Depending on your use case either a view or iterator is better, for this particular case I have a feeling that an iterator is better.

Implementation sketch as an iterator

Here I'll outline a start of an implementation of a random iterator and add a utility function in_random_order which provides nice syntactic sugar for use in range based for loops. I guess you could think of in_random_order as a poor-man's temporary view. Anyway here we go:

template<typename It>
class random_iterator{
    using it_traits = std::iterator_traits<It>;
public:
    // You must always provide these for all iterators
    // or explicitly specialise std::iterator_traits
    // for your iterator. It's easier to just provide them here.
    using iterator_category = it_traits::iterator_category;
    using value_type = it_traits::value_type;
    using difference_type = it_traits::difference_type;
    using pointer = it_traits::pointer;
    using reference = it_traits::reference;

    // This stores one iterator to each iterator position in the original
    // container. Any modification of the original container invalidates
    // this iterator. We could use an LCG to avoid the linear memory.
    random_iterator(It first, It last){
        while(first != last){
            its.emplace_back(first);
            ++first;
        }
        // Shuffle the its vector.
    }

    random_iterator& operator ++(){
        ++it;
        return *this;
    }

    // Depending on if you want an input iterator, forward iterator,
    // bidirectional or random access iterator you need to implement
    // various methods here. Check the concepts page linked earlier
    // for what you must provide.

    reference operator *(){
        return *(*it); // First de-ref gets original iterator
    }

private:
    std::vector<It> its;
    std::vector<It>::iterator it;
};

template<typename Iterator>
class random_order{
public:
    in_random_order(Iterator f, Iterator l){
        first = f;
        last = l;
    }

    random_iterator<Iterator> begin(){
        return random_iterator(first, last);
    }

    random_iterator<Iterator> end(){
        return random_iterator(); // Figure out how to properly make end
    }

private:
    Iterator first;
    Iterator last;
};

// These functions are necessary because template argument deduction is not
// done for class templates. This is the final piece of the puzzle
// that enables the syntax below.
template<typename Container>
random_order in_random_order(Container& c){
    return random_order<Container::iterator>(c.begin(), c.end());
}

template<typename Iterator>
random_order in_random_order(Iterator first, Iterator last){
    return random_order<Container::iterator>(first, last);
}

When the above is properly implemented you should be able to do:

std::vector<X> data;

for(auto& value : in_random_order(data)){
    // stuff
}

// Or just a range:
for(auto& value : in_random_order(data.begin() + 10, data.begin() + 20)){
    // stuff
}

Note that for range based for to work, the object to be iterated over must provide member methods begin and end; or there must exist specialisations of std::begin and std::end for the object that is being iterated over.

This is why we invent the random_order class, it basically just provides the necessary boilerplate to make the fancy syntax possible in the for loop.

In fact, if you implement different types of iterators like for example filtering iterators and suitable iterables like filter which provide the requisite begin/end methods, you can do all manners of fun stuff by nesting iterators types like for example:

for(auto& : in_random_order(filter(data,
                [](const X& x) { return x.isHappy();})) {

Although you can do the same with views I'd think the syntax is a bit more cumbersome.

Note: The above code uses a vector of indexes which is undesirable for an iterator. Iterators should ideally be a lightweight objects and are often assumed to be, for example they are often passed by value. This makes code that uses the random iterator slightly inefficient. This is worth keeping in mind or looking for a solution that doesn't need a lot of memory like this implementation does.


Spring Service Method Logic I am learning spring and am wondering about this service class's method. Is there a way to split this method up? Is there a way to use spring framework to make this method more readable? Should I move any of it to the controller or the DAO or should I split it into more service methods?

// UserBean has fields: username, password, first name, last name, etc.
// The UserBean parameter has 2 non-null fields: username and password.
public UserBean getUserDetails(UserBean bean) {
    UserEntity entity = new UserEntity();
    BeanUtils.copyProperties(bean, entity);

    // Gets user from DB based on UserBean's username and password.
    // Returns null if not found.
    entity = this.userDao.getUser(entity);

    if (entity == null) {
        return null;
    }

    // Fill in the rest of the UserBean's data from the DB
    UserBean userBean = new UserBean();
    BeanUtils.copyProperties(entity, userBean);
    // calculate time since last login based on DB timestamp
    userBean.setTimeSinceLastLogin(this.getSecondsSinceLastLogin(entity.getLastLogin()));

    // update DB with new timestamp
    entity.setLastLogin(new Date());
    this.userDao.updateUser(entity);

    return userBean;
}

Perhaps if this method:

this.userDao.getUser(entity);

Accepted just a String as input (the username?) and possibly a password, you could then call it without having to mess around creating the UserEntity beans.

So this:

UserEntity entity = new UserEntity();
BeanUtils.copyProperties(bean, entity);

// Gets user from DB based on UserBean's username and password.
// Returns null if not found.
entity = this.userDao.getUser(entity);

Would become

UserEntity entity = this.userDao.getUser(bean.getUserName());

Maybe rename the entity/bean variable too to some thing that makes it a bit clearer what they are.

Also in the second half of the method, there seems to be no need to copy the properties to a UserBean and then back again. Just update the time since last login on the UserEntity directly.


Multi platform ASCII spinner in C Okay I've just finished writing a nice tiny C program to have an ASCII spinner (works on Windows and POSIX). I'm quite new to C, so any suggestions would be great. This is designed to be a small function in my other programs, so please point out anything that could cause problems when been integrated into a larger program.

The only part I am not 100% sure about is how I handle the standard output buffer when it's printed the next character in the cycle. I don't want the buffer to fill up with invisible backspace characters, so is using fflush on stdout the best way?

#ifdef _WIN32
    #include <windows.h>
    #define sleepThr(x) Sleep(x)
#else
    #include <unistd.h>
    #define sleepThr(x) usleep((x) * 1000)
#endif

#include <stdio.h>

#define INTERVAL 200

int main()
{
    int counter = 0;

    while (1)
    {
        switch (counter)
        {
            case 0:
                printf("\b|");
                break;

            case 1:
                printf("\b/");
                break;

            case 2:
                printf("\b-");
                break;

            case 3:
                printf("\b\\");
                counter = -1;
                break;
        }

        fflush(stdout);

        sleepThr(INTERVAL);

        counter++;
    }

    return 0;
}

Using fflush is pretty much required as output would otherwise be buffered until a newline character is printed.

You could use counter as an index to a character array instead of a big clunky repetitive switch. If you have a char *spinner = "|/-\\";, spinner[0] will be "|", spinner[1] will be "/", etc.

char *spinner = "|/-\\"; printf("\b%c", spinner[counter]); is going to print the common backspace character and the correct "spinner symbol" for each value of counter.  The spinner variable does not need to be declared because it's a string literal: printf("%c", "|/-\\"[counter]); can be used instead.

Apart from the unnecessary switch, I'd say you have an excessive amount of blank lines. If they are used more scarcely, their relative effect will be greater. Readability becomes more important as the program gets bigger.

/*
 * Separated groups of code:
 * If there were empty lines between all statements, it would be
 * difficult to tell the groups apart.
 */

foo(bar->baz);
foo(bar->quux);

counter++;
counter %= 4;

Here's a modified version:

#ifdef _WIN32
    #include <windows.h>
    #define sleepThr(x) Sleep(x)
#else
    #include <unistd.h>
    #define sleepThr(x) usleep((x) * 1000)
#endif

#include <stdio.h>

#define INTERVAL 200

int main()
{
    int counter;
    while (1)
    {
        for (counter = 0; counter < 4; counter++)
        {
            printf("\b%c", "|/-\\"[counter]);
            fflush(stdout);
            sleepThr(INTERVAL);
        }
    }

    return 0;
}


Cryptographic key exchange I initially posted this on StackOverflow, and was redirected here.
I have been playing around with Bouncy Castle (Java) for the last few days, and I've reached the point where I believe I can securely exchange secret keys with a Diffie-Hellman exchange.

Having read many posts underlining the difficulty of properly implementing a cryptographic exchange, I would like your honest opinion on my work.
All the underlying cipher operations are based on Bouncy Castle and may therefore be deemed reliable.

String message = "Hello World";

AESCipher aes_client = new AESCipher(256);
RSACipher rsa_server = new RSACipher(2048);

// (Public key sent over the wire)
RSACipher rsa_client = new RSACipher(rsa_server.getPublicKey().getModulus(),
                                     rsa_server.getPublicKey().getExponent());

// The client encodes his AES key with the RSA public key:
byte[] aes_key = rsa_client.encode(aes_client.getKeyBytes());
byte[] aes_iv = rsa_client.encode(aes_client.getInitializationVector());

// (Data sent back over the wire)
byte[] decoded_aes_key = rsa_server.decode(aes_key);
byte[] decoded_aes_iv = rsa_server.decode(aes_iv);

// The server creates an AES server which uses the client key:
AESCipher aes_server = new AESCipher(decoded_aes_key, decoded_aes_iv);

byte[] encoded_message = aes_client.encode(message.getBytes());
byte[] decoded_message = aes_server.decode(encoded_message);

System.out.println(new String(decoded_message));

Can this exchange be considered safe? Should I be sticking with SSL Sockets, eventhough it'd hurt to thrash my hard work?
Thanks in advance for your input!

(By the way, my Bouncy-Castle-Wrapping-Library is totally open-source, so just let me know if you want the repository's URL.)

It's very difficult to answer you, because what you've posted is a test-bed where your byte arrays are passed around in the context of a high-level language, as opposed to a real situation where you'd have (at the very least) two processes communicating down a pipe, TCP socket or similar. The details of how you do this are crucial; getting them wrong will destroy your security just as easily as if you get the crypto wrong.

I haven't thought about your code very much, but these questions immediately come to mind:

How are you sending and receiving data over the wire - how are your packets delimited and parsed?
How are the crypto keys being generated - specifically, what's their random source?
How are keys being stored at either end?
What RSA padding and AES mode are you using?
How do you know that the key you've got really belongs to the person you think it does, and not some man in the middle? In other words, how do you establish trust in that key?

As others have said, you should be reusing an existing library to use a well-established protocol. You might care to read the stick figure guide to AES: pay particular note to the Foot-Shooting Prevention Agreement; what goes for not inventing your own crypto algorithms, also goes for protocols.


Unwrapping optional properties from an optional array of objects I'm currently working with an SDK where methods return an optional array of objects with optional properties.  Only the array itself is optional, it will either be nil or always contain non-optionals.  I'm struggling to find a cleaner way to unwrap optional properties for each non-optional in the array:

class Person {
    var id: String?
}

func someOptionalArrayOfPeople() -> [Person]? {
    // returns either nil or some array of Person objects
}

let people = someOptionalArrayOfPeople()

if let unwrappedPeople = people {

    // 1
    for person in unwrappedPeople {
        if let id = person.id {
            print(id) // too many nested statements
        }
    }

    // 2
    for person in unwrappedPeople where person.id != nil {
        print(person.id!) // stil force unwrapping
    }
}

For option 1, I'm not thrilled about unwrapping the initial optional array, and I feel that there should be a way to skip the unwrapping of the id.  Option 2 attempts to do that with a where clause, but the compiler isn't smart enough to determine that person.id is non-nil even if we explicitly check that id != nil.

What are my "Swifty" options for unwrapping optional properties on an optional array of objects?  I am using Swift 2.2 with Xcode 7.3.1.

There are several options, but in any case you can use

public func flatMap<T>(@noescape transform: (Self.Generator.Element) throws -> T?) rethrows -> [T]

which returns an array containing the non-nil results of applying the
transformation to all sequence elements.

For example

if let unwrappedPeople = people {
    for id in unwrappedPeople.flatMap({ $0.id }) {
        print(id)
    }
}

to iterate over the non-nil ids, or

if let unwrappedPeople = people {
    let ids = unwrappedPeople.flatMap { $0.id }
    print(ids)
}

to get the non-nil ids as an array.

To get rid of the outer unwrapping/optional binding, you can use
optional chaining:

people?.flatMap({ $0.id }).forEach {
    print($0)
}

If people is nil, then flatMap() is not applied. Otherwise
it returns an array of the non-nil ids, which are enumerated with
forEach().

I think that all these methods (as well as your first method) are "Swifty",
you have to decide how compact your code should be.


Multithreaded HiLo game For my CS class I have to write a HiLo game using multithreading.  I am brand new to multithreading and not sure how to best implement it.  The program below works, but I am wondering if there is a better way to do it.  When it is run the user will input an int, which is the amount of time that they will have to guess the correct number.  If the timer runs out the game will end.  I am not supposed to use the Timer object but instead use the System.currentTimeMillis().

import java.util.Random;
import java.util.Scanner;

class Game implements Runnable {
    private static long time;
    private long timer;
    private static long gameTime;

    public Game(int n){
        gameTime=n;
    }

    public void run() {
        time=System.currentTimeMillis();
        while(true){
            timer=(System.currentTimeMillis()-time)/1000;
            if(timer>=gameTime){
                System.out.println("Oops! Time is up - try again.");
                break;
            }
        }
    }
}

public class Hilo {

    public static void main(String[] args) {
        if (args.length!=1){
            System.err.println("Must enter time");
        }
        Random rand = new Random();
        int max=100;
        int min=1;
        int number=rand.nextInt((max-min)+1)+min;
        int gameTime=Integer.parseInt(args[0]);
        System.out.println("Welcome to HiLo!");
        System.out.println("You have "+gameTime+" seconds to guess a number between 1 and 100.");
        Thread clock1 = new Thread(new Game(gameTime));
        clock1.start();

        while(clock1.isAlive()==true){
            System.out.println(">");
            Scanner sc = new Scanner(System.in);
            int input = sc.nextInt();

            if(clock1.isAlive()==true&&input==number){
                System.out.println("You Win!");
                break;
            }else if(clock1.isAlive()==true&&input<number){
                System.out.println("Higher!");
            }else if(clock1.isAlive()==true&&input>number){
                System.out.println("Lower!");
            }
        }
    }
}

Spacing
First of all, yourcodeispackedverytightly, try using spaces a bit more. This is preferred:
timer = (System.currentTimeMillis() - time) / 1000;
int number = rand.nextInt((max - min) + 1) + min;

Static variables
private static long time;
private static long gameTime;

I see absolutely no need to have these as static. As it is right now, your code would get problematic if you instantiated one Game, waited for a while, and then created another game. Each Game should have it's own time and gameTime values.

Run and while (true)
public void run() {
    time=System.currentTimeMillis();
    while(true){
        timer=(System.currentTimeMillis()-time)/1000;
        if(timer>=gameTime){
            System.out.println("Oops! Time is up - try again.");
            break;
        }
    }
}

So here you have a repeating loop that does more or less absolutely nothing as fast as it can. As you already know how long you should wait, use Thread.sleep(delay) instead.

==true
while(clock1.isAlive()==true){

Using ==true is redundant, simply this is enough:
while (clock1.isAlive()) {

Scanner resource
Scanner sc = new Scanner(System.in);

This line should give you a compiler warning because you're not closing the scanner. Consider using a try-with-resources statement.
You only need to create the scanner once, when you start the program. Then you can close it safely at the end of your program.

if, if, if...
if(clock1.isAlive()==true&&input==number){
    System.out.println("You Win!");
    break;
}else if(clock1.isAlive()==true&&input<number){
    System.out.println("Higher!");
}else if(clock1.isAlive()==true&&input>number){
    System.out.println("Lower!");
}

Do you see any common things there? clock1.isAlive() is being checked on every if. Sure, it is possible that it is alive in the first check and then dies before the second if, but that is very unlikely and we're talking about microseconds of difference there. Do the clock1.isAlive check once, and then check compare with the number.
if (clock1.isAlive()) {
    if (input == number) {
        System.out.println("You Win!");
        break;
    } else if(input < number) {
        System.out.println("Higher!");
    } else if(input > number) {
        System.out.println("Lower!");
    }
}


Playing card class, written using enums I wrote a card class a while back in this post here: Previous Question
I know it has been a long time but I recently came back to the project and wrote it using an enum as suggested in the answer to increase readability and to make input less error prone. I've never really used enums in Python so the question is did I do it correctly or is there a better way to do it?

from enum import Enum

class Suit(Enum):
    CLUB, HEART, DIAMOND, SPADE = range(1, 5)

class Rank(Enum):
    TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, NINE, TEN = range(2,11)
    JACK, QUEEN, KING, ACE = 'J', 'Q', 'K', 'A'

class Card(object):
    """Models a playing card, each Card object will have a suit, rank, and weight associated with each.

    possible_suits -- List of possible suits a card object can have
    possible_ranks -- List of possible ranks a card object can have
    Suit and rank weights are initialized by position in list.
    If card parameters are outside of expected values, card becomes joker with zero weight
"""

    def __init__(self, suit, rank, in_deck = False):
        if suit in Suit and rank in Rank:
            self.suit = suit
            self.rank = rank
            self.suit_weight = suit.value
            self.rank_weight = rank.value
        else:
            self.suit = "Joker"
            self.rank = "J"
            self.suit_weight = 0
            self.rank_weight = 0
        self.in_deck = in_deck

    def __str__(self):
        """Returns abbreviated name of card

        Example: str(Card('Spades', 'A') outputs 'AS'
        """
        return str(self.rank.value) + str(self.suit.name[0])

    def __eq__(self, other):
        """Return True if cards are equal by suit and rank weight"""
        return self.suit_weight == other.suit_weight and self.rank_weight == other.rank_weight

    def __gt__(self, other):
        """Returns true if first card is greater than second card by weight"""
        if self.suit_weight > other.suit_weight:
            return True
        if self.suit_weight == other.suit_weight:
            if self.rank_weight > other.rank_weight:
                return True
        return False

    def modify_weight(self, new_suit_weight = None, new_rank_weight = None):
        """Modifies weight of card object"""
        if new_suit_weight:
            self.suit_weight = new_suit_weight
        if new_rank_weight:
            self.rank_weight = new_rank_weight

    def is_in_deck(self):
        """Return True if card is in a deck, else false"""
        return self.in_deck

    def get_suit(self):
        return self.suit

    def get_rank(self):
        return self.rank

    def get_suit_weight(self):
        return self.suit_weight

    def get_rank_weight(self):
        return self.rank_weight

Python is not Java (or C++, or ...). There is no need to have a getter and setter for every attribute.

Instead of calling card.modify_weight(new_suit_weight=3), you can just do card.suit_weight = 3, so that method is not needed at all.
Instead of calling card.get_suit(), just use card.suit, so all those get_* methods are superfluous.

The rest looks fine to me, except for the closing part of the docstring of Card, those """ should be indented the same distance the opening """ are.


How can I check if a sequence is alternating? I need to write a program that reads a sequence of integers and determines if the sequence is alternating. An alternating sequence is a sequence of numbers in which every number should be greater or smaller than the one before it according to the base alternation. For example, 1 5 4 8 2 10 is alternating.

I wrote this program but I am not sure it is 100% correct.

import java.util.Scanner;

  public class alternating {

 public static void main(String[] args) {
    // TODO Auto-generated method stub
Scanner scan = new Scanner(System.in);
int cases = scan.nextInt();
for (int testcase = 0; testcase < cases; testcase++) {
    int n = scan.nextInt();
    int[] sequence = new int[n];
    for (int i = 0; i < sequence.length; i++)
        sequence[i] = scan.nextInt();
    if (alternating(sequence))
        System.out.println("alternating");
    else
        System.out.println("not alternating");
}

  }

  public static boolean alternating(int[] sequence) {
   for (int i = 0; i < sequence.length - 2; i++) {
    if (sequence[i] <= sequence[i + 1]) {
        if (sequence[i + 1] <= sequence[i + 2])
            return false;
    } else {
        if (sequence[i] >= sequence[i + 1])
            if (sequence[i + 1] >= sequence[i + 2])
                return false;

    }

}
return true;
     }

      }

It's not correct for short sequences with less than 3 items.
The method incorrectly returns true for these example sequences:

empty sequence
singleton list (one item)
sequence with two items that are equal

For the first 2 cases you might even want to throw IllegalArgumentException as arguably it doesn't make much sense to test for alternation in lists with 0 or 1 item, but I think returning false is reasonable.

You can do this by adding these conditions at the start of the method:

    if (sequence.length < 2) {
        return false;
    }
    if (sequence.length == 2) {
        return sequence[0] != sequence[1];
    }

But actually I would simplify the body of the for loop like this:

    if (sequence.length < 2) {
        return false;
    }
    int prevCompare = Integer.compare(sequence[0], sequence[1]);
    if (prevCompare == 0) {
        return false;
    }
    for (int i = 1; i < sequence.length - 1; ++i) {
        int compare = Integer.compare(sequence[i], sequence[i + 1]);
        if (prevCompare != -compare) {
            return false;
        }
        prevCompare = compare;
    }
    return true;

To verify the implementation I recommend adding some unit tests, for example:

@Test
public void testEmptyList_Is_NotAlternating() {
    assertFalse(alternating(new int[0]));
}

@Test
public void testSingletonList_Is_NotAlternating() {
    assertFalse(alternating(new int[]{1}));
}

@Test
public void test2Items_Increasing_Is_Alternating() {
    assertTrue(alternating(new int[]{1, 2}));
}

@Test
public void test2Items_Decreasing_Is_Alternating() {
    assertTrue(alternating(new int[]{3, 2}));
}

@Test
public void test2Items_Same_Is_NotAlternating() {
    assertFalse(alternating(new int[]{3, 3}));
}

@Test
public void test3Items_Same_Is_NotAlternating() {
    assertFalse(alternating(new int[]{3, 3, 3}));
}

@Test
public void test4Items_Same_Is_NotAlternating() {
    assertFalse(alternating(new int[]{3, 3, 3, 3}));
}

@Test
public void test3Items_Increasing_Is_NotAlternating() {
    assertFalse(alternating(new int[]{3, 4, 5}));
}

@Test
public void test4Items_Increasing_Is_NotAlternating() {
    assertFalse(alternating(new int[]{3, 4, 5, 6}));
}

@Test
public void test3Items_Decreasing_Is_NotAlternating() {
    assertFalse(alternating(new int[]{43, 34, 25}));
}

@Test
public void test4Items_Decreasing_Is_NotAlternating() {
    assertFalse(alternating(new int[]{43, 34, 25, 16}));
}

@Test
public void test4Items_AlternatingThenMonotonic_Is_NotAlternating() {
    assertFalse(alternating(new int[]{43, 34, 35, 16, 16, 16}));
}

@Test
public void test4Items_Alternating_Is_Alternating() {
    assertTrue(alternating(new int[]{43, 34, 35, 16}));
}


Defining constants in JavaScript I am aware JavaScript does not have the concept of constant variables, which in traditional languages, you would usually define as being static/class related since there's no point in having them defined in each object instance.

So we have to make do with defining regular vars in uppercase, and just treating those like constants.

But I wondered whether there's a particular style to make it clearer to other developers that this variable is a constant, other than to define it in uppercase. I have the following plugin I am writing:

    (function () {
    var URL_KEY = 'CurrentURL',
        appName = 'MyApp',
        directoryName = 'MyAppDir';

    function GUID() {
        function s4() {
            return Math.floor((1 + Math.random()) * 0x10000)
                .toString(16)
                .substring(1);
        }

        return s4() + s4() + '-' + s4() + '-' + s4() + '-' + s4() + '-' + s4() + s4() + s4();
    }

    function setURL(domain) {
        if (!domain) {
            throw 'domain parameter required';
        }

        localStorage.setItem(URL_KEY, domain);
    }

    function getURL() {
        return localStorage.getItem(URL_KEY) || null;
    }

    window.App = window.App || {
        name: appName,
        directory: directoryName,
        createGUID: function () {
            return GUID();
        },
        getURL: function () {
            return getURL();
        },
        setURL: function (newURL) {
            setURL(newURL);
        }
    };
}());

My constant currently is URL_KEY - which is blending into the regular variables which is confusing. Is there any particular style recommended to separate the constants and the regular variables?

I am aware there is just one variable currently, but as this module grows there will be more.

You could a least separate the variables declarations into constants and variables:

var URL_KEY = 'CurrentURL';

var appName = 'MyApp',
    directoryName = 'MyAppDir';

You could alternatively make a function that returns the constant.

function URL_KEY() { return 'CurrentURL'; }

Replacing a function (like URL_KEY = function() {...}) might be more of a mental barrier for some.


Generic function for loading a function from a DLL library I am trying to write a generic function for loading a function from a DLL library. I am in no way an expert on DLLs, that is why I ask.

We have discussed it first in my Stack Overflow question to discover the most obvious errors, and there were some crucial ones.

Current version of my code follows:

function LoadFunctionFromLibrary(const LibraryName, FunctionName: string; out FunctionPointer: Pointer): Boolean;

var
  LibraryHandle: THandle;

begin
  Result := False;
  FunctionPointer := nil;

  LibraryHandle := Winapi.Windows.LoadLibrary(PChar(LibraryName));
  if LibraryHandle = 0 then Exit;

  FunctionPointer := Winapi.Windows.GetProcAddress(LibraryHandle, PChar(FunctionName));

  if Assigned(FunctionPointer) then
    Result := True;
end;

This version of the function has been successfully tested on my EnableInput function:

function EnableInput(const Enable: Boolean): Boolean;

var
  BlockInput: function(Block: BOOL): BOOL; stdcall;

begin
  Result := LoadFunctionFromLibrary('User32.dll', 'BlockInput', @BlockInput) and
            BlockInput(not Enable);
end;

Just like Dangph, I wonder why you need this. There are other better approaches suggested in the previous comments.

Even if this is useful, Your solution is not great:
 1. You repeatedly load the library and the function.
 2. There is no way for you to free the loaded libraries.

Here is a better way to do it:

type
  TFunctionLoader = class
  private
    FLibraries: TStrings;   // This stores the library handles and names
    FFunctions: TStrings;   // This stores the function pointers and names
  public
    constructor Create;
    destructor Destroy; override;
    function LoadFunction(const LibraryName, FunctionName: string;
                          out FunctionPointer: Pointer): Boolean;
  end;

{ TFunctionLoader }

constructor TFunctionLoader.Create;
begin
  FLibraries := TStringList.Create;
  FFunctions := TStringList.Create;
end;

destructor TFunctionLoader.Destroy;
var
  i: Integer;
begin
  for i := 0 to FLibraries.Count - 1 do     // free all opened libraries
    Windows.FreeLibrary(THandle(FLibraries.Objects[i]));
  FLibraries.Free;                          // free other objects
  FFunctions.Free;
end;

function TFunctionLoader.LoadFunction(const LibraryName, FunctionName: string;
  out FunctionPointer: Pointer): Boolean;
var
  i: Integer;
  LibraryHandle: THandle;
begin
  i := FFunctions.IndexOf(FunctionName);    // Is the function already loaded?
  if i >= 0 then
  begin                                     // Yes, just return the stored pointer
    FunctionPointer := Pointer(FFunctions.Objects[i]);
    Exit(True);
  end;

  i := FLibraries.IndexOf(LibraryName);     // No, test if the library is already loaded
  if i < 0 then
  begin                                     // No, load it and store in FLibraries
    LibraryHandle := Windows.LoadLibrary(PChar(LibraryName));
    if LibraryHandle = 0 then Exit(False);  // Failed, quit
    i := FLibraries.AddObject(LibraryName, TObject(LibraryHandle));
  end;

  // Load the function from the library
  FunctionPointer := Windows.GetProcAddress(THandle(FLibraries.Objects[i]),
                                            PChar(FunctionName));
  Result := Assigned(FunctionPointer);      // succeeded?
  if Result then                            // Add the function to FFunctions
    FFunctions.AddObject(FunctionName, TObject(FunctionPointer));
end;

With above you can define var FunctionLoader: TFunctionLoader; somewhere and add this to the end of the unit

initialization
  FunctionLoader := TFunctionLoader.Create;
finalization
  FunctionLoader.Free;
end.

Use it as

Result := FunctionLoader.LoadFunction('User32.dll', 'BlockInput', @BlockInput)
          and BlockInput(not Enable);

All functions and libraries will be loaded only once and will be automatically freed.


Convert object array to hash map using lodash The use case is to convert an array of objects into a hash map where one property is the key and the other property is the value. Common case of using this is converting a "link" object in a hypermedia response into a hash map of links.

jsfiddle

function toHashMap(data, name, value) {
    return _.zipObject(_.pluck(data, name),
                       _.pluck(data, value));
}

function toMap(data, name, value) {
    return _.reduce(data, function(acc, item) {
        acc[item[name]] = item[value];
        return acc;
    }, {});
}

var data = [
    { rel: 'link1', href: 'url1'},
    { rel: 'link2', href: 'url2'},
    { rel: 'link3', href: 'url3'},
    { rel: 'link4', href: 'url4'},
];

console.log(toHashMap(data, 'rel', 'href'));

console.log(toMap(data, 'rel', 'href'));

toHashMap appears much more readable, but less efficient. toMap seems to have the best efficiency at the expense of readability.

Is there a more elegant solution - i.e. both readable and efficient? I suspect that Lodash might have a function to do this already, but I haven't found anything like this after looking at the API documentation.

I think you are looking for _.keyBy (or _.indexBy in older versions)

_.keyBy(data, 'rel');


Part 2 of blackjack game I posted the first part of this game earlier, and now I finished the second part.

From the first part we just had to have a random value to get the cards, and the bet always stayed at $25. For the second part the user could choose their bet, enter 0 to quit the game completely, and just hit enter to default to 25 on their first round, or can stay with their bet from the previous round.

My code is super messy and very long. I know it can be cleared up and made much shorter as I've seen people with around 70 lines of code.

import random

def main():
  restart = True

  bank_balance = 1000
  player_name = input("Please enter your name: ")
  #Default bet
  user_bet=25
  while restart:

    print (f"Welcome {player_name}, your bank balance is ${bank_balance} ")

    bet = input_bet(user_bet, bank_balance)

    #Returned value from input_bet to stop the program
    if (bet == -10):
      break

    win_lose = play_hand(player_name, bet)
    bank_balance+=win_lose

    print(f'Your bank balance: ${bank_balance}')

def input_bet(bet, money):
  correct = False
  while not correct:
    if (money <=0):
      print('You ran out of money')
      return -10

    str_bet=input("Bet? (0 to quit, press 'Enter' to stay at $25) ")

    if (str_bet == ''):
      print(f'Staying at ${bet}')
      return bet

    try:
      enough_money = False
      while not enough_money:
        bet=int(str_bet)
        if (bet > money):
          print('not enough money')
          str_bet=input("Bet? (0 to quit, press 'Enter' to stay at $25) ")
          bet=int(str_bet)
        if (bet < 0):
          print('Enter a number greater than 0')
          str_bet=input("Bet? (0 to quit, press 'Enter' to stay at $25) ")
          bet=int(str_bet)
        elif (bet == 0):
          return 0
        elif (bet <= money):
          print(f'Betting ${bet}')
          enough_money=True
          return bet

      correct = True

    except ValueError:
      print('Please enter a whole number')

def play_hand(name, bet):
  player= []
  dealer= []

  play_again = True

  dealer.append(random.randint(1, 11))

  player.extend([random.randint(1, 11), random.randint(1, 11)])

  print ('The dealer received card of value', *dealer)
  print(name, 'received cards of value', player[0], 'and', player[-1])
  print(f'Dealer total is {sum(dealer)}')
  print(f"{name}'s total is {sum(player)}", '\n')

  stay = False
  bust = False

  while (sum(player) <= 21 and stay == False and play_again == True):
    hors= input(f"Type 'h' to hit and 's' to stay ")
    if (hors == 'h'):
      new_card= random.randint(1, 11)
      player.append(new_card)
      print(f'{name} pulled a {new_card}')

      print(f'Dealer total is {sum(dealer)}')
      print(f"{name}'s cards are", *player)
      print(f"{name}'s total is {sum(player)}", '\n')

    elif (hors == 's'):
      stay=True
      print('stay')

  if (sum(player) > 21 ):
    bust = True
    print('You busted!')
    return -bet

  while (stay == True and sum(dealer) < 17 and bust == False and play_again == True):
    dealer.append(random.randint(1, 11))
    print('The dealers cards are', *dealer)
    print('The dealers total is', sum(dealer), '\n')

  if (sum(dealer) <= 21 and sum(dealer) > sum(player)):
    print("The dealer wins!")
    return -bet

  elif (sum(player) <= 21 and sum(player) > sum(dealer)):
    print("You win!")
    return bet

  if (sum(dealer) > 21):
    print ('You win! The dealer busted!')
    return bet

  if (sum(dealer) == sum(player)):
    print('Its a Tie! ')
    return 0

main()

Indentation

You should indent 4 spaces. Here's the PEP 8 guidelines on this.

Equality

Instead of

while (sum(player) <= 21 and stay == False and play_again == True):

do this

while(sum(player) <= 21 and not stay and play_again):

You don't need to == True/False, as those variables will equate to those boolean values already within the statement. This applies to other parts of your program as well.

Operator spacing

There should be a space before and after every operator (+ - * / =, etc.) in your program. This increases readability and makes your code look nicer.

Unnecessary parentheses

You don't need parentheses around simple if statements, unless you're trying to group specific conditions with each other. But something like this:

if (str_bet == ''):

should just be this

if str_bet == '':


Python script to store nd array into images I am trying to write a generic Python script that do the following types of tasks:

Load .npy file (the .npy file is of shape (m_samples, channels, row, column), which corresponds to m_samples images, and here the channels are always 1
Check whether a given path exists
Iterate against the .npy file, and save each image into the given path

I am not sure whether I am using the best practices to save an nd array into an image.

import numpy as np
from scipy.misc import toimage, imsave
import os
image_path = "raw"

def ensure_directory_exist(image_path):
    if not os.path.exists(image_path):
        print("Allocating '{:}'",format(image_path))
        os.mkdir(image_path)

if __name__ == '__main__':
    img_array = np.load('imgs_mask_test.npy')
    ensure_directory_exist(image_path)
    for i in range(img_array.shape[0]):
       name = "img"+str(i)+".png"
       imsave(os.path.join(image_path,name),img_array[i,0])

Instead of:

ensure_directory_exist(image_path)

use the built-in os.makedirs:

os.makedirs(image_path, exist_ok=True)

Iterate over the images themselves (rather than their indexes) and so avoid the lookup img_array[i, 0] on each iteration:

for i, img in enumerate(img_array[:, 0, :, :]):
    name = "img{}.png".format(i)
    imsave(os.path.join(image_path, name), img)


How to optimize population projections for speed? I am currently working on a problem of an online judge (not a contest). The program must calculate how many years will require for the population A (which is always less than B) to be higher than B. If the time is more than a Century then print "More than a century" in Portuguese.
Input

1
90000 120000 5.5 3.5

Where 1 is the number of test cases and in the next line are the population of A, population of B, percentage growing rate of A and percentage growing rate of B. For this input the result will be 16 years.

However, my code keeps exceeding the time limit, which is only 1 second. I have used Scanner and System.out.print() however I have been told that such methods will make my code slower and will consume more memory, that is way I buffered the input and output. I cannot use types such as long or int as the data stored the variables is longer than those values.

import java.io.*;
import java.math.BigDecimal;

class Main {
    public static void main(String[] args) throws NumberFormatException, IOException{
        java.io.InputStreamReader iSR = new java.io.InputStreamReader(System.in);
        java.io.BufferedReader bR = new java.io.BufferedReader(iSR, 16 * 1024);
        BufferedWriter bW = new BufferedWriter(new OutputStreamWriter(System.out));
        String tL;
        BigDecimal PRA = BigDecimal.ZERO, PRB = BigDecimal.ZERO, PA = BigDecimal.ZERO, PB = BigDecimal.ZERO;
        int input = Integer.parseInt(bR.readLine());
        for(int j = 0; j < input; j++){
            tL = bR.readLine();
            String [] = arr = tL.split(" ");
            PA = BigDecimal.valueOf(Integer.parseInt(arr[0]));
            PB = BigDecimal.valueOf(Integer.parseInt(arr[1]));
            PRA = BigDecimal.valueOf(Double.parseDouble(arr[2]));
            PRB = BigDecimal.valueOf(Double.parseDouble(arr[3]));
            PRA = PRA.divide(BigDecimal.valueOf(100)); PRA = PRA.add(BigDecimal.ONE);
            PRB = PRB.divide(BigDecimal.valueOf(100)); PRB = PRB.add(BigDecimal.ONE);
            int i = 1;
            while(PA.compareTo(PB) < 0 && i < 103){
                BigDecimal ibd = BigDecimal.valueOf(i);
                PA = PA.multiply(PRA).multiply(ibd);
                PB = PB.multiply(PRB).multiply(ibd);
                i++;
            }
            i--;
            if(i > 100) bW.write("Mais de 1 seculo.\n");
            else bW.write(String.valueOf(i) + " anos.\n");
            bW.flush();
        }
    }
}

First, the syntax error:

        String [] = arr = tL.split(" ");

You probably meant

        String[] arr = tL.split(" ");

Now, let's make all of your variable names readable. Longer names take the same amount of time to access:

    java.io.InputStreamReader inputStream = new java.io.InputStreamReader(System.in);
    java.io.BufferedReader reader = new java.io.BufferedReader(inputStream, 16 * 1024);
    BufferedWriter writer = new BufferedWriter(new OutputStreamWriter(System.out));
    String input;
    BigDecimal GrowthA, GrowthB, PopulationA, PopulationB; //no need to assign initial values to them; the values are never used
    int repetitions = Integer.parseInt(reader.readLine());

Now, as always, choosing a better algorithm is a far bigger deal than small things such as your particular means of input and output. On my computer, the code between input = reader.readLine(); and writer.flush(); takes 6 milliseconds to run once and 24 milliseconds to run 50 times for the example input you gave; make sure that your timing measurement does not include the amount of time it spends waiting for you to enter input. For your algorithm, you have chosen to iteratively simulate each year of population growth; this is mathematically inefficient. Your problem algebraically reduces to

PA*(1+PRA/100)^t>PB*(1+PRB/100)^t, solve for t.
=PA*((PRA+100)/100)^t>...
=PA*(PRA+100)^t*(1/100)^t>...
PA*(PRA+100)^t>...
ln(PA)+t*ln(PRA+100)>ln(PB)+t*ln(PRB+100)
t(ln(PRA+100)-ln(PRB+100))>ln(PB)-ln(PA)
t>(ln(PB)-ln(PA))/(ln(PRA+100)-ln(PRB+100)) (assuming PRA > PRB)

It will be much faster to just use this one computation, and as bonus you will get more precise answers. Also, since you will not be increasing your numbers exponentially in your algorithm, you will now be able to directly use Integer.parseInt instead of having to ever make BigDecimals.

Marginal changes: Testing reveals that using final local variables is faster than reusing larger-scope variables, and that using BufferedReader and BufferedWriter does not create any significant difference in performance (at least on my machine), so unless you test it and get a different result you should use Scanner and System.out just because it's simpler. Also, you should mark all variables that are never modified as final.

    final Scanner scanner = new Scanner(System.in);
    final int repetitions = scanner.nextInt();
    scanner.skip("\n");
    for(int j = 0; j < repetitions; j++){
        final String input = scanner.nextLine();
        final String[] arr = input.split(" ");
        final double PopulationA = Double.parseDouble(arr[0]);
        final double PopulationB = Double.parseDouble(arr[1]);
        final double GrowthA = Double.parseDouble(arr[2]);
        final double GrowthB = Double.parseDouble(arr[3]);
        System.out.print((Math.log(PopulationB)-Math.log(PopulationA))/(Math.log(GrowthA+100)-Math.log(GrowthB+100)) + " anos.\n");
    }

If integer output is required, throw in a call to Math.ceil. On my machine, the code after final String input = scanner.nextLine(); runs once in 3 milliseconds, 50 times in 6 milliseconds, and 10,000 times in 450 milliseconds, with your example input. A doubling of speed for one run, and faster and faster for the same input repeated.


Shortest code to generate a full mesh of elements I am trying to find the shortest code that generates a full mesh of the elements fed into it in graphviz format
e.g. the full mesh of [0, 1, 2] is:

2 -> 0
2 -> 1
1 -> 0

I have the code below and I think there might be a way to combine the if and for loops, and avoid needing to directly create the empty string

def graph(l):
    if not l:
        return ""
    i = l.pop()
    out = ""
    for z in l:
        out += "  {}->{}\n".format(i, z)
    return out + graph(l)
print graph(range(3))

If I get it correctly, the shortest way that I can think of is by using itertools.combinations. Basically, you want all unique combinations, so:

from itertools import combinations

def mesh(L):
    return ["->".join(map(str, comb)) for comb in combinations(L, 2)]

Which will return:

['1->2', '1->3', '2->3']

For: mesh([1, 2, 3])

If you then want to print them as above, just:

for x in mesh([1, 2, 3]):
    print(x)


Filter string with min distance in r I have the following sample data:

target <- "Stackoverflow"
candidates <- c("Stackflow", "Stackflow", "Stckoverfow")

I would like to filter the string from candidates that has the lowest (levensthein) distance to the target string.

I decided to use the pipe operator to improve readability. However, i find it suboptimal that adist doesnt return the strings that corresponds to the distances.

My attempts:

Version 1)

library(magrittr)
candidates  %>%
  adist(y = target) %>%
  which.min %>%
  `[`(x = candidates, i = .)

Version 2)

library(dplyr)
candidates %>%
  adist(y = target) %>%
  data.frame(dist = ., candidates = candidates) %>%
  filter(dist == min(dist)) %>% select(candidates)

Goal:

I want to Improve readability. Memory usage or performance do not matter as data
is pretty small. Using additional packages, like dplyr, is also fine.

In version 1 i think
     `[`(...) is bad to read.

Version 2 seems better to me. But it does not seem optimal to me, that i have to add the strings to the data.frame after having applied adist() to them: data.frame(dist = ., candidates = candidates).

What is wrong with simple code like?:

distances <- adist(candidates, target)
candidates[distances == min(distances)]

This is shorter and, in my opinion, easier to read, as it do not require any knowledge of additional packages.

Also, you mentioned that your data is small and it looks like you are working with vectors, in that case I do not see a point to use data.frame, dplyr etc.


Raku / Perl6: A module to enable escape codes in the win32 console, naming suggestions I have this little project i am about to release.

https://github.com/holli-holzer/perl6-Win-VT

I would be happy about a code review and suggestions for a better name.

Discussion in IRC yielded Terminal::Win::VT or Terminal::Win32::VT::Enable as better alternatives. Or even Win32::Console::VT.

This is the gist of it. There are additional files, but those provide just sugar.

unit module Win::VT;

use NativeCall;

constant STD_INPUT_HANDLE                   = -0xA;
constant STD_OUTPUT_HANDLE                  = -0xB;
constant ENABLE_VIRTUAL_TERMINAL_INPUT      = 0x0200;
constant ENABLE_PROCESSED_INPUT             = 0x0001;
constant ENABLE_PROCESSED_OUTPUT            = 0x0001;
constant ENABLE_VIRTUAL_TERMINAL_PROCESSING = 0x0004;
constant DWORD  := uint32;
constant BOOL   := int32;
constant HANDLE := Pointer[void];

sub GetConsoleMode(HANDLE, DWORD is rw) is native('Kernel32') returns BOOL { * };
sub SetConsoleMode(HANDLE, DWORD) is native('Kernel32') returns BOOL { * };
sub GetStdHandle(DWORD) is native('Kernel32') returns HANDLE { * };

my HANDLE $input-handle;
my HANDLE $output-handle;
my DWORD $output-mode;
my DWORD $input-mode;

INIT {
    if $*VM.osname eq "mswin32"
    {
        $input-handle  = GetStdHandle( STD_INPUT_HANDLE );
        $output-handle = GetStdHandle( STD_OUTPUT_HANDLE );
    }
}

sub vt-on(Bool :$vt-input=True, Bool :$vt-output=True ) returns Bool is export(:MANDATORY)
{
    return False
        unless $input-handle.defined && $output-handle.defined;

    return False
        unless GetConsoleMode($input-handle,  $input-mode) && GetConsoleMode($output-handle, $output-mode);

    my $new-input-mode  = $input-mode  +| ENABLE_PROCESSED_INPUT  +| ENABLE_VIRTUAL_TERMINAL_INPUT;
    my $new-output-mode = $output-mode +| ENABLE_PROCESSED_OUTPUT +| ENABLE_VIRTUAL_TERMINAL_PROCESSING;

    return (
        ( $vt-input  ?? SetConsoleMode($input-handle, $new-input-mode)  !! 1 ) &&
        ( $vt-output ?? SetConsoleMode($output-handle, $new-output-mode) !! 1 )
    ).Bool;
}

sub vt-off() returns Bool is export(:MANDATORY) {
    return (
        $input-handle.defined && $output-handle.defined &&
        SetConsoleMode($output-handle, $output-mode) &&
        SetConsoleMode($input-handle, $input-mode)
    ).Bool;
}

sub cls() is export(:cls) {
    vt-on;
    print chr(27) ~ '[2J' ~ chr(27) ~ '[;H';
    vt-off;
}

Since no one else has replied, here's somewhat of a "rubber duck code review" given that I don't know much about Windows, vt, and NativeCall. Plus I've got a suggestion about the API and module name.

It looks like the not-explicitly-initialized $input-mode and $output-mode variables are actually constants implicitly initialized to zero. I think the code would be less confusing if they were declared as constants or even if you used literal 0s instead of them. Perhaps other code or configuration you have, or have in mind, will vary these values? Anyhow, at the very least, I think a comment making it clear the values the user passes to vt-on don't alter these, and vt-off is resetting them no matter what vt-on did, would be helpful.

From an API perspective, aiui, if one calls vt-on it turns the vt stuff on for both input and output. You have to pass :vt-input(False) if you only want vt-output and vice-versa. And vt-off always turns both off.

To me, having to write vt-input(False) is a bit ugly because:

It repeats the vt;
Mentions input when what one is trying to do is control output;
Requires explicit use of False.

It seems to me it could be more ergonomic to have just one routine, vt. To switch both input and output on use vt :on. To switch both off use vt :off. To switch just one on and leave the other as it is, use vt :on<input> or vt :on<output>. To switch just one off and leave the other as it is, use vt :off<input> or vt :off<output>.

I've never used vt stuff and could imagine one doesn't need the extra flexibility of the API I suggest but my suggestion is really more about keeping the API sweet than it is about the additional flexibility.

As for the module name...

I currently think we should shift all of our emphasis related to module discovery to an assumption of widespread use of global search via keywords, tags, etc. For now that means eg 'terminal' tag search or 'terminal' keyword search but one day there'll surely be search of a module's Pod and other aspects as well. In summary, imo, don't worry about a name's discoverability.

To the degree that a module's name matters, I think it's beyond discoverability. I think the two key things are considering clustering with names already chosen or likely to be chosen by other module authors, and making a name nice in a use statement. For these latter aspects I'd say:

Consider following accepted hierarchical namespace norms already established for modules.perl6.org. But don't sweat it.
If there are no obvious relevant norms to follow then keep a module name fairly short and sweet. Do you really need bumpy case or hyphens? Do you really need more than one :: divider? Then again, don't pick something egregiously short-sighted. You could name it just VT but a module such as yours doesn't warrant such a name.

Other than that the code all makes sense to me and looks fine to me from a pure P6 perspective.


Assign a variant to a variant Quick review:

I've been writing API code which receives user input that can come as either an object or a value type - a problem outlined here.

In order to write an unknown data type to a variable, I first came up with this:

Public Sub LetSet(ByRef variable As Variant, ByVal value As Variant)
    If IsObject(value) Then
        Set variable = value
    Else
        variable = value
    End If
End Sub

called like

Dim result As Variant
LetSet result, objectOrValueType()

However an alternate approach that I've come up with is the following:

Public Property Let LetSet(ByRef variable As Variant, ByVal value As Variant)
    If IsObject(value) Then
        Set variable = value
    Else
        variable = value
    End If
End Property

called like

Dim result As Variant
LetSet(result) = objectOrValueType()

which I like because it feels closer to LetSet a = b - the ultimate goal but one that I don't believe is possible.

Both live in standard modules and are functionally equivalent - the return value of objectOrValueType() (some arbitrary function that generates the unknown data) is stored in result.

So I'm wondering which is better and is there anything I can do to improve on either approach? Any other feedback is, of course, also welcome.

NB, just to add some context, I've come across 2 use cases for this:

A buffered list; this class is drip-fed data of unknown kind from some asynchronous process, and merges it into batches of fixed size - then raises an event when a new batch is ready. Because the internal data store is constantly  being written to, I can't expose that, so I need some way to copy items to variant arrays for each batch and expose those in the event.
Overloaded arithmetic functions; I've defined some Add method that can take integers and sum them with the + operator. However I'm allowing classes to implement IAddable and define their own response to the Add method; this implementation may return an object. So I need to be able to write the result of this calculation to the return variable of the Add method.

For the former I could use a Collection and avoid the issue altogether as @Freeflow points out in their answer - but the batches are fixed size so an array makes more sense for me. For the latter, I don't see a way around using this approach, as the return type of the function is down to the user, not me.

Although you think that the second one is closer to your desired syntax, it is actually somewhat confusing. If LetSet was a function with only one parameter, this would cause a Let coercion on the RHS, if an object is supplied. E.g. if the RHS is a range, the usual behaviour of a Let assignment is that the values in the range are assigned instead of the object. Only because LetSet is parameterized, the object itself is assigned.

So, following the principle of least surprise, I would suggest to use the first version.


Extremely simple character manipulation Given a string, this code

Multiplies odd numbers
Subtracts even numbers, do nothing if only one even number is found
Turns uppercase letters to lowercase and vice versa

I think this code is too long for those operations. There are certainly better ways of doing this. How can I make this shorter, better and faster? And what are better ways of doing this?

#include <string>
#include <cctype>
#include <iostream>

int main() {
    int odd = 0;
    int even = 0;
    std::string letters{};
    bool set = false;
    std::cout << "enter words: ";
    std::string input;
    std::cin >> input;
    for (char c : input) {
        if (isdigit(c)) {
            int number = c-48;
            if (number % 2 == 0) {
                if (!set) {
                    even = number;
                    set = true;
                } else {
                    even -= number;
                }
            } else {
                if (odd == 0) {
                    odd++;
                }
                odd *= number;
            }
        } else if (isalpha(c)) {
            letters += (isupper(c)) ? tolower(c) : toupper(c);
        }
    }
    std::cout << "odd: " << odd << '\n';
    std::cout << "even: " << even << '\n';
    std::cout << "letters: " << letters << '\n';
}

As Jamal implied, std::cin >> input will only extract one word. Since the prompt says words, I would assume this is wrong, and you should use std::getline like he suggested.

set is vaguely named. What is set? Something like firstEven might be a bit clearer (though then off course you have to default to true).

This would be clearer as int number = c - '0';.

(Also, if you're wanting to be incredibly pedantic, it's worth noting that this depends on ASCII (or an ASCII compatible encoding) and is not portable. The standard functions (e.g. std::stoi like MORTAL suggested) are guaranteed to be portable.)

While I'm at it with pednaticness...

if (isdigit(c)) {

That should be std::isdigits (same with toupper/tolower). When a C header is included via the C++ version of the header, it's guaranteed that the functions will be present in the std namespace, but it's only optional that they'll be present in the global namespace. Every major compiler exports them in the global namespace along with std, but CodeReview brings out the pedant in me :p.

Even though you're wanting to make this shorter, I think it's a bit too dense already, and it could use a bit of breaking out into smaller pieces. Luckily, since none of the 3 things interact, you can pretty easily encapsulate each item and keep the code reasonable. Unfortunately... doing so will almost inevitably come at a pretty steep amount-of-code cost.

The best way to do this that I can think of (at least without getting way carried away) is functors (or functor-ish things anyway):

#include <cctype>
#include <iostream>
#include <string>

/** Mutlipy odd numbers together as they're encountered, or default to 0 if none are seen. */
class odd_mult {
private:
    int val_ = 0;
public:
    void operator()(int x) {
        if (x % 2 == 0) { return; }
        val_ = (val_ == 0) ? x : val_ * x;
    }
    int val() const { return val_; }
};

/** Subtract subsequent even numbers from the first one encountered, defaulting to 0. */
class even_sub {
private:
    bool first = true;
    int val_ = 0;
public:
    void operator()(int x) {
        if (x % 2 != 0) { return; }
        if (first) {
            val_ = x;
            first = false;
        } else {
            val_ -= x;
        }
    }
    int val() const { return val_; }
};

/** Build up a string such that each encountered letter has its case reversed (i.e. lower -> UPPER, UPPER -> lower) */
class letter_upcase {
private:
    std::string val_;
public:
    void operator()(char c) {
        if (!std::isalpha(c)) { return; }
        val_ += std::isupper(c) ? std::tolower(c) : std::toupper(c);
    }
    const std::string& val() const { return val_; }
};

int main() {
    odd_mult odd;
    even_sub even;
    letter_upcase letters;

    std::cout << "enter words: ";
    std::string input;
    std::getline(std::cin, input);

    for (char c : input) {
        if (std::isdigit(c)) { // This could go in the functors, but then it'd have to be repeated.
                               // Better would be a number_fn, but that would require templating to do correctly.
            odd(c - '0'); // Creating a string to do the conversion seems a bit excessive. I'm willing to rely on ASCII.
            even(c - '0');
        }
        letters(c);
    }

    std::cout << "odd: " << odd.val() << '\n';
    std::cout << "even: " << even.val() << '\n';
    std::cout << "letters: " << letters.val() << '\n';
}

All in all, I'm not very happy with this approach, even though I'm suggesting it :/. It's incredibly long compared to your code, though a dozen lines or so can be cut out by changing the classes to structs. Additionally, a few sacrifices had to be made in terms of design to avoid it growing even longer. I do still feel though that your code is a bit hard to follow (I had to resort to the English description to understand it after trying to read it a few times -- what if the English hadn't been there?), but it seems this might be a situation where it's just too much logic to try to cram into a small amount of code.


Feedback on Pitch Constellation/ Chromatic Circle in Python Project Hello I'm doing a mini project that is modeled after pitch constellation.
--> https://en.wikipedia.org/wiki/Chromatic_circle
Basically, pitch constellation is modeled after a clock where each note is assigned a number. When the wheel goes clockwise it goes up the scale, and when it goes counterclockwise it goes down. I thought this might be a great idea for a midi project. (My project only goes up the scale.)
Here's how it works:

It prompts for an input of a music note and then shifts the elements so that the list will start with the root key. Then it will return the notes starting with the root note or the note selected.

Then it prompts to enter what mode to return the music note values. The way this done is by assigning the elements in the returned notes to the value selected in the dict variable scale. Which will then select the index number of the items and create a new list.

Lastly, it returns the scale with the key and mode.

That's all it does so far. Eventually, I'd like to combine it with a midi module I found in pythons open library and then have it send midi out. I want to get honest feedback on what areas can be improved in my program before I go any further.

notes = ['C', 'C#', 'D', 'D#',
             'E', 'F', 'F#', 'G', 'G#',
             'A', 'A#','B']

scale = {'major':[0, 2, 4, 5, 7, 9, 11],
    'minor':[0, 2, 3, 5, 7, 8, 10],
    'phrygian':[0, 1, 3, 5, 7, 8, 10],
    'lydian':[0, 2, 4, 6, 7, 9, 11],
    'mixolydian':[0, 2, 4, 5, 7, 9, 10],
    'aeolian':[0, 2, 3, 5, 7, 8, 10],
    'locrian':[0, 1, 3, 5, 6, 8, 10],
    'chromatic':[0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11],
    'wholetone':[0, 2, 4, 6, 8, 10],
    'diminished7thchord': [0, 3, 6, 9],
    'augmentedchord':[0, 4, 8],
    'tritone':[0, 6]}

rootkey = input("Enter the root Note: ").upper()

class CircleFifths:
    """docstring for CircleFifths."""
    def __init__(self, key):
        self.rootkey = rootkey

    def shift(self, rootkey, index=0, s=0,):
        #shifts the leading item (rootkey) entered by the user down the list
        #removes the last term and puts it as the first term
        while index < 12 and s <12:
            s += 1
            index += 1
            new_notes = notes[-s:] + notes[:-s]
            if rootkey == new_notes[0]:
                print(new_notes)
                return new_notes

def main():
    a = CircleFifths(rootkey)
    key = a.shift(rootkey)

    keymode = []

    print('Select a scale and enter it below')
    for k in scale:
        print(k)

    mode = input("Enter the mode: ").lower()
    try:
        x = scale.get(mode)
        keymode = [key[i] for i in x]
        print(keymode)
    except:
        if mode not in scale:
            print("Scale not in list. Please start the program again.")
        return

main()

Immutable sequences
Convert notes and the values in scale from list literals [] to tuple literals () since it's expected that they cannot and will not change.
Musical theory
Consider distinguishing between harmonic and melodic minor, which are different scales.
Global code
Move
rootkey = input("Enter the root Note: ").upper()

into your main(). Also, your constructor
def __init__(self, key):
    self.rootkey = rootkey

needs to refer to its key argument and not the rootkey global.
Shadowing
def shift(self, rootkey, index=0, s=0,):

has a number of issues:

You're shadowing the global rootkey with an identically-named argument
You shouldn't have a rootkey argument at all, since it's already a member on self

Loop like a native
Consider renaming index and s to first_index and first_s, so that you can replace your while with
for index, s in zip(
    range(first_index, 12),
    range(first_s, 12)
):

Inner prints
Delete the print from here:
            print(new_notes)
            return new_notes

If you want to print it, do so at the outer level.
Logic by exception
Consider writing
mode = input("Enter the mode: ").lower()
try:
    x = scale.get(mode)
    keymode = [key[i] for i in x]
    print(keymode)
except:
    if mode not in scale:
        print("Scale not in list. Please start the program again.")
    return

as
mode_name = input("Enter the mode: ").lower()
mode = scale.get(mode_name)
if mode is None:
    print(f"Scale {mode_name} not in list. Please start the program again.")
else:
    key_mode = [key[i] for i in mode]
    print(key_mode)

In other words, where possible do not rely on exceptions (especially with no type specified) to govern your logic; and avoid a double-dictionary lookup by using get.


Reusing Code With Database Cursors I find myself using this bit of code very often when I am retrieving the results from a Cursor

SearchItem searchItem = new SearchItem();
searchItem.setId(cursor.getInt(cursor.getColumnIndex(COLUMN_NAME_ID)));
searchItem.setOrigin(cursor.getString(cursor.getColumnIndex(COLUMN_NAME_ORIGIN)));
searchItem.setDestination(cursor.getString(cursor.getColumnIndex(COLUMN_NAME_DESTINATION)));
searchItem.setTimeStamp(cursor.getLong(cursor.getColumnIndex(COLUMN_NAME_TIMESTAMP)));

Specifically this could be when I am using a CursorAdapter for a ListView and in a DAO object.

What would be a useful design pattern to use in this case?

My first instinct is to have a singleton class which does this. Are there any problems in taking this route?

I am not completely sure if your question is as easy as it sounds (or if I am missing something), but here's my thought on things.

Using a singleton class for this does not make much sense in my opinion. There's no state to be stored, and therefore there's no need for an object at all. Instead, it sounds like you could put it in an utility method.

public static SearchItem createSearchItem(Cursor cursor) {
    SearchItem searchItem = new SearchItem();
    searchItem.setId(cursor.getInt(cursor.getColumnIndex(COLUMN_NAME_ID)));
    searchItem.setOrigin(cursor.getString(cursor.getColumnIndex(COLUMN_NAME_ORIGIN)));
    searchItem.setDestination(cursor.getString(cursor.getColumnIndex(COLUMN_NAME_DESTINATION)));
    searchItem.setTimeStamp(cursor.getLong(cursor.getColumnIndex(COLUMN_NAME_TIMESTAMP)));
    return searchItem:
}