Converting numbers to words - Python I'm new to programming. I tried to find a way to convert numbers into letters with Python. I would like to receive some advice to improve myself. This program works for numbers between 1 and 106  and I would like to know how the logic of the program can be improved.

def changeNumberIntoLetter(value):
    number=numToLetter(value)
    return number

def numToLetter(value): #The function converts the numbers into letters.
    if value==1: return 'one'
    elif value==2: return 'two'
    elif value==3: return 'three'
    elif value==4: return 'four'
    elif value==5: return 'five'
    elif value==6: return 'six'
    elif value==7: return 'seven'
    elif value==8: return 'eight'
    elif value==9: return 'nine'
    elif value==10: return 'ten'
    elif value==11: return 'eleven'
    elif value==12: return 'twelve'
    elif value==13: return 'thirteen'
    elif 13<value<=19: return composeTeen(value)
    elif value>19:
        if value==20: return 'twenty'
        elif value==30: return 'thirty'
        elif value==50: return 'fifty'
        elif value==10**2: return 'one hundred'
        elif value==10**3: return 'one thousand'
        elif value==10**5: return 'one hundred thousand'
        elif value==10**6: return 'one milion'
        elif value>=20: return composeNumbers(value)
        else: exit('Out of range')
    else: return ''

def composeNumbers(value):   #The function build every number biger than 40
    if 40<=value<10**2:
        value1=int(str(value)[0])
        value2= int(str(value)[1])
        if value1==2:
           value1= 'twen'
           return value1 + 'ty' + '-' + numToLetter(value2)
        if value1==3:
           value1='thir'
           return value1 + 'ty' + '-' + numToLetter(value2)
        if value1==8:
            value1='eigh'
            return value1 + 'ty' + '-' + numToLetter(value2)
        elif value1==5:
            value1='fif'
            return value1 + 'ty' + '-' + numToLetter(value2)
        return numToLetter(value1) + 'ty' + '-' + numToLetter(value2)
    elif 10**2<=value<10**3:
        value1=int(str(value)[0])
        value2= int(str(value)[1:])
        return numToLetter(value1) + ' ' + 'hundred' + ' ' + numToLetter(value2)
    elif 10**3<=value<10**4:
        value1=int(str(value)[0])
        value2=int(str(value)[1:])
    elif 10**4<=value<10**5:
        value1=int(str(value)[0:2])
        value2=int(str(value)[2:])
    elif 10**5<=value<10**6:
        value1=int(str(value)[0:3])
        value2=int(str(value)[3:])
    return numToLetter(value1) + ' ' + 'thousand' + ' ' + numToLetter(value2)

def composeTeen(value): #The function takes the unit and then converts it into letter to build the word.
    value= int(str(value)[-1])  #It turns elem in string to take the last position and it converts it again in integer to change it in letters. Then it composes the word adding 'teen' at the end.
    value= numToLetter(value)
    if value=='five': value= 'fif'
    value= value + 'teen'
    return value

This is good work! Well done as a beginner programmer.

def changeNumberIntoLetter(value):
    number=numToLetter(value)
    return number

This function doesn't really do anything useful to be honest. Can't you directly use numToLetter.

if value==1: return 'one'
elif value==2: return 'two'
elif value==3: return 'three'
elif value==4: return 'four'
elif value==5: return 'five'
elif value==6: return 'six'

Instead of creating lot of if statements. Try using a dictionary like this:

NUM_TO_WORD_MAPPING = {1: "one", 2: "two"}

and you can refer the string to number using

if value in NUM_TO_WORD_MAPPING:
    return NUM_TO_WORD_MAPPING[value]
else:
    # ... special cases.

As sergiy-kolodyazhnyy said, you can also use a list or a tuple.

int(str(value)[-1])

use value % 10 to extract numbers easily. This is modulo operator and returns remainder after division.

Bonus Content: Code Style

How your code looks like is as equally important as how you implement it. Because you and other programmers will spend time reading it.

  “Indeed, the ratio of time spent reading versus writing is well over
  10 to 1. We are constantly reading old code as part of the effort to
  write new code. ...[Therefore,] making it easy to read makes it easier
  to write.”

  ― Robert C. Martin, Clean Code: A Handbook of Agile Software
  Craftsmanship

Let's take a look at your function

def numToLetter(value): #The function converts the numbers into letters.

I recommend that you use snake_case (number_to_letter) as it is considered industry standard for python programs. Comments are better done using """ comment """ style.

In this scenario however comment is not really required because your function name says that it's purpose is converting number to letter. People usually forget to update comments and it ends up giving incorrect information. If you want to use documentation comments anyway, make sure that comments are always up to date.

Reading material:

PEP-8 - Coding conventions / standard
PEP-287 - Document comment standard
Documentation related SO Question


Check bitstamp.net exchange rate, compare it with previous rate I have written low quality php code, I'm not a good coder ;) Help me, please, to optimise it ;)

This simple code check bitstamp.net exchange rate, compare it with previous rate and print the result.

<?php
function getPrice($url) {
    $decode = @file_get_contents($url);
    return json_decode($decode, true);
}

$btc = getPrice('https://api.binance.com/api/v1/ticker/24hr?symbol=BTCUSDT');
$btcusdlast = round($btc["lastPrice"], 1);
$rbtc = ($btc["priceChange"]>0) ? 'up' : ' down';

$ltc = getPrice('https://api.binance.com/api/v1/ticker/24hr?symbol=LTCUSDT');
$ltcusdlast = round($ltc["lastPrice"], 1);
$rltc = ($ltc["priceChange"]>0) ? 'up' : ' down';

?>

BTC
    $ <?=$btcusdlast, ' ', $rbtc;?>

LTC
    $ <?=$ltcusdlast, ' ', $rltc;?>

But it's low quality and work very slowly.

Id make your code more functional (if your not going to use OOP) the fact that this code is slow is probably your internet connection (as mentioned in the comments)

First as the api is returning common keys I would just make the rounding & ternary operator as one function you can pass a response to,

Also because you aren't using full names and just short codes the whole code works in a loop, this way you can just add more coin short codes into the $coinsToFetch array

<?php
function fetchAndDecodeData($url)
{
  return json_decode(@file_get_contents($url), true);
}

function getValuesFromBinanceData($data)
{
  return [
    "priceChange"=>$data["priceChange"] > 0 ? "up" : "down",
    "lastPrice"=>round($data["lastPrice"], 1)
  ];
}

$coinsToFetch = ["BTC", "LTC"];

foreach ($coinsToFetch as $coin) {
  $coinData = fetchAndDecodeData('https://api.binance.com/api/v1/ticker/24hr?symbol=' . $coin . 'USDT');
  $data = getValuesFromBinanceData($coinData);
  echo "<h2> $coin " . $data['lastPrice'] . $data['priceChange'] . "</h2>";
}

To address the issue of the speed of your script in relation to your internet you could cache the results (redis) or store them in database (mysql) this would allow you to do some sort of code like this (psudeo code)

If its less than 5 minutes sinse the cache
  Load the data from the cache
Else
  Load from binance api

If you are going to use the caching / storage I would convert this into two scripts;

That runs as cron job to populate cache / storage every x number of minutes
To display the data


Python Simple Hangman Game Q1. Can this code be simplified/ made more efficient?

Q2. Why does the following code have to be indented in the way that it is?
used.append(guess) and so_far = new

Q3. Out of interest, is there a way to print the hangman figure once and just replace the previous figure, to avoid mass text/ reloading in the Python Shell?

The Following code has been slightly adapted from Python Programming for the absolute beginner - Hangman Game

#Python 3.4.3, MAC OSX (Latest)
#Hangman Game

import random
from time import sleep

HANGMAN = (
"""
 ------
 |    |
 |
 |
 |
 |
 |
 |
 |
----------
""",
"""
 ------
 |    |
 |    O
 |
 |
 |
 |
 |
 |
----------
""",
"""
 ------
 |    |
 |    O
 |   -+-
 |
 |
 |
 |
 |
----------
""",
"""
 ------
 |    |
 |    O
 |  /-+-
 |
 |
 |
 |
 |
----------
""",
"""
 ------
 |    |
 |    O
 |  /-+-/
 |
 |
 |
 |
 |
----------
""",
"""
 ------
 |    |
 |    O
 |  /-+-/
 |    |
 |
 |
 |
 |
----------
""",
"""
 ------
 |    |
 |    O
 |  /-+-/
 |    |
 |    |
 |   |
 |   |
 |
----------
""",
"""
 ------
 |    |
 |    O
 |  /-+-/
 |    |
 |    |
 |   | |
 |   | |
 |
----------
""")

WORDS = ("APPLE", "ORACLE", "MIMO", "TESLA")
word = random.choice(WORDS)
POSITIVE_SAYINGS = ("Well done!", "Awesome!", "You Legend!")
MAX_WRONG = len(word) - 1
so_far = ("-") * len(word)
used = []
wrong = 0

print("\t \t Welcome to Hangman!")
print()
input("Press Enter to START: ")

while wrong < MAX_WRONG and so_far != word:
    print()
    print(HANGMAN[wrong])
    print("Word so far: ", so_far)
    print("Letters used: ", used)

    guess = input("Guess a letter: ").upper()
    sleep(1) # Time delay - allows userfriendly reading
    print()

    while guess in used:
        print("Try again... You've already used this letter")
        guess = input("Guess a letter: ").upper()
        sleep(1)
        print()
    used.append(guess)

    if guess in word:
        print(random.choice(POSITIVE_SAYINGS),"...Updating word so far...")

        new = ""
        for i in range(len(word)):
            if guess == word[i]:
                new += guess

            else:
                new += so_far[i]
        so_far = new

    else:
        print("INCORRECT! Try again!")
        wrong += 1

print("Calculating result...")
sleep(1)
if wrong == MAX_WRONG:
    print("UNLUCKY! Better luck next time!")

else:
    print("WINNER! Congratulations!")

print()
print()
input("Press Enter to Leave: ")

This sure looks like a fun game!

So far, it's pretty well constructed, so I just have some suggestions:

To answer your questions...

  Q1. Can this code be simplified/ made more efficient?

Yes. I recommend putting it in a class for simplicity. As far as efficiency goes, the largest running time is O(n), so it is already efficient. It might be possible to simplify it to allow the max running time to be O(log n), but I don't know how.

Class version:

# Whenever possible, only import specific functions from the library
from random import choice
from time import sleep

