Simple program to detect the number of times a vowel occurs in a string I wrote a simple program to print out any vowels found in a string, as well as the amount of times that vowel occurs.

Here is my code:

#include <iostream>
#include <string>
#include <map>

void findVowels(std::string& theString, std::map<char,int>& map);
bool vowelCount(char& temp);
std::ostream& operator<<(std::ostream& out, const std::map<char, int>& map);

int main()
{
    bool keepGoing = true;
    while (keepGoing)
    {
        std::cout << "Enter a string to see how many times vowels were used.\n";
        std::string userInput;
        std::getline(std::cin, userInput);

        std::map<char, int> theMap;
        findVowels(userInput, theMap);

        std::cout << theMap;
        std::cout << "\nWould you like to keep going? Please type Y to continue or N to exit.\n";

        char nextAction;
        std::cin >> nextAction;
        if (nextAction == 'N' || nextAction == 'n')
            keepGoing = false;
        else if (nextAction != 'Y' && nextAction != 'y')
            std::cout << "Incorrect input. Defaulting to yes.\n";
        std::cin.ignore();
    }
    std::cout << std::endl;
    system("pause");
    return 0;
}

void findVowels(std::string& theString, std::map<char, int>& theMap)
{
    int stringSize = static_cast<int>(theString.size());
    for (int i = 0; i < stringSize; i++)
    {
        if (vowelCount(theString[i]))
            theMap[theString[i]]++;
    }
}

bool vowelCount(char& temp)
{
    temp = toupper(temp);
    return (temp == 'A' || temp == 'E' || temp == 'I' || temp == 'O' || temp == 'U');
}

std::ostream& operator<<(std::ostream& out, const std::map<char, int>& theMap)
{
    std::map<char, int>::const_iterator iter = theMap.cbegin();
    std::map<char, int>::const_iterator iterEnd = theMap.cend();
    while (iter != iterEnd)
    {
        out << iter->first << " has " << iter->second << " number of occurences.\n";
        iter++;
    }

    return out;
}

I agree mostly with @Yuri, but I feel that regex is an overkill here. Besides, as they say: "Some people, when confronted with a problem, think "I know, I'll use regular expressions." Now they have two problems".

Check if a letter is a vowel

I'll begin with this function since it's at the core of your program:

// your code
bool vowelCount(char& temp)
{
    temp = toupper(temp);
    return (temp == 'A' || temp == 'E' || temp == 'I' || temp == 'O' || temp == 'U');
}

I didn't understand at first why you would pass the argument as a non-const reference. I realized only after that you relied on the inner toupper to store vowels in theMap as capital letters. That's a bad idea! One function should have one purpose and its purpose should be expressed clearly. In this case it assesses if the letter is a vowel, period.

So use a const reference instead or, even better, a copy of the character you take as an argument. A char is 1 byte, so taking it by reference or pointer never is an optimization and can even be a pessimization.

The other thing I find sub-optimal in your code is the way you test if temp is a vowel. Firstly because there are better, faster ways to find a value in a range,  like binary_search, which is well known as well as available in the standard library (in <algorithm>). Secondly because the vowels' list isn't the same in all languages. For instance, in french, you would need to count 'y', 'à', 'â', 'é', 'è', 'ê', 'î', 'ô', 'ù', 'û', as well. That's why it might be more flexible and readable to keep the list separated from your code.

So, I would have something like:

bool is_vowel(char c, const std::string& sorted_vowel_list) {
    return std::binary_search(sorted_vowel_list.begin(),
                              sorted_vowel_list.end(),
                              std::toupper(c));
}

A Vowels' map

I'd then count the vowels into a map almost the way you do; I'd only use a range-based loop:

for (auto letter : input_string)
    if (is_vowel(letter))
        ++vowel_map[std::toupper(letter)];

If C++17 is available to you, you can then use "structured bindings" to read the map without referring to iterators:

for (auto [vowel, count] : vowel_map)
    std::cout << "Vowel: " << vowel << " has " << count << " occurrences\n";


Addition of 2 integers I've implemented an algorithm found in a book. It adds 2 positive integers of equal size. How can it be improved? It is assumed that inputted data is always positive integers with an equal number of digits.

EDIT
As there are lots of comments regarding the question, I would like to rephrase it. This program tries (yes it tries, could be wrong) to simulate how humans do manual additions of 2 positive integers. This is similar to the technique students learn in (I hope..) primary schools. It is based on the book "Invitation to Computer Science". One obvious improvement to this program is adding validations. What else can be done to improve it?

class Program
{
    static void Main(string[] args)
    {
        Add2Numbers(97,47);
        Console.ReadLine();
    }

    static void Add2Numbers(int a, int b)
    {
        int carry = 0,sum=0;
        List<int> lstA = new List<int>();
        List<int> lstB = new List<int>();
        List<int> lstSum = new List<int>();

        foreach (var item in GetDigit(a))
        {
            lstA.Add(item);
        }

        foreach (var item in GetDigit(b))
        {
            lstB.Add(item);
        }

        for (int j = 0; j < lstB.Count;j++ )
        {
            sum = lstA[j] + lstB[j] + carry;
            if (sum > 9)
            {
                sum = sum - 10;
                carry = 1;
            }
            else
                carry = 0;
            lstSum.Add(sum);
        }

        if (carry > 0)
            lstSum.Add(carry);

        lstSum.Reverse();
        foreach (var item in lstSum)
        {
            Console.Write(item);
        }
    }

    static IEnumerable<int> GetDigit(int number)
    {
        while (number > 0)
        {
            yield return number % 10;
            number = number / 10;
        }
    }

}

I generally prefer foreach to for, but in the case where you are using the index variable to iterate through two lists at once, for is a pretty good option. I still try to use .Zip() instead, as the other answer demonstrated, but sometimes that can get hairy. Here are some smaller improvements that don't involve changes to the overall structure.

It's always a good idea to use descriptive names

I would rename LstA to DigitsOfA (same for B, and Sum)
I would rename GetDigit to GetDigits

It's usually1 a good idea to delay declaration of a variable until it's used

A related concept is reducing variable scope; not using a global variable if a local variable will do. This doesn't have any technical impact, but it does have a readability impact: If I see you using a variable but have to scroll up to see how that variable was declared, the flow of my reading has been interrupted. We could call this "spatial scope".
Here, this would mean declaring sum and carry just before the for loop, instead of at the top of the function.
You could even declare sum inside the loop. It does seem inefficient to instantiate a new variable every iteration of the loop, but (1) the impact will probably be low, even negligible, (2) you can generally trust the compiler to do something smart (3) if making my program a tiny bit easier to read also makes it a tiny bit slower, that's a trade I'll gladly make every time.

I usually prefer var over explicit types when declaring a variable

This is the subject of a fair amount of debate, but here's my reasoning. It's easier for me to read
var prices = new Dictionary<Fruit, decimal>();
var lemon = new Lemon();
var price = 17.50;
prices.Add(lemon, price);
 than
