Conditional execution based on input string's prefix I have this method that basically takes a string and does different things
depending on what the string starts with. Is there a cleaner way to write this method?

public String analyze(BotMessage message)
{

    String lcmsg = message.message().toLowerCase();
    //Not a command
    if(!lcmsg.startsWith("!")) return "-1";

    //Message from whatever
     if(lcmsg.startsWith("!ping")) return "pong!";
     if(lcmsg.startsWith("!tournament")) return tournament(message);

     //message from skype
     if(message.skype())
     {
         if(lcmsg.startsWith("!settournament")) return settournament(message);
         if(lcmsg.startsWith("!promote")) return promote(message);
         if(lcmsg.startsWith("!checkin")) return checkin(message);
         if(lcmsg.startsWith("!checkout")) return checkout(message);
         if(lcmsg.startsWith("!checkedin")) return checkedin(message);
         if(lcmsg.startsWith("!updatetournament")) return updatetournament();

     }

    return "-1";
}

I don't like too much the fact that you use a String as the returned type of your function.

What about using a class hierarchy or exceptions?

I see opportunities to create a hierarchy even for the messages. Why don't you create a BotMessage and a SkypeMessage?

Both should have the logic to decide what to do in the base case, maybe in an analyze method. The analyze method of the SkypeMessage should, in addition, manage the Skype-specific aspects.

Finally you should simplify the code and remove the list of if statements.
Introduce a new set of objects to parse the messages. These objects should have a match method that check whether the string matches with their definition, and a performAction method that generates the return value.


Can someone review my implementation of a KeyValuePair? public class KeyValuePair implements Comparable {

    private Comparable key;
    private Object value;

    public KeyValuePair(Comparable key, Object value) {
    this.key = key;
    this.value = value;
    }

    @Override
    public int compareTo(Object item) throws ClassCastException {
    if (!(item instanceof KeyValuePair)) {
        throw new ClassCastException("A KeyValuePair object was expected");
    }
    return this.key.compareTo(((KeyValuePair) item).key);
    }

    public Comparable getKey() {
    return this.key;
    }

    public Object getValue() {
    return this.value;
    }
}

You shouldn't need to throw ClassCastException explicitly; it would be thrown automatically by the JVM if the cast actually fails.

However, you shouldn't have to deal with casting at all if you use Java Generics properly.

public class KeyValuePair<K extends Comparable<K>, V>
        implements Comparable<KeyValuePair<K, V>> {

    private K key;
    private V value;

    public KeyValuePair(K key, V value) {
        this.key = key;
        this.value = value;
    }

    @Override
    public int compareTo(KeyValuePair<K, V> item) {
        return this.key.compareTo(item.key);
    }

    public K getKey() {
        return this.key;
    }

    public V getValue() {
        return this.value;
    }
}

Edit: As @ortang points out, there's more work to be done!  Since the key and value are immutable, marking them final is helpful.  Also, the class needs to override .equals(), and when that happens, .hashCode() needs to be overridden too.  Finally, it may be worthwhile to handle some odd cases in .compareTo().

public class KeyValuePair<K extends Comparable<K>, V> implements Comparable<KeyValuePair<K, V>> {

    private final K key;
    private final V value;

    public KeyValuePair(K key, V value) {
        this.key = key;
        this.value = value;
    }

    // Arbitrarily decide to sort null keys first.  Values are disregarded for
    // the purposes of comparison.
    @Override
    public int compareTo(KeyValuePair<K, V> item) {
        if (this.key == null) {
            return (item.key == null) ? 0 : -1;
        } else {
            return this.key.compareTo(item.key);
        }
    }

    public K getKey() {
        return this.key;
    }

    public V getValue() {
        return this.value;
    }

    @Override
    public boolean equals(Object other) {
        if (other == null || !this.getClass().equals(other.getClass())) {
            return false;
        }
        KeyValuePair item = (KeyValuePair)other;
        return (this.key == null   ? item.key == null   : this.key.equals(item.key)) &&
               (this.value == null ? item.value == null : this.value.equals(item.value));
    }

    @Override
    public int hashCode() {
        return ((this.key == null)   ? 0 : this.key.hashCode()) ^
               ((this.value == null) ? 0 : this.value.hashCode());
    }
}


Quadratic "Complete the Square" Solver in Python 3 I quickly wrote this program to transform quadratic expressions in general form into vertex form.

\$ax^2 + bx + c \;=\; a(x-h)^2 + k,\quad\text{where}\quad h = -\frac{b}{2a} \quad\text{and}\quad k = c - ah^2 = c - \frac{b^2}{4a}.\$

The actual conversion is easy, but with writing a lexer/parser came pain points:

I tokenize with Regex, and some of the regex patterns are built dynamically using f-strings (which seems suspect).
I use eval to compute terms in parentheses (to make fractions easy to input, for example).

I also feel like there's probably a library for working with ASCII-fied algebraic expressions, (lots of the same kind of code populates sites like Symbolab, Cymath and Desmos,) but I do not know of it.

## Tests:
# user_in = "(16/4)x^2 7x (3**2)" -> "Result is: 4.0(x + (0.875))^2 + (5.9375)" (Passes)
# user_in = "(41*4)x^2 41x 2" -> "Result is: 164.0(x + (0.125))^2 + (-0.5625)" (Passes)
# user_in = "x^2 4" -> "Result is: (x + (2.0))^2 + (-4.0)" (Fails)
# user_in = "x^2 4x 16" -> "Result is (x + (2.0))^2 + 12.0" (Fails)

import re

print("""~~~Square Completer!~~~
Enter the terms of your quadratic equation in standard form
Addition is implied, terms separated by spaces.
Only use one kind of single-letter variable in the whole expression.
Surround computations such as constant multiplication and fractions in parens.
DO NOT put variables inside parens!""")
user_in = input("> ")

# extract the variable
def extract_variable(expression):
    letters = re.findall(r'[a-z]', expression) # all the letters
    if all(match == letters[0] for match in letters): # if all letters == the first one
        return letters[0] # we have our variable
    else:
        raise ValueError("Only use one kind of single-letter variable in the whole expression.")

variable = extract_variable(user_in)

# eval all things in parens and split string into terms
def split_into_terms(expression):
    expression = re.sub(fr"\([^{variable}\)]+\)", lambda m: str(eval(m.group())), expression)
    print(f"working with {expression}")
    return expression.split(' ')

terms = split_into_terms(user_in)
print(terms)

# sort terms into bins

def bin_terms(terms):
    squareds, x_es, constants = [], [], []
    for term in terms:
        term = term.strip().lower()
        if term.endswith('^2'):
            squareds.append(term[:-2]) # without trailing '^2'
        elif term.endswith(variable):
            x_es.append(term)
        elif bool(re.search(r"^-?[0-9\.]+$", term)): # if the term contains only numbers
            constants.append(term)
    return squareds, x_es, constants

squareds, x_es, constants = bin_terms(terms)

print('unprocessed:', squareds, x_es, constants, sep='\n')

def sum_bins(squareds, x_es, constants):
    squareds = [squared[:-1] for squared in squareds if squared.endswith(variable)] # 2x -> 2
    print('squareds step1:', squareds, sep='\n')
    squareds = ['1' if squared == variable else squared for squared in squareds] # x -> 1
    print('step2:', squareds)

    print('\nprocessed:', squareds, x_es, constants, sep='\n')

    constant = sum(float(c) for c in constants)
    print('\n ***EXES: ', x_es,'***')
    x = sum(float(el[:-1]) for el in x_es)
    print('\n ***SUMX: ', x,'***')
    squared = sum(float(el) for el in squareds)

    return squared, x, constant

squared, x, constant = sum_bins(squareds, x_es, constants)

##print('\ntotals:', squared, x, constant, sep='\n')

##constant = constant / squared
##x = x / squared
##squared = 1

##print('\nover square term:', squared, x, constant, sep='\n')

def complete_square(a,b,c):
    """
    Complete the square, transforming a quadratic
    of form
    ax^2 + bx + c
    to form
    a(x + p)^2 - q
    """
    p = b/(2*a)
    q = c - (b**2)/(4*a)
    return p, q

p, q = complete_square(squared, x, constant)
a = squared
print(f'Result is: {a}({variable} + ({p}))^2 + ({q})')

There is a library to help with this! Check out sympy, particularly sympify and parse_expr.


Implement mail sending system elegantly In my project I need to send emails. Emails will have different credentials and senders. I'm thinking about "abstract factory", but will it be good solution?

public abstract class Mail
    {
        public string Body { get; protected set; }
        public string Subject { get; private set; }
        public string EmailTo { get; private set; }

        protected Mail(string subject, string emailTo)
        {
            this.Subject = subject;
            this.EmailTo = emailTo;
        }

        protected abstract void BuildBody();
    }