class Hangman:
    """
    A Hangman class.
    """
    _HANGMAN = (
        """
         ------
         |    |
         |
         |
         |
         |
         |
         |
         |
        ----------
        """,
        """
         ------
         |    |
         |    O
         |
         |
         |
         |
         |
         |
        ----------
        """,
        """
         ------
         |    |
         |    O
         |   -+-
         |
         |
         |
         |
         |
        ----------
        """,
        """
         ------
         |    |
         |    O
         |  /-+-
         |
         |
         |
         |
         |
        ----------
        """,
        """
         ------
         |    |
         |    O
         |  /-+-/
         |
         |
         |
         |
         |
        ----------
        """,
        """
         ------
         |    |
         |    O
         |  /-+-/
         |    |
         |
         |
         |
         |
        ----------
        """,
        """
         ------
         |    |
         |    O
         |  /-+-/
         |    |
         |    |
         |   |
         |   |
         |
        ----------
        """,
        """
         ------
         |    |
         |    O
         |  /-+-/
         |    |
         |    |
         |   | |
         |   | |
         |
        ----------
        """)

    _WORDS = ("APPLE", "ORACLE", "MIMO", "TESLA")
    _POSITIVE_SAYINGS = ("Well done!", "Awesome!", "You Legend!")

    def __init__(self):
        """
        The Python constructor for this class.
        """
        self._word = choice(self._WORDS)
        self._so_far = "-" * len(self._word)
        self._used = []
        self._wrong_answers = 0

    def play(self):
        """
        This is the main driver of the game.
        Plays the game.
        """
        self._reset_game()
        self._start_game()

        # The amount of incorrect answers should be no greater than the length
        # of HANGMAN.
        #
        # Use the length of HANGMAN to ensure there's no index
        # overflow error when printing current progress.
        while self._wrong_answers < len(self._HANGMAN) and self._so_far != self._word:
            self._print_current_progress()
            guess = self._user_guess()
            self._check_answer(guess)

        self._print_result()
        self._play_again()

    # ---------------------------------
    # "Private" methods

    def _check_answer(self, guess):
        """
        Checks to see if the user's guess is correct.
        :param guess: User's guess
        """
        if guess in self._word:
            print(choice(self._POSITIVE_SAYINGS), "...Updating word so far...")

            for i in range(len(self._word)):
                if guess == self._word[i]:
                    # so_far is spliced this way:
                    # so_far [from the start : up until, but not including the
                    #     position of the correctly guessed letter]
                    # + guessed letter
                    # + so_far [from the position next to the
                    #     correctly guessed letter : to the end]
                    self._so_far = self._so_far[:i] + guess + self._so_far[i+1:]

        else:
            print("INCORRECT! Try again!")
            self._wrong_answers += 1

    def _play_again(self):
        """
        Asks the user if he or she would like to play again.
        If the user wants to play again, calls play().
        Otherwise, thanks the user for playing.
        """

        print("Would you like to play again?")
        user_input = input("Enter Y for yes or N for no: ").upper()

        if user_input == "Y":
            self.play()

        else:
            print()
            print("Thank you for playing!")

    def _print_current_progress(self):
        """
        Prints the current progress of the game.
        """
        print()
        print(self._HANGMAN[self._wrong_answers])
        print("Word so far: ", self._so_far)
        print("Letters used: ", sorted(self._used))

    def _print_result(self):
        """
        Prints the result (win or lose).
        """
        sleep(1)
        print()
        print("Calculating result...")
        sleep(1)
        print()
        if self._wrong_answers == len(self._HANGMAN):
            print("UNLUCKY! Better luck next time!")
        else:
            print("WINNER! Congratulations!")

    def _reset_game(self):
        """
        Resets the game by calling the constructor.
        """
        self.__init__()

    def _start_game(self):
        """
        Starts the game by printing an introduction
        and asks the user to hit the ENTER key to continue.
        """
        print()
        print("\t\tWelcome to Hangman!")
        print()
        input("Press Enter to START:")

    def _user_guess(self):
        """
        Asks for the user to guess a letter.
        :returns: User's guessed letter.
        """
        guess = input("Guess a letter: ").upper()
        sleep(1)  # Time delay - allows user friendly reading
        print()

        while guess in self._used:
            print("Try again... You've already used this letter")
            guess = input("Guess a letter: ").upper()
            sleep(1)
            print()

        self._used.append(guess)

        return guess

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

    game.play()

This is also available on secret Gist

If you want to just focus on the method part, you can see the method version by clicking on my Github Gist link

  Q2. Why does the following code have to be indented in the way that it is?
  used.append(guess) and
  so_far = new

From the code in your question, used.append(guess) needs to be outside of the while loop or the guessed letter will be added to the used list repeatedly as long as the while condition is true.

so_far = new needs to be outside of the for loop for the same reason. In my suggested code, you will have no need to use the new variable because string splicing is used instead.

  Q3. Out of interest, is there a way to print the hangman figure once and just replace the previous figure, to avoid mass text/ reloading in the Python Shell?

Currently, I don't know. I'll keep it in my mind and let you know if I find a way.

Update:

In one of your comments below, you asked if the code above was OOP. The answer is kind-of, but no. I didn't define a model or the logic in a separate entity. However, I did create a Gist link that is the OOP version of the code.

You can click on this Gist link to see the OOP version.


Processing player's move in an ASCII rogue-like game I'm making an ASCII roguelike game and while writing the code for processing the player's move I can't decide between these two options. I have defined a class named point to handle positions in the game, like this. I would also like to know if this is a good idea or if I should use a structure instead.

Should I pass an object as a parameter to a function or create it inside?

class point {
public:
    point();
    point(int x, int y);

    int getX() const;
    int getY() const;;

    void setX(int x);
    void setY(int y);

private:
    int x;
    int y;
};

OPTION 1

I define the target point object before calling the function.

point playerPos = player.get_position();
point moveTilePos;

