Parse numbers from text based on relative position to certain substrings The input is a product code. I want to pull out the "percentage" portion of that code. For example, if the code contains the substring BIO, I want all the numbers that follow it until I hit another non-numeric character. You can see from the PATINDEXs the other situations - it probably doesn't add much typing them in English.

Here's the tests:

Product          Returns
DS2BIO5          5%
DS2BIO20WB       20%
DS2BIO2RED7      2%
DS2BIO11         11%
E75              75%
E85              85%
NL10ET87         10%
MG15ET89         15%

I don't have performance issues, mainly because my data set is pretty small. I just don't think I'm competent when it comes to string manipulation in T-SQL and I'd like to use best practices. Also, if I had another condition I would have another IF nesting and I'm already at my limit for what I can tolerate.

ALTER FUNCTION dbo.fnExpectedBlend
    (
        @Item VARCHAR(30)
    )
RETURNS NUMERIC(15, 6)
AS
    BEGIN
        DECLARE @Pos INT

        IF PATINDEX('%BIO[0-9]%', @Item) > 0
            BEGIN
                SET @Pos = PATINDEX('%BIO[0-9]%', @Item) + 3
                SET @Item = SUBSTRING(@Item, @Pos, LEN(@Item))
                SET @Pos = PATINDEX('%[^0-9]%', @Item)
                IF @Pos > 1
                    SET @Item = SUBSTRING(@Item, 1, @Pos - 1)

            END
        ELSE
            BEGIN
                IF PATINDEX('E[0-9][0-9]', @Item) > 0
                    SET @Item = SUBSTRING(@Item, 2, LEN(@Item))
                ELSE
                    BEGIN
                        IF PATINDEX('%[0-9]ET[0-9]%', @Item) > 0
                            SET @Item = SUBSTRING(@Item, 3, 2)
                        ELSE
                            SET @Item = '0'
                    END
            END

        RETURN ISNULL(CAST(@Item AS NUMERIC(15,6)) / 100, 0)
    END

As long as there are just a small number of patterns (and they hardly change) you can use a CASE to find the start position of the digits you want to extract and then simply extract the following n characters if they're digits:

CREATE FUNCTION fnExpectedBlend
    (
        @Item VARCHAR(30)
    )
RETURNS NUMERIC(15, 6)
AS
    BEGIN
        DECLARE @Pos INT

        SET @Pos =
        CASE-- PATINDEX plus length of search string
           WHEN PATINDEX('%BIO[0-9]%', @Item) > 0 THEN PATINDEX('%BIO[0-9]%', @Item) +3 -- PATINDEX plus length of search string
           WHEN PATINDEX('E[0-9]'    , @Item) > 0 THEN PATINDEX('E[0-9]'    , @Item) +1
           WHEN PATINDEX('NL[0-9]%'  , @Item) > 0 THEN PATINDEX('NL[0-9]%'  , @Item) +2
           WHEN PATINDEX('MG[0-9]%'  , @Item) > 0 THEN PATINDEX('MG[0-9]%'  , @Item) +2
           -- easy to extend with additional codes
        END;

    -- assuming there's up to 2 digits, otherwise add another WHEN
    RETURN ISNULL(CAST(CASE WHEN SUBSTRING(@Item, @Pos, 2) LIKE '[0-9][0-9]'
                            THEN SUBSTRING(@Item, @Pos, 2)
                            ELSE SUBSTRING(@Item, @Pos, 1) -- first character is always a digit
                       END AS NUMERIC(15,6)) / 100, 0)
END


Four-function calculator design I am new to C++ and am looking to expand my knowledge. Below is a simple program that I've made. I would like to know how it could be improved, in any way. The introduction of new ways to do things is what I am looking for. These could be anything from improving efficiency to validating input - just whatever you think is most important or beneficial.

float calculate(float x, char y, float z) {
    float answer;

    switch (y) {

        case '+':
            answer = x + z;
            break;

        case '-':
            answer = x - z;
            break;

        case '/':
            answer = x / z;
            break;

        case '*':
            answer = x * z;
            break;

        default:
            return(0);
    }

    cout <<"= "; return answer;
}

int main() {
    float num1;
    float num2;
    char aOp;

    cout << ">> ";
    cin >> num1 >> aOp >> num2;
    cout << calculate(num1, aOp, num2) << endl << endl;
}

Headers.  Did you leave out the includes and function prototype at the top for simplicity or something?  If not, you need to put them here otherwise your program will not compile.
Function-returning and displaying.  The last statement in your function along with the printing of the function's return value in main() will work, but it's not a good way to write it.

Instead, move the cout <<"= "; from your function (just keep return answer;) to your std::cout statement in main().
Function arguments/parameters.  You may want to keep like-types together for clarity and so that you don't mismatch them, which would cause bugs.  Whichever order is easiest to remember.

For instance, consider these:

calculate(num1, num2, aOp);            // function call
calculate(float x, float y, char z) {} // function definition

Early return in switch.  Instead of having a local variable to update and return at the end, you could instead return from the respective case:

switch (y) {
    case '+': return x + z;
    case '-': return x - z;
    case '/': return x / z;
    case '*': return x * z;
    default: std::logic_error("invalid operator"); // from <stdexcept>
}

Notice this default statement.  If the user enters an invalid operator, but no input validation was done beforehand, and exception will be thrown.  Overall, it's best to have a useful default, and this is one example.
You should also make sure the user isn't dividing by 0.  If so, don't let the calculation take place, otherwise there will be problem.  For such a case, you can just terminate from main() early:

if (num2 == 0 && aOp == '/')
{
    std::cout << "You cannot divide by 0!  Terminating...";
    return EXIT_FAILURE;
}


C# Recording Traffic Data I would like to record client information when they go into a location on the web site so I created this ProcessRequest class that will collect data and insert it into the database. This will collect HttpContext and send that data to a Store Procedure that will then insert into a database table.

namespace DataCollection
{
    public class siteDataCollection
    {
        public void ProcessRequest(HttpContext context)
        {
            string conn = ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString;
            string filename = Path.GetFileName(context.Request.Url.AbsolutePath);

            string customerGuid = context.Request.Cookies["customerguid"].Value.ToString();
            customerGuid = customerGuid.Replace("%2D", "-");

            if(customerGuid is null) {

                customerGuid = HttpContext.Current.Session.SessionID;
            }

            SQLHelper sqlh = new SQLHelper(conn);
            SqlDataReader dr = null;

            SqlParameter[] parms = new SqlParameter[] {
                            new SqlParameter("@CustomerGUID", SqlDbType.NVarChar, 50),
                            new SqlParameter("@FileName", SqlDbType.NVarChar, 255),
                            new SqlParameter("@REMOTE_ADDR", SqlDbType.NVarChar,15),
                            new SqlParameter("@LOCAL_ADDR", SqlDbType.NVarChar,15),
                            new SqlParameter("@HTTP_USER_AGENT", SqlDbType.NVarChar,255),
                            new SqlParameter("@HTTP_REFERER", SqlDbType.NVarChar,255)
                            };

            parms[0].Value = customerGuid;
            parms[1].Value = filename;
            parms[2].Value = HttpContext.Current.Request.ServerVariables["REMOTE_ADDR"];
            parms[3].Value = HttpContext.Current.Request.ServerVariables["LOCAL_ADDR"];
            parms[4].Value = HttpContext.Current.Request.ServerVariables["HTTP_USER_AGENT"];
            parms[5].Value = HttpContext.Current.Request.ServerVariables["HTTP_REFERER"];

            dr = sqlh.ExecuteReaderStoreProcedure("usp_InsertFileLocation", parms);

            context.Response.Clear();
            context.Response.ClearContent();
            context.Response.ClearHeaders();
            context.Response.AddHeader("Pragma", "no-cache");
            context.Response.AddHeader("Expires", "-1");

            context.Response.ContentType = "application/pdf";
            byte[] bytePDF = File.ReadAllBytes(filename);
            context.Response.BinaryWrite(bytePDF);
            context.Response.End();
        }
    }
}

The parms array could be initialized like this:

new SqlParameter("@REMOTE_ADDR", SqlDbType.NVarChar,15) { Value = HttpContext.Current.Request.ServerVariables["REMOTE_ADDR"] }

It gives minimal performance gain, cause you don't need to access the array members, and it becomes more robust against the order of parameters. You could add a new one on top, without the need to change the indexes of all the other ones.

This is useless

 SqlDataReader dr = null;

it can be done right here:

 var dr =  sqlh.ExecuteReaderStoreProcedure("usp_InsertFileLocation", parms);

It's still useless, you can omit the entire dr variable.

In general I cannot see what this routine is doing. Does it a Query or an Insert on the Database. It Executes a Reader, but there's something called "InsertFileLocation". So this need at least some comment. For a clear design you might divide Taking the Request and Storing something in two different routines. Cause you should be able to test it separately.

If you have a HttpContext as a Parameter, why do you access the static variable HttpContext.Current. It is probably the same reference, but it's not good habit to use a static workaround if you already have a local parameter with the same content.


Optimizing mouse in/out I have this code that places a marker and on mouse-over this marker is scaled out and then back to the 'original' scale:

this.drawPerson = function () {
   self.svg.append("path")
        .attr("d", personPath)
        .attr("transform", "translate(100,100)scale(0.1)")
        .attr("class", "member")
        .style("fill", "steelblue")
        .on("mouseover", function(){
            d3.select(this).transition()
                .style("fill", "red")
                .attr("transform", "translate(100,100)scale(0.2)")
            })
        .on("mouseout", function() {
            d3.select(this).transition()
                .style("fill", "steelblue")
                .attr("transform", "translate(100,100)scale(0.1)")
            });
}

The x and y are the coordinates for the position on the canvas.

Here is the example: http://jsfiddle.net/SuTZR/8/

There does not seem to be a great way of solving this.

The best I can propose is to capture the style that you will re-set to into a function and use that function both during initialization and mouseout, this makes the code DRY'er, but not necessarily nicer:

this.drawPerson = function () {

  function style( svg ){
    return svg.style("fill", "steelblue")
              .attr("transform", "translate(100,100)scale(0.1)");
  }

   style( self.svg.append("path")
        .attr("d", personPath)
        .attr("class", "member") )
        .on("mouseover", function(){
            d3.select(this).transition()
                .style("fill", "red")
                .attr("transform", "translate(100,100)scale(0.2)")
        })
        .on("mouseout", function() {
            style( d3.select(this).transition() )
        });
}


Hailstone Sequence in NASM For practice, I wrote some NASM code that prints out the hailstone sequence of a (unfortunately, hardcoded) number.
This is by far the most complex code I've ever written in NASM. I'd like advice on anything, but specifically:

I'm trying to abide by CDECL. Am I doing anything off?
The multiplication part seems overly complicated. The problem is, mul doesn't take an immediate, and the register that I want to multiply is ebx, not eax, so I need to do a couple movs before I can multiply.
Anything else that's worth mentioning.

hail.asm:
global _start

section .data
    newline: db `\n`
    end_str: db `1\n`