public class AccountMail : Mail
    {
        private string _firstName;
        private string _lastName;
        private string _email;
        private string _siteDomain;
        public AccountMail(string subject, string emailTo, string firstName, string lastName, string siteDomain, string email): base(subject, emailTo)
        {
            _firstName = firstName;
            _lastName = lastName;
            _email = email;
            _siteDomain = siteDomain;
            BuildBody();
        }

        protected override sealed void BuildBody()
        {
             StringBuilder bodyBuilder = new StringBuilder();
             base.Body = bodyBuilder.ToString();
        }

That is abstract Mail and concrete Mail. Do I really need abstract MailSender and concrete MailSender for each email? This is quick solution, because I needed this functionality right now. It's working like this:

public abstract class MailSender
    {
        public abstract void SendMail(string subject, string emailTo, string firstName, string lastName);
    }
public class AccountMailSender : MailSender
    {
        public override void SendMail(string subject, string emailTo, string firstName, string lastName)
        {
            AccountMail mail = new AccountMail(subject, emailTo, firstName, lastName, "");
            Task.Factory.StartNew(() =>
            {
                MailMessage mailMessage = new MailMessage(ConfigSettings.EmailFrom, mail.EmailTo, mail.Subject, mail.Body);
                mailMessage.IsBodyHtml = true;
                SmtpClient client = new SmtpClient(ConfigSettings.EmailServer, ConfigSettings.EmailPort);
                client.DeliveryMethod = SmtpDeliveryMethod.Network;
                client.UseDefaultCredentials = false;
                client.Credentials = new System.Net.NetworkCredential(ConfigSettings.EmailUser, ConfigSettings.EmailPassword);

                try
                {
                    client.Send(mailMessage);
                }
                catch
                {

                }
            });

        }
    }

I know that it's bad and that I need to refactor, but I also need your advice.

Abstract base classes are only useful if you're inheriting functionality. So, what you've done with Mail is good, because all of your inheritors get their properties "for free". In contrast, there's no sense in your MailSender class. Inheritors get no functionality, but are forced to adhere to the contract you've decided on. That's what Interfaces are for.

What I actually don't like is that it's only possible to send an email to exactly one person with your classes. Very soon, you'll wish that you could send an email to a bunch of people at once. EmailTo should mimic the class that you're wrapping up and return a collection.

You're also missing any concept of From in your Mail class. I would expect an email message to know who it's coming from.


Summarizing the score of a personality quiz This function takes a list of questions and list of answers provided by the user.

The list of answers is always a list of booleans (for true and false) and the list of questions takes the following form:

{
  quadrant: Number
}

If the user answers "true" to a question then they gain a point towards that quadrant. (Think of it like a personality test). If they answer false, nothing happens.

The quadrants are arranged along two axes like this:

Q1 | Q4
---+---
Q2 | Q3

The X axis is called "Warmth" and the Y axis is called "Dominance".

This function aims to calculate which quadrant you are in given your answers. If you are on a boundary, it returns an array with the quadrants that you span. If there is a 4-way tie, Quadrant 4 is assumed.

It looks very messy and difficult to read. How could I improve it? I'd specifically like to pull the whole thing into a reduce function if I can to avoid the nasty if/else tree at the end.

calculateResult = (questions, answers) => {
    const res = _.zip(questions, answers)
      .filter(x => x[1]) // Only get correct answers
      .map(x => x[0].quadrant) // Pluck the quadrant number
      .reduce(
        (res, x) => {
          if (x == 1 || x == 4) {
            res.dominance++;
          } else {
            res.dominance--;
          }

          if (x == 3 || x == 4) {
            res.warmth++;
          } else {
            res.warmth--;
          }
          return res;
        },
        {
          dominance: 0,
          warmth: 0
        }
      );

    if (res.dominance > 0) {
      if (res.warmth > 0) {
        return [4];
      } else if (res.warmth < 0) {
        return [1];
      } else {
        return [1, 4];
      }
    } else if (res.dominance < 0) {
      if (res.warmth > 0) {
        return [3];
      } else if (res.warmth < 0) {
        return [2];
      } else {
        return [2, 3];
      }
    } else {
      if (res.warmth > 0) {
        return [3, 4];
      } else if (res.warmth < 0) {
        return [1, 2];
      } else {
        return [4];
      }
    }
};

Test Cases

The "questions" parameter pretty much always looks the same. (The program pulls 3 questions from each quadrant)

var questions = [
  { quadrant: 1 },
  { quadrant: 1 },
  { quadrant: 1 },
  { quadrant: 2 },
  { quadrant: 2 },
  { quadrant: 2 },
  { quadrant: 3 },
  { quadrant: 3 },
  { quadrant: 3 },
  { quadrant: 4 },
  { quadrant: 4 },
  { quadrant: 4 }
];

The "answers" parameter is always a list of booleans denoting true/false. Here are some test cases from my test suite.

const answersForQuadOne = [ // Should return [ 1 ]
  true,
  true,
  true,
  false,
  false,
  false,
  false,
  false,
  false,
  false,
  false,
  false
];

const answersOnBoundary = [ // Boundary should return [ 1, 2 ]
  true,
  true,
  true,
  true,
  true,
  true,
  false,
  false,
  false,
  false,
  false,
  false
];

Let me review your code first and then provide an alternative:

The naming for the reduce-function is a little confusing, since you're just using x. Generally the lambda parameters are problematically named. Only fixing the naming should already give huge boosts to maintainability:

const res = _.zip(questions, answers)
    .filter(qa => qa[1])
    .map(qa => qa[0].quadrant)
    .reduce(
        (res, quadrant) => {
        [...]
    );

that was the simple part. Along the way we kind of made the comments in the code superfluous. I'll not review the assignment of the return quadrants though it seems like there could be some simplifications if you didn't try to directly return a hardcoded array.

Let's reexamine your approach. You already stated that you're basically trying to accumulate increments and decrements along two axes. Let's consider the following then:

.filter(qa => qa[1])
.map(qa => {
  switch (qa[0].quadrant) {
     case 1:
       return { dominance: 1, warmth: -1 };
     case 2:
       return { dominance: -1, warmth: -1 };
     case 3:
       return { dominance: -1, warmth: 1 };
     case 4:
       return { dominance: 1, warmth: 1 };
     default:
       return { dominance: 0, warmth: 0 };
  }
}).reduce(
  (res, assessment) => {
    return { dominance: res.dominance + assessment.dominance,
             warmth: res.warmth + assessment.warmth }
  },
  { dominance: 0, warmth: 0 }
);

This clarifies your logic tremendously and makes obvious to the reader which quadrant entails which change in assessment


Python unit testing for a Stack data structure I am completely new to unit testing. I read this chapter in Dive Into Python 3, and decided to give it a try on a basic data structure: a stack.

Here is the stack code (taken from "Problem Solving with Algorithms and Data Structures using Python"):

class Stack:
     def __init__(self):
         self.items = []

     def isEmpty(self):
         return self.items == []

     def push(self, item):
         self.items.append(item)

     def pop(self):
         return self.items.pop()

     def peek(self):
         return self.items[len(self.items)-1]

     def size(self):
         return len(self.items)

Here is my testing code:

import unittest

class StackTester(unittest.TestCase):

    def test_stack_init(self):
        s = Stack()
        self.assertEqual(0, s.size())

    def test_push1_size(self):
        item = 5
        s = Stack()
        s.push(item)
        self.assertEqual(1, s.size())

    def test_push1_items(self):
        s = Stack()
        s.push(5)
        self.assertEqual([5], s.items)

    def test_push2_size(self):
        s = Stack()
        s.push(5)
        s.push(6)
        self.assertEqual(2, s.size())

    def test_push2_items(self):
        s = Stack()
        s.push(5)
        s.push(6)
        self.assertEqual([5,6], s.items)

    def test_push2_pop1_size(self):
        s = Stack()
        s.push(5)
        s.push(6)
        s.pop()
        self.assertEqual(1, s.size())

    def test_push2_pop1_value(self):
        s = Stack()
        s.push(8)
        s.push(9)
        self.assertEqual(9, s.pop())

    def test_push2_pop2_size(self):
        s = Stack()
        s.push("Glob")
        s.push("Blob")
        s.pop()
        s.pop()
        self.assertEqual(0, s.size())

    def test_push2_pop2_value(self):
        s = Stack()
        s.push("Glob")
        s.push("Blob")
        s.pop()
        self.assertEqual("Glob", s.pop())

    def test_isEmpty_init(self):
        s = Stack()
        self.assertEqual(True, s.isEmpty())

    def test_isEmpty_push1(self):
        s = Stack()
        s.push(5)
        self.assertEqual(False, s.isEmpty())

    def test_isEmpty_push1_pop1(self):
        s = Stack()
        s.push(1)
        s.pop()
        self.assertEqual(True, s.isEmpty())

    def test_peek_push1(self):
        s = Stack()
        s.push(88)
        self.assertEqual(88, s.peek())

    def test_peek_push3_pop1(self):
        s = Stack()
        s.push(7)
        s.push(889)
        s.push(3)
        s.pop()
        self.assertEqual(889, s.peek())

    def test_pop_empty(self):
        s = Stack()
        self.assertRaises(IndexError, s.pop)

    def test_peek_empty(self):
        s = Stack()
        self.assertRaises(IndexError, s.peek)

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

Am I leaving anything out? Is 16 tests for a stack overkill? What are the best practices for something like this?

There are certainly some good tests here. In a related question for Java, the answer by Phil Wright states:

  Here are some tests I'd perform at the very least. There may be more
  you'd want:

  Create an empty Stack. Test that its size is 0.
  Push an element onto the stack. Test that its size is now 1.
  Push another element onto the stack. Test that its size is now 2.
  Pop an element from the stack. Test that it matches the 2nd pushed value. Check that the size of the stack is now 1.
  Pop an element from the stack. Test that it matches the 1st pushed value. Check that the size of the stack is 0.
  Attempt to pop an element from the stack. You should receive an ArrayIndexOutOfBounds exception.

At a quick glance, you do most (if not all of these tests). So in some sense the coverage is good.

The amount of testing may be overkill. You have to realize you can't test every possible thing. With that in mind your tests should:

Find current issues with your code.
Nip likely future issues.
Prevent errors you've had in the past from coming back.

The past, present, and future. So for example, your function test_peek_push3_pop1 may have a good reason for being there. Maybe you had designed a stack before and had issues with pushing 3, popping 1, and then peeking. I've never personally had such an issue. It seems like a weird test case I wouldn't consider it, but maybe you have. If you have a compelling reason to have that test you should explain your reasoning in a comment so I can understand the purpose of the test. Ultimately:

  Think and comment WHY you have a test.


Is there a better way to make a function silent on need? I got a piece of code I'm not pleased with; Does anyone would have a better idea?

def myFunc(verbose=True):
    if not verbose:
        print = functools.partial(globals()['print'], file=open(os.devnull, 'w'))
    else:
        # Required, othewise I got the error 'local print variable was referenced before being declared' a few lines below…
        print = globals()['print']

    # Bunch of code with tons of print calls […]

I also tried the classical sys.stdout = open(os.devnull, 'w') but it can't work with my code because my print function is defined as such at the beginning of the code:

print = functools.partial(print, file=codecs.getwriter('utf8')(sys.stdout.buffer)) # Make sure the output is in utf-8

But I'm not really fan about having to use globals twice.

It depends on what you are looking to do.  For localized 'quiet' code, I use this:

class NoStdStreams(object):
    def __init__(self,stdout = None, stderr = None):
        self.devnull = open(os.devnull,'w')
        self._stdout = stdout or self.devnull or sys.stdout
        self._stderr = stderr or self.devnull or sys.stderr

    def __enter__(self):
        self.old_stdout, self.old_stderr = sys.stdout, sys.stderr
        self.old_stdout.flush(); self.old_stderr.flush()
        sys.stdout, sys.stderr = self._stdout, self._stderr

    def __exit__(self, exc_type, exc_value, traceback):
        self._stdout.flush(); self._stderr.flush()
        sys.stdout = self.old_stdout
        sys.stderr = self.old_stderr
        self.devnull.close()

then it is as easy as:

with NoStdStreams():
   print('you will never see this')

You could easily adapt it to:

with NoStdStreams(verbose):
    print('you may see this')


Python text-based game New to Python, learned a couple things. I'm trying to make a text based game and was told to come here for a review. Any tips would be appreciated to improve my code. I would like to expand this the more I learn.

import time
import random

def  game():
    #intro text
    print('******************************')
    print('     Welcome to the game!')
    print('*******************************')

    time.sleep(1)

    print('You awaken in a cave, unable to see. Darkness surrounds you.')
    time.sleep(1)
    print('\nSuddenly a small flickering light is seen in the distance.')
    time.sleep(1)
    print('\nyou get on your feet and decide wether you should head towards the light or wait where you are and hope help finds its way.')

#first question
def firstquestion():
    while True:
        print('\nDo you get up and see what could be the source of this light? \nOr do you wait?')
        Answer = input()
        if Answer == 'get up' or Answer == 'I get up':
            print('You begin to walk towards the light.')
            break
        elif Answer == 'wait':
            print('you wait, no one comes and you die...')
            break
        else:
            print('try again...')
#second question
def secondquestion():
    while True:
        print('\nThe light turns out to be a torch. Do you take this torch? enter y / n. ')
        Answer = input()
        if Answer == 'y':
            print('good choice, you pick up the torch and walk the out of the cave.')
            break
        elif Answer == 'n':
            print('well now your blind and dead...')
            break
        else:
            print('try again...')

game()
firstquestion()
secondquestion()
print()

As @l0b0 pointed out, you probably messed up your indents upon copying. Assuming they work:

First, looking at your question() functions, you can see a pattern, they prompt the user with a question. The user then has only two choices (binary like). If the user fails to enter a valid answer, they get prompted again. So why not create a function that gets passed the question and the binary answers? This also allows for easier expansion down the line like so:

def binary_question(question, answers):
    while True:
        print(question)
        Answer = input()
        if Answer in answers:
            print(answers[Answer])
            break
        else:
            print('try again...')

Questions = [
    '\nDo you get up and see what could be the source of this light? \nOr do you wait?',
    '\nThe light turns out to be a torch. Do you take this torch? enter y / n.',
]

Answers = [
    {
        'get up': 'You begin to walk towards the light.',
        'I get up': 'You begin to walk towards the light.',
        'wait': 'you wait, no one comes and you die...',
    },
    {
        'y': 'good choice, you pick up the torch and walk the out of the cave.',
        'n': 'well now your blind and dead...',
    },
]

game()
for que, ans in zip(Questions, Answers):
    binary_question(que, ans)

If you are not familiar with dictionaries or zip() look them up.

Second, you can allow for more choices by changing the user input strings to lower case and populating your Answers list to be only lower case. That way the user can enter "I get up" or "i get up", or "I GET UP", etc, and still get a valid response. Like so:

def binary_question(question, answers):
    while True:
        print(question)
        Answer = input()
        Answer = Answer.lower()
        if Answer in answers:
            print(answers[Answer])
            break
        else:
            print('try again...')

Questions = [
    '\nDo you get up and see what could be the source of this light? \nOr do you wait?',
    '\nThe light turns out to be a torch. Do you take this torch? enter y / n.',
]

Answers = [
    {
        'get up': 'You begin to walk towards the light.',
        'i get up': 'You begin to walk towards the light.',
        'wait': 'you wait, no one comes and you die...',
    },
    {
        'y': 'good choice, you pick up the torch and walk the out of the cave.',
        'n': 'well now your blind and dead...',
    },
]

game()
for que, ans in zip(Questions, Answers):
    binary_question(que, ans)

As you can see, we made use of str.lower() function and wrote lowercase only answers in our Answers dict.

Third, I just realized that if the player dies in question one, question two still gets asked. You need a return value to your main loop so you can break in case you are dead:

def binary_question(question, answers):
    while True:
        print(question)
        Answer = input()
        Answer = Answer.lower()
        if Answer in answers:
            print(answers[Answer][0])
            return answers[Answer][1]
        print('try again...')

Questions = [
    '\nDo you get up and see what could be the source of this light? \nOr do you wait?',
    '\nThe light turns out to be a torch. Do you take this torch? enter y / n.',
]

# can use named tuples here to make code more readable
# read about it if interested.
Answers = [
    {
        'get up': ('You begin to walk towards the light.', True),
        'i get up': ('You begin to walk towards the light.', True),
        'wait': ('you wait, no one comes and you die...', False),
    },
    {
        'y': ('good choice, you pick up the torch and walk the out of the cave.', True),
        'n': ('well now your blind and dead...', False),
    },
]

game()
for que, ans in zip(Questions, Answers):
    alive = binary_question(que, ans)
    if not alive:
        break

The new boolean values in Answers[] signify if that answer kills the player. In this case, True = alive and False = dead.


Determining the least-accurately known physical constants So, I thought I'd do this using scipy.constants:

import numpy as np
from scipy.constants import physical_constants

constants = np.array([(k,)+v+(v[2]/abs(v[0]),) for k,v in physical_constants.items()],
              dtype=[('name', 'S50'), ('val', 'f8'), ('units', 'S10'),
                     ('abs_unc', 'f8'), ('rel_unc', 'f8')])
constants.sort(order='rel_unc')
print(constants[constants['rel_unc'] > 0][['name', 'rel_unc']][-10:])

I was reasonably pleased to be using a structured array, but it all seems a bit... obscure and unpythonic. Any suggestions?

OK... so first thing I've realised: the boolean test for constants with relative uncertainties > 0 is not necessary if I want the largest 10 values (which are all non-negative), so I can ditch that. And a few comments wouldn't go amiss:

import numpy as np
from scipy.constants import physical_constants

# Create a structured array with fields for each constant's name, value, units,
# absolute uncertainty and relative uncertainty. The first of these is the key
# in the physical_constants dictionary; the next three are stored in a tuple
# as the corresponding value. Calculate the final field entry, the relative
# uncertainty from the absolute uncertainty and the absolute value of the
# constant.
constants = np.array([(k,)+v+(v[2]/abs(v[0]),)
                        for k,v in physical_constants.items()],
                     dtype=[('name', 'S50'), ('val', 'f8'), ('units', 'S10'),
                            ('abs_unc', 'f8'), ('rel_unc', 'f8')])
constants.sort(order='rel_unc')
print(constants[['name', 'rel_unc']][-10:])

However, pace Jeff Gohlke's comment, I'm reluctant to split up the list comprehension (which I think is a nice feature of Python). And something like

make_record = lambda k, v: (k,)+v+(v[2]/abs(v[0]),)
constants = np.array([make_record(k,v) for k,v in physical_constants.items()],
              dtype=[('name', 'S50'), ('val', 'f8'), ('units', 'S10'),
                     ('abs_unc', 'f8'), ('rel_unc', 'f8')])

is really the worst of both worlds.
How to maintain readability without, e.g. using temporary arrays?

I think the biggest problem here is that you're trying to do too much in a single line.  It seems obscure because it is obscure.  If I were looking at this outside of a Code Review setting, I would think that someone had purposefully obfuscated it or condensed it for some character count restriction.

Consider breaking apart your lines in order to make your code more legible.  Whoever looks over or has to maintain your code in the future (even if it's just you) will be incredibly grateful for it.

I was going to try and break this down into a legitimate block of code, but I have absolutely no idea what your various variables are supposed to represent.  What does the v list hold (e.g., what is v[0]? what is v[2]?).  But let's take your last line of code, which is at least somewhat comprehensible:

print(constants[constants['rel_unc'] > 0][['name', 'rel_unc']][-10:])

You're doing like four things here.  You're comparing to produce a bool, extracting an element of a list, performing a string slice operation, and printing a value.  Again, it seems obscure because it is obscure.

I can't even begin to trace out what you're trying to do in your first line of code, unfortunately, or I would.

I have a story which demonstrates the point.  On one of my more recent projects at work, a contractor had produced some code with several nested ternary conditions.  It looked something like this, in one line of JavaScript:

var_1 ? long_complex_variable_1 : var_2 ? long_complex_variable_2 :
    var_3 === var_4 ? (var_5 ? long_complex_variable_3 : long_complex_variable_4) :
    long_complex_variable_5

It took me awhile to figure out exactly what he was trying to accomplish.  When I asked him about it later, he said he thought he was being very clever and doing something good for the code by being "short and succinct".

But it's not clever, and it's far from helpful.  It's just frustrating for the people who have to maintain your code later.


Excel-formula-analysis state machine This is one of my first useful scripts. I run it on a highlighted text selection within Notepad++, but I've provided a sample formula and commented out the Scintilla-specific references.

I'm a rank amateur: Am I using good idiom? Is my approach to a state machine sensible?

# input_text = editor.getSelText()
input_text = '=HLOOKUP(TablePeriods[[#Headers],[YearMonth]],TablePeriods[[#All],[YearMonth]],MATCH([@Date],INDIRECT("TablePeriods[[#All],["&[@Account]&"]]"),-1))'

# initialize the state table
EVENTS = 11
STATES = 7
state_table = [[[0,0] for x in range(EVENTS)] for x in range(STATES)]

# event list and corresponding state table
event_list =     [ "\"",   "&",   "(",   ")",   ",", "\n\r\t ",    "[",   "]",   "{",   "}"    ] # last event is anything that doesn't match one of the other actions
state_table[0] = [[1,0], [0,1], [5,0], [0,3], [0,4],     [0,5],  [2,0], [0,0], [6,0], [0,0], [0,0]] # normal state
state_table[1] = [[0,0], [1,0], [1,0], [1,0], [1,0],     [1,0],  [1,0], [1,0], [1,0], [1,0], [1,0]] # double-quote comment
state_table[2] = [[2,0], [2,0], [2,0], [2,0], [2,0],     [2,0],  [3,0], [0,0], [2,0], [2,0], [2,0]] # inside bracketed table reference
state_table[3] = [[3,0], [3,0], [3,0], [3,0], [3,0],     [3,0],  [4,0], [2,0], [3,0], [3,0], [3,0]] # inside double-bracketed table reference
state_table[4] = [[4,0], [4,0], [4,0], [4,0], [4,0],     [4,0], [-1,0], [3,0], [4,0], [4,0], [4,0]] # inside triple-bracketed table reference (I don't think this exists)
state_table[5] = [[1,2], [0,2], [5,2], [0,0], [0,2],     [0,5],  [2,2], [0,2], [0,2], [0,2], [0,2]] # found left-paren; only wrap and insert if not empty, like =row()
state_table[6] = [[6,0], [6,0], [6,0], [6,0], [6,0],     [6,0],  [6,0], [6,0], [6,0], [0,0], [6,0]] # inside curly-braced array

# initialize the state, parenthesis depth, and output text
current_state = 0
paren_depth = 0
output_text = ""

# define the tab and new line characters
TAB_CHAR = "\t"
NEW_LINE = "\r\n"

for z in input_text:
    for a in range(len(event_list)):
        if z in event_list[a]:
            takeaction = state_table[current_state][a][1]
            current_state = state_table[current_state][a][0]
            break
    else:
        takeaction = state_table[current_state][-1][1] # this sets takeaction to the value when the test character does not match any event
        current_state = state_table[current_state][-1][0] # this sets current_state to the value when the test character does not match any event
    if current_state == -1:
        current_state = 5 # -1 is an error and should be handled with some sort of error operation
    if takeaction == 0: # just pass the characters along
        output_text += z
    elif takeaction == 1: # place character at same depth as only one on line
        output_text += NEW_LINE + TAB_CHAR*paren_depth + z + NEW_LINE + TAB_CHAR*paren_depth
    elif takeaction == 2: # previous character was a left paren and this one is not a right-paren
        paren_depth = paren_depth + 1
        output_text += NEW_LINE + TAB_CHAR*paren_depth + z
    elif takeaction == 3: # character is a right paren not immediately following a left paren
        paren_depth = paren_depth - 1
        output_text += NEW_LINE + TAB_CHAR*paren_depth + z
    elif takeaction == 4: # character is a comma, so start a new line at the paren depth
        output_text += z + NEW_LINE + TAB_CHAR*paren_depth
    elif takeaction == 5: # strip whitespace from the input outside of quotes
        pass
    else:
        pass # should raise error, since the state table includes more actions than are defined

# editor.replaceSel(output_text)
print(output_text)

My code, without sample, is on Gisthub.

You can use list unpacking to transform :

        takeaction = state_table[current_state][a][1]
        current_state = state_table[current_state][a][0]

into the more concise :

        current_state, takeaction = state_table[current_state][a]

. Also, it shows that the type you should be using to convey current_state and takeaction should probably be something more like a tuple than like a list.

The pythonic way to loop is not to use range and len but just to go through your iterable with for i in my_iterable:. If you do need the index, enumerate is what you need.

   for a in range(len(event_list)):
        if z in event_list[a]:
            takeaction = state_table[current_state][a][1]
            current_state = state_table[current_state][a][0]

becomes :

for i,event in enumerate(event_list):
    if z in event:
        current_state, takeaction = state_table[current_state][i]
        break

Remove whatever code is not needed.

elif takeaction == 5: # strip whitespace from the input outside of quotes
        pass
    else:
        pass # should raise error, since the state table includes more actions than are define

is roughly the same as nothing. If you want to ensure the value is correct, you can use assert.

From PEP 8 :

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

Remove duplicated logic :

        takeaction = state_table[current_state][a][1]
        current_state = state_table[current_state][a][0]

and

   takeaction = state_table[current_state][-1][1] # this sets takeaction to the value when the test character does not match any event
    current_state = state_table[current_state][-1][0] # this sets current_state to the value when the test character does not match any event

are quite similar and probably can be factorised out.

Do not repeat yourself (bis) : you do not need to define the state table twice. Just do it once and for all and everything should be ok :

state_table = [
    [(1,0), (0,1), (5,0), (0,3), (0,4), (0,5), ( 2,0), (0,0), (6,0), (0,0), (0,0)], # normal state
    [(0,0), (1,0), (1,0), (1,0), (1,0), (1,0), ( 1,0), (1,0), (1,0), (1,0), (1,0)], # double-quote comment
    [(2,0), (2,0), (2,0), (2,0), (2,0), (2,0), ( 3,0), (0,0), (2,0), (2,0), (2,0)], # inside bracketed table reference
    [(3,0), (3,0), (3,0), (3,0), (3,0), (3,0), ( 4,0), (2,0), (3,0), (3,0), (3,0)], # inside double-bracketed table reference
    [(4,0), (4,0), (4,0), (4,0), (4,0), (4,0), (-1,0), (3,0), (4,0), (4,0), (4,0)], # inside triple-bracketed table reference (I don't think this exists)
    [(1,2), (0,2), (5,2), (0,0), (0,2), (0,5), ( 2,2), (0,2), (0,2), (0,2), (0,2)], # found left-paren; only wrap and insert if not empty, like =row()
    [(6,0), (6,0), (6,0), (6,0), (6,0), (6,0), ( 6,0), (6,0), (6,0), (0,0), (6,0)], # inside curly-braced array
]

Final result :

This is what my code is like at the end. I am too lazy to change the string concatenations for a better solution. Also, I'm still not quite happy with the multiple if statements but I have nothing better to suggest at the moment.

#!/usr/bin/python

# input_text = editor.getSelText()
input_text = '=HLOOKUP(TablePeriods[[#Headers],[YearMonth]],TablePeriods[[#All],[YearMonth]],MATCH([@Date],INDIRECT("TablePeriods[[#All],["&[@Account]&"]]"),-1))'

# initialize the state table
# event list and corresponding state table
event_list =     [ "\"",   "&",   "(",   ")",   ",", "\n\r\t ", "[",   "]",   "{",   "}"    ] # last event is anything that doesn't match one of the other actions
state_table = [
    [(1,0), (0,1), (5,0), (0,3), (0,4), (0,5), ( 2,0), (0,0), (6,0), (0,0), (0,0)], # normal state
    [(0,0), (1,0), (1,0), (1,0), (1,0), (1,0), ( 1,0), (1,0), (1,0), (1,0), (1,0)], # double-quote comment
    [(2,0), (2,0), (2,0), (2,0), (2,0), (2,0), ( 3,0), (0,0), (2,0), (2,0), (2,0)], # inside bracketed table reference
    [(3,0), (3,0), (3,0), (3,0), (3,0), (3,0), ( 4,0), (2,0), (3,0), (3,0), (3,0)], # inside double-bracketed table reference
    [(4,0), (4,0), (4,0), (4,0), (4,0), (4,0), (-1,0), (3,0), (4,0), (4,0), (4,0)], # inside triple-bracketed table reference (I don't think this exists)
    [(1,2), (0,2), (5,2), (0,0), (0,2), (0,5), ( 2,2), (0,2), (0,2), (0,2), (0,2)], # found left-paren; only wrap and insert if not empty, like =row()
    [(6,0), (6,0), (6,0), (6,0), (6,0), (6,0), ( 6,0), (6,0), (6,0), (0,0), (6,0)], # inside curly-braced array
]

# initialize the state, parenthesis depth, and output text
current_state = 0
paren_depth = 0
output_text = ""

# define the tab and new line characters
TAB_CHAR = "\t"
NEW_LINE = "\r\n"

for z in input_text:
    for i,event in enumerate(event_list):
        if z in event:
            break
    else:
        i = -1
    current_state, takeaction = state_table[current_state][i]
    if current_state == -1:
        current_state = 5 # -1 is an error and should be handled with some sort of error operation
    if takeaction == 0: # just pass the characters along
        output_text += z
    elif takeaction == 1: # place character at same depth as only one on line
        output_text += NEW_LINE + TAB_CHAR*paren_depth + z + NEW_LINE + TAB_CHAR*paren_depth
    elif takeaction == 2: # previous character was a left paren and this one is not a right-paren
        paren_depth = paren_depth + 1
        output_text += NEW_LINE + TAB_CHAR*paren_depth + z
    elif takeaction == 3: # character is a right paren not immediately following a left paren
        paren_depth = paren_depth - 1
        output_text += NEW_LINE + TAB_CHAR*paren_depth + z
    elif takeaction == 4: # character is a comma, so start a new line at the paren depth
        output_text += z + NEW_LINE + TAB_CHAR*paren_depth
    else:
        assert(takeaction == 5) # strip whitespace from the input outside of quotes
        pass # should raise error, since the state table includes more actions than are defined

# editor.replaceSel(output_text)
print(output_text)


Extending IPrincipal.IsInRole() In my case, I needed to give roles to users, but to specific departments, and I did not want to implement a new authorization mechanism which will require me to access the db everytime I want to check if the user is authorized for a specific role to a specific department.

So I thought I will just create an extension method IsInRole with two parameters (role, departmentId). Here's the exact code:

public static bool IsInRole(this IPrincipal principal, string role, int departmentId)
{
    var ident = principal?.Identity;
    if (ident == null)
        return false;

    if (departmentId <= 0)
        return false;

    var roleDeparments = ((ClaimsIdentity)ident).FindFirst($"{role} departments")?.Value;
    if (string.IsNullOrWhiteSpace(roleDeparments))
        return false;

    var inInRole = roleDeparments
        .Split(',')
        .Any(d => d == departmentId.ToString() || d == "all");

    return inInRole;
}

The user roles are saved in the db and are fetched once upon logging in, if the departmentId is set to NULL then it means this role is general to all departments. The Identity object is created at login time using the following method:

private ClaimsIdentity CreateIdentity(User user)
{
    var identity = new ClaimsIdentity(MyAuthentication.ApplicationCookie, ClaimsIdentity.DefaultNameClaimType, ClaimsIdentity.DefaultRoleClaimType);
    identity.AddClaim(new Claim("http://schemas.microsoft.com/accesscontrolservice/2010/07/claims/identityprovider", "Active Directory"));
    identity.AddClaim(new Claim(ClaimTypes.Name, user.Id));
    identity.AddClaim(new Claim(ClaimTypes.GivenName, user.Name));
    identity.AddClaim(new Claim("DepartmentId", user.DepartmentId.ToString()));

    var roles = user.UserRoles
        .GroupBy(r => r.Role);

    foreach (var role in roles)
    {
        var departments = role
            .Select(r => r.DepartmentId)
            .Distinct()
            .ToList();

        bool isAll = departments.Any(d => d == null);

        string departmentIds = isAll
            ? "all"
            : string.Join(",", departments);

        identity.AddClaim(new Claim($"{role.Key.Name} departments", departmentIds));
    }

    return identity;
}

Now, I can simply do something like:

if (User.IsInRole("schedule viewer", 2))
{
   .....
}

Is this the right approach?

IsInRole()
I don't like how the method parameters are validated. IMO the null-propagation operator shouldn't be used for everything because it reduces the readability. Having a null-propagation operator for "safely" "accessing" a property is OK if the object is the first on the right hand of an assignment like in checking for var ident = principal?.Identity; but nevertheless I personally would stick to a combined null check like so
if (principal == null || principal.Identity == null) { return false; }

But why areyou checking this at the beginning of the method ? I would check simple types first like departmentId <= 0.
The null-propagation operator here
var roleDeparments = ((ClaimsIdentity)ident).FindFirst($"{role} departments")?.Value;

could be missed at first glance making a maintainer of the code wonder what this is about, at least if he/she has not that good eyes like me. He/she can't grasp at first glance that you "safely" access the Value property here.
Can you spot the error here ?
var inInRole = roleDeparments
    .Split(',')
    .Any(d => d == departmentId.ToString() || d == "all");

Aside from spelling error in inInRole you could store the result of departmentId.ToString() in a variable to check in the Any(), but maybe the compiler is smart enough to do this for you.
Omitting braces {} for single line if statemenst is your responsibility. I wouldn't do it. If I would do it then only like if (departmentId <= 0) return false;
Using abbreviations leads to less readable code. Why don't you rename ident  to identity ?
Implementing the mentioned points leads to
public static bool IsInRole(this IPrincipal principal, string role, int departmentId)
{
    if (departmentId <= 0) { return false; }
    if (principal == null || principal.Identity == null) { return false; }

    var identity = principal.Identity;

    var claim = ((ClaimsIdentity)identity).FindFirst($"{role} departments");
    if (claim == null) { return false; }

    var roleDeparments = claim.Value;
    if (string.IsNullOrWhiteSpace(roleDeparments)) { return false; }

    var isInRole = roleDeparments
        .Split(',')
        .Any(d => d == departmentId.ToString() || d == "all");

    return isInRole;
}

CreateIdentity()
This made me wonder
bool isAll = departments.Any(d => d == null);

reading isAll and Any. After rereading your question I stumbled over

if the departmentId is set to NULL then it means this role is general to all departments

This is screaming for a comment because a maintainer of the code should know this as well. Together with a better name it would be clear for a maintainer what is happening here like so
// If one of the user.UserRoles has a DepartmentId which is null,
// the role is general for all departments
bool isGeneralForAllDepartments = departments.Any(d => d == null);

string departmentIds = isGeneralForAllDepartments
    ? "all"
    : string.Join(",", departments);


StackEgg autoclicker I wrote a quick and dirty auto-clicker for the StackEgg. Any comments, both on code and effectiveness?

function doRound() {
    var stats = [];
    $(".egg-stat").each(function () {
        // Find the number of hearts for each stat.
        var title = $(this).attr("title");
        title && stats.push(parseInt(title, 10));
    });

    // Find the lowest stat and click the button next to it.
    var i = stats.indexOf(Math.min.apply(Math, stats));
    var buttons = $(".egg-action").find("button");

    buttons[i].click();
}

window.setInterval(doRound, 7000);

Best voting decision should be made just before the 20s timer ends to count most of the votes.
Instead of $(".egg-action").find("button") use a '.egg-action > button" selector
Too much jQuery, go back to VanillaJS and for loops

PS: works on Firefox also


Apply labels from a stream to a Bitraversable Labelling Traversable containers is an ancient and honorable test of skill. My task here is to create a generic labelling mechanism for a Bitraversable, such as a tuple, or a graph with vertex and edge labels.

bilabelWith
    :: Bitraversable t
    => (l ->  indexl -> l') -> (r ->  indexr -> r')
    -> Stream indexl        -> Stream indexr
    -> t l r -> t l' r'
bilabelWith f g xs ys =
  let labelLeft  = fmap (withStateLens fst (\(_, y) x' -> (x', y))) (labelOneWith f)
      labelRight = fmap (withStateLens snd (\(x, _) y' -> (x, y'))) (labelOneWith g)
  in flip evalState (xs, ys) . bitraverse labelLeft labelRight

labelOneWith :: (a -> index -> b) -> a -> State (Stream index) b
labelOneWith f u = do
    (Stream.Cons x xs) <- get
    put xs
    return (f u x)

withStateLens :: (s -> t) -> (s -> t -> s) -> State t a -> State s a
withStateLens thrust merge u = do
    s <- get
    let t = thrust s
    let (r, t') = runState u t
    put (merge s t')
    return r

Here is how it may be applied:

λ bilabelWith (+) (*) (Stream.fromList [2]) (Stream.fromList [3]) (5, 7)
(7,21)

A more complicated example requires us to obtain a Bitraversable graph.

circular :: (DynGraph gr, Bitraversable gr) => gr a b -> gr (V2 Double, a) b
circular gr =
  let n = fromIntegral (Graph.noNodes gr)
      coords =
        let alpha = (pi * 2 / n)
        in Stream.prefix [ 0.8 * V2 (sin (alpha * i)) (cos (alpha * i)) | i <- [1.. n] ]
            $ error "circular: Impossible error: not enough coordinates to label all points."
  in bilabelWith (flip (,)) const coords (Stream.repeat ()) gr

Does this code make sense? Is there anything to make more compact or clarify?

The fmap in labelLeft should be (.). Control.Lens.Zoom provides something like withStateLens. Inline definitions that are only used once.

bilabelWith
    :: Bitraversable t
    => (l ->  indexl -> l') -> (r ->  indexr -> r')
    -> Stream indexl        -> Stream indexr
    -> t l r -> t l' r'
bilabelWith f g xs ys = flip evalState (xs, ys) .
  bitraverse (zoom _1 . pops . f) (zoom _2 . pops . g)

pops :: (s -> a) -> State (Stream s) a
pops = flip fmap $ state $ \(Stream.Cons x xs) -> (x, xs)


Simple PHP inheritance using an abstract class I have created simple class inheritance of an abstract class Pet which is basically the skeleton of all other children classes.

abstract class Pet {

    private static $relations = array(
        'Cat',
        'Dog'
    );

    public static final function getClassById($id){
        return self::$relations[$id];
    }

    protected static $foods = array(

    );

    public static function getEdibleFoods(){
        $class = get_called_class();
        return $class::$foods;
    }

    abstract public function play();
    abstract public function eat();
    abstract public function sleep();

}

Then I have the Dog and Cat classes

class Dog extends Pet {

    protected static $foods = array(
        'bones'
    );

    public function eat() { }

    public function play() { }

    public function sleep() { }

}

class Cat extends Pet {

    protected static $foods = array(
        'fish'
    );

    public function eat() { }

    public function play() { }

    public function sleep() { }

}

As you see, the only difference between them is the foods that they like. My concern about this code is the getEdibleFoods static method in the parent class.

public static function getEdibleFoods(){
    $class = get_called_class();
    return $class::$foods;
}

In order to not have to implement it in all children classes, I figured I can use get_called_class() instead of self to return the foods static variable. Notice that every child class has its own foods array and they are not all the same.

My problem is that this does not look like clean code. I'd like to ask if this is good practice and if not, how is it done the right way?

I think the true problem is that the method shouldn't be static at all.

You should think about static methods as namespaced functions. They logically belong in the same category as the class they're defined on, but they do not actually operate on objects of that class. This means that their use (at least proper use) actually comes up a lot less than you'd think it would.

Typically static methods tend to be utility functions and this is for good reason. Utility methods are basically the only thing that don't have state, and things with state shouldn't be static. As a result of this, if a method is static, it should be idempotent.

Another concern in usability. Having a static method exist on a class hierarchy rather than just the top level ends up in a rather odd situation. Consider code consuming your Pet hierarchy. Obviously this hierarchy exists for the sake of polymorphism, so we can assume that your code takes objects by type Pet. Guess what? This makes calling getEdibleFoods incredibly unwieldy.

function doSomethingWithPetFoods(Pet $pet) {
    // How in the world do I call getEdibleFoods?
    // get_class($pet)::getEdibleFoods? Ew.
}

Don't forget that static methods also equal strong coupling (conveniently, you typically don't care if your coupled to a utility method). It's a bit hard to illustrate with a non-contrived example, so you'll have to bear with me.

function calculateSalesPrice($price, $tax) {
    return Math::round($price * (1 + $tax), 2); // Round to 2 digits
}

What could be wrong with this? It's just a simple round. The problem is that the code is now coupled to a very specific round. What if the United States demands that sales prices be rounded up to the nearest penny, and Argentina dictates that sales prices be rounded down to the nearest penny.

While this is a rather contrived example and its relevence to your code is rather limited, it's worth remembering.

Another problem is that you're (probably) not asking "what do cats eat?" when you call it. You're asking "what does this cat eat." It's a subtle difference, but an important one. What if two dogs eat different things? Or what if a certain cat is allergic to fish? This is why you have to be incredibly sure that a static method has absolutely not relation to state.


Many-To-Many movie director system I have the following relationship in EF Core 3.1.1 (Many-To-Many):
public partial class Movie
{
    public Movie()
    {
        Directors = new HashSet<MovieDirector>();
    }

    public int Id { get; set; }
    public string Title { get; set; }

    public virtual ICollection<MovieDirector> Directors { get; set; }
}

public partial class Director
{
    public Director()
    {
        MovieDirector = new HashSet<MovieDirector>();
    }

    public string Id { get; set; }
    public string Name { get; set; }

    public virtual ICollection<MovieDirector> MovieDirector { get; set; }
}

public partial class MovieDirector
{
    public int Id { get; set; }
    public int MovieId { get; set; }
    public string DirectorId { get; set; }

    public virtual Movie Movie { get; set; }
    public virtual Director Director { get; set; }
}

And I have the following method to create a Director:
public Director CreateDirector(int movieId, Director director)
{
    var movie = _repository.Movie.FirstOrDefault(w => w.Id == movieId);

    movie.Directors.Add(new MovieDirector{ Director = director });

    _repository.SaveChanges();

    return director;
}

The CreateDirector method can perfectly create a new Director including relationships.
My biggest question is if every time I need to insert in a table that has a relationship I will need:

Find the parent/root entity (Movie)

Create a new MovieDirector object

Associate the new MovieDirector object with the parent/root, like:
movie.Directors.Add(new MovieDirector)

Is this correct or is there some kind of shortcut?

When two entities are related in many-to-many relationship, neither of them are a parent/root entity nor a dependent/child entity. They are two individual entities totally independent of each other. They are related, but neither of them depends on the other.
At database level a parent/child dependency is mapped through a foreign-key. For a many-to-many relationship there is no foreign-key involved, and the relationship is mapped through a joining table, which we model with a joining entity at code level, (the MovieDirector entity in your example).
So, if Director and Movie are in a many-to-many relationship, then Movie is by no means a parent entity of Director, or vice-versa. Therefore, you should always be able to create a Director entity independent of any information referencing any movie. And from that perspective I see a major flaw in your code - currently you cannot create a Director entity independently, you need a movieId for that (which you shouldn't).
Think about a possible scenario where you might need to select one or more directors while creating a new movie, and that might be the first time some of those directors gets linked with a movie. What I'm trying to imply is, you can always have a director, or want to create one, for whom there is no movie in the database.
The signature of your CreateDirector method should be like -
public Director CreateDirector(Director director)

First setup any related entities (if there is any) before reaching the CreateDirector method -
director.MovieDirectors.Add(new MovieDirector { MovieId = movieId });

and then call for creating the new director -
public Director CreateDirector(Director director)
{
    _repository.Directors.Add(director);
    _repository.SaveChanges();
    return director;
}

Now you can create a director independently or, along with its related entities. I hope that simplifies your current approach too.


Simple War game in Swift I'm new to swift and I want to know if I'm doing something wrong or a harder way. I made a simple war game app. Tames of variables aren't correct, I know, but I was doing it quickly and I didn't worry about names of variables in that small project so I named them in English with Polish grammar.

import UIKit

class ViewController: UIViewController {

    @IBOutlet weak var LeftCardOutlet: UIImageView!
    @IBOutlet weak var RightcardOutlet: UIImageView!
    @IBOutlet weak var LeftScoreOutlet: UILabel!
    @IBOutlet weak var RightscoreOutlet: UILabel!

    var karty = ["card2","card3","card4","card5","card6","card7","card8","card9","card10","jack","queen","king"]

    var scoreplayer:Int=0
    var CPUscore:Int=0
    override func viewDidLoad() {
        super.viewDidLoad()

    }

    override func didReceiveMemoryWarning() {
        super.didReceiveMemoryWarning()

    }
    @IBAction func DealClicked(_ sender: UIButton) {
        var random1:Int = 0
        var random2:Int = 0
        random1 = Int(arc4random_uniform(UInt32(12)))
        random2 = Int(arc4random_uniform(UInt32(12)))
        var im1=UIImage(named:"\(karty[random1])")
        var im2=UIImage(named: "\(karty[random2])")
        LeftCardOutlet.image=im1
        RightcardOutlet.image=im2
        if random1>random2 {
            scoreplayer+=1
            LeftScoreOutlet.text=String(scoreplayer)
        }
        if random1<random2 {
            CPUscore+=1
            RightscoreOutlet.text=String(CPUscore)
        }
    }

}

Welcome to Swift! Everything looks like it's working correctly so I'm adding some thoughts that will apply as you continue working and as the code gets more complex...

You're not doing anything in your viewDidLoad or didReceiveMemoryWarning except call the super class so my preference is to remove them.

I'd really recommend not capitalizing your property and function names (e.g. use leftCardOutlet instead of LeftCardOutlet), once you start writing more in swift it will make reading your card harder.

I'd suggest extracting your random number generation into a computed property, and instead of hardcoding the number 12 using the size of your cards array.

Nothing wrong with using non-english grammar in variable names (actually Swift works really well with unicode so feel free to use fur Polish alphabet) but try to be consistent, e.g. either scorePlayer, scoreCPU or computerScore, playerScore -- not a mix of both.

Finally, to help prevent bugs/logic errors as the code gets more complex:

Default to assigning new variables as 'let', and change only to var if you find you're modifying it at some later time.
Don't create a variable if you're only going to use it in one place (e.g. next line or couple of lines down) shorter/more concise code is normally easier to read and rationalize as it gets more complex.
When you have if/else logic make sure you're capturing all cases. In this case there's the chance the numbers will tie, it's worth adding a placeholder block with a brief comment there (or to print message) even if you've not decided yet what to do there.

A few minor changes:

import UIKit

class ViewController: UIViewController {

    // made outlet property names shorter, don't need to include that they are outlet in the name
    @IBOutlet weak var leftCard: UIImageView!
    @IBOutlet weak var rightCard: UIImageView!
    @IBOutlet weak var leftScore: UILabel!
    @IBOutlet weak var rightScore: UILabel!

    var karty = ["card2","card3","card4","card5","card6","card7","card8","card9","card10","jack","queen","king"]

    // (personal preference, but removed `Int` type declaration since it's easily implied)
    var scorePlayer = 0
    var scoreCPU = 0

    private var randomDeal: Int {
        // (instead of hardcoding 12, use the size of your cards array)
        return Int(arc4random_uniform(UInt32(karty.count)))
    }

    @IBAction func dealClicked(_ sender: UIButton) {

        // (use 'let' instead of 'var' since they won't change in the scopy of this function)
        let random1 = randomDeal
        let random2 = randomDeal

        // (got rid of `im1`, `im2` variables and just assigning directly here)
        leftCard.image = UIImage(named:"\(karty[random1])")
        rightCard.image = UIImage(named: "\(karty[random2])")

        if random1 > random2 {
            //player wins
            scorePlayer += 1
            leftScore.text = String(scorePlayer)
        }
        else if random1 < random2 {
            // computer wins
            scoreCPU += 1
            rightScore.text = String(scoreCPU)
        }
        else {
            // tie - do nothing
            print("tie: nobody wins")
        }
    }
}


Compare values of elements in Stream I'm having fun with Java's Stream library and lambdas.

The following code looks for persons within a list that have the same ID (which might indicate that something's wrong with the data) and prints out each group of people that share one ID.

I'm not sure if I'm doing this the most concise way, though.

This is the Person class:

public class Person {
    private String name;
    private String id;

    public Person(String name, String id) {
        this.name = name;
        this.id = id;
    }

    public String getId() {
        return id;
    }

    @Override
    public String toString() {
        return "Person [name=" + name + ", id=" + id + "]";
    }
}

This is the code I'd love to get feedback on:

// Set up test data
List<Person> people = Arrays.asList(new Person("Michael", "1"),
        new Person("Tobias", "2"), new Person("Nicole", "3"),
        new Person("Sarah", "3"));

// Group persons by their ID
Map<String, List<Person>> peopleById = people.stream().collect(
        Collectors.groupingBy(Person::getId));

// Print out groups of people that share one ID
peopleById
        .values()
        .stream()
        .filter(peopleWithSameId -> peopleWithSameId.size() > 1)
        .forEach(
                peopleWithSameId -> System.out
                        .println("People with identical IDs: "
                                + peopleWithSameId));

The code does what I want and I think it's readable but I doubt that this is the most I can get out of Java's functional features.

I wonder if you know a more elegant way to solve the problem.

Your code and Java 8 usage looks fine in general to me.

I do see an issue with the Person class, it looks like you are intending it to be an immutable class, if so, then you should also enforce it.

You need to ensure that the name and id fields can never be changed, you can do this by adding final to them. Your code currently seems to be safe, but it is not. I can extend Person and offer a method there to change the name and id fields, which violates the assumed variant of that those fields in Person are immutable.

Simply changing it to the following will do:

public class Person {
    private final String name;
    private final String id;
    ...
}

Onto the Java 8 usage now.

It is a good thing that you use the Collectors.groupingBy to provide a Map<String, List<Person>>, you cannot do it much faster either way if you want it to work with any kind of List<Person> as input and in this way you'll save yourself from nasty bugs and reimplementing what lots of people have already done, namely the grouping by operation.

Printing the offending values using Stream seems fine as well, except that you may rewrite it to look a little bit cleaner, something like this could work:

peopleById.values().stream()
    .filter(personList -> personList.size() > 1)
    .forEach(personList -> System.out.println("People with identical IDs: " + personList);

This is my personal preference on how to format it though, the only real change is to rename peopleWithSameId to personList, as it is simply a List<Person> and nothing more or less.

You've done a good job overall.


Small library for logging to MySQL I've thrown together a utility for my programs to log to a database, but the code looks like a mess and I don't know how to clean it up. This library is going to be used from all of my other programs and it is therefore important to keep the code clean and secure.

#include <log_entry.h>

#include <my_global.h>
#include <mysql.h>

#include <stdio.h>
#include <string.h>

#define STR_BUFFER_SIZE 256

#define NELEMS(x)  (sizeof(x) / sizeof(x[0]))

void finish_with_error (MYSQL *);

int ltdb (LOG_ENTRY *ent)
{
    MYSQL *con = NULL;
    char time[16], date[16];
    char query[STR_BUFFER_SIZE];
    int len;

    con = mysql_init(NULL);
    if(NULL == con){
        fprintf(stderr, "%s\n", mysql_error(con));
        return 1;
    }

    if(NULL == mysql_real_connect(con, "localhost", "user", "password", "database", 0, NULL, 0)){
        finish_with_error(con);
        return 1;
    }

    strftime(time, NELEMS(time), "%H:%M:%S", ent->tm_info);
    strftime(date, NELEMS(date), "%Y-%m-%d", ent->tm_info);

    len = snprintf(query, STR_BUFFER_SIZE,
    "INSERT INTO `logg` ("
    "   severity,event,source,time,date"
    ") VALUES ("
    "   '%d', '%s', '%s', '%s', '%s'"
    ")",    ent->severity,
            ent->event,
            ent->source,
            time,
            date);

    if(len >= STR_BUFFER_SIZE){
        fprintf(stderr, "MySQL query too long! (> %d)\n", STR_BUFFER_SIZE);
        return 1;
    }

    if(mysql_query(con, query)){
        finish_with_error(con);
        return 1;
    }
    mysql_close(con);

    return 0;
}

void finish_with_error(MYSQL *con)
{
    fprintf(stderr, "%s\n", mysql_error(con));
    mysql_close(con);
}

I'm trying to achieve a fast and simple way to process my log entries, but I'm not really happy with the code structure in general. Any suggestions for improvements would be much appreciated.

Here is the table structure of logg

CREATE TABLE IF NOT EXISTS `logg` (
  `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
  `severity` tinyint(4) NOT NULL,
  `event` varchar(128) NOT NULL,
  `source` varchar(32) NOT NULL,
  `time` time NOT NULL,
  `date` date NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB  DEFAULT CHARSET=latin1 AUTO_INCREMENT=18 ;

I limit the length to avoid any potential security risk, and save storage in my database. I don't really need any longer log messages.

Here is also the definition of my LOG_ENTRY struct:

typedef struct {
    int severity;
    char *event ;
    char *source;
    struct tm *tm_info;
} LOG_ENTRY;

Assuming that LOG_ENTRY.event can have arbitrary string content, your code is vulnerable to an SQL injection attack.  You must escape strings in queries, and not copy them blindly into the query.

The 256-byte limit for the query buffer seems artificially limiting.  I would expect your library to do whatever it takes to compose and execute the right SQL query without bungling it with an undersized buffer.  One way to do that without malloc() or variable-length arrays is to define the struct to match the table definition.

typedef struct {
    int severity;
    char event[129]; /* 128 characters plus NUL. Latin1 means 1 byte per character */
    char source[33];
    struct tm tm_info;
} LogEntry;

The buffer would need to be large enough to accommodate the fixed text and the substituted strings, keeping in mind that mysql_real_escape_string() can double the number of bytes.  (I would use sizeof(FIXED_QUERY) + 2 * sizeof(LogEntry) + 3 * sizeof(int) + 20 as a generous estimate of the needed buffer size.)  Out of paranoia for unterminated strings, I'd use placeholders like "%.128s" for snprintf().

ltdb is such a cryptic function name.  log_to_database() would be much better.

Requiring the program to be recompiled to change the password is not proper.  The caller should have to make a call to initialize the library with the database connection parameters.  Another approach, which is still sloppy but slightly better, is for your library to read the connection parameters from a configuration file.


Write byte array to end of file I'm writing a byte array at the end of file. The code works fine, but can you please suggest performance improvements?

static void Main(string[] args)
    {
        string PathToFile = "C:\\myfile.m4a";
        WriteAaray(PathToFile, GetMyList());
    }
public static List<float> GetMyList()
    {
        // This list is dynamically generated can go upto 5000 points
        var mylist = new List<float> { 4, 90, 54, 6, 5324, 423, 432, 4353, 463, 5, 24, 32, 4, 32, 43, 3, 56, 3 };
        return mylist;
    }
public static bool WriteAaray(string originalFile, List<float> myFloatPoints)
    {
        FileStream inputFileStream = null;
        inputFileStream = new FileStream(originalFile, FileMode.Open);
        var length = inputFileStream.Length;
        inputFileStream.Seek(length, SeekOrigin.Begin);
        StringBuilder mystring = new StringBuilder();
        string myImpPoints = "[MYIMPPOINTS]";
        mystring.Append(myImpPoints);
        var points = string.Join(",", myFloatPoints.Select(f => f.ToString()));
        mystring.Append(points);
        byte[] byteArray = Encoding.ASCII.GetBytes(mystring.ToString());
        inputFileStream.Write(byteArray, 0, byteArray.Length);
        inputFileStream?.Close();
        return true;
}

Code analysis

Let's run through your code bit by bit.

First a note about paths: use verbatim strings to disable escape sequences, so you don't need to escape those slashes: @"C:\myfile.m4a" instead of "C:\\myfile.m4a".

public static bool WriteAaray(string originalFile, List<float> myFloatPoints)
{

originalFile is a path, not a file (stream), and I'm not sure why it's called 'original', so I would rename it to filePath.
I'd also fix the typo in Aaray.

    FileStream inputFileStream = null;
    inputFileStream = new FileStream(originalFile, FileMode.Open);

The 'input' in inputFileStream is misleading: you're not reading from this file, you're writing to it. I'd rename it to just file.
There's no need for that first line: just write var file = new ... instead.
File streams are disposable, so it's better to use a using statement here.
Note that the File class contains several useful methods, such as File.Open, or the very convenient File.AppendAllText.

    var length = inputFileStream.Length;
    inputFileStream.Seek(length, SeekOrigin.Begin);

There's no need for that local variable (length): just pass file.Length directly into Seek.
Instead of manually seeking, consider opening the file with FileMode.Append. Not only does that seek to the end for you, it also creates the file if it doesn't exist yet (in your code that'll throw an exception, which will crash your program because you're not catching it).

    StringBuilder mystring = new StringBuilder();
    string myImpPoints = "[MYIMPPOINTS]";
    mystring.Append(myImpPoints);

While string builders are often more efficient than string concatenation, in this case you're only concatenating two strings (this header and the result of that string.Join call), so a string builder won't help here - it might even be less efficient. Note that string.Join uses a string builder internally, and with 5000 floats it certainly is useful there.
Again, no need for a local variable (myImpPoints) - just pass it into Append directly.

    var points = string.Join(",", myFloatPoints.Select(f => f.ToString()));
    mystring.Append(points);

There's no need for that Select call - one of the overloads of string.Join takes an IEnumerable<T>, so you can pass your list of floats in directly.
On the other hand, this only gives you 7 digits of precision. If that's a problem, then you should keep the Select part, but be sure to specify "R" (round-trip) or "G9" (recommended for performance reasons) as the to-string format.
Again, no need for a local variable (points).
Note that formatting and parsing string representations of floats is slower than writing and reading them as binary data.

    byte[] byteArray = Encoding.ASCII.GetBytes(mystring.ToString());
    inputFileStream.Write(byteArray, 0, byteArray.Length);
    inputFileStream?.Close();

Consider using a StreamWriter instead. That takes care of encoding and buffering, so you don't have to mess with string builder buffers and byte arrays and all that.
With a using statement around the file stream, you don't need to call Close manually here. Either way, you don't need an 'elvis operator' here - inputFileStream obviously won't be null.

    return true;
}

You're returning a boolean to indicate success, but that's actually somewhat misleading: this method can fail, but instead of returning false (which I would expect, with such a method signature) it'll throw an exception. Either catch it and return false (but then the caller won't know why it failed), or don't return anything and let the method throw (but document it).

Alternatives

5000 floats isn't a whole lot, so unless you need to create a lot of these files, the following code should be fast enough:

File.AppendAllText(filePath, "[MYIMPPOINTS]" + string.Join(",", values), Encoding.ASCII);

If, after actually measuring performance, that turns out to be too slow, then here's what your code could look like with the above notes taken into account:

using (var file = File.Open(originalFile, FileMode.Append, FileAccess.Write))
using (var writer = new StreamWriter(file, Encoding.ASCII)) // buffer size can be adjusted if necessary
{
    writer.Write("[MYIMPPOINTS]");

    if (values.Count > 0)
        writer.Write(values[0]);

    for (int i = 1; i < values.Count; i++)
    {
        writer.Write(',');
        writer.Write(values[i]);
    }
}


Find max element in increasing-decreasing array Question:  Find the largest in a list that list is stored as two sections, one in ascending order and the other in descending order.  Elements are distinct.

E.g:  Input [2 3 4 5 6 7 10 9 8 7]

Output: 10

My solution is below.  Works on my unit-tests.  Want to know if there any improvements from the coding point of view (esp if it is asked in an interview).

long getMaxIndex(long *arr, size_t size)
{
    long left = 0;
    long right = size - 1;

    if (arr == nil) return 0;

    while (left < right)
    {
        long mid = left + ((right-left)/2);

        if(mid == size -1)
            return mid;

        if (arr[mid] > arr[mid-1] && arr[mid] > arr[mid+1])
        {
            return mid;
        }

        if (arr[mid] > arr[mid-1])
        {
            left = mid;
        }
        else
        {
            right = mid-1;
        }

        if (arr[mid] > arr[mid+1])
        {
            right = mid;
        }
        else
        {
            left = mid+1;
        }
    }

    return left;
}

Your code will segfault for array size = 2. Rest looks correct to me.

Specifically, for size = 2,

left = 0
right = 1
mid = 0

And hence, this line:

if (arr[mid] > arr[mid-1] && arr[mid] > arr[mid+1])

will try to access arr[mid-1] which is arr[-1] and can segfault/return incorrect answer.


Project Euler #12 runs very slowly The problem is :

  The sequence of triangle numbers is generated by adding the natural numbers. So the 7th triangle number would be 1 + 2 + 3 + 4 + 5 + 6 + 7 = 28. The first ten terms would be:

  1, 3, 6, 10, 15, 21, 28, 36, 45, 55, ...

  Let us list the factors of the first seven triangle numbers:

  1: 1

  3: 1,3

  6: 1,2,3,6

  10: 1,2,5,10

  15: 1,3,5,15

  21: 1,3,7,21

  28: 1,2,4,7,14,28

  We can see that 28 is the first triangle number to have over five divisors.

  What is the value of the first triangle number to have over five hundred divisors?

The code works now, when I set the condition in the last loop to 60, etc. instead of 500. But 500 is too big for it and it takes forever to answer (actually I didn't have enough patience to see how much time it takes, it takes a bit too much).

How can I make it faster?

def num_of_divisors(x):
    k=0
    b=x

    while x%2==0:
        x=x//2
        k=k+1
    z=1
    while x!=1:
        for y in range(3,b+1,2):
            a=0
            while x%y==0:
                x=x//y
                a=a+1
            z=z*(a+1)
    return(z*(k+1))

def triangular(n):
    return(n*(n+1)//2)

for n in range(1,10000):
    jk=triangular(n)
    if num_of_divisors(jk)>500:
        print(triangular(n))
        break

You're most of the way there, so I'll give you a hint to avoid spoiling the problem:

Your function triangular shows that you know that the formula for the \$ n \$th triangular number is $$ T(n) = { n(n+1) \over 2 }. $$ What numbers can possibly be divisors of \$ T(n) \$? Take a look at some small values of \$ n \$ and see if there is a pattern.

As far as code review goes, some quick points:

Better choice of variable names would make the code easier to read. For example k and a are essentially the same thing (each is the power of a prime in the factorization of x) so why do they have such different names?
y loops over the odd numbers, but if you're solving lots of Project Euler problems then you have probably written a prime number sieve and so you could use that here (and avoid the special case for 2 while you're about it).
Use augmented assignment to make the code shorter and simpler, for example, z=z*(a+1) can be written z *= a + 1.


Create a matrix with all elements having the value of the shortest path to a given position You're given the number of rows and columns and the indices for the reference point. Reference point is = 0.
i.e:
3 4 3 3
output:
4 3 2 3
3 2 1 2
2 1 0 1

I managed to solve the problem using the easiest solution there is. I calculated the elements from the same column, and then using 2 for loops I used for every single element the already calculated value from the respective row as a new reference point.
I was stuck on this at first because I tried to come up with a formula so I asked on stackoverflow, and told me to submit a code, any solution.. so I came up with this:

#include <iostream>
using namespace std;
int main()
{
    int v[501][501], i, j, m, n, o, p;
    cin >> m >> n >> o >> p;
    for (i = o; i >= 1; i--)
            v[i][p] = o - i;
    for(i = o;i <= m; i++)
        v[i][p] = i - o;
    for(i = 1; i <= m; i++)
        for(j = 1; j <= n; j++){
            if(j < p)
                v[i][j] = -j + p + v[i][p];
            else if(j > p)
                v[i][j] = j - p + v[i][p];
        }
    for(i = 1 ; i <= m; i++){
        for(j = 1; j <= n; j++)
            cout << v[i][j] << " ";
        cout << '\n';
    }
}

How can I improve it?

Some suggestions:

Use variable names that tells the reader what they are.
Dont do using namespace std; in the global namespace.
Always check that <stream> >> variable actually worked or else your program will run with uninitialized variables and cause undefined behaviour if they are read.
Use an unsigned type when dealing with subscripting.
If you use a hardcoded array size, check that the values entered by the user actually fits in the array.
Don't use a hardcoded array size when the required size is unknown at compile time.
Use 0-based arrays instead of 1-based.

When it comes to the actual algorithm, it seems like that the shortest path from any point is
 abs(point.row - reference.row) + abs(point.column - reference.column)
which would give code like this:

#include <cstdlib>
#include <iostream>
#include <limits>
#include <vector>

// a helper type to keep the reference point
class point_t {
public:
    point_t() : point_t(0, 0) {}
    point_t(unsigned Row, unsigned Col) : row(Row), col(Col) {}

    // a function to calculate the distance to another point
    unsigned distance_to(const point_t& p) const {
        return static_cast<unsigned>(
            std::abs(static_cast<int>(row) - static_cast<int>(p.row)) +
            std::abs(static_cast<int>(col) - static_cast<int>(p.col)));
    }

    // reading the point from an istream
    friend std::istream& operator>>(std::istream& is, point_t& p) {
        return is >> p.row >> p.col;
    }

private:
    unsigned row;
    unsigned col;
};

int main() {
    unsigned rows;
    unsigned cols;
    point_t ref;

    if(std::cin >> rows >> cols >> ref) {
        // check that it's not too big
        if(rows > std::numeric_limits<int>::max() ||
           cols > std::numeric_limits<int>::max()) {
            std::cerr << "matrix too big\n";
            return 1;
        }

        // create a 2D vector
        std::vector<std::vector<unsigned>> mat(rows, std::vector<unsigned>(cols, 0));

        // fill the matrix
        for(unsigned row = 0; row < rows; ++row) {
            for(unsigned col = 0; col < cols; ++col) {
                // use the distance_to function
                mat[row][col] = ref.distance_to(point_t(row, col));
            }
        }

        // print the result
        for(const auto& row : mat) {
            for(auto col : row) std::cout << col;
            std::cout << '\n';
        }
    }
}

Input (with a 0-based reference point)

3 4 2 2

Output

4323
3212
2101


Going to the thread pool and back using custom awaiters I've figured out a way to switch threads in the middle of a method. The benefit of that seems super obvious to me: that's what everyone wants to do anyway, and splitting up methods, using callbacks, or lambdas, or whatever is just code bureaucracy to make it work.

But if this is so obvious to me, Microsoft guys probably thought of that too, yet didn't implement it as the default way of doing things in TPL. This makes me nervous. These custom awaiters seem to run correctly, but maybe I just haven't hit the gotchas yet.

What could possibly go wrong?

public static class UsageTest
{
    public static async Task DoItTheUsualWay()
    {
        // update UI
        Dictionary<string, string> cantUseVar = null; // have to initialize
        await Task.Run(() =>
        {
            // do heavy lifting
            var cantAccessVarOutsideTheLambda = new object();
            cantUseVar = new Dictionary<string, string>();
        });
        // update UI
    }

    public static async Task UseTheCoolNewAwaiters()
    {
        // update UI
        await GoToThreadPool.Instance;
        // do heavy lifting
        var smoothSailing = new object();
        await GoToMainThread.Instance;
        // update UI
        smoothSailing.ToString();
    }
}

public class GoToThreadPool : INotifyCompletion
{
    public static readonly GoToThreadPool Instance = new GoToThreadPool();

    public bool IsCompleted
    {
        get { return Thread.CurrentThread.IsThreadPoolThread; }
    }

    public GoToThreadPool GetAwaiter()
    {
        return this;
    }

    public void GetResult() { }

    public void OnCompleted(Action continuation)
    {
        ThreadPool.QueueUserWorkItem(o => continuation());
    }
}

public class GoToMainThread : INotifyCompletion
{
    public static readonly GoToMainThread Instance = new GoToMainThread();

    public bool IsCompleted
    {
        get { return Thread.CurrentThread == Application.Current.Dispatcher.Thread; }
    }

    public GoToMainThread GetAwaiter()
    {
        return this;
    }

    public void GetResult() { }

    public void OnCompleted(Action continuation)
    {
        Application.Current.Dispatcher.BeginInvoke(continuation);
    }
}

What you are after is yielding to a different thread, which has been covered in this question. One difference is that Andrew's code implements ICriticalNotifyCompletion vs you're implementing just INotifyCompletion. I haven't found any good documentation which one to prefer and what Critical exactly means. It may mean that his code works in some weird corner case scenarios where yours might not - hard to say. Most articles state that it's optional to implement the critical interface so it may not matter.

The other difference is that Andrew's code doesn't provide an option to switch back plus he's using a struct while you are using a class (which is just a potential performance optimization)

The main conclusion was that Microsoft didn't provide this out of the box since they felt it was prone to abuse.

In general I prefer Andrew's usage pattern of TaskEx.YieldToThreadPoolThread() over your more direct await GoToThreadPool.Instance since it's slightly more readable.

So combining the two implementations one could end up with something like this:

public static class TaskEx
{
    public static YieldToThreadAwaitable YieldToThreadPool()
    {
        return new YieldToThread(null);
    }

    public static YieldToThreadAwaitable YieldToMainThread()
    {
        return new YieldToThread(Application.Current.Dispatcher.Thread);
    }

    public static YieldToThreadAwaitable YieldToThread(Thread target)
    {
        return new YieldToThread(target);
    }
}

public class YieldToThreadAwaitable : INotifyCompletion
{
    private Thread _Target;

    public YieldToThreadAwaitable(Thread target)
    {
        Target = target;
    }

    public bool IsCompleted
    {
        get { return _Target != null ? Thread.CurrentThread == _Target : Thread.CurrentThread.IsThreadPoolThread; }
    }

    public YieldToThreadAwaitable GetAwaiter()
    {
        return this;
    }

    public void GetResult() { }

    public void OnCompleted(Action continuation)
    {
        if (_Target == null)
        {
            ThreadPool.QueueUserWorkItem(o => continuation());
        }
        else
        {
            Dispatcher.FromThread(_Target).BeginInvoke(continuation);
        }
    }
}


Is there a better way to get the index of a minimum? Consider the following reproducible example:

# note that lh is a standard ts dataset that ships with R
lh
# fit an R model
ar.mle<-ar(lh,method="mle")

# now get the min AIC, this is the relevant line:
ar.mle$aic[ar.mle$aic==min(ar.mle$aic)]

This works fine and gives back the smallest AIC value and it's index, which is the suggested AR order. I feel I am repeating myself in this last line of code. Is there an easier way to obtain index and value?
I know I could use partial autocorrelations to determine the level, too. This is not a stats question, but an R indexing question.

perhaps the function which.min() would do the trick?

which.min(ar.mle$aic)

it won't shorten your code all that much:

ar.mle$aic[which.min(ar.mle$aic)]


Check is_permutation on a pair of same sized arrays - follow-up After considering the suggestions of my previous question, Check whether given a pair of arrays are permutation of each other, I changed the code.

Resolved the bug reported earlier
In place of std::set using std::unordered_map to attain \$O(n)\$ complexity
In place of indexing array used std::array

#include<iostream>
#include<algorithm>
#include<unordered_map>
#include<array>
template <typename T1, std::size_t N>
std::unordered_map<int,int> make_unordered_map(std::array<T1,N> & array)
{
    std::unordered_map<int,int> temp_unordered_map;
    auto temp_unordered_map_end = temp_unordered_map.end();
    for( auto &x: array)
    {
        auto it_temp_unordered_map = temp_unordered_map.find(x);
        if( it_temp_unordered_map == temp_unordered_map_end)
        {
            temp_unordered_map.insert(std::pair<T1,int>(x,1));
        }
        else
        {
            ++(it_temp_unordered_map->second);
        }
    }
    return temp_unordered_map;
}

template <typename T1, std::size_t N1, std::size_t N2>
bool is_permutation(std::array<T1,N1> & array1, std::array<T1,N2>&  array2)
{
    if( N1  != N2)
       return false;
    else
    {
        std::unordered_map<int,int> first_map = make_unordered_map(array1);
        std::unordered_map<int,int> second_map = make_unordered_map(array2);

        std::pair<std::unordered_map<int,int>::iterator,std::unordered_map<int,int>::iterator> myPair=
                std::mismatch(first_map.begin(),first_map.end(),second_map.begin(),
                [](std::pair<const int,int>& seed1, std::pair<const int,int> & seed2)
                  { return seed1.second == seed2.second;}
            );

        if(( myPair.first == first_map.end()) && (myPair.second == second_map.end()))
            return true;
        else
            return false;
    }
}

int main()
{
    std::array<int,5> array1 {{1,3,1,4,5}};
    std::array<int,4> array2 {{1,1,4,3}};

    if(::is_permutation<int,array1.size(), array2.size()>(array1, array2))
        std::cout<< " Arrays are permutation of each other\n";
    else
        std::cout<< " Arrays are not permutation of each other\n";

    return 0;
}

Genericity

Your algorithm currently only works for std::array and nothing else, which is a little bit sad: you could make it work for other sequence containers as well. You could improve this by making you algorithm take two pairs of iterators like the C++14 version of std::is_permutation:

template <typename ForwardIterator1, typename ForwardIterator2>
bool is_permutation(ForwardIterator1 first1, ForwardIterator1 last1,
                    ForwardIterator2 first2, ForwardIterator2 last2);

Note that I used forward iterators since the operations you use do not require anything more specialized to work.

You will have to make some compromises like using std::distance to check whether the sizes are the same, which eventually means that the size comparison will only be \$O(n)\$ for random-access iterators. In a future C++ world with concepts and a standardized std::size, you could have a signature like this:

bool is_permutation(const ForwardIterable& cont1,
                    const ForwardIterable& cont2);

Here, I used the ForwardIterable concept which represents types that have forward iterators. You could then use std::size to compute the size of the container; for std::list, you would have a size in \$O(1)\$ for example while it does not have random-access iterators.

However, we're still stuck in a world without concepts. We can emulate concept constraints but it gets pretty messy, so let's stuck to the iterator version for now even if it means that it won't be optimal.

Smaller concrete things

A few things easy to change that may make you smile:

If you intend to go the genericity way, then use std::begin and std::end instead of the begin and end functions since the former will work on more sequence containers.
These lines could be improved:

if(( myPair.first == first_map.end()) && (myPair.second == second_map.end()))
    return true;
else
    return false;

First, it's better to always use braces with if and other control structures to avoid small and silent errors in the future. But in this case, you can actually get rid of the condition altogether:

return myPair.first == first_map.end()
    && myPair.second == second_map.end();

Having to deal with std::pair whenever you want to use std::map and friends can be bothersome. Fortunately, there are ways to hide them. For example, you can change this line:

temp_unordered_map.insert(std::pair<T1,int>(x,1));

into this one:

temp_unordered_map.emplace(x, 1);

The function make_unordered_map does not tell much more than its return type. The name count_occurrences would probably be more suitable and provide more information.
If you plan to keep your function working only for std::array, then you can actually get rid of this runtime check:

if( N1  != N2)

You can replace it by a compile-time static_assert to warn users that they are doing something bad right from the start:

static_assert(N1 == N2, "is_permutation only accepts arrays of the same size");


Representing historical prices of certain financial instruments I have an object which represents historical prices of certain financial instruments as below:

 class DailyPrice
        {
            public DateTime? Date { get; set; }
            public double? Price { get; set; }
        }

This needs to be converted into QuarterlyPrice objects which will have all daily prices aggregated:

class QuarterlyPrice
        {
            public QuarterlyPrice()
            {
                DailyViews = new List<DailyPrice>();
            }
            public DateTime? Date { get; set; }
            public double? Price { get; set; }
            public List<DailyPrice> DailyViews { get; set; }
        }

Now I need to aggregate the daily prices into quarterly prices. I already have the logic of finding the quarters given a date range and the aggregation logic is as follows:

class Aggregater
        {
            public IEnumerable<QuarterlyPrice> GetQuarterlyAggViews(List<DailyPrice> dailyViews)
            {
                var dates = dailyViews.Where(x => x.Date.HasValue).Select(x => x.Date.Value).ToList();
                var dictionary = dailyViews.ToDictionary(x => x.Date.Value, x => x);

                //Will give me quarterly dates from the last to fist.
                var allQuarters = dates.Last().GetAllQuarters(dates.First());

                foreach (var quarter in allQuarters)
                {
                    var quarterlyPrice = new QuarterlyPrice();
                    var daily = dictionary.Where(x => x.Key <= quarter).Select(lx => lx.Value).ToList();
                    quarterlyPrice.DailyPrice.AddRange(daily);
                    quarterlyPrice.Date = quarter;
                    quarterlyPrice.Price = daily.Sum(x => x.Price);
                    yield return quarterlyPrice;
                }
            }
        }

My questions are the following:

My daily prices can spawn 10-20 years and there will be as many daily price object. While aggregating them I'm putting them in the dictionary assuming the lookup would be quicker. Is this the correct thing to do? Is there a way to optimize the look up a bit further?
How can I write a better aggregation logic for converting daily to quarterly price?

Note: The daily price list is sorted using .NET 4.0.

What about a slightly different way of creating your classes?

class Quarter
{
    int Year { get; set; }
    List<DailyPrice> Prices { get; set; }
}
class DailyPrice
{
    public DailyPrice(DateTime? date, double? price)
    {
        Date = date;
        Price = price;
        QuarterPeriod = FindOrCreateQuarterObject(this);
    }
    public DateTime? Date { get; set; }
    public double? Price { get; set; }
    public Quarter QuarterPeriod { get; set; }
}

As you create your DailyPrice objects, you can add to the pointer list in the appropriate quarter.  That way if you are collecting aggregate data by quarter all you have to do is spin through your quarters, and then do the aggregation on each quarter's list.

It takes a little more time upfront, and a few more pointers, but you'll save time if you plan to do multiple calculations.


Eulerian Tour in Python This is a recursive algorithm implementation of Eulerian tour search.

I guess there is no way to make it more efficient (except rewriting with loops instead of recursion). Any advice on style?

def sub(visited, _cur, graph):
    if not graph:
        return visited + [_cur]
    for i, edge in enumerate(graph):
        cur, nex = edge
        if _cur not in edge:
            continue
        _graph = graph[:]
        del _graph[i]
        if _cur == cur:
            res = sub(visited + [cur], nex, _graph)
        else:
            res = sub(visited + [nex], cur, _graph)
        if res:
            return res

def find_eulerian_tour(graph):
    head, tail = graph[0], graph[1:]
    prev, nex = head
    return sub([prev], nex, tail)

assert find_eulerian_tour([(1, 2), (2, 3), (3, 4), (4, 1)]) == [1, 2, 3, 4, 1]
assert find_eulerian_tour([
    (0, 1), (1, 5), (1, 7), (4, 5),
    (4, 8), (1, 6), (3, 7), (5, 9),
    (2, 4), (0, 4), (2, 5), (3, 6),
    (8, 9)
]) == [0, 1, 7, 3, 6, 1, 5, 4, 8, 9, 5, 2, 4, 0]

Comments and good docstrings would go a long way. Obviously if I was familiar with the algorithm I'd understand easier, but you're not helping a lot with your naming. Why are you using _cur and cur? Do you just want to use the same name twice without clashing? If so you should do it another way, because _var is the Python style of indicating a variable that should be treated as private and not modified externally. It's an idiom for classes and looks out of place here.

Even still, cur, nex mean current and next, right? As you noted you shouldn't use next because it's a builtin name, but using next_branch or something more descriptive is better than stripping away a letter. Also if I've got the wrong end of the stick and nex is something else, that also just means you need a better shortened word or a comment explaining what nex is.

Also more whitespace will make the flow of your code easier to read than a solid block of text:

def sub(visited, _cur, graph):
    if not graph:
        return visited + [_cur]

    for i, edge in enumerate(graph):
        cur, nex = edge
        if _cur not in edge:
            continue

        _graph = graph[:]
        del _graph[i]

        if _cur == cur:
            res = sub(visited + [cur], nex, _graph)
        else:
            res = sub(visited + [nex], cur, _graph)

        if res:
            return res

Even before adding code and more readable names, this breaks up what's happening so a reader can parse it easier.