switch (input) {
    case MOVE_UP:
        moveTilePos.setX(playerPos.getX());
        moveTilePos.setY(playerPos.getY()-1);
        processPlayerMove(player, moveTilePos);

which is defined as follows:

void Level::processPlayerMove(Player &player, point target) {

    char moveTileSymbol;

    moveTileSymbol = getTile(target);
    switch(moveTileSymbol){
        case WALL_SYMBOL:
            break;
        case GROUND_SYMBOL:
            setTile(player.get_position(), GROUND_SYMBOL);
            player.set_position(target);
            setTile(target, PLAYER_SYMBOL);
            break;
    }
}

I think this is more readable overall but requires a setup that maybe should be part of the function.

Option 2

Everything is handled by the function:

point playerPos = player.get_position();

switch (input) {
    case MOVE_UP:
        processPlayerMove2(player, playerPos.getX(), playerPos.getY() -1)

Which is defined like this:

void Level::processPlayerMove2(Player &player, int targetX, int targetY) {

    char moveTileSymbol;
    point target;

    target.setX(targetX);
    target.setY(targetY);

    moveTileSymbol = getTile(target);
    switch(moveTileSymbol){
        case WALL_SYMBOL:
            break;
        case GROUND_SYMBOL:
            setTile(player.get_position(), GROUND_SYMBOL);
            player.set_position(target);
            setTile(target, PLAYER_SYMBOL);
            break;
    }
}

This case is more succinct, as the function handles everything.

point playerPos = player.get_position();
point moveTilePos;

moveTilePos.setX(playerPos.getX());
moveTilePos.setY(playerPos.getY()-1);
processPlayerMove(player, moveTilePos);

Well, of course you shouldn't do that. Declaring an "uninitialized" variable to be initialized later via mutation is one of the cardinal sins of modern C++ programming. One reasonable way to write this code would be

point playerPos = player.get_position();
processPlayerMove(
    Point(playerPos.getX(), playerPos.getY() - 1)
);

But if you find yourself writing Point(p.getX(), p.getY()-1) often enough, then you should factor it out into a function:

point playerPos = player.get_position();
processPlayerMove(playerPos.upward(1));

And then you can eliminate that pesky helper variable:

processPlayerMove(player.get_position().upward(1));

There, that looks clean enough!

Next, look at your switch statement and refactor it to be clearer.

switch (input) {
    case MOVE_UP:
        processPlayerMove(player.get_position().upward(1));
        break;
    case MOVE_DOWN:
        processPlayerMove(player.get_position().downward(1));
        break;
    case MOVE_LEFT:
        processPlayerMove(player.get_position().leftward(1));
        break;
    case MOVE_RIGHT:
        processPlayerMove(player.get_position().rightward(1));
        break;
}

Can you think of a similar way to simplify this code? What would it look like?


"Consecutive Heads" program So... I have a program in which I want to flip heads three times in the row.

What I'm asking for is for proposals of other solutions for this program, in pro way, as You do in natural sense.

That's my code as Java novice.

/*
 * File: ConsecutiveHeads.java
 * ----------------
 * This program flips a coin repeatly until three consecutive heads
 * are tossed.
 */

import acm.program.*;
import acm.util.*;

public class ConsecutiveHeads extends ConsoleProgram {
    /* Run the program */
    public void run() {
        println("This program flips a coin until there are three" +
                "heads in the row.");
        while(counter != 3) {
            FlipACoin();
        }
        println("Yupii! There are already three same heads in the row :)");
    }

    /* Flip a coin. Then if heads are tossed, increment our counter.
     * In tails case, zero counter. */
    private void FlipACoin() {
        boolean rank = rgen.nextBoolean();
        if(rank) {
            println("heads");
            counter++;
        } else {
            println("tails");
            counter = 0;
        }
    }

    /* Create an instance variable for the random number generator */
    private RandomGenerator rgen = new RandomGenerator();

    /* Create an instance variable for counter detecting three heads in row */
    private int counter = 0;
}

Your function is called FlipACoin(), but you are doing more than just flipping the coin. A more appropriate name might be FlipACoinAndCountConsecutiveHeadRolls().

I would suggest keeping the function as simple as the name suggests and return a string (or boolean) from FlipACoin(), then examine the result outside the function. This will also allow you to make the counter a local variable to the run function, which will make the class smaller.


Collecting data from Vector to Map I wrote the following piece of code and want to hear your opinion, in this snippet I have a vector called aggregateFeaturesForML which has elements of Class with 3 fields: sourceip, key, value. what I want is to collect all the key-value pairs that have the same sourceip and form a nice histogram (for each IP the keys are unique), for that purpose I collect the key-value pairs in a map called histogram and then use its toString function to print a {key=value, key=value} formation. I store the histograms inside another map called aggregator.

Any comments on the readability, writing manner, other stuff will be appritiated.

Definition:

private HashMap<String, HashMap<String, String> > aggregator;
protected Vector<SingleResult> aggregateFeaturesForML = new Vector<SingleResult>(); //Single result has 3 fields: IP, key, value

Code:

        String previousIp = aggregateFeaturesForML.get(0).getSourceip();
        if(!aggregator.containsKey(previousIp))
        {
            aggregator.put(previousIp, new HashMap<String, String>());
        }

        HashMap<String, String> histogram = new HashMap<String, String>();
        for(int iterator=0;iterator<aggregateFeaturesForML.size();iterator++)
        {
            SingleResult sr = aggregateFeaturesForML.get(iterator);
            String ip = sr.getSourceip();

            if(ip != previousIp)
            {
                HashMap<String, String> mapForIP = aggregator.get(previousIp);
                mapForIP.put(key, histogram.toString());
                aggregator.put(previousIp, mapForIP);

                if(!aggregator.containsKey(ip))
                {
                    aggregator.put(ip, new HashMap<String, String>());
                }

                histogram.clear();
                previousIp = ip;
            }

            histogram.put(sr.getKey(), sr.getValue());
        }
        HashMap<String, String> mapForIP = aggregator.get(previousIp);
        mapForIP.put(key, histogram.toString());
        aggregator.put(previousIp, mapForIP);

Beware of hard-coding the first element of a list like aggregateFeaturesForML.get(0). This can throw an exception is aggregateFeaturesForML is empty so you should check for that first.
Do not compare String with == (or !=). In the following

if(ip != previousIp)

you are comparing the Strings ip and previousIp with !=. Strings are compared using their equals method, so you should have instead:

if(!ip.equals(previousIp))

Prefer to program against interfaces. Instead of

HashMap<String, String> mapForIP = aggregator.get(previousIp);

use:

Map<String, String> mapForIP = aggregator.get(previousIp);

As a side-note, if you are using Java 8, your code could be written a lot more simply using the Stream API, that directly provides a way to group elements with a classifier, using the groupingBy collector. In this case, when two elements are classified the same way, they are collected into a Map with the toMap collector.

Map<String, Map<String, String>> aggregator =
    aggregateFeaturesForML.stream()
                          .collect(Collectors.groupingBy(
                              SingleResult::getSourceip,
                              Collectors.toMap(SingleResult::getKey, SingleResult::getValue)
                          ));


Sorting Exclusive of Specified Indexes in List To further generalize the algorithm (past the doc string): Given a list E(1) E(3) E(2) E(4) _ E(0) _ where E(N + 1) is ahead of E(N) when sorted and _ represents a "static element" (that is, an element whose index and value will remain unchanged despite the sorting), the list returned should be E(0) E(1) E(2) E(3) _ E(4) _.

In the following code, non-static elements would be the elements affected by the sorting, while a static element would be unaffected (i.e, an _).

def static_sort(old_list: list, static_index_list: list) -> list:
    """
    Returns a list whose non-static elements (defined to be the ones whose
    indexes are not specified in the static_index_list) are sorted in ascending order.
    The static elements (with indexes specified in the static_index_list) remain unchanged
    in both numeric value and index in the original list.

    static_sort([1, 9, 3, 5], [0, 2]) -> [1, 5, 3, 9]
    static_sort([0, 8, 2, 6], [2]) -> [0, 6, 2, 8]

    :param static_index_list: A list of indexes whose associated elements in
    old_list should be exclusive of the sorting in the rest of the list.
    :param old_list: The unsorted list, to-be sorted using the comparator and
    static_index_list.
    """

    sorted_list = []
    non_sort_subjects = list(map(lambda static_index: old_list[static_index], static_index_list))

    def sort_subject_filter(element):
        return element not in non_sort_subjects

    sort_subjects = sorted(list(filter(sort_subject_filter, old_list)))

    whole_index = sort_subject_index = 0
    while whole_index < len(old_list):
        if whole_index in static_index_list:
            sorted_list.append(non_sort_subjects[whole_index - sort_subject_index])
        else:
            sorted_list.append(sort_subjects[sort_subject_index])
            sort_subject_index += 1
        whole_index += 1

    return sorted_list

Reviews on the algorithm itself in addition to formatting/code style are appreciated.

Limitations

The implementation will not work well for certain inputs with duplicate values. The spec doesn't say that there won't be duplicate values, so you should not assume that.

This limitation can be fixed by excluding items to sort by index instead of value.

Performance

element not in non_sort_subjects and whole_index in static_index_list will be linear searches, since non_sort_subjects and static_index_list are lists. A simple improvement would be to search in sets.


Plus Minus in Python You're given an array containing integer values. You need to print the
  fraction of count of positive numbers, negative numbers and zeroes to
  the total numbers. Print the value of the fractions correct to 3
  decimal places.

  Input Format

  First line contains \$N\$, which is the size of the array.  Next line
  contains \$N\$ integers \$A_1, A_2, A_3, ⋯, A_N\$, separated by spaces.

  Constraints

  \$1 \le N \le 100\$
  \$−100 \le A_{i} \le 100\$

Solution

from __future__ import division

N = int(raw_input())
ary = map(int, raw_input().split())

count_negatives = len(filter(lambda x: x < 0, ary))
count_positives = len(filter(lambda x: x >0, ary))
count_zeros = len(filter(lambda x: x == 0, ary))

print count_positives / N
print count_negatives / N
print count_zeros / N

It seems that this problem has a very straightforward solution but I am more interested if there is a better functional approach.

Print the value of the fractions correct to 3 decimal places.

You didn't really do this part. You're just printing the full float. You can use str.format() to print to three places. With this syntax:

print "{:.3f}".format(count_positives / N)

The .3f syntax tells Python to print the first three digits only. Note that str.format will actually round the value, not just truncate it:

"{:.3f}".format(1.3449)
>>> '1.345'

Though, since you're just printing the value by itself, you can use the format builtin where you pass the string and the format as a string.

format(count_positives / N, '.3f')

You don't need a filter to count the zeros. Lists have a built in count function which will return the number of times a particular object occurs in it. Like this:

count_zeros = ary.count(0)


Implementing the observer pattern for a stock exchange scenario I am trying to implement the observer pattern for a stock exchange scenario. Multiple stocks can be registered at a stock exchange. There will be stock observers who are observing some specific stocks. Please advise how this implementation can be improved.

stock_exchange.hpp

#ifndef STOCK_EXCHANGE_HPP
#define STOCK_EXCHANGE_HPP

#include "stock_observer.hpp"
#include "stock.hpp"
#include<unordered_map>
#include<string>
#include<list>

class Stock_Observer;
class Stock;

class Stock_Exchange
{
    std::unordered_map<std::string, Stock*> registered_stocks;
    std::unordered_map<std::string, std::list<Stock_Observer*>> stock_observers;

    public:
        void notify_observers(std::string stock_name);
        void unregister_observer(Stock_Observer& so, std::string stock_name);
        void register_observer(Stock_Observer& so, std::string stock_name);
        void add_stock(Stock& new_stock);
        void remove_stock(Stock& s);
        Stock& get_stock(std::string stock_name);
};

#endif // STOCK_EXCHANGE_HPP

stock_exchange.cpp

#include "stock_exchange.hpp"
#include "stock_observer.hpp"
#include<algorithm>

void Stock_Exchange::add_stock(Stock& new_stock)
{
    registered_stocks[new_stock.get_name()] = &new_stock;
}

void Stock_Exchange::remove_stock(Stock& s)
{
    registered_stocks.erase(s.get_name());
    stock_observers.erase(s.get_name()); //Memory leak!!!
}

Stock& Stock_Exchange::get_stock(std::string stock_name)
{
    return *registered_stocks[stock_name];
}

void Stock_Exchange::register_observer(Stock_Observer& observer, std::string stock_name)
{
    stock_observers[stock_name].push_back(&observer);
}

void Stock_Exchange::unregister_observer(Stock_Observer& observer, std::string stock_name)
{
    auto l = stock_observers[stock_name];
    l.erase(std::find(l.begin(), l.end(),&observer));
}

void Stock_Exchange::notify_observers(std::string stock_name)
{
    for(auto o : stock_observers[stock_name])
    {
        o->notify(stock_name);
    }
}

stock_observer.hpp

#ifndef STOCK_OBSERVER_HPP
#define STOCK_OBSERVER_HPP

#include "stock.hpp"
#include "stock_exchange.hpp"
#include<string>

class Stock_Exchange;

class Stock_Observer
{
    static int id_sequence;
    int id;
    Stock_Exchange* se;
    public:
        Stock_Observer(Stock_Exchange* s)
        {
            se = s;
            id = id_sequence++;
        }
        void notify(std::string stock_name);
};

#endif // STOCK_OBSERVER_HPP

stock_observer.cpp

#include "stock_observer.hpp"
#include<iostream>

int Stock_Observer::id_sequence = 1;

void Stock_Observer::notify(std::string stock_name)
{
    std::cout << "Object " << id << " notified about " << stock_name << std::endl;
}

stock.hpp

#ifndef STOCK_HPP
#define STOCK_HPP
#include "stock_exchange.hpp"
#include<string>

class Stock_Exchange;

class Stock
{
    std::string name;
    double price;
    Stock_Exchange* se;

    public:
        Stock(Stock_Exchange* s, std::string stock_name, double stock_price)
        {
            se = s;
            name = stock_name;
            price = stock_price;
        }
        std::string get_name();
        void set_price(double p);

};
#endif // STOCK_HPP

stock.cpp

#include "stock.hpp"

std::string Stock::get_name()
{
    return name;
}

void Stock::set_price(double p)
{
    price = p;
    se->notify_observers(this->name);
}

main.cpp

#include<iostream>
#include "stock_exchange.hpp"
#include "stock_observer.hpp"
#include "stock.hpp"

int main()
{
    Stock_Exchange se;

    Stock_Observer so1(&se), so2(&se);

    Stock infy(&se,"INFY",1234.33);
    Stock tcs(&se,"TCS",1100);
    Stock sbi(&se,"SBI",2312.33);

    se.register_observer(so1,"INFY");
    se.register_observer(so2,"INFY");
    se.register_observer(so1,"TCS");

    infy.set_price(1250.33);
    tcs.set_price(999.32);
    sbi.set_price(2300.00);

}

Make immutable class members const

If you have a member of a class that is set once at construction time, but will never change, then make it const. For example, in Stock:

class Stock {
  const std::string name;
  ...
  public:
  Stock(..., std::string stock_name, ...): name(stock_name) {
    ...
  }

And once it is const, you can safely make name public and remove get_name().

Avoid circular references

Your stock exchanges contain stocks, but stocks also refer to stock exchanges. This circular reference is unnecessary and might give you problems; for example you had to add forward references to class Stock_Observer and class Stock in stock_exchange.hpp. Conceptually, a stock is not tied to a specific stock exchange, so just remove the member variable Stock::se.

Of course, the problem is now how to notify observers when the stock price changes. One way to handle that is to allow one to set a callback function in a Stock, for example:

#include <functional>

class Stock {
  std::function<void(Stock &)> price_changed_callback;
  double price;
  ...
  public:
  void set_price_changed_callback(std::function<void(Stock &)> callback) {
    price_changed_callback = callback;
  }

  void set_price(double p) {
    price = p;
    if (price_changed_callback)
      price_changed_callback(*this);
  }
  ...
}

Then the stock exchange calls set_price_changed_callback() when it registers the stock, and sets it to something like its notify_observers() function.
If you want to have a Stock be part of multiple exchanges, then you'd better look for a library that handles more complex callback registration for you, like libsigc++.

Another option would be to move set_price() to the stock exchange itself.

Think carefully about ownership

In your code, stock exchanges have pointers to stocks, and stocks have a pointer to a stock exchange. But what if a stock goes out of scope before a stock exchange, or vice versa? It's not a problem in your simple main(), but once your code grows the chance you will make a mistake will increase. It's best to use the right tools to control ownership.

One possibility is to use shared_ptr<>. Make it so you can only create a shared_ptr to a Stock:

class Stock {
  private:
  Stock() { ... }

  public:
  static std::shared_ptr<Stock> make_stock(args) {
    return std::make_shared<Stock>(args);
  }
}

And have Stock_Exchange keep a map of shared_ptrs:

class Stock_Exchange {
  std::unordered_map<std::string, std::shared_ptr<Stock>> registered_stocks;
  ...
}

Alternatively, if you'd only ever have stocks associated with a single exchange, you could have Stock_Exchange take direct ownership:

class Stock_Exchange {
  std::unordered_map<std::string, Stock> registered_stocks;
  ...
  public:
  register_stock(std::string name, double price) {
    register_stocks.emplace(std::make_pair(name, Stock(name, price));
  }
}

Of course, if you go that route there is little point in having a class Stock anymore.

Use references when iterating over a container where possible

In this part of your code:

for(auto o : stock_observers[stock_name])
{
    o->notify(stock_name);
}

You are actually making a copy of each observer. It is almost always better to use a reference when iterating through a container:

for(auto &o : stock_observers[stock_name])
{
    o->notify(stock_name);
}


Convert integer to English words I was recently asked an interview question with regards to algorithm design. The challenge is as follows:

Given a 6 or less digits positive integer (0 - 999999 inclusive), write a function englishify(number: int) that returns the full English equivalent of that number. Here are some samples of the structure you are expected to generate:
1 - One
222 - Two Hundred And Twenty two
1234 - One Thousand, Two Hundred and Thirty Four
31337 - Thirty One Thousand, Three Hundred And Thirty Seven
100100 - One Hundred Thousand And One Hundred
200111 - Two Hundred Thousand, One Hundred And Eleven

As you may be able to see, there is a key requirement when it comes to formatting this:

There should be a comma, not 'And', after the thousands and before the hundreds, if both exist.

I have attempted this challenge with the code below:
def englishify(number):
    #Numbers 0-19 (unique numbers)
    OneToNine= 'One Two Three Four Five Six Seven Eight Nine'.split()
    TenToNineteen = 'Ten Eleven Twelve Thirteen Fourteen Fifteen Sixteen Seventeen Eighteen Nineteen'.split()
    ZeroToNineteen = [''] + OneToNine + TenToNineteen

    #Numbers >= 20 at intervals of 10
    Tens = 'Twenty Thirty Forty Fifty Sixty Seventy Eighty Ninety'.split()

    #Additional function for ease of processing of numbers
    def englishifyHundreds(number):
    #Special case: Number = 0
        if number == 0:
            return 'Zero'

        #1. Number from 1-19
        if number < 20:
            return ZeroToNineteen[number]

        #2. Number from 20-99
        if number >= 20 and number < 100:
            result = Tens[int(number/10)-2] + ' ' + ZeroToNineteen[int(number%10)]
            return result.rstrip()

        #3. Number from 100-999
        if number >= 100:
            #Separating hundreds digit and tens digit
            tens = number - ((number//100)*100)

            #Accounting for edges = 0 (number = 100, 200, ...)
            if number%100 == 0:
                return ZeroToNineteen[int(number/100)] + ' Hundred'

            else:
                return ZeroToNineteen[int(number/100)] + ' Hundred And ' + englishifyHundreds(tens)

    #Actual processing of number
    if len(str(number)) <= 3:
        return englishifyHundreds(number)

    else:
        #Splitting number into 'thousands' digits and 'hundreds' digits
        thousands = int(str(number)[:-3])
        hundreds = int(str(number)[-3:])

        #Accounting for edges = 0 (thousands = 1000, 2000, ...)
        if thousands % 1000 == 0:
            return englishifyHundreds(thousands) + ' Thousand'

        else:
            #Accounting for if hundreds == 0:
            if hundreds == 0:
                return englishifyHundreds(thousands) + ' Thousand'

            #Accounting for cases where comma is not necessary
            elif hundreds % 100 == 0 or hundreds < 100:
                return englishifyHundreds(thousands) + ' Thousand And ' + englishifyHundreds(hundreds)

            #Remaining cases implementing comma
            else:
                return englishifyHundreds(thousands) + ' Thousand, ' + englishifyHundreds(hundreds)

I am currently trying to rack my brains thinking of ways to optimize this, but with my limited knowledge of recursions and algorithms this is the best I can churn out for now. Hopefully I can seek some opinions from some of the more experienced programmers around here.

It's often best to write this kind of function along with its tests.  Let's start with a simple test:
import doctest;

def englishify(number):
    """Format NUMBER into standard English form.
    NUMBER must be in range [0..999999]
    >>> englishify(0)
    'Zero'
    """

    return 'Zero'

if __name__ == '__main__':
    doctest.testmod()

Now, start adding more tests and make each one pass before moving on to the next.
You'll find that when you reach the bigger numbers, there's a useful recursive property.  We can split off the thousands and the hundreds, format each non-zero part separately, and then join using one of the techniques from Joining words together with a comma, and “and”.  For example, consider these inputs:

123456 ⟶ 123 Thousand , 4 Hundred and 56
123056 ⟶ 123 Thousand and 56
123400 ⟶ 123 Thousand and 4 Hundred

Now those individual numbers can be formatted into words (and the last case will have two "and"s in normal English: "One hundred and twenty-three thousand, four hundred and fifty-six.'

Modified code
import doctest;

def englishify(number):
    """Format NUMBER into standard English form.
    NUMBER must be in range [0..999999]
    >>> englishify(0)
    'Zero'
    >>> englishify(10)
    'Ten'
    >>> englishify(20)
    'Twenty'
    >>> englishify(99)
    'Ninety Nine'
    >>> englishify(100)
    'One Hundred'
    >>> englishify(101)
    'One Hundred and One'
    >>> englishify(1001)
    'One Thousand and One'
    >>> englishify(1201)
    'One Thousand, Two Hundred and One'
    >>> englishify(123201)
    'One Hundred and Twenty Three Thousand, Two Hundred and One'
    """

    Units = [None, 'One', 'Two', 'Three', 'Four', 'Five',
             'Six', 'Seven', 'Eight', 'Nine',
             'Ten', 'Eleven', 'Twelve', 'Thirteen', 'Fourteen', 'Fifteen',
             'Sixteen', 'Seventeen', 'Eighteen', 'Nineteen']
    Tens = [None, None, 'Twenty', 'Thirty', 'Forty', 'Fifty',
            'Sixty', 'Seventy', 'Eighty', 'Ninety']

    if number < 20:
        return Units[number] or 'Zero'

    if number < 100:
        return ' '.join(filter(None, [Tens[number//10], Units[number%10]]))

    # Larger numbers - break down and englishify each part
    parts = list(filter(None,
                        map(lambda quantity, number:
                                englishify(number) + quantity if number else None,
                            [' Thousand', ' Hundred', ''],
                            [number // 1000, number // 100 % 10, number % 100])))

    if len(parts) == 1:
        return parts[0]
    return ' and '.join([', '.join(parts[:-1]), parts[-1]])

if __name__ == '__main__':
    doctest.testmod()

The extension to support millions and more should now be obvious - just add the unit and its extraction to the list passed to map().


Getting enum values of Pseudo-Enum classes By "Pseudo-Enum" classes I mean cases where someone's used a class with public static properties to emulate enum behaviour; e.g. as described here.

Per the example below, this is useful for classes such as [System.Management.Automation.VerbsCommon], where there is a list of values we're interested in, but no "Get All Values of This Type" method.

I wrote this function for the above type, and it works well for that, but maybe I've missed a trick / there are likely other scenarios that this code could cover which I've not thought of yet.

Clear-Host
function Get-EnumValues {
    [CmdletBinding()]
    param (
        [Parameter(Mandatory,ValueFromPipeline)]
        [Type]$Type
    )
    process {
        if ($Type) {

            Write-Verbose ("Type: {0}" -f $Type.ToString())
            Write-Verbose ("IsEnum? {0}" -f $Type.IsEnum)

            if ($Type.IsEnum) {
                [System.Enum]::GetNames($Type)
            } else {
                #handle pseudo-enums (i.e. for cases where this trick's been used https://stackoverflow.com/a/15713789/361842)
                $Type.GetFields() | ?{$_.IsStatic -and $_.IsPublic} | select -ExpandProperty Name
            }
        } else {
            Write-Verbose ("TypeName is Null")
            $null
        }
    }
}

$t = [type][System.ConsoleColor]
Get-EnumValues $t -Verbose
$t = [type][System.Management.Automation.VerbsCommon]
Get-EnumValues $t -Verbose

Output:

VERBOSE: Type: System.ConsoleColor
VERBOSE: IsEnum? True
Black
DarkBlue
DarkGreen
DarkCyan
DarkRed
DarkMagenta
DarkYellow
Gray
DarkGray
Blue
Green
Cyan
Red
Magenta
Yellow
White
VERBOSE: Type: System.Management.Automation.VerbsCommon
VERBOSE: IsEnum? False
Add
Clear
Close
Copy
Enter
Exit
Find
Format
Get
Hide
Lock
Move
New
Open
Optimize
Push
Pop
Remove
Rename
Reset
Resize
Set
Search
Show
Skip
Step
Join
Redo
Split
Switch
Select
Undo
Unlock
Watch

I'm aware that the shorthand coding style sometimes frowned upon, such as:

$Type.GetFields() | ?{$_.IsStatic -and $_.IsPublic} | select -ExpandProperty Name

could be written as:

$Type.GetFields() | Where-Object{$_.IsStatic -and $_.IsPublic} | Select-Object -ExpandProperty Name

I've taken the view whereby I allow shorthand for the most common, general-purpose cmdlets (i.e. select for select-object, % for foreach-object, ? for where-object) as those should be familiar to any powershell developer with any experience; whilst for all others I avoid aliases completely.  My view is that these common aliases are more easily readable, and are unlikely to cause confusion.

Let's look for scenarios that this code could cover.

Next 143296.ps1 script, using Get-Type function - Get exported types in the current session

Exports all types from all namespaces in the current session to $allTypes, then
Selects those with non-empty (pseudo)enumeration to auxiliary Hashtable $ppp and $subTypes.

Script 143296.ps1:

$allTypes = Get-Type -Namespace * -ErrorAction SilentlyContinue
$allTypes.Count
$ppp=@{}              ### auxiliary Hashtable object, could be useful though
$allTypes |
    ForEach-Object {
        $aux = $_ | Get-EnumValue -AsFlags
        if ( $aux.Count -ne 0 ) { $ppp.add( $_.FullName, $_.BaseType) }
    }
$ppp.Count
$subTypes = $allTypes | Where-Object { $ppp.ContainsKey($_.FullName)}
$subTypes.Count
$subGrups = $subtypes | Group-Object -Property BaseType
$subGrups.Count
### make public the most frequent items:
$subGrups | Sort-Object -Property Count -Descending | Select-Object -First 15

<# ### another approach to make public results:
$allBases = $ppp.Keys | %{ "{0,15} {1}" -f ($ppp[$_] | Select -ExpandProperty name), $_ } | sort
$allBases.Count
$subBases = $ppp.Keys | %{ ($ppp[$_] | Select -ExpandProperty name)} | sort -Unique
$subBases.Count
#>

Output:

PS D:\PShell> D:\PShell\CR\143296.ps1
12413
2490
2490
182

Count Name                      Group
----- ----                      -----
 1710 System.Enum               {System.StringSplitOptions, System.StringComparison, System....
  201 System.Object             {System.String, System.EventArgs, System.BitConverter, Syste...
   58 System.ValueType          {System.DateTime, System.DateTimeOffset, System.Boolean, Sys...
   53 System.Attribute          {System.ComponentModel.BindableAttribute, System.ComponentMo...
   28 System.Windows.Freezable  {System.Windows.Media.Animation.ThicknessKeyFrame, System.Wi...
   24 System.Windows.Framewo... {System.Windows.Shapes.Shape, System.Windows.Controls.Panel,...
   22 System.Windows.Control... {Microsoft.VisualStudio.Text.Editor.OutliningMarginHeaderCon...
   15 System.Windows.Media.A... {System.Windows.TextDecoration, System.Windows.Media.Animati...
   11 System.Windows.Control... {System.Windows.Window, System.Windows.Controls.Primitives.B...
    9 System.Windows.Depende... {System.Windows.Navigation.JournalEntry, System.Windows.Cont...
    8 System.Globalization.C... {System.Globalization.GregorianCalendar, System.Globalizatio...
    8 System.Windows.Control... {System.Windows.Controls.Canvas, System.Windows.Controls.Vir...
    7 System.Windows.Media.P... {System.Windows.Media.ArcSegment, System.Windows.Media.Bezie...
    7 System.Windows.Control... {System.Windows.Controls.Border, System.Windows.Controls.Ink...
    7 System.Windows.Media.G... {System.Windows.Media.CombinedGeometry, System.Windows.Media...

The Get-EnumValue function is explained using Comment-Based Help (CBH):

Set-StrictMode -Version latest
function Get-EnumValue {
    <#
    .SYNOPSIS
        Return names (and values) for an enumeration or enumeration-like object.

    .DESCRIPTION
        With -AsFlags or no switch: return Ordered Dictionary of names and values
                                    for an enumeration (or enumeration-like) object.
        With -NoValues switch: return an array of names for such object.

    .PARAMETER Type
        Pass in an actual type, or a string for the type name.

    .PARAMETER NoValues
        If True, then output would be [string[]] array of names;
           otherwise, [OrderedDictionary] type type.
        If True, then -AsFlags parameter is ignored.

    .PARAMETER AsFlags
        If True *and* specified <type> is attributed 'FlagsAttribute' (some [Enum] types)
           then output values would be hexadecimal-like strings (8+ hexadecimal ciphers);
           otherwise, output values would follow <type> type.
        Applies only if -NoValues parameter evaluates to False.

    .INPUTS
        Valid [type], or a string for the type name.

    .OUTPUTS
        Always returns [OrderedDictionary] or [system.array] object (see switches).
        Never outputs Null; returns an empty (.Count -eq 0) object instead.

    .NOTES
        If -NoValues evaluates to True, then -AsFlags is ignored.

    .EXAMPLE
        # Enum, flag enumeration (FlagsAttribute present on object supplied)
        [System.AttributeTargets]|Get-EnumValue -AsFlags -Verbose
    .EXAMPLE
        # Enum, value enumeration (no FlagsAttribute on object supplied)
        [System.Runtime.InteropServices.VarEnum]|Get-EnumValue -AsFlags -Verbose
    .EXAMPLE
        # Struct (not Enum)
        [System.Double]|Get-EnumValue -Verbose
    .EXAMPLE
        # Class (not Enum): useful or at least interesting output
        [System.Security.Claims.ClaimValueTypes]|Get-EnumValue -AsFlags | ft -a -Wrap

    .REMARKS
        Based on 'Get-EnumValues' function and used in answer to:
        http://codereview.stackexchange.com/questions/143296/getting-enum-values-of-pseudo-enum-classes
        Useful related article: 'Get-Type' - Get exported types in the current session
        https://gallery.technet.microsoft.com/scriptcenter/Get-Type-Get-exported-fee19cf7

    .FUNCTIONALITY
        Tested: Windows 8.1/64bit, Powershell 4
    #>
    [CmdletBinding(PositionalBinding=$false)]
    param (
        [Parameter(Mandatory,ValueFromPipeline,Position=0)]$Type,
        [parameter(Mandatory=$false)][Switch][bool]$NoValues,
        [parameter(Mandatory=$false)][Switch][bool]$AsFlags
          )
    begin {

        Function EnumNoEnum ([type]$noEnumClass) {
            $enumFlagsHash = [ordered]@{}
            $noEnumClass.GetFields() |
                Where-Object {$_.IsStatic -and $_.IsPublic} |
                    Select-Object -ExpandProperty Name |
                        ForEach-Object {
                            $enumFlagsHash.ADD( $_, $noEnumClass::$_)
                        }
            return $enumFlagsHash
        }

        Function EnumFlags ([type]$enumClass, [bool]$enumFlags) {
            $enumFlagsHash = [ordered]@{}
            [System.Enum]::GetNames($enumClass) |
                ForEach-Object {
                    if ($enumFlags) {
                        $enumFlagsHash.ADD( $_, "0x{0:x8}" -F $($enumClass::$_.value__))
                    } else {
                        $enumFlagsHash.ADD( $_, $($enumClass::$_.value__))
                    }
                }
            return $enumFlagsHash
        }
    }

    process {
        Try { $Type = [type]$Type} Catch { $Type = $null }
        if ( $Type) {
            Write-Verbose ("Type:   {0}" -f $Type.ToString())
            ### ToDo: find out $enumFlagsAttribute in a more efficient way
            $enumFlagsAttribute = ''
            if ($Type.CustomAttributes |
                Where-Object {$_.AttributeType.Name -eq 'FlagsAttribute'})
                    { $enumFlagsAttribute = 'FlagsAttribute'}
            Write-Verbose ("IsEnum? {0,-20} {1}" -f
                $Type.IsEnum.ToString(), $enumFlagsAttribute)

            if ($Type.IsEnum) {
                if (-not $NoValues) {
                    if ( $AsFlags -and $enumFlagsAttribute) {
                        #   flags enumeration
                        $TypeOut = . EnumFlags $Type $AsFlags
                    } else {
                        #   value enumeration
                        $TypeOut = . EnumFlags $Type $False
                    }
                } else {
                    $TypeOut = [System.Enum]::GetNames($Type)
                }
            } else {
                # handle pseudo-enums i.e. for cases where this trick's been used
                #                      http://stackoverflow.com/a/15713789/361842
                if (-not $NoValues) {
                    $TypeOut = . EnumNoEnum $Type
                } else {
                    $TypeOut = $Type.GetFields() |
                        Where-Object {$_.IsStatic -and $_.IsPublic} |
                            Select-Object -ExpandProperty Name
                }
            }
            if ( $TypeOut ) { $TypeOut } else { ,@() }
        } else {
            Write-Verbose ("Null or invalid TypeName")
            if ( $NoValues ) { ,@() } else { [ordered]@{} }
        }
    }
}

I think that information about FlagsAttribute on a particular enum type could matter (added to verbosed output), see FlagsAttribute class.

See also the AsFlags parameter in the above function: it causes that values of the FlagsAttribute enum have a hexadecimal look (fake, however sometimes useful).


What would be the best way to update data from associative array? I recieve this array in my controller:

"sort" => array:6 [
    0 => array:2 [
        "id" => "153"
        "sort" => "2"
    ]
    1 => array:2 [
        "id" => "152"
        "sort" => "1"
    ]
    2 => array:2 [
        "id" => "154"
        "sort" => "3"
    ]
    3 => array:2 [
        "id" => "166"
        "sort" => "4"
    ]
    4 => array:2 [
        "id" => "155"
        "sort" => "5"
    ]
    5 => array:2 [
        "id" => "156"
        "sort" => "6"
    ]
]

I have made this and it works:

foreach ($request['sort'] as $model) {
    Model::where('id', $model['id'])->update(['sort' => $model['sort']]);
}

There is probably a better way to do this....
This will make so many requests on DB how many items there are inside that array....
Anyone has better ideas? Maybe there is a way to make that update all at once so there is only one request on DB?

"Premature optimization is the root of all evil."

As long as this code works and causes no problem, nothing should be really done. Trying to "improve" it, most likely you will cause a severe harm to readability, maintainability and - ironically - performance.

The only thing could be suggested out of the blue is to wrap the loop in a transaction. It will be a sensible move by itself, and and also could improve the speed in case of some certain settings of the database engine.


Passing multiple parameters from the command line in Java I am working on a project in which I need to pass multiple arguments from the command line-

Below is the use case I have-

From the command line, I will be passing atleast four paramaters- noOfThreads, noOfTasks, startRange and tableName1, so if I am passing these four thing, then I need to store them in a variable and for any table names- I need to add it into the string list so that I can use them in my other code.
Secondly, I can pass five parameters instead of four just like above- So five parameters can be- noOfThreads, noOfTasks, startRange , tableName1 and tableName2. So here tableName2 is extra. so if I am passing these five thing, then I need to store them in a variable and here I am passing tableName1 and tableName2 as two tables so I will be storing these two tables in a string list.
Thirdly, I can pass six parameters instead of five just like above- So sixparameters can be- noOfThreads, noOfTasks, startRange , tableName1, tableName2 and tableName3. So here tableName3 is extra. so if I am passing these six thing, then I need to store them in a variable and here I am passing tableName1 , tableName2 and tableName3 as three tables so I will be storing these three tables in a string list again.

So for above scenario, I have the below code with me. It is looking very ugly currently as I have lot of repetition in my code as mentioned below. Is there any way I can make it more cleaner?

Below is my code-

private static List<String> databaseNames = new ArrayList<String>();
private static int noOfThreads;
private static int noOfTasks;
private static int startRange;
private static String tableName1;
private static String tableName2;
private static String tableName3;

public static void main(String[] args) {

if (args.length > 0 && args.length < 5) {

    noOfThreads = Integer.parseInt(args[0]);
    noOfTasks = Integer.parseInt(args[1]);
    startRange = Integer.parseInt(args[2]);
    tableName1 = args[3];
    databaseNames.add(tableName1);
} else if (args.length > 0 && args.length < 6) {
    noOfThreads = Integer.parseInt(args[0]);
    noOfTasks = Integer.parseInt(args[1]);
    startRange = Integer.parseInt(args[2]);
    tableName1 = args[3];
    tableName2 = args[4];
    databaseNames.add(tableName1);
    databaseNames.add(tableName2);
} else {
    noOfThreads = Integer.parseInt(args[0]);
    noOfTasks = Integer.parseInt(args[1]);
    startRange = Integer.parseInt(args[2]);
    tableName1 = args[3];
    tableName2 = args[4];
    tableName3 = args[5];
    databaseNames.add(tableName1);
    databaseNames.add(tableName2);
    databaseNames.add(tableName3);
}
}

Extract out the common logic before the if. The following is more or less the same:

noOfThreads = Integer.parseInt(args[0]);
noOfTasks = Integer.parseInt(args[1]);
startRange = Integer.parseInt(args[2]);
tableName1 = args[3];
databaseNames.add(tableName1);
if (args.length >= 5) {
    tableName2 = args[4];
    databaseNames.add(tableName2);
}
if (args.length >= 6) {
    tableName3 = args[5];
    databaseNames.add(tableName3);
}

Note that running the program with less than five parameters only prints an exception:

Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: 0
    at ...Main.main(Main.java:16)

You should handle this and print a user friendly error message or usage help.


Temporal kernel filtering on video frames coming in a loop I have a video which I read in a loop frame by frame. For every frame, I want to do temporal kernel filtering, the coefficients of which come from the input variable model (a dictionary). The temporal kernel is an array that has size of 15. So, basically, for every frame we apply this function. The variables scale, clip_size are constants. Firstly I add borders across the obtained frame. Then I start a buffer of size (15,240,320) where (240,320) is the clip size. Variable bindex is used to write the buffer. The buffer variable will store only last 15 modified frames, hence bindex needs to be updated at every function iteration. Variable sindex is used to traverse through the buffer and apply the buffer, hence it has to be returned too.  Since the input_frames are coming from a video in a loop, I had to make some variables be both inputs and outputs. Is there a way to speed this up? Any better way to do this?

def filtered_output(input_frame,model, Buffer,bindex,sindex,scale,clip_size):
    # padding is required
    top=model['item'][0]
    bot=model['item1'].shape[1]-top-1
    left=model['item'][1]
    right=model['item1'].shape[0]-left-1
    # NewDepth = 15
    # clip_size is [240,320]
    # In the external loop, bindex = 0, sindex =0 in the beginning
    # We now create a copy of our current frame, with appropriate padding
    frame2= cv2.copyMakeBorder(input_frame,top,bot,left,right,cv2.BORDER_CONSTANT,value=0.5)
    Buffer[bindex] = scipy.signal.convolve2d(frame2, model['item1'], boundary='symm', mode='valid')
    sindex = (bindex+1) % NewDepth # point to oldest image in the buffer
    temp=np.zeros(tuple(clip_size),dtype=float)
    temp = model['TempKern'][0]*Buffer[sindex]

    sindex = (sindex+1) % NewDepth
    for j in range(1, NewDepth) :  # iterate from oldest to newest frame
        temp = temp + model['TempKern'][j]*Buffer[sindex]
        sindex = (sindex+1) % NewDepth
    bindex = (bindex+1) % NewDepth
    temp=np.nan_to_num(temp)
    output_frame = ApplyCubicSpline(model['CubicSplineCoeffs'],temp)*scale
    return output_frame, Buffer,bindex,sindex

My question is whether we can make this function faster somehow (not that it is slow)? If yes, how? Is it the best practice to use bindex, sindex as both inputs and outputs to the function? Are there any alternatives?

Maybe not the answer you're looking for, but I focused on that loop:

for j in range(1, NewDepth) :  # iterate from oldest to newest frame
    temp = temp + model['TempKern'][j]*Buffer[sindex]
    sindex = (sindex+1) % NewDepth

To speed this up you could:

create a variable to avoid accessing model['TempKern'] (dict access)
use in-place adition for temp

improved:

tk = model['TempKern']
for j in range(1, NewDepth) :  # iterate from oldest to newest frame
    temp += tk[j]*Buffer[sindex]
    sindex = (sindex+1) % NewDepth

Aside: outside the loop you do:

temp=np.zeros(tuple(clip_size),dtype=float)
temp = model['TempKern'][0]*Buffer[sindex]

so you can remove the first line, as it has no effect, temp is overwritten at the next line.


Implementing PLC "Timer Pulse" function block in C++ I have been implementing a library of the PLC function blocks. One of the function blocks is the "Timer Pulse" function block which behavior is following:

I have implemented this function block in this manner

Tpulse.h:

#include <stdint.h>
#include "LogicBlk.h"

namespace LogicBlocks
{

// Timer Pulse i.e. logic one at the output for specified time as soon as 0->1 at the input
class Tpulse : public LogicBlk{
public:
    Tpulse(uint32_t*, uint32_t, uint32_t, uint32_t);
    virtual ~Tpulse();

    void Update(void);

private:

    uint32_t *m_BitsArray;
    uint32_t m_Input;
    uint32_t m_Output;
    uint32_t m_PulseTime;
    uint32_t m_StartTime;
    uint32_t m_ElapsedTime;
    uint32_t m_CurrentTime;

};

}

Tpulse.cpp:

LogicBlocks::Tpulse::Tpulse(uint32_t *bitsArray, uint32_t input, uint32_t output, uint32_t pulseTime):
                            m_BitsArray{bitsArray}, m_Input{input}, m_Output{output}, m_PulseTime{pulseTime}{
                                m_StartTime     = 0;
                                m_ElapsedTime   = 0;
                                m_CurrentTime   = 0;
}

LogicBlocks::Tpulse::~Tpulse(){
}

void LogicBlocks::Tpulse::Update(void){

    if(Utils::TestBitSet(m_BitsArray, m_Input) && Utils::TestBitClr(m_BitsArray, m_Output) && !m_ElapsedTime){

        m_StartTime = GetTick();
        SetBit(m_BitsArray, m_Output);

    }else if(Utils::TestBitSet(m_BitsArray, m_Output)){

        m_CurrentTime = GetTick();
        m_ElapsedTime = m_CurrentTime - m_StartTime;

        if(m_ElapsedTime >= m_PulseTime){

            ClrBit(m_BitsArray, m_Output);
            if(TestBitClr(m_BitsArray, m_Input)){

                m_ElapsedTime = 0;
            }
        }

    }else if(TestBitClr(m_BitsArray, m_Input) && TestBitClr(m_BitsArray, m_Output)){

        m_ElapsedTime = 0;

    }

}

LogicBlk.h

namespace LogicBlocks
{

class LogicBlk {
public:

    virtual void Update(void) = 0;

private:

};

}

Based on the tests which I have already done it seems to me that it works fine but I am not sure. Please can anybody assess my code from functional and programming style point of view? Thank you in advance for any suggestions.

I see a number of things that may help you improve your code.

Let the compiler write default functions

The Tpulse destructor literally does nothing and is what the compiler would have generated anyway.  To indicate that, eliminate the function in Tpulse.cpp and declare it as default in Tpulse.h:

virtual ~Tpulse() = default;

If you define one special member function, define them all

Sometimes called the "rule of five," if you delete or define any of the special member functions, you should delete or define them all. See C.21

Use override where appropriate

If you're intending to override a virtual base function, you should explicitly say so to help catch errors.  In this case it's the Update function which should be marked override:

void Update() override;

See C.128  Also note that unlike in C, Update(void) and Update() mean the same thing in C++.  I prefer the shorter form, but sometimes coding guidelines (often written by or for C programmers) require the long form.

Use namespaces consistently

In some places within Tpulse::Update, TestBitClr() is written with a Utils:: namespace prefix and in other place not.  This is inconsistent and confusing to the reader who may be left wondering if there are two versions.  Instead, I'd recommend either always using the explicit namespace or putting a using namespace Util; within the Update function.

Prefer std:: namespace versions of functions and types

Rather than uint32_t, I'd recommend using std::uint32_t and explicitly adding #include <cstdint>.  This makes it clear which type you mean and will be robust even if someone later introduces a local uint32_t which does, unfortunately, sometimes happen in embedded systems projects.

Use parameter names in function templates

It's best to make the interface clear and explicit, and with a function call like this:

Tpulse(uint32_t*, uint32_t, uint32_t, uint32_t);

It's not clear what the various uint32_t values represent.  If they were written like this, it would be better:

Tpulse(std::uint32_t *bitsArray, std::uint32_t input, std::uint32_t output, std::uint32_t pulseTime):

See I.4.

Think about the possible correct use of volatile

In embedded systems we often encounter one of the few correct uses of volatile.  In this case, I'm wondering about the m_BitsArray pointer.  If, as I suspect, this is memory-mapped I/O, then this "memory" is not really solely under the control of the C++ environment since external asynchronous signals may cause those bits to change outside program control.  For that reason, it may be that it should be declared as volatile to indicate this fact.  See CP.200.  On the other hand, if this array might also be use by other threads within your program, you will need to add explicit synchronization of some kind.  See CP.8

Use const where practical

It appears to me that m_Input and m_Output should never change within the lifetime of a Tpulse object, so I'd suggest that both of those member data functions should be declared const.  The same is probably true of m_PulseTime.

Minimize the interface

It seems to me that the Tpulse class could eliminate m_ElapsedTime and m_CurrentTime in favor of only using m_StartTime and perhaps a bool running.  If you need other named variables within Update they can be local.

Use helper functions

The code could be much simpler to read and understand with the use of a few private helper functions:

bool LogicBlocks::Tpulse::input() const {
    return Utils::TestBitSet(m_BitsArray, m_Input);
}

bool LogicBlocks::Tpulse::output(bool value) {
    if (value) {
        Utils::SetBit(m_BitsArray, m_Output);
    } else {
        Utils::ClrBit(m_BitsArray, m_Output);
    }
    return value;
}

Simplify the code

The Update code is a little more complex than it needs to be.  Essentially, either the timer is running and we do timer things, or it's not yet running, but we receive an input that tells us to start it.  The only other slightly tricky thing is that we don't allow the timer to restart until it's expired and the input is low.

So we can simplify the code, using a bool running member variable and the helper functions shown above:

void LogicBlocks::Tpulse::Update(){
    if (running) {
        auto elapsed = Utils::GetTick() - m_StartTime;
        if (elapsed >= m_PulseTime) {
            output(false);
            running = input();
        }
    } else if (input()) {
        m_StartTime = Utils::GetTick();
        running = output(true);
    }
}

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people.  Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code.  This affects not only code reviews, but also maintenance of the code in the future, by you or by others.  One good way to address that is by the use of comments.  Another good technique is to include test code showing how your code is intended to be used.

In this case I wrote this main to do some testing:

#include "Tpulse.h"
#include "Utils.h"
#include <iostream>
#include <iomanip>
#include <chrono>
#include <thread>

int main() {
    using namespace Utils;
    using namespace std::chrono_literals;
    uint32_t reg{0};
    constexpr uint32_t inbit{0x80};
    constexpr uint32_t outbit{0x2};
    LogicBlocks::Tpulse tp(&reg, inbit, outbit, 5);
    for (int i=0; i < 20; ++i) {
        std::this_thread::sleep_for(1s);
        if (i == 2 || i == 9) {
            SetBit(&reg, inbit);
        } else if (i == 4 || i == 16) {
            ClrBit(&reg, inbit);
        }
        std::cout << std::dec << "t = " << i << ", reg = 0x" << std::hex << reg;
        tp.Update();
        std::cout << ", updated to 0x" << reg << '\n';
    }
}

Here's the output:

t = 0, reg = 0x0, updated to 0x0
t = 1, reg = 0x0, updated to 0x0
t = 2, reg = 0x80, updated to 0x82
t = 3, reg = 0x82, updated to 0x82
t = 4, reg = 0x2, updated to 0x2
t = 5, reg = 0x2, updated to 0x2
t = 6, reg = 0x2, updated to 0x2
t = 7, reg = 0x2, updated to 0x0
t = 8, reg = 0x0, updated to 0x0
t = 9, reg = 0x80, updated to 0x82
t = 10, reg = 0x82, updated to 0x82
t = 11, reg = 0x82, updated to 0x82
t = 12, reg = 0x82, updated to 0x82
t = 13, reg = 0x82, updated to 0x82
t = 14, reg = 0x82, updated to 0x80
t = 15, reg = 0x80, updated to 0x80
t = 16, reg = 0x0, updated to 0x0
t = 17, reg = 0x0, updated to 0x0
t = 18, reg = 0x0, updated to 0x0
t = 19, reg = 0x0, updated to 0x0


hangman with ten chances So this is the code I wrote for hangman in python. I want to know of can shorten this code more further and make it look more professional

# importing random function
import random, hanged
from word_meaning import meanings

# Dictionary of random words
word_dict = '''whistle,
park,
picayune,
attach,
grubby,
gusty,
neck,
necessary,
day,
compete,
memory,
interfere,
sophisticated,
lumpy,
spiky,
frail'''.split(',\n')

# System chooses a random word
rand_word = word_dict[random.randint(0, len(word_dict)-1)]

print('Welcome to HANGMAN.\nGuess correct letters to complete the hidden word, Else hangman will be Hanged')

n = len(rand_word)
hidden_word = ((n-1)*'*')
hidden_word_1 = hidden_word.split('*')
print(f'Hidden Word: {hidden_word}*')
print(f'Hint: {meanings[str(rand_word)]}')
#This is the dictionary imported from another file. it gives the meaning of the hidden word as a hint

chances = 10

def user_lose():
    global chances
    print(f'Oops wrong guess\n Chances Left : {chances - 1}')
    if chances > 1:
        chances -= 1
        user_guess()
    else:
        hanged.hangman()
#hanged.Hangman() this is the function which shows pic of hangman. imported from another file

def user_guess():
    global chances
    if '' in hidden_word_1:
        user_input = str(input('\nGuess a letter: ')[0]).lower()
        if user_input in rand_word:
            if user_input in hidden_word_1:
                user_lose()
            else:
                if chances > 0:
                    letter_index = rand_word.index(user_input)
                    letter_index_r = rand_word.rindex(user_input)
                    hidden_word_1[letter_index] = user_input
                    hidden_word_1[letter_index_r] = user_input
                    print(hidden_word_1)
                    user_guess()

        else:
            user_lose()

    else:
        print('Hurrah, You saved Hangman')

user_guess()

hanged.py file

def hangman():
    print('''You could not save hangman\n
                                   -------------------
                                   |         |
                                   |         |
                                   |         |
                                   |      ( ` ` )
                                   |         |
                                   |        /|\\
                                   |       / | \\
                                   |         |
                                   |        / \\
                                   |       /   \\
                                   |
                                   |
                                   |
                                   |                 ''')

I don't have word_meaning, is that another dependency ?

Since you are already using the random module, you can simply select a random element from a list with random.choice like this:

rand_word = random.choice(word_dict)

instead of:

rand_word = word_dict[random.randint(0, len(word_dict)-1)]

Here you are picking only one element at a time, but if you wanted to select several unique elements (no repeating items) then you would use random.sample.

Your list of words is a constant, make it so:

word_dict = (
'attach', 'compete', 'day', 'frail', 'grubby', 'gusty', 'interfere', 'lumpy', 'memory',
'necessary', 'neck', 'park', 'picayune', 'sophisticated', 'spiky', 'whistle'
)

(did a simple search & replace in Notepad++). NB: I have sorted it too.

The way you are splitting the string is awkward and the comma delimiter is superfluous. This design choice forces you to waste one line per word and makes the code longer for no benefit.

Note that I made it a tuple (non mutable) instead of a list, hence the parentheses.

You could also load the list from a text file, if you want flexibility and separate data from code.

It is not safe to rely on \n as a line break delimiter because it is platform-specific. On Windows/Mac it is different although Python should still interpret it the same. But you never know. Think about portability issues.

Relying on global variables is probably not so Pythonic. You could pass chances as a parameter to your functions. And if you make changes to that value, then return the value in your functions.

Using a global variable could make debugging harder because the variable can be altered in so many places. If you refactor your functions at some point the variable could even be out of sync.

I would aim for minimal variable scope. If hidden_word_1 (not a great name) is only used in user_guess then it need no be defined as a global variable.

I cannot run your code right now but I am not convinced you need a recursive function. You simply need a while loop eg while chances > 0: and decrement the counter after each wrong response.


Creating number pattern (triangle numbers) in C++ with minimum loops We were asked to make a triangular number pattern in C++ with minimum loops:

____1_____
___2__3____
__4__5__6__
7__8__9__10

Code:

#include <iostream>

using namespace std;
int main() {
    int n=0, r=0, i=0;
    cout << "No. of rows: ";
    cin >> r;

    for( n=1; n<=r; n++) {
        for( i=1; i<=r-n; i++) {
            cout << "  ";
        }
        for( i=(n*(n-1)/2)+1; i<=(n*(n+1)/2); i++ ) {
            if( i<10 )
                cout << " " << i << "  ";
            else
                cout << i << "  ";
        }
        cout << "\n";
    }
    return 0;
}

Output:

Here's what I'd like reviewed:

Is it wise to use pattern-generating formulas? For example, for putting value of i in the last loop, I used the formula for the pattern \$1,2,4,7\$... as \$\frac{n*(n-1)}{2}+1\$. Is it efficient? This involves four operations in each iteration, and it's fairly slow.
Is it possible to reduce the number of loops? Is it better to reduce variables or reduce loops?

Spacing

Use as much whitespace as needed, it makes it easier to read code, especially formulas or expressions with a lot of operators.

using namespace std;

This is discouraged, I don't think it makes much difference in a program of this size, but it can be a bad habit to rely on.

int n=0, r=0, i=0;

Try and only declare variables in the smallest scope possible, as in right when you need them. Since n and i are only ever used inside the loops, only declare them there.

I would avoid assigning 0 to r, as then if later the program crashes, and I see r no value, it makes the bug much easier to spot than if r has a valid value.

for( n=1; n<=r; n++) {
    for( i=1; i<=r-n; i++) {
        cout << "  ";
    }

This is completely up to you, but I would start the loops at 0, and go until n < r or i < r-n. That is because I would consider that a standard loop, and it is very quick to see how many times it loops, what stopping condition is, and what it loops over. I understand why you are starting from 1, the number of rows, but since this is never actually used, an off by one error wouldn't actually be noticeable. It is barely worth thinking about, but if you are writing for loops over and over, it makes it easier to read. Take this with a grain of salt.

for( i=(n*(n-1)/2)+1; i<=(n*(n+1)/2); i++ ) {

As we are printing out the natural numbers, starting from 1, and going to r*(r+1)/2, I would just have a variable that keeps counting upwards. Then I don't have to work out what number the row starts with each time, and you are incrementing i anyway, so there is essentially no extra work done by having a count, and you save a few operations too.

    if( i<10 )
        cout << " " << i << "  ";
    else
        cout << i << "  ";
}

This is exactly what padding was made for. I don't really know how to pad with cout, but printf makes it very easy. It looks like:

printf("%*d in the bed, and the little one said %s", the_amount_of_spaces, the_number_to_print, "a_piece_of_inpsiring_text");

Where everything inside the quotes is what to print, % means the next piece is a variable, %d means an int, %s means a string

There are some other things you can do, like if you never want to change the number of spaces, you can hardcode it like:
    printf("%3d ", count);

Here is a reference for it

Below are all the changes I would make thrown together, if you have any questions, ask away

#include <iostream>
#include <stdio.h>

int main() {
    int r;
    std::cout << "No. of rows: ";
    std::cin >> r;

    int count = 1;
    for(int n = 1; n <= r; n++) {
        for(int i = 0; i < r - n; i++) {
            std::cout << "  ";
        }

        while(count <= (n*(n + 1)/2)) {
            printf("%3d ", count);
            count++;
        }
        std::cout << "\n";
    }
    return 0;
}

The padding works well up until 45 rows are entered, after that, it was too wide for my screen to properly read.


Iterating string characters and passing them to a BIOS API Which style of printing strings from the SI registry in NASM is more correct?

I'm learning assembly and x86 architecture, but I'm rather new in it, so I've prepared two styles (in NASM) of printing a string, which is held in the SI register.

For a clear text, the samples are at the bottom of the question content.  I also want you to provide me with some information about the correctness of the code style in ASM. The most significant place in both code for me is the loop for printing a char from the whole string, so I want you to tell me which variant is more correct (or maybe both variants are bad and you can provide me a much better sample).

Both samples don't need an OS and could be run as *.img (floopy image) in virtual machine or with the mkisofs (or similar software) be prepared as *.iso (CD image). In these code samples - I'm working with the BIOS to print the string.

Also, about code style. I don't know which style is more correct in ASM, for e.g. in the 1st variant I'm using the name of the functions like: print_string, but as for the 2nd variant, I'm using the style, which is closer to C# name style PrintCharacter, so which variant of naming is more suitable in ASM language?

To compile in NASM, just use this syntax:

nasm -f bin -o file.img file.asm

The first one (http://pastebin.com/32mDPump):

[BITS 16]

start:
    mov ax, 07C0h
    add ax, 288
    mov ss, ax
    mov sp, 4096
    mov ax, 07C0h
    mov ds, ax
    mov si, text_string
    call print_string
    jmp $

text_string db 'Hello World!', 0

print_string:
    mov ah, 0Eh

.repeat:
    lodsb
    cmp al, 0
    je .done
    int 10h
    jmp .repeat

.done:
    ret

times 510-($-$$) db 0
dw 0xAA55

The second one (http://pastebin.com/FnP0NGTY):

[BITS 16]

mov si, HelloString
call PrintString
jmp $

PrintCharacter:
    mov ah, 0x0E
    mov bh, 0x00
    mov bl, 0x07
    int 0x10
    ret

PrintString:
    next_character:
        mov al, [si]
        inc si
        or al, al
        jz exit_function
        call PrintCharacter
        jmp next_character
    exit_function:
        ret

HelloString db 'Hello World', 0

times 510-($-$$) db 0
dw 0xAA55

The two samples don't quite do the same thing. The first sample starts by setting the location of the stack and data segments to some 'magic' location in memory:

mov ax, 07C0h
add ax, 288
mov ss, ax
mov sp, 4096
mov ax, 07C0h
mov ds, ax

The second sample doesn't (so it will use the SS and DS locations which are left in those registers by the O/S which loaded the program). I don't understand what 07C0h is and why there is that difference between the two programs.

Also the styles of assembly are different, for example one uses 0Eh and the other uses 0x0E. Are they both supported by NASM?

The first sample ends with jmp $ which I don't understand: does it mean "jump to here" i.e. "infinite loop"? The .done: ret label seems to be unreachable.

The second sample ends with exit_function: ret which I guess returns to the O/S.

  The most significant place in both code for me is the loop for printing a char from the whole string, so I want you to tell me which variant is more correct (or maybe both variants are bad and you can provide me a much better sample).

Your using BIOS int 10h which Wikipedia says is:

Teletype output AH=0Eh  AL = Character, BH = Page Number, BL = Color (only in graphic mode)

The first sample doesn't initialize the BX register (so it may may contain a semi-random value), the second does. BX may or may not have an effect (in fact this whole "BIOS Teletype output" API may or may not work) depending on current the video mode.

If you're interested in speed then the following algorithm can be much faster than using the BIOS interrupt:

Set the right video mode
Set registers (for example, es:di) to point to the main memory location which is shared with the video card (perhaps 0xB0000 if I recall correctly)
Write the character into video memory

Or instead of writing characters the the BIOS one at a time, there's a BIOS API which lets you write an entire string. I copied the following from 8086 BIOS and DOS interrupts (IBM PC):

INT 10h / AH = 13h - write string.

input:
AL = write mode:
    bit 0: update cursor after writing;
    bit 1: string contains attributes.
BH = page number.
BL = attribute if string contains only characters (bit 1 of AL is zero).
CX = number of characters in string (attributes are not counted).
DL,DH = column, row at which to start writing.
ES:BP points to string to be printed.
example:
mov al, 1
mov bh, 0
mov bl, 0011_1011b
mov cx, msg1end - offset msg1 ; calculate message size.
mov dl, 10
mov dh, 7
push cs
pop es
mov bp, offset msg1
mov ah, 13h
int 10h
jmp msg1end
msg1 db " hello, world! "
msg1end:

As for your print loops:

print_string:
    mov ah, 0Eh

.repeat:
    lodsb
    cmp al, 0
    je .done
    int 10h
    jmp .repeat

The first one (above) looks tight: i.e. it's good and short, with the mov ah, 0Eh performed once outside the loop. lodsb doesn't affect flags, so the separate cmp is necessary. There may be a faster way to compare al with 0, perhaps something like or al,al.

I don't have a BIOS reference manual, but one thing it's lacking is a check of the 'return code' to see whether the output might have failed. This suggests that if the call fails it might leave a return code in ah. You want your ah preserved, so you might want to check that at least the first one didn't fail, and then have a loop which assumes that subsequent won't fail.

If your string weren't null-terminated, if instead you knew the length of it ...

HelloString:
dw 0bh
db 'Hello World'

... then you could load the length into cx and use the loop instruction, something like:

mov si, HelloString
lodsw
mov cx,ax
mov ah,0eh
.repeat:
lodsb
int 10h
loop .repeat
; .done:
ret

Beware that the above may not be the fastest way to loop, anymore. I once read that the more complicated opcodes (like loop) perform more slowly on modern CPUs than using several primitive instructions (e.g. dec cx and jnz). The reason for that is that a developer (or compiler) will choose and place those more primitive opcodes in such as way as to use both pipelines and not cause a stall. Writing assembly which keeps both pipelines busy is an advanced art (which, they try to build-in to compilers). If I look at the output from a compiler, I hardly recognize it any more! Because, it often uses several instructions instead of one or two. Nevertheless I think that the loop above is compact and idiomatic (if you're writing code for another programmer to read, not writing code in order use every last CPU cycle).

The second one doesn't seem to be optimized for size or speed:

PrintCharacter:
    mov ah, 0x0E
    mov bh, 0x00
    mov bl, 0x07
    int 0x10
    ret

PrintString:
    next_character:
        mov al, [si]
        inc si
        or al, al
        jz exit_function
        call PrintCharacter
        jmp next_character
    exit_function:
        ret

Why not use lodsb instead of mov al, [si] and inc si?
Why set bl and bh instead of setting bx?
Why not set bx and ah before the loop begins (I think that a call to int 10h should preserve the contents of the bx register?
Why make PrintCharacter a subroutine to be called, instead of including those instructions inline?

  Also, about code style. I don't know which style is more correct in ASM, for e.g. in the 1st variant I'm using the name of the functions like: print_string, but as for the 2nd variant, I'm using the style, which is closer to C# name style PrintCharacter, so which variant of naming is more suitable in ASM language?

The NASM manual seems to use naming conventions more like print_string. Chapter 9 talks about writing assembly called from high-level languages. Names of HLL functions are subject to name mangling so if you want to write the C printf function in assembly you might need to call it _printf.


Quiz on factoring quadratic expressions I am a beginner, and it is my first program in C. It works well, but I was wondering if there is a shorter way to do it, and how I could have done it better.

It is supposed to make a quadratic expression and ask the user to factor it. If the user factors it correctly (s)he proceeds to the next level, which is a slightly harder question. If (s)he cannot solve it correctly, (s)he stays on the same level, unless (s)he wants to exit.

# include <stdio.h>
# include <stdlib.h>
# include <ctype.h>
# include <string.h>
# include <math.h>
# include <time.h>

int main() {

  srand(time(0));
  int a, b, c, level=1, randMax=level*5;
  char ch;

  // generate the problem to ask
  int ask(){

    // generate non-zero random number for first coefficient
    // that is less than the level of the game, but not zero
    int nonZeroCoefficient(){
      int n = (rand()%(level+2) - (level));
      if ( n == 0 ){
      n++;
      }
      return n;
    }

    // generate non-zero random number

    int nonZeroRand(){
      int n=(rand()%randMax - (randMax));
      if (n == 0){
        n++;
      }
      return n;
    }

    int f = nonZeroCoefficient();
    int h = nonZeroCoefficient();
    int k = nonZeroRand();
    int g = nonZeroRand();

    a = f*h;
    b = (f*k + g*h);
    c = g*k;

    // shows the Level
    printf("\n\n\n\n --- Level %d ---", level);

    // generate the question
    printf("\nfactor the following expression\n%dx^2 %+dx %+d",a ,b, c);
    puts("\n\nEnter your answer in the form of (fX+g)(hX+k)");
    int inputF, inputG, inputH, inputK;
    printf("\nEnter f:");
    scanf("%d", &inputF);
    printf("Enter g:");
    scanf("%d", &inputG);
    printf("Enter h:");
    scanf("%d", &inputH);
    printf("Enter k:");
    scanf("%d", &inputK);

      if (inputF*inputH == a && ((inputF*inputK+inputG*inputH== b) && (inputG*inputK == c))){
        printf("\nRight on!\n");
        level++;

      }else{
        printf("\nSorry! your answer is not currect!\nthe currect answer is:\nf:%d\ng:%d\nh:%d\nk:%d\n \ntry again",f, g, h, k);
      }

    }

    do{
      ask();
      printf("\nDo you want to continue? (Y/N)");
      scanf (" %c", &ch);
    } while(ch == 'y'|| ch == 'Y');

}

Where you have your own bounds for rand, calculate them directly. Instead of

int nonZeroCoefficient(){
  int n = (rand()%(level+2) - (level));
  if ( n == 0 ){
  n++;
  }
  return n;
}

try

int nonZeroCoefficient(){
  int n = (rand()%(level+2) - (level)) + 1;
  return n;
    }

(modify the first part of the equation to suit). This also reduces a skew or bias in your randomness where anything <1 becomes one (i.e. the number 1 becomes over-represented in the random samples).

You also define your functions within your main(). This makes the code harder to read/understand and will make it harder to maintain. As a result, main() will only be:

int main() {
    do{
      ask();
      printf("\nDo you want to continue? (Y/N)");
      scanf (" %c", &ch);
    } while(ch == 'y'|| ch == 'Y');

}

As you don't return anything from main() or ask(), void will suffice. The following are the functions that should be defined separately from your main code

int nonZeroCoefficient() // noting the comment I made about about generating the random number
int nonZeroRand() // noting the comment I made about about generating the random number
int ask()

Both nonZeroCoefficient() and nonZeroRand() functions will be single line functions that could be replaced by a single function

int newRand(int f1, int f2){ //f1 could be level+2 or rand Max, F2 could be level or randMax
      int n = (rand()%(f1) - (f2)) + 1;
      return n;
}


Who likes that post? Following on from my last question, I've been continuing to look at what information is available from the SE Data Explorer, I thought it would be interesting to know who else had favorited a question.  Which led me to this query:

DECLARE @FavoriteType TINYINT = 5;

SELECT
       Votes.UserId AS [User Link],
       Votes.PostId AS [Post Link]
FROM Votes
JOIN Posts
  On Votes.PostId = Posts.Id
WHERE Votes.PostId = ##QuestionID##
AND Votes.VoteTypeId = @FavoriteType
ORDER BY Votes.CreationDate DESC

Whilst this is vaguely interesting, it's not really all that useful.  However, I thought what might be useful is to know what other questions those users might have favorited.  If enough of them favorite another question then since we already have something in common, it's probably an interesting question.  With that in mind, I came up with this query:

DECLARE @FavoriteType TINYINT = 5;

SELECT
  TOP 100
  Posts.Id AS [Post Link],
  Posts.Score AS [Score],
  Count(1) AS [Favorited By]
FROM Votes [Favorited Post]
JOIN Votes [Other Favorites]
  On [Favorited Post].UserId = [Other Favorites].UserId
JOIN Posts
  On [Other Favorites].PostId = Posts.Id
WHERE [Favorited Post].PostId = ##PostID##
AND [Favorited Post].VoteTypeId = @FavoriteType
AND [Other Favorites].VoteTypeId = @FavoriteType
GROUP BY
  Posts.Id, Posts.Score
Order by
  [Favorited By] desc

I've encountered various DBA's that were adamant about using COUNT(1) or COUNT(*), so I'm not really sure what the current flavour of the month is.  Happy for feedback on either of the queries.

Overall, very good
I inspected your query closely and did not find anything standing out that could really be improved much. Here are some good points I found:

Your formatting is consistent and the indentation makes it clear how your code is structured.

You used a clear variable for Favorite type instead of magic numbers.

Your aliases are clear and descriptive.

There are no unnecessary operations, joins or sub-queries that I could find.

Here are a few very minor points to consider.

Using AS operator for table aliases/correlation names. Here is an answer on Stack Overflow that covers the topic.

You could use a small query with your variable to make the intent a bit more clear, like DECLARE @FavoriteType TINYINT = (SELECT Id FROM VoteTypes WHERE Name = 'Favorite'); although this could impact performance slightly, if you have a whole bunch of variables like this. With just a few though the performance difference is negligible.

As for the COUNT(1) vs. COUNT(*), honestly there is no difference. Either way the query plan optimizer will execute them identically because they are both non-nullable expressions. See more details on Stack Overflow and a similar answer on Database Administrators which also covers COUNT(some_field) which behaves differently.


Finding the next XML element I am looking for a better way to do a while cycle with the variable update.

The background is that I'm writing a StAX-line XML reader in JavaScript and want to implement a skipElement function. The function may only be called on the start tag and should skip everything until the closing end tag.

this.nextTag() skips to the next tag and returns Jsonix.XML.Input.START_ELEMENT or Jsonix.XML.Input.END_ELEMENT depending on whether it is a start or an end tag.
numberOfOpenTags holds the number of open tags, increased/decreased accordingly.
If we have a closing tag and numberOfOpenTags is 1, we're done.

skipElement : function() {
    if (this.eventType !== Jsonix.XML.Input.START_ELEMENT) {
        throw new Error("Parser must be on START_ELEMENT to skip element.");
    }
    // We have one open tag at the start
    var numberOfOpenTags = 1;
    // Skip to the next tag
    var et = this.nextTag();
    // If we have an END_ELEMENT and there is exactly one tag still open, we're done
    while (et !== Jsonix.XML.Input.END_ELEMENT || numberOfOpenTags !== 1)
    {
        if (et === Jsonix.XML.Input.START_ELEMENT)
        {
            numberOfOpenTags++;
        }
        else
        {
            numberOfOpenTags--;
        }
        et = this.nextTag();
    }
    return et;
}

I am looking for improvements. What I don't like:

this.nextTag() is called on two occasions.
Another check for et within the cycle: if (et === Jsonix.XML.Input.START_ELEMENT).
Too verbose if/else in the cycle.

How about don't go to the next element before the loop, just check for the counter to reach zero, get the next element first in the loop, and the use a conditional operator to determine how to change the counter:

skipElement : function() {
  if (this.eventType !== Jsonix.XML.Input.START_ELEMENT) {
    throw new Error("Parser must be on START_ELEMENT to skip element.");
  }
  var numberOfOpenTags = 1;
  var et;
  while (numberOfOpenTags > 0) {
    et = this.nextTag();
    numberOfOpenTags += et === Jsonix.XML.Input.START_ELEMENT ? 1 : -1;
  }
  return et;
}