section .text
    print_string:  ; (char* string, int length)
        push ebp
        mov ebp, esp

        push ebx

        mov eax, 4
        mov ebx, 1
        mov ecx, [ebp + 8]
        mov edx, [ebp + 12]
        int 0x80

        pop ebx

        mov esp, ebp
        pop ebp

        ret

    print_int:  ; (int n_to_print)
        push ebp
        mov ebp, esp

        push ebx
        push esi

        mov esi, esp  ; So we can calculate how many were pushed easily

        mov ecx, [ebp + 8]

        .loop:
            mov edx, 0  ; Zeroing out edx for div
            mov eax, ecx  ; Num to be divided
            mov ebx, 10  ; Divide by 10
            div ebx
            mov ecx, eax  ; Quotient

            add edx, '0'
            push edx  ; Remainder

            cmp ecx, 0
            jne .loop

        mov eax, 4  ; Write
        mov ebx, 1  ; STDOUT
        mov ecx, esp  ; The string on the stack
        mov edx, esi
        sub edx, esp  ; Calculate how many bytes were pushed
        int 0x80

        add esp, edx

        pop esi
        pop ebx

        mov esp, ebp
        pop ebp

        ret

    main_loop:  ; (int starting_n)
        push ebp
        mov ebp, esp

        push ebx

        mov ebx, [ebp + 8]  ; ebx is the accumulator
        .loop:
            push ebx
            call print_int
            add esp, 4

            push 1
            push newline
            call print_string
            add esp, 8

            test ebx, 1
            jz .even
            .odd:
                mov eax, ebx
                mov ecx, 3  ; Because multiply needs a memory location
                mul ecx
                inc eax
                mov ebx, eax
                jmp .end

            .even:
                shr ebx, 1

            .end:
                cmp ebx, 1
                jnz .loop

        push 2
        push end_str
        call print_string
        add esp, 8

        pop ebx

        mov esp, ebp
        pop ebp

        ret

    _start:
        push 1000  ; The starting number
        call main_loop
        add esp, 4

        mov eax, 1
        mov ebx, 0
        int 0x80

Makefile:
nasm hail.asm -g -f elf32 -Wall -o hail.o
ld hail.o -m elf_i386 -o hail

Multiplying by 3

The multiplication part seems overly complicated. The problem is, mul doesn't take an immediate, and the register that I want to multiply is ebx, not eax, so I need to do a couple movs before I can multiply.

This is all true, but based on the premise that the mul instruction must be used. Here are a couple of alternatives:

imul ebx, ebx, 3, listed in the manual as a signed multiplication, but that makes no difference, because only the low half of the product is used.
lea ebx, [ebx + 2*ebx], even the +1 can be merged into it: lea ebx, [ebx + 2*ebx + 1]. As a reminder, lea evaluates the address on the right and stores it in the destination register, it does not access memory despite the square-brackets syntax. 3-component lea takes 3 cycles on some processors (eg Haswell, Skylake), making it slightly slower than a 2-component lea and a separate inc. 3-component lea is good on Ryzen.

Dividing by 10
The simplest way is of course to use the div instruction, but that's not the fastest way, and it's not what a compiler would do. Here is a faster way, similar to how compilers do it, based on multiplying by a fixed-point reciprocal of 10 (namely 235 / 10, the difference between 235 and 232 is compensated for by shifting right by 3, the remaining division by 232 is implicit by taking the high half of the output of mul).
; calculate quotient ecx/10
mov eax, 0xCCCCCCCD
mul ecx
shr edx, 3
mov eax, ecx
mov ecx, edx
; calculate remainder as n - 10*(n/10)
lea edx, [edx + 4*edx]
add edx, edx
sub eax, edx

push edx in print_int
This will put 4 bytes on the stack for every character of the decimal representation of the integer, 1 actual char and 3 zeroes as filler. That looks fine when printed because a zero does not look like anything, so I'm not sure if this should be classed as a bug, but it just seems like an odd thing to do. The characters could be written to some buffer byte-by-byte, with a store and decrementing the pointer, then there would not be zeroes mixed in. A similar "subtract pointers to find the length"-trick could be used, that's a good trick.
Small tricks

mov edx, 0  ; Zeroing out edx for div

That's fine but xor edx, edx is preferred, unless the flags must be preserved.

    jmp .end
.even

Given that n is odd, 3n+1 is even, so you could omit the jump and have the flow of execution fall straight into the "even" case. Of course that means that not all integers in the sequence are printed, so maybe you can't use this trick, depending on what you want from the program.
If skipping some numbers to accelerate the sequence is OK, here is an other trick for that: skip a sequence of even numbers all at once by counting the trailing zeroes and shifting them all out.
tzcnt ecx, ebx
shr ebx, cl

   mov esp, ebp
   pop ebp

If you want (it doesn't make a significant difference, so it's mostly personal preference), you can use leave instead of this pair of instructions. Pairing the leave with enter is not recommended because enter is slow, but leave itself is OK. GCC likes to use leave when it makes sense, but Clang and MSVC don't.

       cmp ecx, 0
       jne .loop

That's fine, but there are a couple of alternatives that you may find interesting:

test ecx, ecx
jne .loop

Saves a byte, thanks to not having to encode the zero explicitly.

jecxz .loop

This special case can be used because ecx is used. Only 2 bytes instead of 5 or 4. However, unlike a fusible arith/branch pair, this costs 2 µops on Intel processors. On Ryzen there is no downside.


Game loop decorator for Pygame I've built a simple decorator that wraps a function in a general Pygame game loop. It also allows for the programmer to set the tick rate.

import pygame
import sys

pygame.init()

def game_loop(tick_rate=60):
    """
    This simple decorator wraps functions
    with a pygame game loop. Here's an
    example of usage:

        @game_loop(tick_rate=70)
        def main():
            ...
    """
    def game_loop_decorator(function):
        def wrapper(*args, **kwargs):
            clock = pygame.time.Clock()

            while True:
                clock.tick(tick_rate)
                function(*args, **kwargs)

                for event in pygame.event.get():
                    if event.type == pygame.QUIT:
                        pygame.quit()
                        sys.exit(0)

                pygame.display.flip()
        return wrapper
    return game_loop_decorator

Here's some example usage:

screen = pygame.display.set_mode((640, 480))
pygame.display.set_caption("Test game!")

@game_loop(tick_rate=100)
def main():
    font = pygame.font.SysFont("monospace", 15)
    text = font.render("Look! A game loop!", 1, (255, 255, 255))
    screen.blit(text, (50, 50))

if __name__ == "__main__":
    main()

style

PEP8

There is one too many blank line after your imports.
It is debatable if there is too many blank lines in your decorator too. I personally think it's good without the blank lines.
If you ignore the docstring then 17.6% of the function is blank lines.

  Use blank lines in functions, sparingly, to indicate logical sections.

You may feel the white space is needed, and so this section can be happily ignored.

PEP257

  Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description.

Yours has this short summary with the elaborate description.
I would be thrown off if your summery came up in my IDE,as I would be confused where this example is.

  The docstring is a phrase ending in a period. It prescribes the function or method's effect as a command ("Do this", "Return that"), not as a description; e.g. don't write "Returns the pathname ..."

Your summary line is more of a description. I'm not good with words, but the summary could be changed to, "Decorate a function inside a pygame loop, at the specified tick-rate.".

  The docstring for a function or method should summarize its behavior and document its arguments, return value(s), side effects, exceptions raised, and restrictions on when it can be called (all if applicable). Optional arguments should be indicated. It should be documented whether keyword arguments are part of the interface.

Yours does not explain what tick_rate is. A good way to do this is also explained in the PEP.

"""Form a complex number.

Keyword arguments:
real -- the real part (default 0.0)
imag -- the imaginary part (default 0.0)
"""

I was thrown off a bit to see the event handler below the main of the code. I think there is no difference, however it's probably best to keep to the norm.

Overall there's almost no problems with your style. Your docstrings could follow PEP257 more, as the biggest problem would be the summarized line at the beginning.

code

You should add the ability to pass a function to handle pygame events.
This is as the programmer currently would have to use a hack to get user input, or not use the decorator. Both bad options.

for a rudimentary example of what the handler could look like:

def even_handler(event):
    if event.type == pygame.QUIT:
        raise SystemExit

You could however use pygame.quit();sys.exit(0), but then if you exit multiple times you would need to make a function to exit, and it would remove a little added abstraction.

you would then need to add it to the decorator. The nicest way would probably be:

def game_loop(event_function, tick_rate=60):

    # replace the current event handler.
    for event in pygame.event.get():
        try:
            event_function(event)
        except SystemExit:
            pygame.quit()
            sys.exit(0)

This has the downside of having to pass an event-handler, unless you pass a default one, like the one above. However it allows the programmer to handle user input, a must in most pygame applications.


Simulation of 2 dice rolls I wrote a program that records how many times 2 fair dice need to be rolled to match the probabilities for each result that we should expect.

I think it works but I'm wondering if there's a more resource-friendly way to solve this problem.

import random

expected = [0.0, 0.0, 0.028, 0.056, 0.083,
         0.111, 0.139, 0.167, 0.139, 0.111,
         0.083, 0.056, 0.028]

results = [0.0] * 13  # store our empirical results here

emp_percent = [0.0] * 13  # results / by count

count = 0.0  # how many times have we rolled the dice?

while True:
    r = random.randrange(1,7) + random.randrange(1,7)  # roll our die
    count += 1
    results[r] += 1
    emp_percent = results[:]

    for i in range(len(emp_percent)):
        emp_percent[i] /= count
        emp_percent[i] = round(emp_percent[i], 3)

    if emp_percent == expected:
        break

print(count)
print(emp_percent)

I question the validity of this exercise. I wouldn't really expect the percentages to line up for a long, long, long time. Possibly never. Running a few times I got anywhere from 290k to nearly 8 million. That's a lot of die rolls, and a pretty wide range.

That said, we can certainly improve your code. First, there's random.randint() which is more directly what you want. Let's just put that in a function:

def die_roll():
    return random.randint(1, 6)

Now we don't need that weird 7.

Next, results should be ints. After all, we're iterating upward. We only need to convert them to floats when we do the calculation. We can also use itertools.count() for the loop:

results = [0] * 13
for count in itertools.count(start=1):
    results[die_roll() + die_roll()] += 1

Ok, now that we have our results, we need to compare against expected. Rather than doing a list copy, division, round on every element - let's go one at a time. After all, if the percentage for 2 doesn't line up, why even calculate the percentage for 3?

The percentage for any one roll is:

round(1. * r / count, 3)

for r in results. And the expectation is e in expected. We want to iterate over both lists simultaneously... "zipping" through them. For that, there's itertools.izip():

if all(round(1. * r/count, 3) == e
       for r, e in itertools.izip(results, expected)):
    break

Lastly, don't hardcode your percentages! These have well defined values. We have a pyramid (0, 1, 2, 3, 4, 5, 6, 5, 4, 3, 2, 1) that we just multiply by 1/36. So that's:

def prob(x):
    count = x-1 if x <= 7 else 13-x
    return round(count/36., 3)

expected = [0.0] + [prob(x) for x in range(1, 13)]

So the full solution is:

import random
import itertools

def prob(x):
    count = x-1 if x <= 7 else 13-x
    return round(count/36., 3)

expected = [0.0] + [prob(x) for x in range(1, 13)]

results = [0] * 13