Dictionary<Fruit, decimal> prices = new Dictionary<Fruit, decimal>();
Lemon lemon = new Lemon();
decimal price = 17.50;
prices.Add(lemon, price);
 In the former, the information I immediately want to know (what are the variables' names? how are they used?) all flows nicely down the left edge.

It's nearly always easier (to write and to read) to use LINQ's .ToList() than to call List.Add() in a loop.

This means you can initialize your lists quite easily as var digitsOfA = GetDigits(a).ToList();

Implicit braces do make your code shorter and prettier, but they are also the source of a very common bug. You write
if (awake)
    GetCoffee();
 then I come along and update your code to
if (awake)
    GetCoffee();
    GetBreakfast();
All of a sudden I'm eating breakfast while I'm still asleep. And if you're thinking I'd have to be a real bonehead to make that mistake, you're right! But it's a mistake I've made before, it's a mistake that will be made again... And it's a mistake that would be impossible if the original code were
if (awake)
{
    GetCoffee();
}
 So consider very carefully how likely it is that others might modify your code in the future, or that you might modify it before you have your coffee... I recommend the braces every time.
You can dodge the whole brace issue, however, by modular arithmetic instead of if:
carry = sum / 10;
sum = sum % 10;

I quite like the GetDigits function as it stands! That's an excellent use of yield return, in my opinion.

1This advice does not apply to Javascript, thanks to variable hoisting


Custom UUID based on time I've been using random UUIDs—from Rust, e.g. Uuid::new_v4()—for my primary keys in a MySQL database. I recently happened across this, which suggests that UUIDs with an appropriately positioned time component offer some performance improvements.

I created a naive UUID generator like:

fn uuid_new() -> ::uuid::Uuid {

    use ::rand::Rng;

    let now = ::std::time::SystemTime::now()
        .duration_since(::std::time::UNIX_EPOCH)
        .unwrap();

    let (secs, nans): (u64, u32) = (now.as_secs(), now.subsec_nanos());

    let rbts: [u8; 6] = ::rand::thread_rng().gen();

    let bytes = [
        // (secs >> 56) as u8,
        // (secs >> 48) as u8,
        (secs >> 40) as u8,
        (secs >> 32) as u8,
        (secs >> 24) as u8,
        (secs >> 16) as u8,
        (secs >>  8) as u8,
        (secs >>  0) as u8,
        (nans >> 24) as u8,
        (nans >> 16) as u8,
        (nans >>  8) as u8,
        (nans >>  0) as u8,
        rbts[0], rbts[1], rbts[2], rbts[3], rbts[4], rbts[5]
    ];

    ::uuid::Uuid::from_bytes(&bytes).unwrap()

}

I have no requirements for my UUIDs except speed and uniqueness. I'm truncating the first two octets of the secs u64, since they don't seem necessary for the near future.

Am I committing any faux pas here? I don't really know anything about the theory or practice of UUIDs. I'm a little bit worried since the value of get_version_num is gibberish for my UUIDs (though I am not using version number in any way).

Or are there any alternate (Rust) implementations for UUIDs with a time component?

No, that is not one of the five standard-compliant ways to generate a UUID.  What you have written is just an obfuscated concatenation of a timestamp and a random number.

The easiest way might be to use the MySQL UUID() function instead of generating the UUID in Rust.


Load SSH key remotely I often create new accounts or virtual machines, and then have to load my SSH public key/signature in to the authorized keys on that account. That process can be tedious, so I created this script that pushes my authorization on the remote machine. So, typically when you ssh to a remote machine you have to enter your password, etc. To automate jobs, though, you often don't want to do that, and key-based authentication allows you to authenticate without the password.

There are other ways to push keys around, but this makes it simple to fix things after the fact, or to initialize new accounts, etc.

I am looking for a review of any and all aspects, including the way that SSH is set up, and so on.

Here's an example transcript of what the script outputs:

panabox:~/bin> loadkey sol@solarium
Installing Key...
sol@solarium's password:
Checking Key ...
Great!
panabox:~/bin> loadkey sol@solarium
Already Works!
panabox:~/bin>

And here is the script:

#!/bin/bash

if [ "$#" -ne 1 ]
then
  echo "Must supply user@hostname:   loadkey <user@hostname>"
  exit 1
fi

host=$1

ssh -2 -o BatchMode=yes $host "echo hi" >& /dev/null && echo Already Works! && exit 0

echo Installing Key...
install="mkdir -p ~/.ssh && chmod 700 ~/.ssh && cat - >> ~/.ssh/authorized_keys && chmod 600 ~/.ssh/authorized_keys"
cat ~/.ssh/id_rsa.pub | ssh -2 $host $install

echo Checking Key ...
ssh -2 -o BatchMode=yes $host "echo hi" >& /dev/null && echo Great! && exit 0

echo Failed to install key.
exit 2

In modern systems, there's ssh-copy-id for this. But yeah it's not everywhere. In systems that don't have it, I have a similar script like yours, but to mimic the "standard", I call it ssh-copy-id.sh. Name it as you like, but I don't think "loadkey" really sums up the action of appending your public key to the authorized list file on a remote server. So I'd pick something better.

Cats are sure cute creatures, but you don't need one here:

cat ~/.ssh/id_rsa.pub | ssh -2 $host $installcat

You can use input redirection:

ssh -2 $host $install < ~/.ssh/id_rsa.pub

Lastly, the ssh ... echo hi stuff is repeated twice. It would be better to put it in a function. You can chain the different final echo + exit commands after the function with && normally.


YouTube-to-Spotify converter I have been doing some JS lately and I would like to get some constructive criticism.

The project I am working on

var SpotifiedList = [],
youtubeSongs, spotifySongs = 0,
spotifiedSongs = 0,
playNext;

$(document).ready(function () {
  var id = window.location.hash;
  if (id) {
    $("#playlistID").val(id.substring(1));
    startProcess();
  }
});

function resetTimer() {
  console.log("timer reset!")
  clearTimeout(playNext);
}

function startProcess() {
  $("#errorOccured").hide();
  if ($("#playlistID").val().length < 5) { // Checks if there is something entered
    $(".alert-danger").fadeIn().html("Please enter a valid Youtube playlist ID.");
  } else {
    window.location.hash = $("#playlistID").val();
    $("#list").html('This might take some time.');
    $(".alert-info").html("Loading");
    $(".alert-danger").hide();
    retrieveYoutubePlaylist($("#playlistID").val(), 1, [], 1);
  }
}

function retrieveYoutubePlaylist(id, startIndex, list, k) {
  $("#loading").show();
  $.ajax({
    url: 'https://gdata.youtube.com/feeds/api/playlists/' + id + '?v=2&alt=json&start-index=' + startIndex + '&max-results=50',
    dataType: 'json',
    success: function (data) {
      if (typeof data.feed.entry === 'undefined') {} else {
        for (var i = 0; i < data.feed.entry.length; i++) {
          if (k == 1) $("#list").html("");
          var title = data.feed.entry[i].title.$t;
          list.push(title);
          retrieveSpotify(title);
          k++;
        }
        retrieveYoutubePlaylist(id, startIndex + 50, list, k);
        youtubeSongs = list.length;
      }
    },
    error: function (data, a, err) {
      if (err) {
        $(".alert-danger").fadeIn().html("Error Happened: " + err);
      }
    }
  });
}

function retrieveSpotify(title) {
  var url = 'http://ws.spotify.com/search/1/track.json?q=' + parseTitle(title);
  $.getJSON(url, function (data) {
    spotifySongs += 1;
    if (data.tracks.length > 0) {
      spotifiedSongs += 1;
      $("#list").append('
' + data.tracks[0].artists[0].name + ' - ' + data.tracks[0].name + '
');
      var info = {
        'url': data.tracks[0].href,
        'length': Math.round(data.tracks[0].length * 1000),
        'name': data.tracks[0].artists[0].name + ' - ' + data.tracks[0].name
      }
      SpotifiedList.push(info);
    }
    if (youtubeSongs == spotifySongs) {
      $(".alert-info").html('
\
        <button type="button" class="btn btn-default" onClick="play();">\
         Play/Skip\
        </button>\

Converted songs: ' + spotifiedSongs + '/' + youtubeSongs);
    }
  });
}

function parseTitle(title) {

  var newTitle = title
  .replace(/[-!$%^&_+|~=`{}:";'<>?,.\/]/g, '')
  .replace(/  /g, ' ')
  .replace(/\[.*\]/g, '')
  .replace(/lyrics/g, '')
  .replace(/\(.*\)/g, '')
  .replace(/\*.*\*/g, '')
  .replace(/   /g, ' ');

  return newTitle;
}

function play() {
  clearTimeout(playNext);
  var song = SpotifiedList[Math.floor((Math.random() * SpotifiedList.length))];
  window.location = song.url;
  x = (song.length / 1000) >> 0;
  seconds = x % 60
  x = (x / 60) >> 0;
  minutes = x % 60;
  console.log('Playing: ' + song.name + ' (' + minutes + ':' + seconds + ')');
  playNext = setTimeout(play, song.length);
}

General

I could work with this code, I only have a few remarks.

Globals

You should consider as a minimum to have your 4 globals in an object.

var player =
{
  spotifiedList : [],
  youtubeSongs : 0,
  spotifySongs : 0,
  spotifiedSongs : 0,
  playNext : 0
}

You could even add most of the functions to the player object

resetTimer

You should remove the console.log, which makes this a simple one liner. If you keep it ( function name neatly explains what it does ), make sure to call it from play() instead of doing a straight clearTimeout(playNext).

startProcess

You could consider caching the jQuery queries, maybe in the before proposed player object.

retrieveYoutubePlaylist

if (typeof data.feed.entry === 'undefined') {} else { is just wrong.

retrieveSpotify

Encoding onclick= is wrong as well.
Building HTML should be contained to separate functions.

parseTitle

I have no idea what that does, a few comments are needed there

play

I am curious, what does >> 0 get you ? Force it into an integer?


Extracting a Decimal from a string I have the following method which extracts a decimal value from a string.

using System;
using System.Linq;
using System.Text;

namespace Foo.Bar.Common.Converters
{
    public static class DecimalConverter
    {
        public static Decimal ExtractDecimalFromString(string str)
        {
            var sb = new StringBuilder();
            foreach (var c in str.Where(c => c == '.' || Char.IsDigit(c)))
            {
                sb.Append(c);
            }
            return Convert.ToDecimal(sb.ToString());
        }
    }
}

Is this the most effecticve way to do it?

Regular expressions can be a good way of processing text input in situations like this. Your current process finds every digit in the input string, concatenates it together, and then parses the result.

I doubt that this is a good solution because it makes examples like: "There are 10 people arriving at 9pm" parse as 109. Is that what you want?

Even then, Regular expressions are probably a good solution:

str = Regex.Replace(str, @"[.\D+]", "");

In str, replace all non-digits with "" (i.e. remove any non-digits).

Your code will still fail with an exception for input like "Hello", which has no digits, because the ToDecimal call will fail. Values with multiple . characters will also be interesting...

Additionally, your code does not support negative input values. This can be a trick to accomplish, but, again, using regular expressions, it is not horrendous.

Finally, I feel you should only be parsing the first set of decimal-like digis in the input, not all of them.

Putting this all together, I would have:

use regex
parse the first set of digits
accept negative inputs

And use the code:

    public static Decimal ExtractDecimalFromString(string str)
    {

        Regex digits = new Regex(@"^\D*?((-?(\d+(\.\d+)?))|(-?\.\d+)).*");
        Match mx = digits.Match(str);
        //Console.WriteLine("Input {0} - Digits {1} {2}", str, mx.Success, mx.Groups);

        return mx.Success ? Convert.ToDecimal(mx.Groups[1].Value) : 0;
    }

I have put this, with some test cases, in this Ideone here;


Algorithm to find leaders in an array An element in an array X is called a leader if it is greater than all
  elements to the right of it in X. The best algorithm to find all
  leaders in an array.

  Solves it in linear time using a left to right pass of the array
  Solves it in linear time using a right to left pass of the array
  Solves it using divide and conquer in time \$Θ(n \log n)\$
  Solves it in time \$Θ(n^2)\$

At the start the right most element will always be a leader. If an element is greater than our current_max, it will a leader. Add this element to leaders. Set current_max to this element and carry on leftward. Time complexity would be \$O(n)\$.

Can you give an algorithm for another way or better way to find leader in an array?

#include <iostream>
using namespace std;

/* C++ Function to print leaders in an array */
void printLeaders(int arr[], int size)
{
    int max_from_right =  arr[size-1];

    /* Rightmost element is always leader */
    cout << max_from_right << " ";

    for (int i = size-2; i >= 0; i--)
    {
        if (max_from_right < arr[i])
        {
            max_from_right = arr[i];
            cout << max_from_right << " ";
        }
    }
}

/* Driver program to test above function*/
int main()
{
    int arr[] = {16, 17, 4, 3, 5, 2};
    int n = sizeof(arr)/sizeof(arr[0]);
    printLeaders(arr, n);
    return 0;
}

First off this is a C solution there is nothing inherently C++ about this code. You just seem to be using the output stream. We refer to this as kind of code (in the C++ community) as C with classes. Note this is not a good designation.

As C and C++ are completely different languages with different styles and idioms you should not write a piece of code for one and expect it to be good code in the other language. The fact that the compiler may compile it is just a historical artifact. You should remove one of the tags from question (Since it will not compile as C I suggest you remove the C tag).

Since the question is tagged C++ I will review your code as a C++ program and point out why it is badly designed.

Never do this:

using namespace std;

See: Why is “using namespace std;” considered bad practice?

Useless comments are worse then no comments.

/* C++ Function to print leaders in an array */
void printLeaders(int arr[], int size)

I don't need a comment to tell me this. The name of the function and its parameters are actually very well named. Restrict your comments to a description of why (the code describes how) and well named functions and variables describe what is happening.

C++ code prefers to use iterators it situations like this.

void printLeaders(int arr[], int size)

The problem here is that it is so easy to use this incorrectly. Arrays decay into pointers way to easily. If you use the iterator interface this will prevent this happening accidentally.

template<typename I>
void printLeaders(I begin, I end) {
    // Code
}

// Calling:
int arr[] = {16, 17, 4, 3, 5, 2};
printLeaders(std::begin(arr), std::end(arr));
// If you change the code now and accidentally cause the array to decay into a
// pointer then the compiler will generate a compile time error. The current
// code is way to susceptible to refactoring mistakes.

// Also it allows you to use any container that supports the iterator
// interface (which is all of them including built in array).

You have forgotten to validate your input.

    int max_from_right =  arr[size-1];

If the size is zero. Then this is undefined behavior.

I know you are printing here.
But that looks like for de-bugging.

        if (max_from_right < arr[i])
        {
            max_from_right = arr[i];
            cout << max_from_right << " ";
        }

You could just use std::max() to simplify the code.

You should basically never use the built in array.

    int arr[] = {16, 17, 4, 3, 5, 2};

The container library contains several classes for storing a set of obejcts that are better. std::vector<int> for dynamically sized objects or std::array<int,size> for fixed size containers.

Built in arrays are hard to use correctly. Passing them by reference the syntax is non-trivial. They decay into pointers way to easily. Getting the size of an array is obnoxious as you have to remember to plant the correct division that is easy to mistype (with no compiler checking).

No need to return 0 at the end of main (its special).

    return 0;

Normally in C++ you leave out the return value to indicate that main can never fail. If you explicitly plant return 0 here it is an indication that it succeeds but also a hint there are other return paths that will fail (so I start looking for other return statements in main())


How to simplify nested conditional for loop in R? I have this for loop for creating a list of variables:

vars <- c( 'beta.o', 'sd.y')
for (x in c('ghs', 'site', 'trt')) {
  if(model.parms[[x]] == 1) {
    data <- data[, which(names(data) != x)]
  } else {
    data <- data
    if(x!='ghs') {
      vars <- c(vars, paste('sd.', x, sep = ''))
    }
    m <- min(model.parms[[x]], 5)
    for (i in 1:m) {
      if(i == 1 && x == 'site') {
        vars <- c(vars, 'beta.site[1]')
      }
      if (i > 1) {
        vars <- c(vars, paste('beta.', x, '[', i, ']', sep=''))
      }
    }
  }
}

It has been bothering me terribly, and I have failed the last two times I have tried to replace it, although conceptually it should be able to be written in a few lines. Any advice?

This bit:

for (i in 1:m) {
  if(i == 1 && x == 'site') {
    vars <- c(vars, 'beta.site[1]')
  }
  if (i > 1) {
    vars <- c(vars, paste('beta.', x, '[', i, ']', sep=''))
  }
}

Says handle the first pass differently from all the others.  So I would replace it with this:

if (x == 'site') {
  vars <- c(vars, 'beta.site[1]')
}
for (i in 2:m) {
  vars <- c(vars, paste('beta.', x, '[', i, ']', sep=''))
}


Functions that build a path to the model and command files for some tests I'm currently working in a model-based testing tool. I'm writing some integration tests for it, which require two kind of input:

Model files: which contain models to be parsed and run by the tool.
Command files: which contain commands to be input to the tool.

To reduce the boilerplate I've written two functions that build a the path to the model and command files depending on the type of tests. The solution I came up with is shown below. However I don't like the fact that I have to use a dummy parameter in my type-classes.

{-
TorXakis - Model Based Testing
Copyright (c) 2015-2017 TNO and Radboud University
See LICENSE at root directory of this repository.
-}
{-# LANGUAGE OverloadedStrings #-}
-- | Utilities for working with tests paths within 'sqatt'.
module Paths
    ( BenchTest (BenchTest)
    , IntegrationTest (IntegrationTest)
    , txsFilePath
    , txsCmdPath
    )
where

import           Data.Text
import           Filesystem.Path
import           Filesystem.Path.CurrentOS
import           Prelude                   hiding (FilePath)

-- | Types of tests supported by 'sqatt'.
class TestType a where
    -- | Directory that contains the model and command files that are used in
    -- the tests. This is relative to 'sqatt' root folder.
    dataDir :: a -> FilePath

-- | Test of the examples used in the benchmarks.
data BenchTest = BenchTest
instance TestType BenchTest where
    dataDir _ = "data" </> "bench"

-- | Integration test.
data IntegrationTest = IntegrationTest
instance TestType IntegrationTest where
    dataDir _ = "data" </> "integration-test"

-- | Make a TorXakis model file path. It appends the given directory to the
-- test data directory, and appends the '.txs extension to the given file path.
--
txsFilePath :: TestType a
            => a
            -> FilePath -- ^ Directory of the current example.
            -> Text     -- ^ File name of the current example.
            -> FilePath
txsFilePath tt currExampDir fp =
  dataDir tt </> currExampDir </> fromText fp <.> "txs"

-- | Make a TorXakis commands file path. It appends the given directory to the
-- test data directory, and appends the .txscmd extension to the given file
-- path.
txsCmdPath :: TestType a
           => a        -- ^ Test type. This will determine the location of the test data directory.
           -> FilePath -- ^ Directory of the current example.
           -> Text     -- ^ File name of the current example.
           -> FilePath
txsCmdPath tt currExampDir fp =
  dataDir tt </> currExampDir </> fromText fp <.> "txscmd"

Your code is fine and contains documentation, so there's not much to say at that point. That being said, your TestType problem can be tackled by transforming the type class to a data type:

data TestType = BenchTest | IntegrationTest

dataDir :: TestType -> FilePath

However, that just removes the constraint and makes your program less flexible in the long run. So let us go back to the drawing board. The data directory is part of the test configuration. That sounds like a valid new data type:

data TestConfiguration = TestConfiguration
                       { dataDir :: FilePath
                       , ...
                       }

defaultConfiguration :: TestConfiguration
defaultConfiguration = TestConfiguration
                     { dataDir = "data" </> "test"
                     , ...
                     }

Your two functions can now use TestConfiguration as first parameter instead of a TestType. This lets your user define adhoc configurations easily:

myConfiguration = defaultConfiguration
                { dataDir = "data" </> "my" </> "tuesday"
                , verbosity = VerboseLow
                , logFormat = TimeStamps
                , ... -- you get it
                }

Whether you want to allow record syntax, lenses or setters is left to you.


Extending Sieve of Eratosthenes beyond a billion - follow-up Follow up form Extending Sieve of Eratosthenes beyond a billion

Taking suggestions from the comments on the previous post:

Updated Code: This takes about 22 seconds for MAX as \$10^9\$ to reach "done". This is a significant improvement.

void sieve_of_eratosthenes(){
    bool* a;
    a = (bool*)malloc(MAX * sizeof(bool));
    memset(a, true, MAX);
    unsigned long int i = 1;
    while (i < ceil(sqrt(MAX))){
        while (((++i)<MAX) && (!a[i]));

        if (2 * i >= MAX)//I am keeping this test. Can't Figure out if it is still relevant
            break;

        for (unsigned long int k = i * i; k < MAX; k += i)
            if (a[k])
                a[k] = false;
    }
    std::cout << "done\n";
    for (unsigned long int i = 2; i < MAX; i++)
        if (a[i])
            std::cout << i << "\n";
    free(a);
    getchar();
    getchar();
}

Further improved:

This takes about 24 seconds for MAX as \$10^9\$ to reach "done". This is a very strange behavior as I am processing only odd numbers now. So this should take less time(must be less than 22 seconds). Also, it prints from \$3\$ to \$2*MAX + 1\$ but even that isn't such a big margin. Good thing is with \$10^9\$ array size, I am able to find primes up to \$2*10^9 + 1\$.

void sieve_of_eratosthenes(){
    bool* a;
    a = (bool*)malloc(MAX * sizeof(bool));
    memset(a, true, MAX);
    unsigned long int i = 0;
    while (i < ceil(sqrt(MAX))){

        while (((++i)<MAX) && (!a[i]));

        //if ((2 * i+1) >= MAX)//This test isn't going to be relevant anymore.
            //break;
        int j = 2 * i + 1;

        //std::cout << "element is " << 2*i+1 << "\n";

        for (unsigned long int k = i + j; k < MAX; k += j)

            if (a[k])
                //Open the following comment very carefully
                //std::cout << "removes " << 2 * k + 1 << "\n";
                a[k] = false;
    }
    std::cout << "done\n";
    for (unsigned long int i = 1; i < MAX; i++)
        if (a[i])
            std::cout << (2*i+1) << "\n";

    free(a);
    getchar();
    getchar();
}

Why is the strange behavior in time? Is there a limit to how much a malloc can allocate? Going beyond a billion for MAX still gives me error. How can I further improve this?

There are a few things you could do to improve your code.

Use braces

Constructions like this are a bug waiting to happen:

for (unsigned long int k = i + j; k < MAX; k += j)
    if (a[k])
        //Open the following comment very carefully
        //std::cout << "removes " << 2 * k + 1 << "\n";
        a[k] = false;

If anyone ever uncomments that line, the a[k] = false will then be outside the for loop and the program will not work correctly.  Such errors are easily avoided by the use of braces:

for (unsigned long int k = i + j; k < MAX; k += j) {
    if (a[k]) {
        //no particular care is now required
        //std::cout << "removes " << 2 * k + 1 << "\n";
        a[k] = false;
    }
}

Check for allocation failure

The code doesn't currently check for an allocation failure, but it should.  Calls to malloc or calloc can fail, and your program should be robust enough not to fail if that happens.

Reduce the range of search for primes

We already know that the largest usable prime is \$<\sqrt{\text{MAX}}\$, but then there is this line:

while (((++i)<MAX) && (!a[i]));

We don't really need to search all the way to MAX.  Also, for clarity, I'd rewrite that as a for loop:

// skip over composites to next prime
for (++i; i<sqrtMAX && a[i]; ++i)
{}

Skip more composite numbers

In the actual sieve, the code skips forward and then marks all odd multiples of the most recently discovered prime.  However, the code currently doesn't skip as many as it could.

For instance, if the most recently discovered prime is 7, the code currently skips to 21, but we can do better.  We can skip to 7*7 = 49 because any other smaller multiple of 7 will already have been eliminated.  That transforms the inner loop from this:

for (unsigned long int k = i + j; k < MAX; k += j) {

To this:

for (unsigned long int k = 2*i*(i+1); k < MAX; k += j) {

Print as you go

Rather than searching the entire array at the end, only the part of the array after \$\sqrt{\text{MAX}}\$ really needs to be searched since all of the other primes were identified during sieveing.  We can print them as they are discovered and save some time searching.

Increment rather than multiply

In the various loops, the expression 2*i+1 (stored in j in the first loop) is used to convert from an index to the actual numerical value it represents.  It speeds things up a bit if the code simply increments j each loop increment.

Clean up the loop

Instead of using nested while loops, it seems to me to be cleaner if you use a for loop and inner if to determine whether the number is composite and should simply be skipped, or prime and should be sieved.

A new sieve:

Using all of these suggestions, I get this, which is about 8% faster on my machine than the original.

void sieve2()
{
    bool *a = (bool*)malloc(MAX * sizeof(bool));
    if (a == NULL) {
        return;  // bad alloc
    }
    memset(a, true, MAX);
    const unsigned long sqrtMAX = ceil(sqrt(MAX));
    unsigned long i;
    unsigned long j;
    std::cout << "done\n";
    for (i=1, j=2*i+1; i < sqrtMAX; ++i, j+=2) {
        if (a[i]) {  // it's a prime
            std::cout << j << '\n';
            for (unsigned long int k = 2*i*(i+1); k < MAX; k += j) {
                if (a[k]) {
                    a[k] = false;
                }
            }
        }
    }
    for ( ; i < MAX; ++i, j+=2) {
        if (a[i]) { // it's a prime
            std::cout << j << '\n';
        }
    }
    free(a);
}

Further improvements

There are many other things you might explore to further speed the code:

Use bits rather than bytes to store values.
Reimplement with pointers rather than array indexing.
Create a multithreaded version

Use classes

Since this is supposed to be C++, why not use a class?  Here's a very simple class template that implements something like a bitset.

template <typename T>
class Bits
{
public:
    Bits(size_t sz) :
        bitsperunit{8*sizeof(T)},
        units{sz/bitsperunit + 1},
        bits{new T[units]}
    {
        for (unsigned i=0; i < units; ++i) {
            bits[i] = ~0;
        }
    }
    void clear(unsigned index) {
        unsigned n=index/bitsperunit;
        T mask=1u<<(index%bitsperunit);
        bits[n] &= ~mask;
    }
    bool operator[](unsigned index) const {
        unsigned n=index/bitsperunit;
        T mask=1u<<(index%bitsperunit);
        return bits[n] & mask;
    }
    virtual ~Bits() {
        delete[] bits;
    }
private:
    const unsigned bitsperunit;
    const size_t units;
    T *bits;
};

Now let's reimplement using that class:

void sieve3()
{
    Bits<uint8_t> a(MAX);
    const unsigned long sqrtMAX = std::ceil(std::sqrt(MAX));
    unsigned long i;
    unsigned long j;
    for (i=1, j=2*i+1; i < sqrtMAX; ++i, j+=2) {
        if (a[i]) {  // it's a prime
            for (unsigned long int k = 2*i*(i+1); k < MAX; k += j) {
                    a.clear(k);
            }
        }
    }
    std::cout << "done\n";
    for (i=1, j=3 ; i < MAX; ++i, j+=2) {
        if (a[i]) { // it's a prime
            std::cout << j << '\n';
        }
    }
}

On my machine this is still faster than either of the other two implementations (original or the one above).


Novice Tic Tac Toe in Java I've never done anything in java, but today I had a couple hours of free time so I decided I would learn it and make a Tic Tac Toe game. Please give me some constructive criticism on anything you would do differently.

import java.util.Scanner;
import java.util.stream.IntStream;;

public class TicTacToe {

    static Scanner scan = new Scanner(System.in);
    static char board[] = {'A','B','C','D','E','F','G','H','I'};
    static final int scoreboard[] = {1,2,7,6,0,4,3,8,9};
    static int player = 0;
    static boolean gameOver = false;
    static int score[][] = {{0,0,0,0,0},{0,0,0,0}};
    static int turn[] = {0,0,5};

    public static void main(String[] args) {
        while (gameOver == false) {

            askForTurn();

        }
    }

    public static void askForTurn() {

        System.out.printf("\n-------------\n| %s | %s | %s |\n-------------\n| %s | %s | %s |\n-------------\n| %s | %s | %s |\n-------------",board[0],board[1],board[2],board[3],board[4],board[5],board[6],board[7],board[8]);
        boolean valid = false;
        System.out.printf("\nPlayer %o's turn:", (player+1));
        char input = scan.next().toUpperCase().charAt(0);
        int i;
        for (i = 0; i < 9; i++) {

            if (input == board[i]) {

                if (player == 0) {

                    valid = true;
                    board[i] = 'O';
                    score[0][turn[0]] = scoreboard[i];
                    turn[0]++;

                } else {

                    valid = true;
                    board[i] = 'X';
                    score[1][turn[1]] = scoreboard[i];
                    turn[1]++;

                }

            }

        }

        if (valid == false) {

            System.out.print("\nInvalid square");
            askForTurn();

        } else {

            checkForWin();
            player++;

            if (player%2 == 0) {

                player = 0;
            }

        }

    }

    public static void checkForWin() {

        int sum1 = IntStream.of(score[0]).sum();
        int sum2 = IntStream.of(score[1]).sum();
        int x;

        ifWin(1, 0, 3, sum1%10, 0);
        ifWin(1, 0, 3, sum2%10, 1);

        for (x = 0; x < turn[0]; x++) {

            ifWin(1, 0, 4, (sum1 - score[0][x])%10, 0);

        }

        for (x = 0; x < (turn[1] - 1); x++) {

            ifWin(1, 0, 4, (sum2 - score[1][x])%10, 1);

        }

        if (turn[0] == 5) {

            for (x = 0; x < 3; x++) {

                ifWin(1, 0, 5, (sum1-score[0][0]-score[0][x+1])%10, 0);
                ifWin(x, 0, 5, (sum1-score[0][1]-score[0][x+1])%10, 0);
                ifWin(x, 1, 5, (sum1-score[0][2]-score[0][3])%10, 0);
                ifWin(x, 1, 5, 0, 2);

            }

        }

    }

    public static void endGame(int player) {

        if (player == 3) {

            System.out.print("\nTie!");

        } else {

            System.out.printf("Player %o wins!", player);

        }

        gameOver = true;

    }

    public static void ifWin(int x, int z, int turnCheck, int statement, int player) {

        if (x > z && turn[player] == turnCheck && statement == 0) {

            endGame(player + 1);

        }

    }

}

Thanks for sharing your code.
This looks pretty good for a novices code since you managed to remove some code duplication by using parameterized methods.
here is what I would change:
Naming
Finding good names is the hardest part in programming, so always take your time to think about the names of your identifiers.
Naming Conventions
Please read (and follow) the
Java Naming Conventions.
eg .: variables of type  boolean (like gameOver) or methods returning a  boolean should start with *is, has, can or alike.
Don't surprise your readers
Your method askForTurn() does nor only acquire user input, it actually does the turn. So it should better be named doTurn().
Magic numbers
replace the literal numbers by constants with expressive names.
Separation of Concerns
askForTurn() does more than one thing:

it acquires user input,
validates the user input
translates the input into a field position
checks the end condition

each of this actions should live in a method of its own.
hidden recursion/implicit error handling
In case of an invalid input you do a recursive call to askForTurn().
Beside that this also belongs in the Don't surprise your readers category it is a potential memory leak.
Use of the static key word
The only method that should be static in your code should be main.
For the rest of your code you should avoid the static key word unless you have a good reason to use it.
In your case it would be quiet easy to do:
move the while loop from main into a new (non static) method and call it on an instance of the class:
public static void main(String[] args) {
    new TicTacToe().runTheGame();
}

This would safe you from all the statics in your program.


Exporting array of tabs to new workbooks, save them based on tab name Brief:

This is a question geared towards optimization and general better coding practices. I'm self-taught in VBA so I'm cautious of bad practices, but they do come up.

My Reasoning:

I break the links (break_links) because these new files would be sent out and I don't want the external references to cause errors within the spreadsheet formulas. It also makes for better historical files IMO so as the main file is updated we have the historical numbers saved rather than referencing the new numbers.

I did take this component from somewhere online, so I'm still unsure how things work when you put something like (ByRef wb As Workbook) into the sub name ie: when to do it, how to do it, etc.

The Name reference is a cell that has a fiscal year and period labeled for historical files.

My Complaint:

Despite turning off screen updating and all that it still works like a clunky recorded macro where I sit for about 20-30 seconds while it does it's thing, and I'm just exploring to see if there's anything I can do to improve this.

Sub NewWorkbooks()
'This will make seperate workbooks for each of the tabs listed
Dim wb As Workbook
Dim NewBook As Workbook
Dim ws As Worksheet
Dim Name As String
Name = ActiveWorkbook.Worksheets("Reference Tables").Range("_Name")

Call TurnOffFunctions
Set wb = ActiveWorkbook

For Each ws In Workbooks("P&L Master Sheet"). _
Worksheets(Array("Accessories", "Leather", "Residential", "Rugs"))
    ws.Copy
    Set NewBook = ActiveWorkbook
    With NewBook
        Call break_links(NewBook)
        .SaveAs Filename:="C:\Users\Monthly P&L's\Historical Files\" & "P&L " & Name & " - " & ws.Name
        .Close 'SaveChanges:=True
    End With
Next

Call TurnOnFunctions

End Sub
Sub break_links(ByRef wb As Workbook)
Dim Links As Variant
On Error Resume Next
Links = wb.LinkSources(Type:=xlLinkTypeExcelLinks)
On Error GoTo 0

If Not IsEmpty(Links) Then
    For i = 1 To UBound(Links)
        wb.BreakLink _
        Name:=Links(i), _
        Type:=xlLinkTypeExcelLinks
    Next i
End If

End Sub
Private Sub TurnOffFunctions()

    Application.ScreenUpdating = False
    Application.DisplayStatusBar = False
    Application.DisplayAlerts = False
    Application.Calculation = xlCalculationManual
    Application.EnableEvents = False

End Sub
Private Sub TurnOnFunctions()

    Application.ScreenUpdating = True
    Application.DisplayStatusBar = True
    Application.DisplayAlerts = True
    Application.Calculation = xlCalculationAutomatic
    Application.EnableEvents = True

End Sub

I do have a few suggestions:

First of all, you don't need to reactivate your main
workbook every loop, since you hold it's reference in a variable
anyway.
Second, instead of creating new workbook and copying the sheet over, just make a copy to new workbook (i.e. wks.Copy creates copy in new workbook, you can then assign it to a variable since, as you said yourself, it is now the active workbook)
Lastly, unless you want to remove the references (for example if the links contain sensitive information), then I wouldn't worry about it. Excel stores the last value from links in cache within the file.

Depending on your setup, suggestion 2 might actually speed it up a little by reducing the number of actions.

To copy sheet to new workbook, simply ws.Copy, then you can set newWs = Activesheet (or using your naming set NewBook = Activeworkbook) to save it's reference, or just skip that also and put Activeworkbook into your With.


Python: Loop through folders, convert XLSX files to CSV removing non ASCII As the subject suggests:

Loop recursively through folders
All files are XLSX
Remove all non ASCII characters
Save as a CSV

Can it be improved with regards to speed? Would using openpyxl help?

import os
import xlrd
import csv

for subdir, dirs, files in os.walk("C:\Users\Alan\Downloads\Knowledge"):
    for file in files:
        filepath = subdir + os.sep + file

        wb = xlrd.open_workbook(filepath)
        sh = wb.sheet_by_index(0)
        csv_file = open(filepath.replace(".xlsx","")+'_csv.csv','wb')
        wr = csv.writer(csv_file,quoting=csv.QUOTE_ALL)

        for rownum in xrange(sh.nrows):
            wr.writerow([unicode(val).encode('ascii','ignore') for val in sh.row_values(rownum)])

        csv_file.close()

I'd have a couple of suggestions which you may want to follow:

Use openpyxl module to read xlsx documents, and the csv module to write.

An example might look like this:

import openpyxl
import csv

wb = openpyxl.load_workbook('test.xlsx')
sh = wb.get_active_sheet()
with open('test.csv', 'wb') as f:
    c = csv.writer(f)
    for r in sh.rows:
        # here, you can also filter non-ascii characters
        c.writerow([cell.value for cell in r])

Don't use os.sep.

From docs:

  [...] Note that knowing this is not sufficient to be able to parse or
  concatenate path names...

Instead, you could use normpath from os.path to normalize the path.

From the docs:

  Normalize a pathname by collapsing redundant separators and up-level
  references so that A//B, A/B/, A/./B and A/foo/../B all become A/B.
  This string manipulation may change the meaning of a path that
  contains symbolic links. On Windows, it converts forward slashes to
  backward slashes.

Don't use + separator to join certain paths. Instead, just use os.path.join().

As for the speed improvement, you'll need to go through all the subfolders in your parent folder and find all the .xslx files. Perhaps skipping non-xlsx files will bring a bit of speed: str.endswith('.xslx').


Creating a python dataframe by parsing JSON API response In this SO question the OP is unable to scrape a table from a dynamically loaded website. In monitoring the web traffic, via Chrome dev tools, I found that there is an API request made that returns a JSON string with the required info.

The following is the answer I wrote to extract the columns of interest from the API response.

Guide to columns of interest:

Course Title = title
Trainer = name (within trainers)
Rating = rating
Vendor = name (within vendors)
IT Path = path_label (within paths)
Skill Level = display (within difficulty)
Course URL = concatenation of base with seoslug

The Vendors field has missing items hence my use of an if statement in the assigment to vendors. I am not sure what the usual placeholder value is for missing string values in Python.

I use repeated list comprehensions in loops over the JSON object data; where data = response.json()

I couldn't think of a way to remove these repeated loops and still have legible code.

I generate a dataframe by joining the lists in a dictionary and then converting with pandas.

I welcome any and all feedback please.

JSON response:

Example JSON dictionary within response. The response has a collection of such dictionaries.

Python 3

import requests
import pandas as pd

def main():
    base = 'https://www.cbtnuggets.com/it-training/'
    response  = requests.get('https://api.cbtnuggets.com/site-gateway/v1/all/courses/for/search?archive=false')

    data = response.json()

    titles = [item['title'] for item in data]
    trainers = [item['trainers'][0]['name'] for item in data]
    ratings = [item['rating'] for item in data]
    vendors = [item['vendors'][0]['display'] if len(item['vendors']) != 0 else 'N/A' for item in data]
    paths = [item['paths'][0]['path_label'] for item in data]
    skillLevel = [item['difficulty']['display'] for item in data]
    links = [base + item['seoslug'] for item in data]

    df=  pd.DataFrame(
      {'Course Title': titles,
       'Trainer': trainers,
       'Rating': ratings,
       'Vendor': vendors,
       'IT Path': paths,
       'Skill Level': skillLevel,
       'Course URL': links
      })

    #print(df)
    df.to_csv(r'C:\Users\User\Desktop\Data.csv', sep=',', encoding='utf-8',index = False )

if __name__ == "__main__":

    main()

The only major thing I would change is that:

if len(item['vendors']) != 0

Is the same as:

if item['vendors']

Because an empty list will return back as False. If you want to try it out:

a = []
bool(a)      # False
b = [1,2,3]
bool(b)      # True

I would also be careful with what you have because those dictionaries that you are converting might have more than one value, in which case you would miss them. This is the line that I am referring to:

    paths = [item['paths'][0]['path_label'] for item in data]


Generating random email addresses The code below generates only 10 email domains. To me, this is bruteforce programming. Is there another random integer library? Could I use something like random.seed() in C++?

What is a more elegant way to generate 100 emails names using some Python libs/APIs?

import random
domains = [ "hotmail.com", "gmail.com", "aol.com", "mail.com" , "mail.kz", "yahoo.com"]
letters = ["a", "b", "c", "d","e", "f", "g", "h", "i", "j", "k", "l"]

def get_one_random_domain(domains):
        return domains[random.randint( 0, len(domains)-1)]

def get_one_random_name(letters):
    email_name = ""
    for i in range(7):
        email_name = email_name + letters[random.randint(0,11)]
    return email_name

def generate_random_emails():

    for i in range(0,10):
         one_name = str(get_one_random_name(letters))
         one_domain = str(get_one_random_domain(domains))
         print(one_name  + "@" + one_domain)

def main():
    generate_random_emails()

In get_one_random_name(), how can I make this email_name string grow with random letters without using "+ "?

How can I choose letters randomly and put them together without +?

From this Stack Overflow question :

import random

foo = ['a', 'b', 'c', 'd', 'e']
print(random.choice(foo))

In your case,

def get_one_random_domain(domains):
        return domains[random.randint( 0, len(domains)-1)]

becomes :

def get_one_random_domain(domains):
        return random.choice(domains)

and maybe removes the need for a function in the first place.

Your letters list can easily be defined with a list comprehension and some manipulation of ord and chr :

>>> [chr(ord('a')+i) for i in range(12)]
['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l']

As suggested in the comments, you can also use string.ascii_lowercase[:12]

Now, your get_one_random_name could be easily improved.

def get_one_random_name(letters):
    email_name = ""
    for i in range(7):
        email_name = email_name + letters[random.randint(0,11)]
    return email_name

using the previous trick, it becomes :

def get_one_random_name(letters):
    email_name = ""
    for i in range(7):
        email_name = email_name + random.choice(letters)
    return email_name

Also, here is a recommandation from PEP 8 :

  For example, do not rely on CPython's efficient implementation of
  in-place string concatenation for statements in the form a += b or a =
  a + b. This optimization is fragile even in CPython (it only works for
  some types) and isn't present at all in implementations that don't use
  refcounting. In performance sensitive parts of the library, the
  ''.join() form should be used instead. This will ensure that
  concatenation occurs in linear time across various implementations.

In your case, you can easily build a list of random letters and call join.

def get_one_random_name(letters):
    return ''.join(random.choice(letters) for i in range(7))

In generate_random_emails :

you don't need conversions.
you don't need temporary variables
you don't need the first argument of range if it is 0.

so you get:

def generate_random_emails():
    for i in range(0,10):
         print(get_one_random_name(letters) + '@' + get_one_random_domain(domains))

Also, it might be a good idea to return a list instead of printing values to make code easier to reuse.

At this stage, the code looks like :

import random
domains = [ "hotmail.com", "gmail.com", "aol.com", "mail.com" , "mail.kz", "yahoo.com"]
letters = string.ascii_lowercase[:12]

def get_one_random_domain(domains):
    return random.choice(domains)

def get_one_random_name(letters):
    return ''.join(random.choice(letters) for i in range(7))

def generate_random_emails():
    return [get_one_random_name(letters) + '@' + get_one_random_domain(domains) for i in range(10)]

def main():
    print(generate_random_emails())

if __name__ == "__main__":
    main()

A good idea would be to change the function names and add arguments instead of hard coded values :

import random
domains = [ "hotmail.com", "gmail.com", "aol.com", "mail.com" , "mail.kz", "yahoo.com"]
letters = string.ascii_lowercase[:12]

def get_random_domain(domains):
    return random.choice(domains)

def get_random_name(letters, length):
    return ''.join(random.choice(letters) for i in range(length))

def generate_random_emails(nb, length):
    return [get_random_name(letters, length) + '@' + get_random_domain(domains) for i in range(nb)]

def main():
    print(generate_random_emails(10, 7))

if __name__ == "__main__":
    main()


Triggering methods that change instance state on other objects using a Dictionary I've been reading up on Actions and delegates for the last couple of days, and I think I'm finally starting to understand, but I wanted to check that I was using them correctly, or at least using them in a way that isn't totally insane.

I'm using a text-based game (as such) as a platform for learning about what I suspect is called event-oriented programming. For a really terrible example, imagine the player has a torch that can set things on fire. I don't want the torch to do target.ChangeState(States.OnFire), that's giving the torch way too much power over other objects - nothing is preventing an action like nonFlammableRock.ChangeState(States.OnFire) I COULD have the torch tell the game manager entity that it wants to set something on fire, and then let the game manager decide if it's allowed to do so... but instead, I came up with this:

TestObject

internal class TestObject
{
    private string instanceVar;
    private Dictionary<string, Action> eventHandlers;

    public TestObject(string newInstanceVar)
    {
        this.instanceVar = newInstanceVar;
        eventHandlers = new Dictionary<string, Action>();
        eventHandlers.Add("event0", PrintInstanceVar);
        eventHandlers.Add("event1", InstanceVarToUpper);
        eventHandlers.Add("event2", ChangeOwnInstanceVar);
    }

    public void PrintInstanceVar()
    {
        Console.WriteLine(instanceVar);
    }

    public void InstanceVarToUpper()
    {
        instanceVar = instanceVar.ToUpper();
    }

    public void Event(string eventName)
    {
        eventHandlers[eventName].Invoke();
    }

    public void ChangeOwnInstanceVar()
    {
        instanceVar = "Object has changed its instance variable";
    }
}

Program

private static void Main(string[] args)
    {
        var test1 = new TestObject("First test obejct");

        test1.Event("event2");
        test1.Event("event0");
        test1.Event("event1");
        test1.Event("event0");

        Console.ReadKey();
    }

In short, I want to tell an object what has happened to it, and then let the object itself decide what to do. In this way, a torch can send a message "ignite" to any object, and the target object itself looks up "ignite" in its dictionary of behaviours, and reacts accordingly. So for a Rock, eventHandlers["ignite"] = () => Console.WriteLine("The rock isn't flammable, the torch had no effect"), whereas for HayBale, eventHandlers["ignite"] = () => Console.WriteLine("The bale of hay catches alight").

Is this a sane approach? Are there any traps I've missed that will come back to haunt me later? Is there a better way of doing this? I know that C# Events exist, but I don't fully understand them yet, and I don't know how to use them to achieve what I'm trying to do here - but if using Events would be better, how would I change this?

Note:

I know individual entities shouldn't be responsible for printing to console - if I made this whole thing, they'd be sending updates as to their state, strings containing action text etc to the game manager, which would be responsible for displaying messages for the player.

This looks ok to me on first glance.

I can only suggest the use of an Enum to strongly type the events that you are registering.

This allows you to search the code base quickly and avoids any typo's you may make:

    private string instanceVar;
    private Dictionary<string, Action> eventHandlers;

    public TestObject(string newInstanceVar)
    {
        this.instanceVar = newInstanceVar;
        eventHandlers = new Dictionary<Events, Action>();
        eventHandlers.Add(Events.event0, PrintInstanceVar);
        eventHandlers.Add(Events.event1, InstanceVarToUpper);
        eventHandlers.Add(Events.event2, ChangeOwnInstanceVar);
    }

    public enum Events
    {
        event0,
        event1,
        event2
    }


More elegant array multiplication than multiple concat statements Background

Trevor wishes to duplicate an array, transforming it by multiplying its elements four times.

Existing Solution

//
var atest =   ['a','b','c'];
var btest =   [];
btest     =   atest.concat(atest).concat(atest).concat(atest);
//
console.log(btest.length);  // btest.length == 12
console.log(btest);         // ["a", "b", "c", "a", "b", "c", "a", "b", "c", "a", "b", "c"]

Goal

Trevor does not like this because it is ugly. Can you help Trevor find his happy place again? It's bugging Trevor.

Here's one that uses call and manually concat a

var a = ['a','b','c'], b = Array.prototype.concat.call(a,a,a,a);

If you wanted an easy way to variably add a, just change i:

for(var a=["a","b","c"],b=[],i=4;i--;) b.push.apply(b,a);


Main loop for a text-based Blackjack game Is there any way to reduce this code? It repeats itself, but I cannot find any way to reduce it because it is only repeating inside one elif. Sorry I'm a beginner my code is quite bad
It repeats here:
           onze(joueurs)
           print("joueur", a + 1, "a", joueurs[a])
           time.sleep(1)
           while sum(joueurs[a]) < 21 or 11 in joueurs[a]:

The code :
while t:
    input("Appuyez sur entrer pour continuer...")
    for a in range(nb):
        while sum(joueurs[a]) < 21 or 11 in joueurs[a]:
            time.sleep(0.5)
            print("main du joueur", a + 1, ":", joueurs[a], "Total :", sum(joueurs[a]))
            if sum(joueurs[a]) > 21 and 11 in joueurs[a]:
                onze(joueurs)
                print("joueur", a + 1, "a", joueurs[a])
            ask = input("Joueur {} voulez vous une carte ?(oui/non)".format(a + 1))
            if ask == "oui":
                askoui(joueurs)
            else:
                asknon()
                t = False
                break
        if sum(joueurs[a]) == 21:
            print("joueur", a + 1, "a BLACKJACK")
            time.sleep(1)
        elif sum(joueurs[a]) > 21 and 11 in joueurs[a]:
            onze(joueurs)
            print("joueur", a + 1, "a", joueurs[a])
            time.sleep(1)
            while sum(joueurs[a]) < 21 or 11 in joueurs[a]:
                time.sleep(0.5)
                print("main du joueur", a + 1, ":", joueurs[a], "Total :", sum(joueurs[a]))
                if sum(joueurs[a]) > 21 and 11 in joueurs[a]:
                    onze(joueurs)
                    print("joueur", a + 1, "a", joueurs[a])
                ask = input("Joueur {} voulez vous une carte ?(oui/non)".format(a + 1))
                if ask == "oui":
                    askoui(joueurs)
                else:
                    asknon()
                    t = False
                    break
            elif sum(joueurs[a]) > 21:
                perdu(joueurs)
                t = False
        elif sum(joueurs[a]) > 21:
            perdu(joueurs)
            t = False

It's "Pythonic" to iterate through items, so assuming that len(joueurs) == nb, your main for loop can be:
    for a, joueur in enumerate(joueurs):

which means all your joueurs[a] become joueur. I'm quite surprised that onze, askoui and perdu functions take the whole joueurs list, not just the current player. How does that work?
I'd guess that the check for possible onze action, which I take it is converting an ace down from 11 points to 1, could more comfortably fit within the askoui function when our current player gets another card, non? Then this main routine could just check against 21, or perhaps even better the askoui function could return a status value.
Difficult to be more specific without getting into some of these auxiliary functions. Here's my best guess at a reduced code that I think should do the same  as you presently have, plus some outstanding issues:
while t:
    input("Appuyez sur entrer pour continuer...")
    for ix, jr in enumerate(joueurs):
        jID = ix+1
        while sum(jr) < 21:
            time.sleep(0.5)
            print("main du joueur", jID, ":", jr, "Total :", sum(jr))
            ask = input("Joueur {} voulez vous une carte ?(oui/non)".format(jID))
            if ask == "oui":
                askoui(joueurs) # why all joueurs?
                if sum(jr) > 21 and 11 in jr:
                    onze(joueurs) # why all joueurs?
            else:
                asknon()
                t = False # immediate halt to play for all players?
                break
        if sum(jr) == 21:
            print("joueur", jID, "a BLACKJACK")
            time.sleep(1) # then play continues
        elif sum(jr) > 21:
            perdu(joueurs) # why all joueurs?
            t = False
        # play continues without message if sum(jr)<21 ?


Re-arranging data on a worksheet based on contents of Column A The following Macro sorts through a worksheet, copying and removing lines based on what appears in column A. If ("A" & i) is not a number, it triggers and moves the data to the appropriate place. The script at the moment is very slow, however. Is there an easier way to achieve this?

Sub FixAbstracts()
Application.ScreenUpdating = False
Dim Rng As Range
Dim i As Long
i = 2
Dim row As Range
Dim lastRow As Long
lastRow = Range("A:A").Cells.SpecialCells(xlCellTypeConstants).Count

While i <= lastRow
    Set Rng = Range("A" & i)
    Set row = ActiveSheet.Rows(i)
    lastRow = Range("A:A").Cells.SpecialCells(xlCellTypeConstants).Count
    If Application.WorksheetFunction.IsNumber(Rng.Value) = True Then
        i = i + 1

    ElseIf IsNumeric(Rng.Value) = False And Application.WorksheetFunction.CountA(Rng) = 1 Then
        Rng.Copy Destination:=Rng.Offset(-1, 13)
        Rng.Delete Shift:=xlToLeft
    ElseIf IsEmpty(Rng) = True And Application.CountA(row) > 0 Then
        Rng.Delete Shift:=xlToLeft
    ElseIf IsEmpty(Rng) = True And Application.CountA(row) = 0 Then
        row.Delete

    Else: Stop
    End If
Wend

End Sub

Your nesting ifs have things in common, so you can restructure it
Option Explicit
Public Sub FixAbstracts()
    Dim lastRow As Long
    lastRow = Cells(Rows.Count, 1).End(xlUp).row
    Dim i As Long
    Dim rowCount As Long
    Dim isEmptyCell As Boolean

    For i = lastRow To 2 Step -1
        isEmptyCell = IsEmpty(Cells(i, 1))
        If isEmptyCell Then
            rowCount = Application.CountA(Rows(i))
            If rowCount > 0 Then Cells(i, 1).Delete xlshift:=xlToLeft
            If rowCount = 0 Then Rows(i).Delete
        ElseIf Not IsNumeric(Cells(i, 1)) Then
            Cells(i - 1, 13) = Cells(i, 1)
            Cells(i, 1).Delete xlshift:=xlToLeft
        End If
   Next i
End Sub

Now you have a For loop so no need to increment i.
It also steps backwards bottom to top so nothing is skipped. If you delete row(10) and go to the next row, the original row(11) is skipped. That's a result of the For loop, but it prefer it much to a Do While.

Is it empty?
We also declared a boolean to check for empty so it's only done once per loop. Now we base our if on that to reduce multiple checks.
So now the logic is

empty? Yes? Count the row. Now do something
empty? No? is it a number by itself? Yes? Do something

Is it a number?
Let's look at the logic of yours

is it a number? yes? goto next iteration
is it a number? no? is it empty? yes? Do something
is it empty? yes? count the values in the row = 0 then do something
is it empty? yes? count the values in the row > 0 then do something

See the reduction? We get rid of the initial isnumeric because the loop will iterate by itself. We also check for empty only once and we only count once.
Not to mention your code is very difficult to follow.
If Application.WorksheetFunction.IsNumber(Rng.Value) = True Then
    i = i + 1

ElseIf IsNumeric(Rng.Value) = False And Application.WorksheetFunction.CountA(Rng) = 1 Then
    Rng.Copy Destination:=Rng.Offset(-1, 13)
    Rng.Delete Shift:=xlToLeft
ElseIf IsEmpty(Rng) = True And Application.CountA(row) > 0 Then
    Rng.Delete Shift:=xlToLeft
ElseIf IsEmpty(Rng) = True And Application.CountA(row) = 0 Then
    row.Delete

Else: Stop
End If

I had to reread this a bunch of times and I'm still not sure if I'm doing all the checking you are. You aren't consistent with how you're checking for a numeric value, you use a worksheet function and the vba function, there's probably no need to use the worksheet function if your data is standardized.
There's also no need to check if something like isempty = true as the result of isempty is either true or false.


Multithreaded number printer in ordered manner On recent interview I was asked to write program that print "Zero" and "One" to stdout from different threads, but in ordered manner:
zero
one
zero
one
zero
one
...

Now I decided to write more generalized solution and this is my first attempt:
#include <string>
#include <vector>
#include <mutex>
#include <initializer_list>
#include <condition_variable>
#include <thread>
#include <iostream>

struct OrderNumberPrinter
{
    OrderNumberPrinter(const std::initializer_list<std::string>& list)
        : m_len(list.size())
        , m_cv(list.size())
    {
        size_t num = 0;
        for(auto it = list.begin(); it != list.end(); ++it, ++num)
        {
            m_workers.emplace_back([this, num, message = *it](){
                while(true)
                {
                    std::unique_lock<std::mutex> lock(m_printLock);
                    m_cv[num].wait(lock, [this, num](){ return m_current == num; });
                    std::cout << message << "\n";
                    std::this_thread::sleep_for(std::chrono::seconds(1));
                    ++m_current;
                    if (m_current == m_len)
                    {
                        m_current = 0;
                    }
                    m_cv[m_current].notify_one();
                }
            });
        }
    }

    ~OrderNumberPrinter()
    {
        for(auto& w: m_workers)
        {
            if (w.joinable())
            {
                w.join();
            }
        }
    }
private:
    int m_len = 0;
    int m_current = 0;
    std::vector<std::condition_variable> m_cv;
    std::mutex m_printLock;
    std::vector<std::thread> m_workers;
};

int main(int argc, char* argv[])
{
    OrderNumberPrinter printer = {"zero", "one", "two", "three"};
    return 0;
}

Here are some things that may help you improve your program.
If it has private members, it should be a class
The OrderNumberPrinter struct really should be a class because it has private member data.  See C.8
Use std::size_t rather than int
The values m_len and m_current should both probably be std::size_t rather than int, not least because they are compared to num and initialized with list.size() which are both explicitly std::size_t.
Exercise all functions
There is no way that the destructor ever gets called in the current code because there is no exit condition in the code.  For that reason, how do you know if it works?  I'd ask that for an interview.  I'd also ask why w.joinable() is tested before calling w.join().  Is there a circumstance in which w.joinable() will return false?
Think about further derivation
What would happen if a user derived from this class?  I'd recommend either declaring the destructor as virtual or declaring the class final to prevent derivation.
Be clear about what each mutex is locking
The name m_printLock suggests that the shared resource being protected is std::cout.  If we used std::basic_osyncstream as suggested in a comment, would you still need a mutex?  If so, what would it then be locking?  Generally, I like to have each mutex just lock one thing because it simplifies maintenance and clarifies the intent to other programmers.


Nodejs async with express server I use the following code which works as expected. The code uses open ID connect to login user. Because I'm pretty new to node and express, it will be great if I can get some tips for the async usage- e.g. if I’m doing it and the error handling correctly.
The code is doing connect via oidc layer
https://en.wikipedia.org/wiki/OpenID_Connect
which does a user login according to client secret and client id, then the authorization server is calling to the redirect route of the application  and if everything is OK the user logged in to the system.
This is the index.js
const express = require('express');
const logon = require('./logon');

const app = express();
const port = process.env.PORT || 4000;

logon(app)
  .then(() => {
    console.log('process started');
  });
app.use(express.json());

app.listen(port,
  () => console.log(`listening on port: ${port}`));

This is the logon.js
const { Issuer, Strategy } = require('openid-client');
const cookieParser = require('cookie-parser');
const cookieSession = require('cookie-session');
const azpi = require('./azpi');
const bodyParser = require('body-parser');
const passport = require('passport');

module.exports = async (app) => {

  let oSrv;
  const durl = `${process.env.srvurl}/.well-known/openid-configuration`;
  try {
    oSrv = await Issuer.discover(durl);
  } catch (err) {
    console.log('error occured', err);
    return;
  }

  app.get('/', prs(), passport.authenticate('oidc'));

  const oSrvCli = new oSrv.Client({
    client_id: process.env.ci,
    client_secret: process.env.cs,
    token_endpoint_auth_method: 'client_secret_basic',
  });

  passport.serializeUser((user, done) => {
    done(null, user);
  });
  passport.deserializeUser((obj, done) => {
    done(null, obj);
  });

  const cfg = {
    scope: 'openid',
    redirect_uri: process.env.ruri,
    response_type: 'code',
    response_mode: 'form_post',
  };

  const prs = () => (req, res, next) => {
    passport.use(
      'oidc',
      new Strategy({ oSrvCli , cfg }, (tokenset, done) => {
        const claims = tokenset.claims();
        const user = {
          name: claims.name,
          id: claims.sub,
          id_token: tokenset.id_token,
        };
        return done(null, user);
      }),
    );
    next();
  };
  app.use(
    bodyParser.urlencoded({
      extended: false,
    }),
  );
  app.use(cookieParser('csec'));
  app.use(
    cookieSession({
      name: 'zta-auth',
      secret: 'csect',
    }),
  );

  app.use(passport.initialize());
  app.use(passport.session());

  app.get('/redirect', async (req, res, next) => {
    passport.authenticate('oidc', async (err, user) => {
      if (err) {
        console.log(`Authentication failed: ${err}`);
        return next(err);
      }
      if (!user) {
        return res.send('no identity');
      }

      req.login(user, async (e) => {
        if (e) {
          console.log('not able to login', e);
          return next(e);
        }
        try {
          const url = await azpi.GetUsers(user.id_token);
          return res.redirect(url);
        } catch (er) {
          res.send(er.message);
        }
      });
    })(req, res, next);
  });
};

Is my async code usage is okay?

Comments
While the code is mostly simple to follow, it would be good to add comments to document what decisions you made. Even if you are the only one maintaining this code your future self might not remember what considerations you had in the past. At least document the input, output and purpose of functions. Some style guides call for comments to be in certain formats - e.g. JSDoc in Google style guide.
Nesting Levels
The nesting levels gets a bit deep towards the end of logon.js- some might describe it a bit as “callback hell”. callbackhell.com has some good tips for keeping code shallow- like naming anonymous functions so they can be moved out (also may allow for use in unit and/or feature tests), modularize (which is already done somewhat with logon.js), etc.
Variable name format
It would be best to use consistent formatting of variable names. I have worked on a PHP codebase that makes quite extensive use of hungarian notation but I am not fond of it and I haven't seen it used very often with JavaScript code. I happened to recently see this Stack Overflow question about Hungarian notation and while it is marked as off-topic it does have many answers. The top voted answer contains a link to an article by Joel Spolsky, CEO and co-founder of Stack Overflow: Making Wrong Code Look Wrong. I know realize that much of the code throughout that codebase is “Systems Hungarian”.
It appears there are two variables named using Hungarian notation (i.e. oSrv and oSrvCli) while most all other variables are all lowercase or camelCase. There seems to be one other outlier: durl- which seems to be the discovery URL. One could name that as discoveryURL, or take the server URL/host name out of it and make it a real constant - e.g. const DISCOVERY_PATH = '/.well-known/openid-configuration'; , which can be appended to process.env.srvurl when used.
Async functions
As was mentioned in a comment, the async keyword has likely been applied to more functions that necessary. For example, only functions that contain the await keyword need to be declared as asynchronous- e.g. the function assigned to module.exports, the callback to req.login()).
Error handling

By default, if authentication fails, Passport will respond with a 401 Unauthorized status, and any additional route handlers will not be invoked.1

passport.authenticate('oidc', async (err, user) => {
    if (err) {
        console.log(`Authentication failed: ${err}`);
        return next(err);
    }

This error handling seems fine. Would it be helpful to log the user object in the callback to the call to passport.authenticate()? If so, consider sanitizing any sensitive information that may be attached. You could also consider an error logging service.
It appears your code follows the Custom Callback option for passport.authenticate(). The sample under that section Custom Callback checks for err and when it is anything other than false-y it will return next(err)

If the built-in options are not sufficient for handling an authentication request, a custom callback can be provided to allow the application to handle success or failure.

app.get('/login', function(req, res, next) {
  passport.authenticate('local', function(err, user, info) {
    if (err) { return next(err); }

If authentication failed, user will be set to false. If an exception occurred, err will be set. An optional info argument will be passed, containing additional details provided by the strategy's verify callback.

It may be useful for you to read other reviews about nodeJS and passport as well as nodeJS and error-handling. The posts below might also be helpful:

Express Passport (node.js) error handling
How to show custom error messages using passport and express

Promisifying passport.authenticate()
This Stack overflow post might be interesting if you want to use await with the call to passport.authenticate():

I've been failing to get passport.authenticate to work at all inside of an async/await or promise pattern. Here is an example I feel should work, but it fails to execute passport.authenticate().

const passport = require("passport");
let user;
try {
   user = await __promisifiedPassportAuthentication();
   console.log("You'll never have this ", user);
} catch (err) {
   throw err;
}
function __promisifiedPassportAuthentication() {
    return new Promise((resolve, reject) => {
        console.log("I run");
        passport.authenticate('local', (err, user, info) => {
             if (err) reject(err);
             if (user) resolve(user);
         });
    });
}

The self-answer claims that the OP needed to invoke the function returned by the call to passport.authenticate() passing req and res, which likely need to get passed to __promisifiedPassportAuthentication().


Struct with abstract type member in c I am writing C code that has some dynamic arrays/list that are represented with a struct that contains the number of elements and a pointer to the elements.

I have some functions that are doing some similar work with that structs, for example printing the elements. To use the same code I decided to create a struct to that holes an abstract data type and the printing function receives a pointer to that struct.

A specific struct looks like that:

typedef struct {
    int numOfElements;
    int *element;

} LIST_INT, *PLIST_INT;

The generic struct look like that:

typedef struct {
    int numOfElements;
    void *element;

} LIST_GEN, *PLIST_GEN;

Is that the right approach?

This is the code example:

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

typedef struct {
    int numOfElements;
    char *element;

} LIST_CHAR, *PLIST_CHAR;

typedef struct {
    int numOfElements;
    int *element;

} LIST_INT, *PLIST_INT;

typedef struct {
    int numOfElements;
    void *element;

} LIST_GEN, *PLIST_GEN;

typedef void(*fpPrintElement)(void *element);

void print_list(PLIST_GEN list, int elementSize, fpPrintElement printElement) {
    int i = 0;
    int num = list->numOfElements;

    for (i = 0; i < num; ++i) {
        printElement(((char *)(list->element) + i * elementSize));
    }

    return;
}

void print_int(int * element) {
    printf("%d ", *element);
}

void print_char(char * element) {
    printf("%c ", *element);
}

void main() {
    LIST_INT list_int;
    list_int.numOfElements = 10;

    list_int.element = malloc(sizeof(list_int.element) * list_int.numOfElements);
    for (int i = 0; i < list_int.numOfElements; ++i) {
        list_int.element[i] = i * 1000;
    }

    LIST_INT list_char;
    list_char.numOfElements = 7;

    list_char.element = malloc(sizeof(list_char.element) * list_char.numOfElements);
    for (int i = 0; i < list_char.numOfElements; ++i) {
        list_char.element[i] = i + 35;
    }
    print_list((PLIST_GEN)&list_int, sizeof(*list_int.element), print_int);
    print_list((PLIST_GEN)&list_char, sizeof(*list_char.element), print_char);
    return;
}

Your code is valid. However note that

sizeof(list_int.element)

and

sizeof(list_chat.element)

return the size of a pointer, but it is not the de size of the type of elements into array. So, for instance, you need to write

sizeof(*list_int.element)


Replacing all the 'u' with specific character that is given by the user I got this question for a Junior Software Developer interview. And came up with this solution in Java:

public class Main {

  public static void main(String[] args) {

    Scanner in = new Scanner(System.in);
    if (!in.hasNextLine()) {
      System.out.println("error");
    }

    String str = in.nextLine();

    char ch = in.next().charAt(0);

    replaceChar(str, ch);
  }

  private static void replaceChar(String str, char ch) {

    String newStr = "";
    for (int i = 0; i < str.length(); i++) {
      if (str.charAt(i) == 'u') {

        newStr = str.replace(str.charAt(i), ch);
      } else {
        System.out.println("there is no \'u\'");
      }
    }

    System.out.println(newStr);
  }
}

The interviewer said this question can be tricky and didn't say anything after my question. What do you think?

After the interview I realized that I messed up with this part:

System.out.println("there is no \'u\'");

I thought this is going to be useful if there is no 'u' in the string but instead it's printed after every character thats no 'u'

Rather than rewrite your code here are some pointers for your next interview.  Yes, I have sat on the interviewer's side of the table a few times.

Whenever you are taking input directly from a user, as here, a rule-of-thumb is that you need to spend about one third of your code checking that the input is valid.  For example, you ignore multiple character input for the second line.  If your user types in "armadillo" at least give warning that the input is too long and you are ignoring the "rmadillo" part.  A few on-screen user prompts would have been nice as well.

One of the things the interviewer is testing is how well you know the more common Java libraries.  As @Adriano said, using String.replaceAll() would be both easier to code and show that you know the libraries.

Another thing being tested is if you are aware of the memory/processing penalties of working with Strings.  Java Strings are immutable so only the built-in methods can change things under the hood.  If you are going to make multiple changes from the outside then do not make the changes directly to the original string.  Convert the String to either a char[] or to a StringBuilder.  Make the changes there and convert the final result back to a String.

Did the interviewer ask you to print a message if there was no 'u' present in the original string?  If not then don't add extra stuff like that.  In a programmer job you have to code what is specified, no more and no less.  The replaceChar() method is the type of general-purpose function that is buried somewhere in a library.  Library users do not want extraneous messages popping up from apparently nowhere.  Unless otherwise specified, the only output from a method should be void, a return value or possibly an exception.  Your printing is a side-effect, and so to be avoided unless specifically required.  I would have written the method as:

private static String replaceChar(String str, char ch) { ... }

and done the printing from main().

You have a 'magic number', the constant 'u'.  Better to pull that out as a declared constant so it is obvious what it is, and to show you are aware of the need to avoid 'magic numbers' in code.

As a minor tweak, you could have checked if the input character was 'u' and immediately returned the original string unchanged.


ksh script to perform an upgrade Could you please review my script?

#!/bin/ksh
T=/bb/bdap/bin/cms/tools
U=$T/upgrader
mv $T/conbase_updates $U/conbase_updates.br.drqs$1
cd $U
grep "^BR" config.alpha > config.br
./ConbaseUpgrader config.br conbase_updates.br.drqs$1

You might want to validate the value of $1, or at least that it's not empty, for example add a line like this near the top:

test "$1" || { echo usage: $0 param; exit 1; }

You are not checking for errors, for example what will happen if the mv command fails for whatever reason? Maybe you have no reason to suspect a problem there, but just in case it would be good to add the -e flag to the shebang (#!/bin/ksh) so that execution will stop immediately there. Otherwise execution would continue after the failed mv, and the grep ... > ... might clobber a file by accident.

In any case, adding the -e flag in the shebang is a good practice in general.

It seems your variables will not normally contain spaces. If they ever do, remember to quote them properly, for example:

mv "$T/conbase_updates" "$U/conbase_updates.br.drqs$1"

(in your current example it's unnecessary)


Cross-platform MySQL database backup using mysqldump In a Laravel (PHP framework) project, we implemented a backup feature - backing up user-generated files and database tables. What we've searched so far, the best solution in backing up mysql database is to run the mysqldump command.

So we made the command:

<?php
$storage_path = 'path/to/backups';
$database = env('DB_DATABASE');
$user     = env('DB_USERNAME');
$pass     = env('DB_PASSWORD');
$host     = env('DB_HOST');
$dir_file = "{$storage_path}/{now()->timestamp}-database.sql.gzip";

$backup_cmd = "mysqldump --user={$user} --password={$pass} --host={$host} {$database} | gzip > {$dir_file}";

// Run the command and take the backup.
exec( $backup_cmd, $output, $mysql_backup_status );

In our MS Windows development environment, we got the errors:

dump($output);
array(
   "'mysqldump' is not recognized as an internal or external command",
   "operable program or batch file."
)

The issue is, path to mysqld or mysqldump is not in the system PATH. After adding the path to PATH and restarting the system the command works fine and take backup.

To make the solution cross-platform-friendly we took an initiative to check whether the command mysqldump is available or not, if not we'll gonna take the path to mysqldump.exe from the user.

And with user input we will change the command like below:

// Check default way - the PATH.
exec( 'mysqldump --version 2>&1', $output, $return_val );
if( $return_val != 0 ) {
   //mysqldump in undefined, so get the user defined path to it
   $path = "C:\laragon\bin\mysql\mysql-5.7.19-winx64\bin"; //get as user input
   $mysqldump = "{$path}\mysqldump.exe";
} else {
   $mysqldump = 'mysqldump';
}

$backup_cmd = "{$mysqldump} --user={$user} --password={$pass} --host={$host} {$database} | gzip > {$dir_file}";

// Run the command and take the backup.
exec( $backup_cmd, $output, $mysql_backup_status );

With this measure the code is working well in both the cases:

mysqldump is in PATH
mysqldump is in NOT in PATH

In our mini test unit it worked. But I wonder:

Is it a balanced way of doing this?
Means, the workaround we did (with all the UI/UX, unwanted trailing slash removal, using of exec() etc.) to simply check how we can run mysqldump without glitch - are we doing good? Or, simply making an easy thing critical?
Is the code cross-OS-friendly?

PS:
We're checking --version but finally executing backup command, because running backup command for testing executes it even with PHP system() function, and create a backup file, and we don't want that. We took exec() over system() as it's $output and $return_value seemed more reliable and silent than of system().

Edit

With the answer by baot a good point came into view, the unescaped user-provided data into exec() command. I's aware about it, but finally forgot it to escape.

I now escaped the user input with escapeshellarg() before using 'em into the exec():

// escape unwanted shell command
$mysqldump = escapeshellarg($mysqldump);

$backup_cmd = "{$mysqldump} --user={$user} --password={$pass} --host={$host} {$database} | gzip > {$dir_file}";

Is the code cross-OS-friendly?

Sure it is working, but IMHO one should never use exec() especially in costumer software with user supplied data to it. I'am gueesing you have to logged in to a admin account but still it's a huge security risk. But if you insist on using it, you should at a minimum check that the user supplied path is a vaild path and do a md5 hash of mysqldump.

I would just use a php native solution like mysqldump-php instead of using exec().

Is it a balanced way of doing this?

You have solved the problem in a simple way, so thats good. The only thing i can see as a hurdle is if you have costumers that isn't that tech savvy. it could be hard for them to find the correct path and/or knowing if mysqldump is installed.

could you flesh out this question further, i dont understand what you are asking? (cant comment yet)


Clang preprocessor concatenates an extra space vs. gcc - standard C99 I maintain a C library project both on gcc and clang.

I am looking for advice on how to make the following macros more portable, terse, readable or standard abiding.

When I compile

#define STRINGIFY(arg) STRINGIFY_(arg)
#define STRINGIFY_(arg) #arg

#define VERSION 1.01
#define PROJECT myProject
#define SUB_PROJECT mySub
#define DASH -

#include STRINGIFY(PROJECT DASH SUB_PROJECT DASH VERSION.c)

I get

clang 6.0.0 standard C99

myProject - mySub - 1.01.c

gcc 7.4.0 standard C99

myProject-mySub-1.01.c

I came up with the following solution

#define STRINGIFY(arg) STRINGIFY_(arg)
#define STRINGIFY_(arg) #arg
#define ID(arg) arg

#define VERSION 1.01
#define PROJECT myProject
#define LIB myLib
#define DASH -
#define DOTC .c

#define FILEVERSION(a, b, c, d, e) STRINGIFY(ID(a)ID(b)ID(c)ID(d)ID(e)ID(DOTC))

#include FILEVERSION(PROJECT, DASH, LIB, DASH, VERSION)

I think this is beyond the jurisdiction of the C standard.  See https://stackoverflow.com/questions/37796947/spaces-inserted-by-the-c-preprocessor.  All the standard says on the topic is a footnote:

  Note that adjacent string literals are not concatenated into a single string literal (see the translation phases in 5.1.1.2); thus, an expansion that results in two string literals is an invalid directive.

https://port70.net/~nsz/c/c11/n1570.html#note170

But this doesn't really matter since you are not making multiply strings.

Investigating GCC and Clang...

GCC does different things depending on what you ask for:

STRINGIFY(PROJECT DASH SUB_PROJECT DASH VERSION.c)

... gets me "myProject - mySub - 1.01.c" with GCC 9.3 and -E, but ...

#include STRINGIFY(PROJECT DASH SUB_PROJECT DASH VERSION.c)

gets me myProject-mySub-1.01.c: No such file or directory.  So -E has spaces and #include doesn't.

Clang is at least consistent.  With Clang 10, #include has spaces: fatal error: 'myProject - mySub - 1.01.c' file not found And Clang 10 and -E also has spaces myProject - mySub - 1.01.c.

In my opinion, Clang's behavior makes the most sense.  Suppose you wanted a space -- how else could you do that?

#define ID(arg) arg

This is a common trick.  I think it's a good idea although it looks a little ugly when you have to use it in so many spots.

#define STRINGIFY(arg) STRINGIFY_(arg)
#define STRINGIFY_(arg) #arg

This is fine, but I think it'd be clearer to define STRINGIFY below the helper macro it uses.

Can you get away with something like:

#if VERSION == 1
#include "library-version-1.h"
#else
#include "library-version-other.h"
#endif


Parsing data from XML in C#, filtering, categorizing in LINQ, displaying on Chart control This is my sample XML file:

<?xml version="1.0" encoding="utf-8"?>
<receipts>
  <receipt id="F66BF89E-1070-4E02-9B98-B934A66D0E16" year="2014" month="9" day="17" place="Place#1">
    <purchase name="Coca-Cola" quantity="1" price="3.99" category="Drinks" />
    <purchase name="Watermelon" quantity="1" price="1.98" />
    <purchase name="Water" quantity="1" price="1.49" category="Drinks" />
  </receipt>
  <receipt id="13ECD7F9-F639-4E65-9C80-9E1A8C2E305B" year="2014" month="9" day="17" place="Place#1">
    <purchase name="Sweets" quantity="1" price="3.98" category="Sweets" />
    <purchase name="Water" quantity="1" price="1.99" category="Drinks" />
    <purchase name="Flour" quantity="1" price="1.99" />
    <purchase name="Bread" quantity="1" price="1.19" />
    <purchase name="Milk" quantity="1" price="1.98" category="Drinks"/>
    <purchase name="Potatoes" quantity="0.345" price="1.49" />
  </receipt>
</receipts>

And this is my approach to parsing, filtering, categorizing and displaying it:

// load XML
XElement xmlReceipts = XElement.Load(@".\receipts.xml");

// create objects from XML
List<Receipt> receipts = new List<Receipt>();

foreach (var xmlReceipt in xmlReceipts.Elements("receipt"))
{
    Guid id = new Guid(xmlReceipt.Attribute("id").Value);
    string place = xmlReceipt.Attribute("place").Value;
    int year = Int32.Parse(xmlReceipt.Attribute("year").Value);
    int month = Int32.Parse(xmlReceipt.Attribute("month").Value);
    int day = Int32.Parse(xmlReceipt.Attribute("day").Value);

    List<ProductLine> products = new List<ProductLine>();

    foreach (var xmlProduct in xmlReceipt.Elements("purchase"))
    {
      string name = xmlProduct.Attribute("name").Value;
      string quantity = xmlProduct.Attribute("quantity").Value;
      string price = xmlProduct.Attribute("price").Value;
      string category = xmlProduct.Attribute("category") == null ? "(no category)" : xmlProduct.Attribute("category").Value;
      products.Add(new ProductLine(name, quantity, price, category));
    }
    receipts.Add(new Receipt(id, place, year, month, day, products));
}

// filter to this month's receipts
var receiptsFiltered = from receipt in receipts
                 where receipt.Date.Year == DateTime.Now.Year &&
                 receipt.Date.Month == DateTime.Now.Month
                 select receipt.Products as List<ProductLine>;

// flatten lists of receipts and products
var productsFiltered = from products in receiptsFiltered
                 from product in products
                 select new { Name = product.Name, Price = product.Price, Quantity = product.Quantity, Category = product.Category };

// group products by category
var productsByCategory = from product in productsFiltered
                 group product by product.Category;

// calculate totals per category
Dictionary<string, double> categoriesTotals = new Dictionary<string, double>();

foreach (var category in productsByCategory)
{
    double totalSum = 0;

    foreach (var product in category)
    {
      totalSum += product.Quantity * (double)product.Price;
    }

    categoriesTotals.Add(category.Key, totalSum);
}

// show data on chart
chart1.Series[0].Points.DataBindXY(categoriesTotals.Keys, categoriesTotals.Values);

Is there a way to simplify it? I tried to use XML to LINQ or linking XML directly to Chart but I couldn't manage grouping before flattening the lists.

Sources:

https://stackoverflow.com/a/6429081/1813219
https://stackoverflow.com/a/8135087/1813219
https://stackoverflow.com/a/7325306/1813219

For the LINQ query at the end SelectMany is your friend.

You should be able to achieve your desired results in a single query:

var dateOfInterest = DateTime.Now; // just put in whatever date interests you

var categoryTotals = receipts
         // filter receipts by the desired month
        .Where(r => IsWithinSameMonthAndYear(r.Date, dateOfInterest))
        // select all products from all filtered receipts as a flat list
        .SelectMany(r => r.Products)
        // group the products by their category
        .GroupBy(p => p.Category)
        // turn into a dictionary keyed of the category key and the value is the weighted sum of the product quantities
        .ToDictionary(g => g.key.Key, g => g.Sum(p => p.Quantity * p.Price));

Little helper method:

// returns true if first is within the same month and year as second
private bool IsWithinSameMonthAndYear(DateTime first, DateTime second)
{
    return first.Year == second.Year && first.Month == second.Month;
}