def die_roll():
    return random.randint(1, 6)

for count in itertools.count(start=1):
    results[die_roll() + die_roll()] += 1

    if all(round(1. * r/count, 3) == e
           for r, e in itertools.izip(results, expected)):
        break

print count

Dropping Floats

Floating point arithmetic is always slow. We could do a bit better by keeping everything integral:

import random
import itertools

def prob(x):
    count = x-1 if x <= 7 else 13-x
    return 1000*count/36

expected = [0] + [prob(x) for x in range(1, 13)]
results = [0] * 13

def die_roll():
    return random.randint(1, 6)

for count in itertools.count(start=1):
    results[die_roll() + die_roll()] += 1

    if all(1000 * r/count == e
           for r, e in itertools.izip(results, expected)):
        break

print count

Generalize arbitrarily

Sometimes, the people want an over-engineered solution. So let's give them an over-engineered solution! We can use itertools.product() to help us determine the odds of particular die rolls. Everything else is straightforward:

def roll_until_match(sides=6, num_dice=2):
    c = collections.Counter(sum(p) for p in
                            itertools.product(xrange(1, sides+1), repeat=num_dice))

    expected = [1000 * c.get(i, 0) / (sides ** num_dice)
                for i in xrange(sides * num_dice + 1)]

    results = [0] * (sides * num_dice + 1)

    def die_rolls():
        return sum(random.randint(1, sides) for _ in xrange(num_dice))

    for count in itertools.count(start=1):
        results[die_rolls()] += 1

        if all(1000 * r/count == e
               for r, e in itertools.izip(results, expected)):
            break

    print count
    print results


Program that teaches kids Chinese characters I wrote code that chooses a random set of Chinese characters, asks the user what the characters are, and if they meet a certain criteria they level up (ex. get more than 50% correct). I want to know if there is a better way to have fewer lists, better organization, etc.

# -*- coding: utf-8 -*-
import random

allnumbers = {"一": "one", "二": "two", "三": "three", "四": "four", "五": "five",
              "六": "six", "七": "seven", "八": "eight", "九": "nine", "十": "ten"}

Numbers = {"四": "four", "五": "five", "六": "six", "七": "seven", "八": "eight",
           "九": "nine", "十": "ten"}

People = {"我": "I or me", "你": "you", "他": "He or him", "她": "Her or she"}

Family = {"爸": "Dad", "妈": "Mom", "哥": "Older brother", "弟": "Younger brother",
          "姐": "Older sister", "妹": "Younger sister"}
NotLearned = [Numbers, People, Family]
Learned = []
Practiced = []
LevelOne = []
LevelTwo = []
Mastered = []
setlist = {"Numbers": Numbers, "People": People, "Family": Family}
allLev = [NotLearned,Learned,LevelOne,LevelTwo,Mastered]
all = [Numbers, People, Family]

charlist = []
set = random.choice(all)
k = set.keys()
k = random.sample(k, len(k))
outof = 0
total = len(set)
while True:
    print "Welcome! Just type in the character requested! If you want to exit, type quit."
    for x in range(0, len(set)):
        char = k[x]
        question = raw_input("Write Chinese for \"{}\" (Hint: it's {}): ".format(set[char], char))
        if question == char:
            print "You got it right!"
            outof += 1
        else:
            print "Sorry, that's incorrect"
    score = float(outof)/float(total)
    print "You got {}%. Good Job!".format(100 * score)

    if set in NotLearned:
        print "Good job learning that set!"
        NotLearned.remove(set)
        Learned.append(set)
    if set in Learned:
        if score >= .5:
            print "Good job practicing!"
            Learned.remove(set)
            Practiced.append(set)
    if set in Practiced:
        if score >= .6:
            print "Good job getting to Level One!"
            Practiced.remove(set)
            LevelOne.append(set)
    if set in LevelOne:
        if score >= .75:
            print "Good job getting to Level Two!"
            LevelOne.remove(set)
            LevelTwo.append(set)
    if set in LevelTwo:
        if score == 1:
            print "Great job mastering {}".format(set)
            LevelTwo.remove(set)
            Mastered.append(set)

First of all, this is a great idea!

Naming

all and set are not good names, because they shadow Python built-in names. set is especially bad, because it actually contains a list, which is misleading.
Also, variable names in Python should use snake_case instead of CamelCase. For example "NotLearned" should be changed to not_learned.

The PEP8 command line tool can tell you these things, I suggest to give it a try!

Also, k is a very poor name for a random list of characters.

Compatibility

It's ok if you have a reason to stay on Python 2.x, but it's still a good idea to make your scripts Python 3.x compatible when it's easy to do.
For example, you could replace print something with print(something),
because the latter works in both versions, the former doesn't.

Bugs

The program has several bugs.

outof is not reset at the beginning of each cycle.
For example from the word set of People with 4 elements,
I "guessed" 3 correctly, got 75% score,
then in the next round I guessed 2 correctly so my score became 125% instead of 50%.
The fix is simple, move outof = 0 inside the loop.

Another bug is in the logic to level up.
In my previous example of getting 75% score,
this happened:

if set in NotLearned:
    # => true => moving set from NotLearned to Learned
if set in Learned:
    # => true, because the set was just moved to Learned
    if score >= .5:
        # => true, I scored 75%
        # => moving set from Learned to Practiced
if set in Practiced:
    # => true, because the set was just moved to Practiced
    if score >= .6:
        # => true, I scored 75%
        # => moving set from Practiced to LevelOne
if set in LevelOne:
    # => true, because the set was just moved to LevelOne
    if score >= .75:
        # => true, I scored 75%
        # => moving set from Practiced to LevelOne

There's a problem with logic here.
Possibly you meant to use elif instead of some (or all?) of those ifs.

Reducing the number of lists

  I want to know if there is a better way to have fewer lists

The way you use the lists, is a bit strange...
From NotLearned, you randomly pick one of the lists (numbers, people or family), and that list will be used for the rest of the game:
words are sampled from this list,
and the list itself is moved through other lists: NotLearned -> Learned -> Practiced -> LevelOne -> ... and so on.

Instead of moving through NotLearned -> Learned -> ..., you could use a simple number to indicate the level, and increment as the user progresses.

Usability

Typing Chinese is problematic if you don't have a Chinese keyboard. I played the game by copy-pasting answers. It could be more interesting and more accessible to present multiple choices that can be selected by entering their numbers.

In each cycle of the main while loop, the words come in the same order. It would be more interesting if the order was randomized.
See the random.shuffle function.

Unused variables

Eliminate all unused variables, they are just noise.


LeetCode: House Robber III C# https://leetcode.com/problems/house-robber-iii/

Please review for performance

  The thief has found himself a new place for his thievery again. There
  is only one entrance to this area, called the "root." Besides the
  root, each house has one and only one parent house. After a tour, the
  smart thief realized that "all houses in this place forms a binary
  tree". It will automatically contact the police if two directly-linked
  houses were broken into on the same night.

  Determine the maximum amount of money the thief can rob tonight
  without alerting the police.

  Gilad's comment: In simple words - you can't take the sum of parent and child. they have to be no directly connected nodes.  for example Max(level 1 +3, level 2)

  Example 1:

Input: [3,2,3,null,3,null,1]

     3
    / \
   2   3
    \   \
     3   1

  Output: 7

  Explanation: Maximum amount of money the thief can rob = 3
  + 3 + 1 = 7.

  Example 2:

Input: [3,4,5,1,3,null,1]

     3
    / \
   4   5
  / \   \
 1   3   1

  Output: 9

  Explanation: Maximum amount of money the thief can rob = 4 +
  5 = 9.

using System;
using System.Collections.Generic;
using GraphsQuestions;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace TreeQuestions
{
    /// <summary>
    /// https://leetcode.com/problems/house-robber-iii/
    /// </summary>
/**
 * Definition for a binary tree node.
 * public class TreeNode {
 *     public int val;
 *     public TreeNode left;
 *     public TreeNode right;
 *     public TreeNode(int x) { val = x; }
 * }
 */

    [TestClass]
    public class HouseRobber3
    {
        [TestMethod]
        public void TestMethod1()
        {

            //     3              // level 1
            //    / \
            //    2   3           // level 2
            //    \   \
            //     3   1          // level 3
            TreeNode root = new TreeNode(3);
            root.left = new TreeNode(2);
            root.left.right = new TreeNode(3);
            root.right = new TreeNode(3);
            root.right.right = new TreeNode(1);
            //Output: 7
            //Explanation: Maximum amount of money the thief can rob = 3 + 3 + 1 = 7.
            HouseRobber3Class robber = new HouseRobber3Class();
            Assert.AreEqual(7, robber.Rob(root));
        }
    }

    public class HouseRobber3Class
    {
        //we can't rob directly connected nodes.
        // so we can rob root which is level 1 + level 3
        // or only level 2 which is root.left+root.right
        // we need to find the max of the two
        public int Rob(TreeNode root)
        {
            return Helper(root, new Dictionary<TreeNode, int>());
        }

        public int Helper(TreeNode root, Dictionary<TreeNode, int> hash)
        {
            if (root == null)
            {
                return 0;
            }

            if (hash.ContainsKey(root))
            {
                return hash[root];
            }

            int sum = 0;
            if (root.left != null)
            {
                sum += Helper(root.left.left, hash) + Helper(root.left.right, hash);
            }

            if (root.right != null)
            {
                sum += Helper(root.right.left,hash) + Helper(root.right.right, hash);
            }

            sum =  Math.Max(root.val + sum, Helper(root.left, hash) + Helper(root.right, hash));
            hash.Add(root, sum);
            return sum;

        }
    }
}

Your testcases are far too simple to be certain your algorithm works. This might be an issue on the challenge's side. Both these testcases have symmetrical solutions. For these testcases, it's best to always rob every second layer. No test cases cover instances where the left side of the tree might follow a different pattern to the right side. Take for example the testcase:

{ 3, 8, 5, 1, 3, 10, 1 }

On the left branch, you'd expect the 8 to be robbed on the second layer, while the right side is more beneficial to rob the third layer, foregoing the first layer altogether.

That said, your algorithm appears to handle this correctly. That, along with the caching you do, appears to be working quite well.


Python script to test music sight reading I decided to write a program to test music theory, and, while it would've been much easier for me to make it elegant and perfect in Java, I thought I'd take the opportunity to get more familiar with Python.

Below is the full script.  It simply prints a staff in treble of bass clef with a note somewhere and asks the user to identify it.

Things I'm particularly interested in getting feedback about:

The @staticmethod in the Staff class.  I tried this both as an instance function (is it even appropriate to call it this in Python?) and as the static version below, but both seemed kind of clunky to me.  It doesn't make much sense to me that I have to refer to the class's fields with the class name when I'm already in the class (e.g., Staff.lines instead of just lines, or self.lines if it's an instance function).  It makes me think I'm doing something wrong.
I've heard multiple times that having constants isn't very Pythonic.  Is there a better way for me to handle the TREBLE_CLEF, BASS_CLEF, TREBLE_CLEF_NOTES, and BASS_CLEF_NOTES fields?
I'm interested in a solution which allows the user to input a command like exit into the prompt in order to leave the program.  I know I could've put this in the test_note function, but that would break the principle that each function should do only one thing and do it well (i.e., the test_note function test's the user's knowledge of the staff -- it shouldn't be responsible at all for controlling the application or its state).  I couldn't think of an elegant way to do this and still preserve that principle, so I just bypassed the issue for now, haha.
Am I using classes appropriately?  Any way I can use them better?

Of course, any and all feedback is welcome, especially including anything which breaks from conventional Python style.  I'd like to be as Pythonic as possible.

import random
import time

class Clef:

    def __init__(self, name, notes):
        self.name = name
        self.notes = notes

    def random_note(self):
        return random.choice(list(self.notes.keys()))

    def get_note(self, index):
        return self.notes[index]

class Staff:

    lines = 9
    line_length = 9
    line_text = '-' * line_length
    space_text = ' ' * line_length
    note_text = 'O'

    @staticmethod
    def display(note_index):
        for i in range(Staff.lines):
            add_note = i == note_index
            print_line = Staff.line_text if i % 2 == 0 else Staff.space_text
            if i == note_index:
                print_line = print_line[:4] + Staff.note_text + print_line[5:]
            print(print_line)

TREBLE_CLEF_NOTES = { 0 : 'F',
                      1 : 'E',
                      2 : 'D',
                      3 : 'C',
                      4 : 'B',
                      5 : 'A',
                      6 : 'G',
                      7 : 'F',
                      8 : 'E' }

BASS_CLEF_NOTES = { 0 : 'A',
                    1 : 'G',
                    2 : 'F',
                    3 : 'E',
                    4 : 'D',
                    5 : 'C',
                    6 : 'B',
                    7 : 'A',
                    8 : 'G' }

TREBLE_CLEF = Clef('Treble', TREBLE_CLEF_NOTES)
BASS_CLEF = Clef('Bass', BASS_CLEF_NOTES)

def test_note(clef, note_index):
    print(clef.name)
    Staff.display(note_index)
    answer = clef.get_note(note_index)
    user_choice = input('Enter the note: ').strip().upper()
    if user_choice == answer:
        print('Correct!')
    else:
        print('Wrong.  This is ' + answer)

def main():
    print('To exit, type CTRL + C')
    while True:
        print('\n')
        clef = random.choice([TREBLE_CLEF, BASS_CLEF])
        test_note(clef, clef.random_note())
        time.sleep(2)

if __name__ == '__main__':
    main()

Warning for other reviewers : input and input are different depending on the version of Python you are using. This is about Python 3.

Instead of defining the notes of a clef as a dictionnary mapping consecutive integers from 0 to n to names, you could use a list.

You'll have the much more natural :

TREBLE_CLEF_NOTES = [ 'F', 'E', 'D', 'C', 'B', 'A', 'G', 'F', 'E' ]
BASS_CLEF_NOTES = [ 'A', 'G', 'F', 'E', 'D', 'C', 'B', 'A', 'G' ]

and you just need to change :

def random_note(self):
    return random.choice(range(len(self.notes)))

add_note = i == note_index does not seem useful : get rid of it.

The way you are playing with string slicing to display a note or a line doesn't need to be that complicated. Also, defining values as class members was a nice touch but the fact that you hardcode indices in the logic with the string slicing kind of removes the all point. As for me, I'll keep things simple and not so well organised for the time being as these values are unlikely to be useful for anything else than displaying.

Here's my suggestion :

def display(note_index):
    for i in range(9):
        line_element = '-' if i%2 else ' '
        print(line_element * 4 + ('O' if i == note_index else line_element) + line_element*5)

I have to go, I'll try to add more details later. In the meantime, if you are interested in Python and music theory, you might be interested by this github project.


Check whether an integer's leftmost bits are ones in Rust I'm trying to validate if an integer represents a valid IPv6 mask, which boils down to checking if all the left-most bits of an u128 are 1s, and all the right-most bits are 0s. For instance:

0 is valid
0xffff_ffff_ffff_ffff_ffff_ffff_ffff_ffff is valid
0xffff_ffff_ffff_ffff_0000_0000_0000_0000 is valid
0xffff_ffff_fff8_0000_0000_0000_0000_0000 is valid
0xffff_ffff_fffe_0000_0000_0000_0000_0000 is valid

but:

1 is invalid (its representation is 0x0000_0000_0000_0000_0000_0000_0000_0001)
0x0fff_ffff_ffff_ffff_ffff_ffff_ffff_ffff is invalid
0xffff_ffff_fff1_0000_0000_0000_0000_0000 is invalid

Here is the function:

/// Check whether the given integer represents a valid IPv6 mask.
/// A valid IP mask is an integer which left-most bits are 1s, and right-most bits are 0s.
fn is_valid_ipv6_mask(value: u128) -> bool {
    // flag to check if we're currently processing 1s or 0s
    let mut count_ones = true;

    // check each byte starting from the left.
    for byte_index in (0..=15).rev() {
        let x = (value >> (byte_index * 8)) & 0xff;
        match x {
            // We're processing 1s and this byte is 0b1111_1111
            0xff if count_ones => continue,
            // We're processing 0s and this byte is 0b0000_0000
            0x00 if !count_ones => continue,
            // We're processing 1s and this byte is 0b0000_0000.
            // That means all the remaining bytes should be 0 for this integer
            // to be a valid mask
            0x00 if count_ones => {
                count_ones = false;
                continue;
            }
            // We're processsing 1s and this byte has at least a 1, so we have
            // to check bit by bit that the left-most bits are 1s and the
            // right-most bits are 0s
            byte if byte > 0 && count_ones => {
                let mut bit_index = 7;
                while (byte >> bit_index) & 1 == 1 {
                    // This does not overflow, because we now this byte has at
                    // least a 0 somewhere
                    bit_index -= 1
                }
                // At this point, all the bits should be 0s
                count_ones = false;
                for i in 0..bit_index {
                    if (byte >> i) & 1 == 1 {
                        return false;
                    }
                }
            }
            // Any other case is an error
            _ => return false,
        }
    }
    true
}

And to test it:

fn main() {
    assert!(is_valid_ipv6_mask(0));
    assert!(is_valid_ipv6_mask(
        0xffff_ffff_ffff_ffff_ffff_ffff_ffff_ffff
    ));
    assert!(is_valid_ipv6_mask(
        0xffff_0000_0000_0000_0000_0000_0000_0000
    ));
    assert!(is_valid_ipv6_mask(
        0xff80_0000_0000_0000_0000_0000_0000_0000
    ));
    assert!(!is_valid_ipv6_mask(
        0xff01_0000_0000_0000_0000_0000_0000_0000
    ));
    assert!(!is_valid_ipv6_mask(
        0xffff_0000_ffff_ffff_ffff_ffff_ffff_ffff
    ));
}

Link to playground.

The problem is that have the feeling that there must be a much simple solution to this problem. After all, bit operations are what computers do, I can't believe there's not more concise and more efficient way to check if an integer is all 1s then all 0s.

The problem is that have the feeling that there must be a much simple solution to this problem.

You're right! Integers in Rust have many useful bitwise operations (as well as other useful ones like wrapping and saturating arithmetic), most of which are exposed as methods on the type. To see the full list, look at the generated documentation for the type, e.g. u128.

Here's a simple way to write is_valid_ipv6_mask:

fn is_valid_ipv6_mask(value: u128) -> bool {
    value.count_zeros() == value.trailing_zeros()
}


Build object with different input, using super-class and sub-class style, Python 3 I want to model a object of air, the temperature of which can be given in 3 different ways:

if the value is known (assuming the temperature is a constant)
if a curve of the values with different time is known (using interpolation)
if the formula of the temperature vs time is known.

My code is below:

from script.interpolate import interp1d
from importlib import import_module

class Air1:
    # case 1: temperature, which is a constant value, is given in the input.
    def __init__(self, meta: dict):
        self.city = meta['city']
        self.temperature = meta['temperature']

class Air2(Air1):
    # case 2: a curve of temperature vs time is given in meta.
    # times is a list, temperatures is a list.
    # so self.get_temperature is an interpolated function.
    # To get a new temperature, I can use new_temp = self.get_temperature(new_time)
    def __init__(self, meta: dict):
        super().__init__(meta=meta)
        times = meta['times']
        temperatures = meta['temperatures']
        self.get_temperature = interp1d(times, temperatures)

class Air3(Air1):
    # case 3: the formula to calculate temperature from time is known.
    # this formula is implemented in a python file called file_of_fomurla.
    # so self.get_temperature is an imported function.
    # To get a new temperature, I can use new_temp = self.get_temperature(new_time)
    def __init__(self, meta: dict):
        super().__init__(meta=meta)
        file_of_formula = meta['file_of_formula']
        self.get_temperature = import_module(file_of_formula + '.my_function')

One thing I have noticed in my code is that .temperature should only be in Air1, since Air2 and Air3 both just have a function to calculate temperature value based on time. However, like in my code, Air2 and Air3 are sub classes of Air1, but their input meta will not have the key 'temperature'. So this is an error.

Do you have any better way to implement the physics behind into the model. Maybe using some abstract class?

As you yourself have noted, the inheritance isn't benefiting you at all. Just write one class.

Passing meta as a dict is unidiomatic. Use **kwargs or parameters with defaults instead.

Consider dropping the get_… prefix.

Also consider passing the formula as code (e.g. a lambda) rather than referencing a module by name.

from script.interpolate import interp1d
from importlib import import_module

class Air:
    def __init__(self, city, **kwargs):
        self.city = city
        self.temperature = (
            lambda time: kwargs['temperature'] if 'temperature' in kwargs else
            interp1d(kwargs['times'], kwargs['temperatures']) if 'times' in kwargs else
            import_module(kwargs['file_of_formula'] + '.my_function') if 'file_of_formula' in kwargs else
            kwargs['formula']
        )


I'm reading all lines of another sheet and I paste the value that I want to the active sheet VBA I have a program that read thousands of lines and return the value that I need. The problem is that request take a lot of time probably like 1min just to search and paste the value, even when I'm saving the code it takes a lot of time...

I think it is around this line that the code is slow :

If Sheets("MT950").Cells(line, 1) Like "-}{5:*" Then

Here is my code :

Function mysolde62(mycurrency As String, swift As String) As Double

Dim SearchString As String
Dim LastLine As Long, line As Long, k As Long
Dim mybegin As Long, myend As Long, test As Long, count As Long
Dim sign As String
Dim myvalue As Double

LastLine = Sheets("MT950").Range("A1").End(xlDown).Row

count = 0
myend = 1

For line = 1 To LastLine

If Sheets("MT950").Cells(line, 1) Like "-}{5:*" Then

SearchString = Sheets("MT950").Range("A" & line).Value

mypos = InStr(1, SearchString, swift, 1)
If mypos <> 0 Then
    count = count + 1
End If

End If
Next line

For k = 1 To count

For line = myend To LastLine
If Sheets("MT950").Cells(line, 1) Like "-}{5:*" Then
    SearchString = Sheets("MT950").Range("A" & line).Value

    mypos = InStr(1, SearchString, swift, 1)
    If mypos <> 0 Then

        mybegin = line
        For linebis = mybegin To LastLine
            If Sheets("MT950").Cells(linebis, 1) Like ":62F:*" Then
                SearchString = Sheets("MT950").Range("A" & linebis).Value

                mypos = InStr(1, SearchString, mycurrency, 1)
                If mypos <> 0 Then
                    myend = linebis
                    test = 1
                End If
                Exit For
            End If
        Next linebis

        If test = 1 Then Exit For
    End If
End If
Next line
If test = 1 Then Exit For
Next k

sign = Mid(Sheets("MT950").Cells(myend, 1).Value, 5, 1)
myvalue = Mid(Sheets("MT950").Cells(myend, 1).Value, 15)
If sign = "D" Then
mysolde62 = -myvalue
Else
mysolde62 = myvalue
End If

End Function

Off the bat it appears that you are looping through the entire sheet "MT950" twice.  Doing this once will improve the speed.  Also, the line you said it might be slow around appears twice in your code.

In your first loop you are running two comparisons, one using the If, Like, Then and then another right after using InStr(1, SearchString, swift, 1).  These can be combined into the following

For Line = 1 To LastLine
    SearchString = Sheets("MT950").Range("A" & Line).Value
    If InStr(1, SearchString, swift, 1) <> 0 and SearchString Like "-}{5:*" Then Count = Count + 1
Next Line

The second loop appears to be the same as the first only with more stuff happening once you get down inside of it.  I'd recommend merging these two loops, there doesn't seem to be a need to loop through all the rows twice.


A Java class for calculating size of iterable/array Another exercise of Java generics:

import java.util.Collection;

/**
 * Created by IDEA on 16/11/14.
 */
public class Size {
    public static <T, I extends Iterable<T>> int size(I data) {
        final int size;
        if (data instanceof Collection) {
            size = ((Collection<T>) data).size();
        } else {
            int counter = 0;
            for(Object i : data) {
                counter++;
            }
            size = counter;
        }
        return size;
    }

    public static int size(int[] data) {
        return data.length;
    }

    public static int size(double[] data) {
        return data.length;
    }

    public static int size(String[] data) {
        return data.length;
    }
}

import java.util.ArrayList;
import java.util.List;

/**
 * Created by IDEA on 16/11/14.
 */
public class TestSize {
    public static void main(String[] args) {
        int[] x = {1, 2, 3};
        List<Integer> y = new ArrayList<>();
        y.add(1);
        y.add(2);
        System.out.println(Size.size(x));
        System.out.println(Size.size(y));
    }
}

For arrays, you are missing a generic int size(Object[] data) method, which can be written as:

public static <E> int size(E[] data) {
     return data.length;
}

This will apply to String[] as well. You will still need one for each kind of primitive though (boolean, short, int, long, float, double, did I miss anything?)

As for your other method...

public static <T, I extends Iterable<T>> int size(I data) {

This one can be cleaned up significantly.

You don't need the <T> here. In fact, you don't need the <I> either. It doesn't matter what kind of iterable it is so you can use Iterable<?>.

As you're not defining a variable of type I anywhere in your method, nor is using that type in another obscure way, all you're really interested in is that it is an Iterable<T>. So:

public static <T> int size(Iterable<T> data) {

Will have the same effect.

Now, are you using the type <T> ? No. In fact, you are iterating over Object so all you're interested in is that it is Iterable<? extends Object>. Which is the same as Iterable<?> since all classes extends Object.

So now we are left with this:

public static int size(Iterable<?> data) {
    final int size;
    if (data instanceof Collection) {
        size = ((Collection<?>) data).size();
    } else {
        int counter = 0;
        for (Object i : data) {
            counter++;
        }
        size = counter;
    }
    return size;
}

And when introducing early return, this:

public static int size(Iterable<?> data) {
    if (data instanceof Collection) {
        return ((Collection<?>) data).size();
    }
    int counter = 0;
    for (Object i : data) {
        counter++;
    }
    return counter;
}

Now that's a method.

Side note: The only method you have that is actually useful is int size(Iterable<?> data). Otherwise it would be easier to just use data.length or data.size();. However, I understand that this is just an exercise in which case it's fine.


Making sure user inputs correct type I have two functions that are used to prompt and recieve input from the user. One is for int and the other is for boolean. They work somewhat similar to each other, one uses try/catch and the other doesn't. Can they be improved upon or is one better than the other? I'm having trouble with the user interface, I'm not sure what should happen when the user gives an invalid input. I asked on userexperience and didn't get much feed back.

/*returns integer to question asked in prompt*/
public static int getNumber(String prompt)
{
    Scanner sc = new Scanner(System.in);
    while (true) {
        System.out.print("\t"+prompt+": ");
        String input = sc.next();
        try {
            return Integer.parseInt(input);
        } catch (NumberFormatException ne) {
            System.out.println("\tPlease enter an integer.");
        }
    }
}

/*returns boolean to question asked in message*/
public static boolean getBool(String message)
{
    Scanner sc = new Scanner(System.in);
    String answer;

    System.out.print("\t"+message+": ");
    while (true) {
      answer = sc.nextLine().trim().toLowerCase();
      if (answer.equals("y")) {
        return true;
      } else if (answer.equals("n")) {
        return false;
      } else {
         System.out.print("\tPlease answer y/n");
      }
    }

}

One problem is "please enter an integer". If the user is non-technical they may not know what an integer is, but I can't think of better wording.

Here is another version of the methods

/*returns integer to question asked in prompt*/
public static int getNumber(String prompt)
{
    Scanner sc = new Scanner(System.in);
    while (true) {
        System.out.print("\t"+prompt+": ");
        String input = sc.next();
        try {
            return Integer.parseInt(input);
        } catch (NumberFormatException ne) {
            System.out.println("\tPlease enter an integer.");
        }
    }
}

/*returns boolean to question asked in message*/
public static boolean getBool(String message)
{
    Scanner sc = new Scanner(System.in);
    String answer;

    do{
      System.out.print("\t"+message+" (y/n): ");
      answer = sc.nextLine().trim().toLowerCase();
      if (answer.equals("y")) {
        return true;
      } else if (answer.equals("n")) {
        return false;
      }
    }while(true);

}

An example of these functions being used is when they are called with

public static void main(String[] args)
{

    System.out.println("Please reply with y or n");
    boolean celsius = getBool("Temperature in celsius");
    boolean wind = getBool("Regularly windy");
    System.out.println("Please enter an integer");
    int minTemp = getNumber("Minimum temperature during day");
    int maxTemp = getNumber("Maximum temperature during the day");
    int minTempNight = getNumber("Minimum temperature during the night");
    int maxTempNight = getNumber("Maximum temperature during the night");
    //setup();
}

Example input/output

run:
Please reply with y or n
    Temperature in celsius (y/n): m
    Temperature in celsius (y/n): 5
    Temperature in celsius (y/n): y
    Regularly windy (y/n): asdf
    Regularly windy (y/n): y
Please enter an integer
    Minimum temperature during day: 5
    Maximum temperature during the day: 20
    Minimum temperature during the night: blah
    Please enter an integer.
    Minimum temperature during the night: 1
    Maximum temperature during the night: words
    Please enter an integer.
    Maximum temperature during the night: 10

If the user is non-technical they may not know what an integer is, but
  I can't think of better wording.

[Positive] Number or [Positive] Whole Number would be alternatives.

Another possibility: Enter a Number between 0 and [X]. Non-technical people will assume that this means they should enter a whole number. If they enter a different number, give feedback accordingly.

Consistency

In your first version: getNumber outputs the prompt each time, while getBool does not. This should be consistent. The names of the input variable should be consistent as well (either prompt or message. I would prefer prompt.

Other

You can do sc.nextInt(); instead of String input = sc.next(); Integer.parseInt(input);.

As already mentioned on UX, show the possible values for the boolean answer in the prompt when asking the question. So

boolean wind = getBool("Regularly windy");

becomes

boolean wind = getBool("Regularly windy (y/n)");

Extract Method

You can extract the method to retrieve a string. I think that this makes the code a bit more readable, and you get getString method for free.

/*returns integer to question asked in prompt*/
public static int getNumber(String prompt) {
    while (true) {
        String input = getString(prompt);
        try {
            return Integer.parseInt(input);
        } catch (NumberFormatException ne) {
            System.out.println("\tPlease enter a positive whole number.");
        }
    }
}

/*returns boolean to question asked in prompt*/
public static boolean getBool(String prompt) {
    while (true) {
        String input = getString(prompt).trim().toLowerCase();
        if (input.equals("y")) {
            return true;
        } else if (input.equals("n")) {
            return false;
        } else {
            System.out.print("\tPlease answer y/n");
        }
    }
}

/*returns string to question asked in prompt*/
public static String getString(String prompt) {
    System.out.print("\t" + prompt + ": ");
    Scanner sc = new Scanner(System.in);
    return sc.nextLine();
}


Frequency Queries hackerrank This is the problem:

  You are given  queries. Each query is of the form two integers
  described below:
  - 1: Insert x in your data structure.
  - 2: Delete one occurrence of y from your data structure, if present.
  - 3: Check if any integer is present whose frequency is exactly. If yes, print 1 else 0.

  The queries are given in the form of a 2-D array query of size q where
  queries[i][0] contains the operation, and queriesi contains the
  data element contains the operation and contains the data element. For
  example, you are given array.
  [[1,1],[2,2],[3,2],[1,1],[1,1],[2,1],[3,2]] The output=[0,1]

The question can be found here;
https://www.hackerrank.com/challenges/frequency-queries/problem?h_l=interview&playlist_slugs%5B%5D=interview-preparation-kit&playlist_slugs%5B%5D=dictionaries-hashmaps

My code does not work some of the cases on hackerrank for some reason but I can't figure out why. It is not caused by timeout or compiler error. It gives a 'wrong answer' in the compiler but when I try to test my code with the same input on VSCode, it actually works. It's the first time I am encountering something like this. If I would be more specific, I tested my code manually against this input

  [1, 3],   [1, 38],   [2, 1],   [1, 16],   [2, 1],   [2, 2],   [1, 64],
  [1, 84],   [3, 1],   [1, 100],   [1, 10],   [2, 2],   [2, 1],   [1,
  67],   [2, 2],   [3, 1],   [1, 99],   [1, 32],   [1, 58],   [3, 2]

and it returns [1,1,0] on vs code when I try it. However, on hackerrank it says the wrong answer even though it expects the same result. Any idea what's going on?

Here is my code in javascript;

function freqQuery(queries) {
  let store = [];
  let output = [];
  let obj = {};
  let checkFreq = false;

  for (let i = 0; i < queries.length; i++) {
    let query = queries[i];

    if (query[0] === 1) {
      store.push(query[1]);
    } else if (query[0] === 2) {
      let index = store.indexOf(query[1]);
      if (index > -1) {
        store.splice(index, 1);
      }
    } else {
      let freq = query[1];
      if (store.length === 0) {
        output.push(0);
      } else {
        obj = charToObj(store);
        for (let number in obj) {
          if (obj[number] === freq) {
            checkFreq = true;
            break;
          } else {
            checkFreq = false;
          }
        }
        if (checkFreq) {
          output.push(1);
        } else {
          output.push(0);
        }
      }
    }
  }

  return output;
}

function charToObj(arr) {
  let obj = {};
  for (let i = 0; i < arr.length; i++) {
    if (obj[arr[i]]) {
      obj[arr[i]] += 1;
    } else {
      obj[arr[i]] = 1;
    }
  }
  return obj;
}

Optimizations

You should think of your algorithm as a black box. As long as input and output match, internally the function has not to reproduce the textual representation of the problem.

With this in mind you can simplify your code. You also have an approach already in the charToObj function. I would use this to solve the whole problem:

create a map
store values in that map and increase their amount, when they reappear
decrease amount, if necessary
test the map for given frequencies

This has a few advantages:

shorter code, easier to follow
no helper function necessary
no need to call splice
no inner loops anymore
no need to build obj over and over again

The new functions could look like this:

function freqQuery(queries) {
    const frequencies = [];
    const result = [];

    for (const query of queries) {
        const action = query[0];
        const value = query[1];

        if (action === 1) {
            if (typeof frequencies[value] === 'undefined') {
                frequencies[value] = 1;
            } else {
                ++frequencies[value];
            }
        }

        if (action === 2) {
            if (frequencies[value]) {
                --frequencies[value];
            }
        }

        if (action === 3) {
            result.push(frequencies.indexOf(value) === -1 ? 0 : 1);
        }
    }

    return result;
}

console.log(
    freqQuery([[1,1],[2,2],[3,2],[1,1],[1,1],[2,1],[3,2]]),
    freqQuery([[3, 4], [2, 1003], [1, 16], [3, 1]]),
    freqQuery([[1, 3], [2, 3], [3, 2], [1, 4], [1, 5], [1, 5], [1, 4], [3, 2], [2, 4], [3, 2]]),
    freqQuery([[1, 5], [1, 6], [3, 2], [1, 10], [1, 10], [1, 6], [2, 5], [3, 2]]),
    freqQuery([[1, 3], [1, 38], [2, 1], [1, 16], [2, 1], [2, 2], [1, 64], [1, 84], [3, 1], [1, 100], [1, 10], [2, 2], [2, 1], [1, 67], [2, 2], [3, 1], [1, 99], [1, 32], [1, 58], [3, 2]]),
);

Instead of the three if-statements you could also use switch:

switch(action) {
    case 1: …
    case 2: …
    case 3: …
}

Small improvements

The bottleneck in this functions is indexOf. An alternative is using .includes():

if (action === 3) {
   result.push(frequencies.includes(value) ? 1 : 0);
}

This is faster for all test cases I ran in Chrome and Node.js. It is sometimes faster but can be slower depending highly on the input in Safari and Firefox.

Here's a test on jsfiddle.

Taking it one step further

So, the bottleneck is still the expensive test in case 3:

frequencies.includes(value)

Let's get rid of this by keeping track of the current frequencies ourselves. The main improvement is, that we finally will only have to check whether a given index evaluates to true:

if (action === 3) {
    result.push(frequencyTracker[value] > 0 ? 1 : 0);
}

We introduce a second array frequencyTracker, which is filled with the amount each frequency occurs. I've used the ternary operator simply to shorten the code.

function freqQuery(queries) {
    const frequencies = [];
    const frequencyTracker = [];
    const result = [];

    for (const query of queries) {
        const action = query[0];
        const value = query[1];
        let index;

        if (action === 1 || action === 2) {
            index = frequencies[value];
            frequencyTracker[index] ? --frequencyTracker[index] : null;
        }

        if (action === 1) {
            typeof frequencies[value] === 'undefined' ? frequencies[value] = 1 : ++frequencies[value];
        }

        if (action === 2 && frequencies[value]) {
            --frequencies[value];
        }

        if (action === 1 || action === 2) {
            index = frequencies[value];
            frequencyTracker[index] ? ++frequencyTracker[index] : frequencyTracker[index] = 1;
        }

        if (action === 3) {
            result.push(frequencyTracker[value] > 0 ? 1 : 0);
        }
    }

    return result;
}

console.log(
    freqQuery([[1,1],[2,2],[3,2],[1,1],[1,1],[2,1],[3,2]]),
    freqQuery([[3, 4], [2, 1003], [1, 16], [3, 1]]),
    freqQuery([[1, 3], [2, 3], [3, 2], [1, 4], [1, 5], [1, 5], [1, 4], [3, 2], [2, 4], [3, 2]]),
    freqQuery([[1, 5], [1, 6], [3, 2], [1, 10], [1, 10], [1, 6], [2, 5], [3, 2]]),
    freqQuery([[1, 3], [1, 38], [2, 1], [1, 16], [2, 1], [2, 2], [1, 64], [1, 84], [3, 1], [1, 100], [1, 10], [2, 2], [2, 1], [1, 67], [2, 2], [3, 1], [1, 99], [1, 32], [1, 58], [3, 2]]),
);

This doesn't look as elegant as the naive solution, but it's way faster. It passes all test cases on HackerRank as well.

Explanation

As requested in the comments, here's a an example to explain how this works in detail:

Before we begin, we create an empty array, which we will become a sparse array later.

Now we run through all queries:

If the action is 1 we set or increase our counter at the given index.

[1, 2] → [undefined, undefined, 1] + [undefined, 1]
[1, 4] → [undefined, undefined, 1, undefined, 1] + [undefined, 2]
[1, 4] → [undefined, undefined, 1, undefined, 2] + [undefined, 1, 1]

If the action is 2 we decrease our counter at the given index.

[2, 2] → [undefined, undefined, 0, undefined, 2] + [1, 0, 1]

If the action is 3 we check whether the value in our frequencyTracker is "true" or "not 0" or "nor undefined".

[3, 1] → [1, 0, 1] → 0
[3, 2] → [1, 0, 1] → 1

Naming

Some variable names are good, others are ambiguous, like:

charToObj()
obj

This should be improved.


Selecting all posts created by user that actual user follows I have a view that displays posts created by users that actual logged in user follows. It works, by I think the way I've done it isn't good.

BTW, UserProfile is my default user model.

UserFollower model

class UserFollower(models.Model):
    followed = models.OneToOneField(UserProfile, related_name="followed")
    followers = models.ManyToManyField(UserProfile, related_name="followers", blank=True)

Post model

class Post(models.Model):
    author = models.ForeignKey(settings.AUTH_USER_MODEL)
    body = models.CharField(max_length=140)

My page view

class MyPage(ListView):
    model = Post

    def get_queryset(self):
        posts = []
        for user in self.request.user.followers.all():
            for post in Post.objects.filter(author=user.followed):
                posts.append(post)

        return posts

Let's focus on the selecting.

class MyPage(ListView):
    model = Post

    def get_queryset(self):
        posts = []
        for user in self.request.user.followers.all():
            for post in Post.objects.filter(author=user.followed):
                posts.append(post)

        return posts

This is a bit convoluted: you're performing a lot of queries (depending on how many followers a user has). Ideally you'd like to perform just one query.

Also, get_queryset looks like it will return a queryset, instead of a list.

class MyPage(ListView):
    model = Post

    def get_queryset(self):
        return Post.objects.filter(author__followers__follower__id=self.request.user.id)

Should work just as well, and returns an actual queryset. (I'm not sure if I got the exact syntax right, please look at https://docs.djangoproject.com/es/1.9/topics/db/queries/#spanning-multi-valued-relationships if it does not work).


MySQL queries to select messages related to a user, using UNION vs. OR I'm just wondering if there is any improvements that can be made on my queries, as i've looked around and posts are saying to use UNION ALL instead of OR but with the performance times it looking like the OR condition works out better.

Could anyone please give me some advise on improving my queries?

Using UNION ALL:

Time taken: Showing rows 0 - 1 (2 total, Query took 0.0016 seconds.)

(SELECT direct_message.*
       FROM `direct_message`
       INNER JOIN direct_message_thread ON direct_message_thread.chat_id = direct_message.id
       WHERE direct_message.recipient_id = '10896'
         AND direct_message.school_id = '1'
         AND direct_message_thread.school_id = direct_message.school_id
       GROUP BY direct_message.id
       ORDER BY direct_message_thread.inserted DESC)
    UNION ALL
      (SELECT direct_message.*
       FROM `direct_message`
       INNER JOIN direct_message_thread ON direct_message_thread.chat_id = direct_message.id
       WHERE direct_message.creator_id = '10896'
         AND direct_message.school_id = '1'
         AND direct_message_thread.school_id = direct_message.school_id
       GROUP BY direct_message.id
       ORDER BY direct_message_thread.inserted DESC)
    UNION ALL
      (SELECT direct_message.*
       FROM `direct_message`
       LEFT JOIN direct_message_group ON direct_message_group.chat_id = direct_message.id
       INNER JOIN direct_message_thread ON direct_message_thread.chat_id = direct_message.id
       WHERE direct_message_group.staff_id = '10896'
         AND direct_message.school_id = '1'
         AND direct_message_thread.school_id = direct_message.school_id
       GROUP BY direct_message.id
       ORDER BY direct_message_thread.inserted DESC)

EXPLAIN OF QUERY

Using OR:

Time taken: Showing rows 0 - 1 (2 total, Query took 0.0011 seconds.)

SELECT direct_message.*,
       IFNULL(direct_message_group.staff_id, 0) AS staff_id
FROM `direct_message`
LEFT JOIN direct_message_group ON direct_message_group.chat_id = direct_message.id
INNER JOIN direct_message_thread ON direct_message_thread.chat_id = direct_message.id
WHERE ((direct_message.recipient_id = '10896'
        OR direct_message.creator_id = '10896'
        OR (direct_message_group.staff_id = '10896'
            AND direct_message_group.active = '0')))
  AND direct_message.school_id = '1'
  AND direct_message_thread.school_id = direct_message.school_id
GROUP BY direct_message.id
ORDER BY direct_message_thread.inserted DESC
LIMIT 0,
      25

EXPLAIN OF QUERY

OR is really bad in Joins, in WHERE it can/should be replaced when the optimizer doesn't use indexes (and OR does).

I would prefer the Union for a simple reason, it's easier to write/understand/extend. Additionally the first two Selects are the same besides one condition, so combining them is simple.

And using aliases really simplifies reading the code.

  (SELECT dm.*
   FROM `direct_message`AS dm
   INNER JOIN direct_message_thread AS dmt
      ON dmt.chat_id = dm.id
     AND dmt.school_id = dm.school_id -- moved from WHERE as this is a join condition
   WHERE (dm.recipient_id = '10896' OR dm.creator_id = '10896')
     AND dm.school_id = '1'
   GROUP BY dm.id
   ORDER BY dmt.inserted DESC)

UNION ALL

  (SELECT dm.*
   FROM `direct_message` AS dm
   INNER JOIN direct_message_thread AS dmt
      ON dmt.chat_id = dm.id
     AND dmt.school_id = dm.school_id
   INNER JOIN direct_message_group AS dmg
      ON dmg.chat_id = dm.id
   WHERE dmg.staff_id = '10896'
     AND dm.school_id = '1'
   GROUP BY dm.id
   ORDER BY dmt.inserted DESC)


writeln() and format() with variadic templates I wanted to get better acquainted with variadic templates, so I decide to try to implement a function like D's writeln(), just for fun.

void writeln()
{
    std::cout << "\n";
}

template<typename T, typename ...Args>
void writeln(T firstArg, Args... extraArgs)
{
    std::cout << firstArg;
    writeln(std::forward<Args>(extraArgs)...);
}

Usage example:

writeln("hello ", "world ", 42, "\t", 3.141592);
writeln(); // prints just a newline

Next, I implemented a format() function, which writes to a string instead of cout:

// Need this because there is no to_string for string in the std namespace.
std::string to_string(const std::string & s)
{
    return s;
}

std::string format()
{
    return "";
}

template<typename T, typename ...Args>
std::string format(T firstArg, Args... extraArgs)
{
    using namespace std;
    std::string s = to_string(firstArg);
    s += format(std::forward<Args>(extraArgs)...);
    return s;
}

// sample:
std::string s = format("hello ", "world ", 42, "\t", 3.141592);

It is uses std::to_string() for the native types. If I want to print custom types, then I can define a local to_string() and the overload resolution should find it.

My concerns are:

I haven't had many chances to use variadic templates so far, so I might be missing some caveat here. I was expecting it to be more complicated... Did I miss some corner case?
Is my use of std::forward appropriate?
Any other comments and suggestion are very welcome.

The biggest thing that sticks out at me is that you're passing all of your Args... parameters by value, thus pretty much making std::forward useless here. To make use of std::forward, the reference type needs to be deduced from the calling context. By itself, std::forward really doesn't do anything except a static_cast to the deduced reference type.
Basically, everywhere you have Args should be passed by Args&&:
template<typename T, typename ...Args>
void writeln(T firstArg, Args&&... extraArgs)

template<typename T, typename ...Args>
std::string format(T firstArg, Args&&... extraArgs)

As a cut down example of the difference this makes, try this example:
#include <memory>
#include <iostream>

struct s
{
    s()
    { }

    s(s&& )
    {
        std::cout << "Move\n";
    }

    s(const s& )
    {
        std::cout << "Copy\n";
    }
};

template <typename T>
void do_forward(T&& v)
{
    sink(std::forward<T>(v));
}

template <typename U>
void sink(U&& u)
{
    std::cout << "In sink\n";
}

int main()
{
    s something;
    do_forward(something);
}

If I run this, the output is:

In sink

If I change the signature of do_foward and sink to:
template <typename T>
void do_forward(T v)

template <typename U>
void sink(U u)

The output is:

Copy
Move
In sink

If we change main to just pass an actual rvalue reference instead of an lvalue:
int main()
{
    do_forward(s{});
}

We remove the first copy operation (from do_forward) but still have a move operation (in sink).


DRY multiple if statements used to show/hide elements based on slider value I'm showing and hiding 4 svg paths based on the value of a slider, all is working as expected, but it still feels a little cumbersome. Does anyone know a way to "DRY" it out a little more?

jsFiddle

$('.volumeSlider').slider({
    value: 1,
    orientation: "vertical",
    range: "min",
    max: 1,
    step: 0.05,
    animate: true,
    slide: function (e, ui) {
        $('p').text(ui.value);
        var v = $('.volume1');
        if (ui.value == 1) {
            v.show();
        }
        if (ui.value < 1) {
            v.slice(0).hide();
            v.slice(1, 4).show();

        }
        if (ui.value <= 0.65) {
            v.slice(0, 2).hide();
            v.slice(2, 4).show();
        }
        if (ui.value <= 0.35) {
            v.slice(0, 3).hide();
            v.slice(4).show();
        }
        if (ui.value === 0) {
            v.hide();
        }
    }
});

I am guessing that the '.volume' elements are like volume indicators, the louder the sound, the more you show.

You can iterate over the '.volume' elements, compare the index / elementCount to ui.value and decide to hide or show per that comparison.

So, something like :

$('.volumeSlider').slider({
    value: 1,
    orientation: "vertical",
    range: "min",
    max: 1,
    step: 0.05,
    animate: true,
    slide: function (e, ui) {
        $('p').text(ui.value);
        var v = $('.volume1');
        for( var i = 0 ; i < v.length ; i++ ){
            ( 1-ui.value > 1 / v.length * (i + 1)) ? v.slice(i).hide() : v.slice(i).show()
        }
    }
});

should work. This approach will work with any number of volume1 elements.
I did not test this, but once you see/get this I am sure you can make this work.

Further optimizations can be caching v.length and 1 / v.length, you could probably also use eq instead of slice()

EDIT : I fixed the code to work with your HTML, the original was

( ui.value < 1 / v.length * (i + 1)) ? v.slice(i).hide() : v.slice(i).show()
which would work if the HTML elements were created in what I see as the correct order.


Better approach on handling unexpected string upon converting it to the enum value I have Grade enum and there is a static method to convert a string value to the particular enum value.

public enum Grade {
    None("N"),
    NotAccepted("-"),
    Accepted("+"),
    Uncertain("?");

    private final String gradeSymbol;

    Grade(String gradeSymbol) {
        this.gradeSymbol = gradeSymbol;
    }

    public String getGradeSymbol() {
        return gradeSymbol;
    }

    public static Grade fromGradeSymbol(final String gradeSymbol) {
        switch (gradeSymbol) {
            case "N":
                return None;

            case "-":
                return NotAccepted;

            case "+":
                return Accepted;

            case "?":
                return Uncertain;

            default:
                throw new IllegalArgumentException("Unexpected grade symbol: " + gradeSymbol);
        }
    }

    @Override
    public String toString() {
        return this.gradeSymbol;
    }
}

My question is, what is better way to handle unexpected grade symbol in the static fromGradeSymbol() method.

Another option I consider is the approach using Optional and returning Optional.empty() instead of throwing IllegalArgumentException.

Like this:

public static Optional<Grade> fromGradeSymbol(final String gradeSymbol) {
    switch (gradeSymbol) {
        case "N":
            return Optional.of(None);

        case "-":
            return Optional.of(NotAccepted);

        case "+":
            return Optional.of(Accepted);

        case "?":
            return Optional.of(Uncertain);

        default:
            return Optional.empty();
    }
}

My thoughts:

Throwing exception looks more logical, because it is being thrown in not expected case, when wrong value has been passed. But throwing exception makes it necessary to catch this exception anywhere where I convert string to the enum.

Use of Optional does not involve exceptions and makes cleaner code, without try-catch construct. But it somehow hides the unexpected input.

Sure, I can assume that every time when Optional does not contain a value we have an unexpected input. But it feels like code smell.

What do you think?

All enumerations have a default valueOf(String) method that throw an IllegalArgumentException so your first idea is coherent.

I would say that it depends (..). It depends of your use cases:

If he conversion would be called form an unsafe (user/api) input, then there are chances that you receive an unexpected symbol. And using Optional will force the developer to handle that case more explicitely.
If the convesrion is made from a safe input (code/config), then you can live with the exception.

In the book Effective Java  from Joshua Bloch (Addison-Wesley Professional, 978-0134685991) there is a full chapter on that subject (in the 2nd edition):

  Enum types have an automatically generated valueOf(String) method that
  translates a constant’s name into the constant itself.

  [..]

  The following code
  (with the type name changed appropriately) will do the trick for any enum, so long
  as each constant has a unique string representation:

// Implementing a fromString method on an enum type
private static final Map<String, Operation> stringToEnum
        = new HashMap<String, Operation>();
static { // Initialize map from constant name to enum constant
    for (Operation op : values())
        stringToEnum.put(op.toString(), op);
    }

// Returns Operation for string, or null if string is invalid
public static Operation fromString(String symbol) {
    return stringToEnum.get(symbol);
}

That chapter (and the whole book) is worth the read: CHAPTER 6 ENUMS AND ANNOTATIONS


Base64 encoding implementation I am learning C and decided to make an implementation of Base64 encoding according to the info Wikipedia provides about it.

My main question is: should I declare the index_table array inside the get_base64_digit function like static char index_table[] = "blabla"?

A general code review would also be appreciated.

funcs.c

static char index_table[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";

static char get_index_digit(int x) {
    return index_table[x];
}

char get_base64_digit(const unsigned int base64, int digit_number){

    switch (digit_number){
            case 0:
                    return get_index_digit(base64 >> 18);
            case 1:
                    return get_index_digit((base64 >> 12) & 077);
            case 2:
                    return get_index_digit((base64 >> 6) & 077);
            case 3:
                    return get_index_digit(base64 & 077);
    }

    return '\0';
}

funcs.h

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

char get_base64_digit(unsigned int base64, int digit_number);

main.c

 #include "funcs.h"

int main(int argc, char* argv[]){

    char* text = argc == 2 ? argv[1] : '\0';

    unsigned final, temp;

    int text_len  = strlen(text);
    int remainder = text_len % 3;

    //Iterate over the input string, three characters at a time.
    for (int x = final = temp = 0; x < (remainder == 0 ? text_len : text_len - remainder); x += 3) {

            final = (text[x] << 16) | (text[x+1] << 8) | text[x+2];

            printf("%c%c%c%c",  get_base64_digit(final, 0), get_base64_digit(final, 1), get_base64_digit(final, 2), get_base64_digit(final, 3));

    }

    //Handle the last bytes in case text_len wasn't a multiple of 3
    if (remainder == 1) {
            final = text[text_len-1] << 16;
            printf("%c%c==\n", get_base64_digit(final, 0), get_base64_digit(final, 1));
    } else if (remainder == 2){
            final = (text[text_len-2] << 16) | (text[text_len-1] << 8);
            printf("%c%c%c=\n",  get_base64_digit(final, 0), get_base64_digit(final, 1), get_base64_digit(final, 2));
    } else
            return 0;

    return 0;
}

funcs.h

Neither funcs.c nor funcs.h require anything from stdio.h or string.h, so you shouldn't be including those there, move them to main.c instead. But you should get into the habit of adding include guards to your headers. So either use this at the top:

#pragma once

Or, to be completely standard and portable:

#ifndef FUNCS_H
#define FUNCS_H

// contents of header here

#endif // FUNCS_H

funcs.c

The static encode mapping is fine as you have it, but would also be fine inside the get_index_digit function (static also of course). Since that is the only direct user, it would be slightly better inside in my opinion.

I'd change your signature and preamble for the encoder to:

#include <assert.h>
// ...

char get_base64_digit(const unsigned int base64, unsigned int digit_number)
{
  assert(digit_number < 4);
  // ...

Passing a negative index or anything greater than 3 is a programming error, it should be caught as early as possible and be a hard failure.

You can get rid of the switch by collapsing all cases:

return get_index_digit((base64 >> (18 - 6*digit_number)) & 077);

Note that this masks the bits even for the zero case, which is safer. But you might want instead to assert on the fact that the bits above index 18 are all zeros - slightly different contract. (I'd prefer just masking them off though.)

main.c

char* text = argc == 2 ? argv[1] : '\0';

That '\0' is bad. Happens to work, but doesn't convey the right information. text is a pointer, not a char. Replace with:

char* text = argc == 2 ? argv[1] : NULL;

But that's actually not a good idea either. What you actually need is an empty string (or bail out).

const char* text = argc == 2 ? argv[1] : "";

Then you have:

unsigned final, temp;

But you're not using temp anywhere, except for setting it to zero. And you're always initializing final to zero in the for loop. So to help simplifying that for a bit:

unsigned final = 0;

Also a slight inconsistency: you're using unsigned here, but used unsigned int in the declaration for get_base64_digit - pick one version and stick to that (they are completely identical).

As for the loop:

for (int x = final = temp = 0; x < (remainder == 0 ? text_len : text_len - remainder); x += 3) {

That line is too long, and more complex than it needs to be. Removing final and temp will help:

for (int x = 0; x < (remainder == 0 ? text_len : text_len - remainder); x += 3) {

And might help you to see that the conditional is unnecessary. If remainder is zero, text_len and text_len - remainder have the same value. So:

for (int x = 0; x < text_len - remainder; x += 3) {

Or move that subtraction out of the loop for an even simpler structure.

The last lines are a bit odd:

} else
        return 0;

return 0;

Remove the else, it is not useful. Also mixing braced and non-braced blocks in an if/else chain isn't a good idea. This is a matter of style, but always using braces is usually safer. If you want to omit them for single-statement blocks, only do that if you only have single-statement blocks in the if/else chain.

Last tidbit: you sometimes have ){ (no space), and sometimes ) { (one space). Stick to one, preferably the one with the space. You also sometimes have an empty line at the start of blocks, sometimes not. If you feel like a bit of vertical separation is good there, try putting the { that starts the block on its own line (as I did in the get_base64_digit above), and look at the Linux coding style guidelines for some other tips that you may, or may not :-), like.

clang-format is an excellent auto-format tool, and a lot of dev tools (including Vim and Emacs) can be made to use it. Try it out.


Phone number duplication detector I am doing the POJ 1002 question. My code works fine but the website says the compilation time is more than 2000ms, which is not accepted. How can I improve my time-wise performance?
Problem Statement

Input — The input will consist of one case. The first line of the input specifies the number of telephone numbers in the directory (up to 100,000) as a positive integer alone on the line. The remaining lines list the telephone numbers in the directory, with each number alone on a line. Each telephone number consists of a string composed of decimal digits, uppercase letters (excluding Q and Z) and hyphens. Exactly seven of the characters in the string will be digits or letters.
Output — Generate a line of output for each telephone number that appears more than once in any form. The line should give the telephone number in standard form, followed by a space, followed by the number of times the telephone number appears in the directory. Arrange the output lines by telephone number in ascending lexicographical order. If there are no duplicates in the input print the line:
No duplicates.

import java.util.Map;
import java.util.Scanner;
import java.util.TreeMap;

public class Main{

public static void main(String[] args) {
    Scanner scan = new Scanner(System.in);
    int total = scan.nextInt();
    scan.nextLine();                // But why?
    String[] numbers = new String[total];
    Map<String, Integer> dict = new TreeMap<String, Integer>();

    for(int i = 0; i < total; i++) {
        numbers[i] = scan.nextLine();
        numbers[i] = convert(numbers[i]);
        if(!dict.containsKey(numbers[i])) {
            dict.put(numbers[i], 1);
        } else {
            dict.put(numbers[i], dict.get(numbers[i]) + 1);
        }
    }

    scan.close();

    boolean hasDuplication = true;
    for(String number: dict.keySet()) {
        if(dict.get(number) > 1) {
            hasDuplication = false;
            System.out.println(number + " " + dict.get(number));
        }
    }
    if(hasDuplication) {
        System.out.println("No duplicates.");
    }
}

public static String convert(String raw) {
    raw = raw.replaceAll("-", "");
    raw = raw.toLowerCase();
    String number = "";
    for(int i = 0; i < raw.length(); i++) {
        number += parse(raw.charAt(i));
    }
    number = number.substring(0, 3) + "-" + number.substring(3);
    return number;
}

public static char parse(char digit) {
    if(digit >= 'a' && digit < 'q') {
        digit = (char) ((digit-'a') / 3 + '2');
    } else if(digit > 'q' && digit < 'z') {
        digit = (char) ((digit-'q') / 3 + '7');
    }
    return digit;
}

}

Chomp the new line

    scan.nextLine();                // But why?

Nothing to do with performance, but Scanner is skipping nextLine() after using next(), nextInt() or other nextFoo()? explains this.

Remove unnecessary data structure

    String[] numbers = new String[total];

You never use numbers as an array.  You could get rid of it entirely.

        numbers[i] = scan.nextLine();
        numbers[i] = convert(numbers[i]);
        if(!dict.containsKey(numbers[i])) {
            dict.put(numbers[i], 1);
        } else {
            dict.put(numbers[i], dict.get(numbers[i]) + 1);
        }

Replace numbers[i] with a single String.

        String number = convert(scan.nextLine());

        Integer count = dict.get(number);
        if (count == null) {
            count = 0;
        }
        count++;

        dict.put(number, count);

Now we don't keep an array around for nothing.

I also changed the get/put pattern.  This saves having to do both the containsKey check and the get.

Consider other data structures

    Map<String, Integer> dict = new TreeMap<String, Integer>();

I would expect a TreeMap to be slower than a HashMap for most applications that depend on insert and read efficiency.  This is especially so since it sorts on keys and what you want is to find all with at least value 2.  You'd need to override the comparison to get that behavior.

You don't use the read behavior that would be helpful.  And you accept the slower writes.

        Map<String, Integer> dict = new HashMap<>();

This should perform faster for larger inputs.

Pick the right method

    for(String number: dict.keySet()) {
        if(dict.get(number) > 1) {
            hasDuplication = false;
            System.out.println(number + " " + dict.get(number));
        }
    }

This is the use case for an entrySet.

    for (Map.Entry<String, Integer> entry : dict.entrySet()) {
        if (entry.getValue() > 1) {
            hasDuplication = true;
            System.out.println(entry.getKey() + " " + entry.getValue());
        }
    }

Now we don't have to do an expensive get lookup operation on each iteration.

I'd also switch the meaning of hasDuplication to match the name.  Don't forget to switch it the other two places as well.

    boolean hasDuplication = true;

to

    boolean hasDuplication = false;

and

    if(hasDuplication) {

to

    if (!hasDuplication) {

Other possibilities

If this doesn't help, consider splitting reading the input from processing it.  So something like

    for (int i = 0; i < numbers.length; i++) {
        numbers[i] = scan.nextLine();
    }

    scan.close();

    for (String number : numbers) {
        number = convert(number);

        Integer count = dict.get(number);
        if (count == null) {
            count = 0;
        }
        count++;

        dict.put(number, count);
    }

I didn't test this, so you may have to declare a new variable rather than reusing number.


Infix to postfix notation in Haskell (Shunting-yard algorithm) I've written an infix to postfix converter in Haskell using the Shunting-yard algorithm. Example of how it works:

$ ./in2post
2 + 2
2 2 +
$ ./in2post
1 + 2 * 3
1 2 3 * +
$ ./in2post
  (5 - 4) * 3+1005/(12-6*(12   -8)  )
5 4 - 3 * 1005 12 6 12 8 - * - / +
$ ./in2post
(2 + 45
2 45 + ERROR

And the source code:

Main.hs

module Main (main) where

import System.IO
import InToPost

main :: IO ()
main = do
    line <- getLine
    let tokens  = tokenise line
        newexpr = shuntYard [] [] tokens
    putStrLn $ untokenise newexpr

InToPost.hs

module InToPost
    ( Token(TNum, TOp)
    , Operator
    , splitTok
    , tokenise
    , untokenise
    , shuntYard
    ) where

import Data.Char (isSpace, isDigit)
import Data.List (groupBy)

data Token = TNum Int
           | TOp Operator
           deriving (Show)
data Operator = Add
              | Sub
              | Mult
              | Div
              | LBrace
              | RBrace
              deriving (Show, Eq)

splitTok :: String -> [String]
splitTok = groupBy (\x y -> isDigit x && isDigit y) . filter (not . isSpace)

str2tok :: String -> Either String Token
str2tok tkn@(c:_)
    | isDigit c = Right $ TNum $ read tkn
    | otherwise = case tkn of
        "+" -> Right $ TOp Add
        "-" -> Right $ TOp Sub
        "*" -> Right $ TOp Mult
        "/" -> Right $ TOp Div
        "(" -> Right $ TOp LBrace
        ")" -> Right $ TOp RBrace
        _   -> Left  $ "No such operator: \"" ++ tkn ++ "\""

tok2str :: Token -> String
tok2str (TNum t) = show t
tok2str (TOp t) = case t of
    Add  -> "+"
    Sub  -> "-"
    Mult -> "*"
    Div  -> "/"
    _    -> "ERROR"

precedence :: Operator -> Int
precedence Add    = 1
precedence Sub    = 1
precedence Mult   = 2
precedence Div    = 2
precedence LBrace = 3
precedence RBrace = 3

-- shuntYard (Operator stack) (Token Queue) (Token Buffer) = new Token Queue
shuntYard :: [Operator] -> [Token] -> [Either String Token] -> Either String [Token]
shuntYard _ _ (Left s:_) = Left s
shuntYard stack queue [] = Right $ queue ++ map TOp stack
shuntYard stack queue (Right (TNum t):ts) = shuntYard stack (queue ++ [TNum t]) ts
shuntYard stack queue (Right (TOp t):ts) =
    shuntYard ustack uqueue ts
  where
    (ustack, uqueue) = case t of
        LBrace -> (t : stack, queue)
        RBrace -> (stail srest, queue ++ map TOp sstart)
        _      -> (t : ssend, queue ++ map TOp ssops)
    (sstart, srest) = break (==LBrace) stack
    currprec = precedence t
    (ssops, ssend) = span (\op -> precedence op > currprec && op /= LBrace) stack
    stail :: [a] -> [a]
    stail (x:xs) = xs
    stail [] = []

tokenise :: String -> [Either String Token]
tokenise = map str2tok . splitTok

untokenise :: Either String [Token] -> String
untokenise (Left s) = s
untokenise (Right ts) = unwords . map tok2str $ ts

Please tell me, what are my bad practices here? For example, the use of Either felt really awkward and I'm sure it can be done better. Also, the case expression in str2tok is quite ugly.

main :: IO ()
main = do
  line <- getLine
  putStrLn $ case traverse str2tok $ splitTok line of
    Left s -> s
    Right ts -> unwords $ map tok2str $ shuntYard ts

-- shuntYard (Token Buffer) = new Token Queue
shuntYard :: [Token] -> [Token]
shuntYard ts = concat queue ++ stack where
  (queue, stack) = (`runState` []) $ for ts $ state . \case
    TNum t -> ([TNum t],)
    TOp LBrace -> ([],) . (LBrace :)
    TOp RBrace -> (map TOp *** drop 1) . break (==LBrace)
    TOp t -> (map TOp *** (t:)) . span (\op -> precedence op > precedence t && op /= LBrace)

Or perhaps:

(queue, stack) = (`runState` []) $ for ts $ \case
  TNum t -> return [TNum t]
  TOp LBrace -> [] <$ modify (LBrace:)
  TOp RBrace -> do
    sstart <- state $ break (==LBrace)
    modify (drop 1)
    return $ map TOp sstart
  TOp t -> do
    ssops <- state $ span $ \op -> precedence op > precedence t && op /= LBrace
    modify (t:)
    return $ map TOp ssops