Coded a Go Script that Sends proxy requests to backend servers I've started Go for the last couple of months and I decided to challenging small projects, so I created this proxy requests script.
It works well (in my opinion at least) but I'm looking for different ways of improving this further like being able to target all servers, or making this more concise and compact. To be honest I think it is readable but again that's my opinion. I'm looking for it to be peer reviewed with no holds barred critism of my code. Why? to learn new ways of improving my coding techniques and making it much more efficient.
If anyone wants to view the code feel free:
package main

import (
    "context"
    "fmt"
    "math/rand"
    "time"
)

type frame struct {
    context.Context
    resp chan<- string
}

type frames chan frame

func main() {
    var serverConns []frames
    const totalServers = 10
    for i := 0; i < 10; i++ {
        serverID := i
        conn := make(frames)
        defer close(conn)
        serverConns = append(serverConns, conn)
        go func() {
            // the server
            for frame := range conn {
                resps := []string{"banana", "apple", "guava"}
                time.Sleep(50*time.Millisecond + time.Duration(rand.Intn(500))*time.Millisecond)
                r := fmt.Sprint("server ", serverID, ": ", resps[rand.Intn(len(resps))])
                select {
                case <-frame.Done():
                case frame.resp <- r:
                }
            }
        }()
    }
    // the proxy
    const maxFanout = 3
    var conns []frames
    connIDs := rand.Perm(len(serverConns))[:totalServers/2]
    for _, connID := range connIDs {
        fmt.Println("selected server:", connID)
        conns = append(conns, serverConns[connID])
    }
    fanoutConn := make(frames)
    go func() {
        for p := range fanoutConn {
            ctx, cancel := context.WithCancel(p.Context)
            firstResp := make(chan string)
            for _, serverConn := range conns {
                serverConn := serverConn
                fwdframe := frame{
                    Context: ctx,
                    resp:    firstResp,
                }
                go func() {
                    serverConn <- fwdframe
                }()
            }
            response := <-firstResp
            cancel()
            p.resp <- response
        }
    }()
    // the client
    for {
        r := make(chan string)
        p := frame{
            Context: context.Background(),
            resp:    r,
        }
        fanoutConn <- p
        fmt.Println(<-r)
    }
}

I'm looking for it to be peer reviewed with no holds barred critism of
my code. Why? to learn new ways of improving my coding techniques and
making it much more efficient. Judas Is Back

The evaluation criteria for a code review:
Code should be correct, maintainable, robust, reasonably efficient, and, most importantly, readable. Code should be useful and have real value.

I tried to read your code. I found it to be an unreadable, sprawling, blob of code. Sorry!
The first step is to give the code some structure. For example,

On the Criteria To Be Used in Decomposing Systems into Modules, D.L.
Parnas, Carnegie-Mellon University, Communications of the ACM, December
1972, Volume 15, Number 12.

Parnas quotes Gouthier and Pont:

A well-defined segmentation of the project effort ensures system
modularity. Each task forms a separate, distinct program module. At
implementation time each module and its inputs and outputs are
well-defined, there is no confusion in the intended interface with
other system modules. At checkout time the in- tegrity of the module
is tested independently; there are few sche- duling problems in
synchronizing the completion of several tasks before checkout can
begin. Finally, the system is maintained in modular fashion; system
errors and deficiencies can be traced to specific system modules, thus
limiting the scope of detailed error searching.

After spending ten minutes on basic cleanup, this is what I have:
Playground: https://play.golang.org/p/tsPqSe0urdv
The main function summarizes the program.
func main() {
    // the servers
    serverConns := servers()
    // the proxies
    fanoutConn := proxies(serverConns)
    // the client
    client(fanoutConn)
}

The server and servers functions encapsulate the server related processing, promoting information hiding. I did a lot of basic cleanup.
// the server
func server(id int, conn frames) {
    defer close(conn)

    server := fmt.Sprint("server ", id, ": ")
    resps := []string{"banana", "apple", "guava"}

    for frame := range conn {
        time.Sleep(time.Duration(50+rand.Intn(500)) * time.Millisecond)

        resp := server + resps[rand.Intn(len(resps))]

        select {
        case <-frame.Done():
        case frame.resp <- resp:
        }
    }
}

// the servers
func servers() []frames {
    const totalServers = 10
    serverConns := make([]frames, totalServers)
    for id := range serverConns {
        conn := make(frames)
        serverConns[id] = conn
        go server(id, conn)
    }
    return serverConns
}

There is more cleanup to be done on these and other functions.
As the code gets more readable, we move closer to other code review goals like correctness and maintainability.
proxy.go:
package main

import (
    "context"
    "fmt"
    "math/rand"
    "time"
)

type frame struct {
    context.Context
    resp chan<- string
}

type frames chan frame

// the server
func server(id int, conn frames) {
    defer close(conn)

    server := fmt.Sprint("server ", id, ": ")
    resps := []string{"banana", "apple", "guava"}

    for frame := range conn {
        time.Sleep(time.Duration(50+rand.Intn(500)) * time.Millisecond)

        resp := server + resps[rand.Intn(len(resps))]

        select {
        case <-frame.Done():
        case frame.resp <- resp:
        }
    }
}

// the servers
func servers() []frames {
    const totalServers = 10
    serverConns := make([]frames, totalServers)
    for id := range serverConns {
        conn := make(frames)
        serverConns[id] = conn
        go server(id, conn)
    }
    return serverConns
}

// the proxy
func proxy(conns []frames, fanoutConn frames) {
    for p := range fanoutConn {
        ctx, cancel := context.WithCancel(p.Context)
        firstResp := make(chan string)
        for _, serverConn := range conns {
            serverConn := serverConn
            fwdframe := frame{
                Context: ctx,
                resp:    firstResp,
            }
            go func() {
                serverConn <- fwdframe
            }()
        }
        response := <-firstResp
        cancel()
        p.resp <- response
    }
}

// the proxies
func proxies(serverConns []frames) frames {
    connIDs := rand.Perm(len(serverConns))[:len(serverConns)/2]
    var conns []frames = make([]frames, 0, len(connIDs))
    for _, connID := range connIDs {
        fmt.Println("selected server:", connID)
        conns = append(conns, serverConns[connID])
    }
    fanoutConn := make(frames)
    go proxy(conns, fanoutConn)
    return fanoutConn
}

// the client
func client(fanoutConn frames) {
    for {
        resp := make(chan string)
        p := frame{
            Context: context.Background(),
            resp:    resp,
        }
        fanoutConn <- p
        fmt.Println(<-resp)
    }
}

func main() {
    // the servers
    serverConns := servers()
    // the proxies
    fanoutConn := proxies(serverConns)
    // the client
    client(fanoutConn)
}


Math Random Addition I am learning python from CS106. Please review my code for Khan Sole Academy, I think my code not efficient. Please add critique and advice for beginner like me.
About Code
Generate 2 random numbers and add those numbers. If the user supplied answer is correct, row will increase by 1, goal is 3. If answer is false, row will be reset to zero.
Question From Assignment CS106

Your program should be able to generate simple addition problems that involve adding two 2-digit integers (i.e., the numbers 10 through 99).  The user should be asked for an answer to each problem.  Your program should determine if the answer was correct or not, and give the user an appropriate message to let them know.  Your program should keep giving the user problems until the user has gotten 3 problems correct in a row.
(Note: the number of problems the user needs to get correctly in a row to complete the program is just one example of a good place to specify a constant in your program)

   import random

MIN_NUM = 10
MAX_NUM = 99
row = 0

def main():
    """
    Pre Condition: Generate random number. Additional num 1 and num 2
    Post Condition : Check user answers. If answer correct add row, if incorrect reset row to zero
    """
    row = 0
    while row < 3:
        # number generator
        num_1 = random.randint(MIN_NUM, MAX_NUM)
        num_2 = random.randint(MIN_NUM, MAX_NUM)
        print("What is " + str(num_1) + " + " + str(num_2) + " ?")
        total = num_1 + num_2
        #user answer
        user_input = int(input("Your answer: "))
        # Check user answer
        # if answer is incorrect, reset row to zero
        if user_input == total:
            row += 1
            print("Correct!  You've gotten " + str(row) +" correct in a row ")
        else:
            print("Incorrect. The expected answer is " + str(total))
            row = 0

if __name__ == "__main__":
    main()

Actually, your code is efficient - it does exactly what it is supposed to do, and does not take any extra steps.
There are a few other issues, of the kind that are expected from a new coder in the process of learning, so here are some tips:
Global row variable
In Python, if you declare a variable outside of a function and then one with the same name inside a function (by assigning it value, as you do with row), you create a new variable, that "overshadows" the global one.
Basically, the top row in you code is never used, instead, main has its own local copy.
But that is ok, because you don't need a global row variable any way.
As a general practice it is best to avoid global variables completely.
There are very few cases where they are needed, especially in object oriented languages like Python.
String formatting
When you want to compose a string from multiple parts, including values in variables you should use formatting instead of string concatenation with '+'.
It is a more robust method allowing for prettier output, and makes your code more readable and more convenient to modify should the need arise.
Also, if you are putting together a lot of parts, this will be more efficient.
If you are using Python 3.6 or newer, you can do this:
print(f"What is {num_1} + {num_2} ?")
...
print(f"Correct!  You've gotten {row} correct in a row.")

If you are on an older version of Python, you can do this:
print("What is {} + {} ?".format(num_1, num_2))
...
print(f"Correct!  You've gotten {} correct in a row.".format(row))

As you can see, you no longer have to convert numbers to strings manually, the formatting function will do it for you!
Pay attention to hints in questions
You have used constants for range of random numbers, but not for the maximum number of correct answers in a row.
The question almost requires you use a constant by saying:

(Note: the number of problems the user needs to get correctly in a row to complete the program is just one example of a good place to specify a constant in your program)

In general, numbers that have meaning but are written as is directly in the code (like the number of correct answers) are called "magic numbers" and are considered bad practice.
Just add at the top:
MAX_IN_ROW = 3

And use MAX_IN_ROW everywhere else in the code you need to check this value.
Input validation
This may be an advanced topic you have not covered yet, but as a rule of thumb all programs must guard against bad or invalid user input.
Some questions, especially on time constrained exams tell you to omit such checks, but if not explicitly instructed to do so, and if you can, you should check all input before using it.
For this question, exception handling would be appropriate:
try:
    user_input = int(input("Your answer: "))
except ValueError:
    print("Invalid answer! You must type a positive decimal number.")
    continue


Buying in cash or coins Does anyone have any ideas on how I can refactor these methods to get rid of the duplication? The first, third and fourth lines of both methods do the same thing. I'm having trouble for some reason because the second argument of both methods are of different types (or rather they represent different types of values).

I was think of using procs and having the different code be a block, but I couldn't find a solution.

def buy_in_cash(symbol, cash_amt) # second argument is cash
  market_price = @exchange.price_by_symbol(symbol)
  coin_amt = cash_to_coin(cash_amt, market_price) # different
  withdraw_cash(cash_amt)
  @transactions << Transaction.new(symbol, coin_amt, market_price)
end

def buy_in_coin(symbol, coin_amt) # here second argument is coin
  market_price = @exchange.price_by_symbol(symbol)
  cash_amt = coin_to_cash(coin_amt, market_price) # different
  withdraw_cash(cash_amt)
  @transactions << Transaction.new(symbol, coin_amt, market_price)
end

In my opinion, "buying" should be encapsulated in it's own method that needs to know the symbol, cash and coin amounts.

Since buying is transactional and both actions need to happen or be rolled back, I think it's a good candidate for it's own method and shouldn't be mixed with converting currencies whioch in my opinion is a pre-requisite to buying. If, for example, there is an error with converting the currencies, buy shouldn't be called at all.

Moreover, there may be side effects to buying like managing stock and dispatching the goods so that makes it an even better candidate for it's own method.

Maybe like this:

def buy(symbol, cash_amt, coin_amt)
  # Both coin and cash amount need to be passed
  raise ArgumentError unless coin_amt && cash_amt

  withdraw_cash(cash_amt)
  @transactions << Transaction.new(symbol, coin_amt, market_price)
end

def market_price(symbol)
  @exchange.price_by_symbol(symbol)
end

def buy_in_cash(symbol, cash_amt)
  coin_amt = cash_to_coin(cash_amt, market_price(symbol))
  buy(symbol, cash_amt, coin_amt)
end

def buy_in_coin(symbol, coin_amt)
  cash_amt = coin_to_cash(coin_amt, market_price(symbol))
  buy(symbol, cash_amt, coin_amt)
end


Wrapping jQuery empty() To Include A Callback I needed to write a method to extend the jQuery empty() function to include a callback. Since I didn't want to mess with namespaces, closures, etc, I instead wrote a regular function to wrap empty().

var emptyWithCallback = function(node, callback) {
    console.log('Entered emptyWithCallback()');
    if (typeof callback != 'function') {
        return false;
    }
    var doTheCallback = callbackWrapper();
    var doTheEmpty = myEmpty();

    function callbackWrapper() {
        var deferredCallback = $.Deferred();
        callback();
        deferredCallback.resolve();

        return deferredCallback.promise();
    }

    function myEmpty() {
        var deferredEmpty = $.Deferred();
        $(node).empty();
        deferredEmpty.resolve();

        return deferredEmpty.promise();
    }

    doTheCallback.then(doTheEmpty);
}

I was attempting to use the $.Deferred() in case the callback contained an ajax call, in order to not execute the .empty() until the callback concluded.

Does this code look like it will work? It seems to work on simple callbacks, such as a single alert() or console.log().

I can't really see how wrapping empty() with a $.Deferred object manages to achieve anything; after all empty() executes immediately and doesn't need the consumer to wait for a function callback.

Instead think of how you might be using empty() with an asynchronous context and then abstract that. For example if it is an AJAX request which you need to empty an element out, then fetch, then display the new data your implementation might look like this:

var clearAndThenFetch = function($element, ajaxOptions, callback) {
    var deferred = $.Deferred();

    $element.empty();

    $.ajax($.extend({
        success: function(response) {
            callback && callback(deferred, response) || deferred.resolve();
        },
        error: function(xhr) {
            deferred.reject(JSON.parse(xhr.responseText).errors);
        }
    }, ajaxOptions));

    return deferred.promise();
};

Otherwise I can't really see the need for abstraction here.

Edit: In regards to Flambino's comment, why do you need the empty() deferred at all? Remember that DOM operations are synchronous and so don't need to use promises. With this in mind we can rewrite your function like so:

var emptyWithCallback = function(node, callback) {
    console.log('Entered emptyWithCallback()');
    if (typeof callback != 'function') {
        return false;
    }

    var doTheCallback = function() {
        var deferredCallback = $.Deferred();
        callback();
        deferredCallback.resolve();

        return deferredCallback.promise();
    }

    doTheCallback.then(function() {
        $(node).empty();
    });
}


WPF application depending on large file I've got a quite simple WPF application, trying to implement a correct MVVM pattern.

The application in itself is simple: one listItem and one textbox + button for searching. On init, the entire content of a "large" (~50k to ~1million lines) file is loaded into the listItem. When the users enters text in textbox and clicks on the button, I go through the entire file and get the corresponding elements and refresh the listItem.

Right now, everything is fairly dirty, as you can see in the code below. I'm wondering how I can deal with this file. Should I keep everything in memory like I'm doing right now? Or should I reparse the file every time I need to? And how would all of this fit in MVVM pattern? I was thinking of a "Messages" class as a Model.

MainWindow.xaml.cs

public partial class MainWindow : Window
{
    public MainWindow()
    {
        this.LoadConversation();
    }

    private MessagesProcessor _messageProcessor = new MessagesProcessor("c.txt");

    private void LoadConversation()
    {
        foreach (var msg in _messageProcessor.Messages)
        {
            ListBoxConversation.Items.Add(msg.ToString());
        }
    }

    private void onSearchBtnClick(object sender, RoutedEventArgs e)
    {
         var results = new List<Message>();
         results.AddRange(this._messageProcessor.GetSearchResults(word));
         foreach (var result in results.OrderBy(p => p.Date))
         {
                ListBoxConversation.Items.Add(result.ToString());
         }
    }
}

MessageProcessor.cs

 public class MessagesProcessor
 {
    private List<Message> _messages;

    public List<Message> Messages
    {
        get
        {
            return _messages;
        }
    }

    public MessagesProcessor(string pathOfBaseFile)
    {
        this._messages = ParseBaseMessageFile(pathOfBaseFile);
    }

    public IEnumerable<Message> GetSearchResults(string word)
    {
        return this._messages.Where(msg => msg.Content.Contains(word, StringComparison.OrdinalIgnoreCase));
    }
 }

Message.cs

public class Message
{
    public DateTime Date { get; set; }
    public string Content { get; set; }
    public string Author { get; set; }

    public override string ToString()
    {
        return "[" + this.Date.ToString("d") + " " + this.Date.ToString("t") + "]" + this.Author + " : " + this.Content;
    }
}

This is actually a nice example of how re-writing an application using MVVM can make life easier. It looks like you have the wrong idea about MVVM (your code should not be in the code behind). As a starting point, you would need an empty MVVM application with a main viewmodel and view, and know how to set up data binding and property changed events. If you haven't done this before, I would recommend taking a look at MVVM light which is a small framework that can get you up and running quickly.

The idea is that your view and viewmodel should not care where the messages are coming from - that is not their concern. This is in line with SRP and means that you can change how you read from the files (or read from somewhere else entirely) without touching any code in your V and VM layers. I would suggest starting with something like this:

ViewModel

public class MainViewModel : ViewModelBase
{
    private IMessageRepository _messageRepository;

    public MainViewModel(IMessageRepository messageRepository)
    {
        _messageRepository = messageRepository;
        FilteredMessages = _messageRepository.GetMessages();
    }

    // This will be bound to the search Button via a command
    public void FilterMessages(string searchString)
    {
        var messages = _messageRepository.GetMessages();
        FilteredMessages = messages.Where(m => m.Contains(searchString, StringComparison.OrdinalIgnoreCase))
    }

    // This will need to be bound to the ListBox and raise PropertyChangedEvents when set
    public IEnumerable<Message> FilteredMessages { get; private set; }
}

Model

public interface IMessageRepository
{
    IEnumerable<Message> GetMessages();
}

public abstract class FileMessageRepository : IMessageRepository
{
    protected readonly string FilePath;

    protected FileMessageRepository(string filePath)
    {
        FilePath = filePath;
    }

    public abstract IEnumerable<Message> GetMessages();

    protected Message BuildMessage(string line)
    {
        ...
    }
}

public class LazyFileMessageRepository : FileMessageRepository
{
    public LazyFileMessageRepository(string filePath) : base(filePath)
    {
    }

    public override IEnumerable<Message> GetMessages()
    {
        // This will read from the file as it is enumerated
        return File.ReadLines(FilePath).Select(BuildMessage);
    }
}

public class EagerFileMessageRepository : FileMessageRepository
{
    private Message[] _cachedMessages;

    public EagerFileMessageRepository(string filePath) : base(filePath)
    {
    }

    public override IEnumerable<Message> GetMessages()
    {
        // This will read from the file once (slow) and keep the result in memory
        return _cachedMessages ?? (_cachedMessages = File.ReadAllLines(FilePath).Select(BuildMessage).ToArray());
    }
}

With this implementation you are free to choose which IMessageRepository you use, and can carry out performance/profiling tests as required.


VBA lookup to complete matrix I have written the following the code to complete a matrix based on data provided in a second worksheet, but the code is really slow (note that s1=12,000, s2=40 and s3 = 200,000). Any suggestions on how to make this code faster?

Sub UpdateMatrix()

Dim wsOverview As Worksheet, wsData As Worksheet
Dim rngTable As Range
Dim varAccount As Variant, varData As Variant
Dim i As Long, t As Long

Set wsOverview = ThisWorkbook.Worksheets(1)
Set wsData = ThisWorkbook.Worksheets(2)

Set rngTable = wsOverview.Range("A:A")
i = Application.WorksheetFunction.CountA(rngTable) + 1

Set rngTable = wsData.Range("A:A")
t = Application.WorksheetFunction.CountA(rngTable)

For s1 = 2 To i
varAccount = wsOverview.Range("A" & s1).Value

For s2 = 1 To 37
varData = wsOverview.Range("A1").Offset(0, s2).Value

For s3 = 2 To t

    If varAccount = wsData.Range("B" & s3).Value And varData = wsData.Range("A" & s3).Value Then

    wsOverview.Range("A" & s1).Offset(0, s2).Value = wsData.Range("F" & s3).Value

    Exit For

    End If

Next s3

Next s2

Next s1

End Sub

I'm seeing several good practices in your code, you have a solid foundation to build upon. There are a few things to point out, but to answer your basic performance question -- process all your data in memory arrays. You'll see a tremendous performance improvement.

Several comments then, illustrated in the example code below:

Good declarations for your separate Worksheet variables; they are nicely descriptive.
Single-letter variables are not very descriptive (though a common practice is to restrict use of single-letter variables as loop indexes). So my suggestion is to rename these to ovRows and dataRows. These hold the number of rows detected on each sheet, so the variable name should echo that usage.
Use constants declared for fixed values. From what I can tell in your code, you have a limit to the number of columns on both sheets. Assuming this is a hard-coded value, declaring these as Const makes it easier to understand (and to change later if necessary).
Pull the overview and data ranges into local (memory-based) arrays for processing. This is the setup for the real speed.
Modify the loop to use the memory arrays. The example below is all-array, all the time. (If I've correctly understood your rows/columns logic.)
When the processing is completed, "write" the updated data back to the worksheet.

Thanks to @MacroMarc, I've updated the errors in my code (which was all done off the top of my head).

Option Explicit

Sub UpdateMatrix()
    Dim wsOverview As Worksheet, wsData As Worksheet
    Dim rngTable As Range

    Set wsOverview = ThisWorkbook.Worksheets("Sheet1")
    Set wsData = ThisWorkbook.Worksheets("Sheet2")

    Dim ovRows As Long
    Set rngTable = wsOverview.Range("A:A")
    ovRows = Application.WorksheetFunction.CountA(rngTable) + 1

    Dim dataRows As Long
    Set rngTable = wsData.Range("A:A")
    dataRows = Application.WorksheetFunction.CountA(rngTable)

    '--- set up memory based arrays
    Dim overviewRange as Range
    Dim overview As Variant
    Const OV_COL_LIMIT = 37
    set overviewRange = wsOverview.Range("A1").Resize(ovRows, COL_LIMIT)
    overview = overviewRange

    Dim dataRange As Range
    Dim data As Variant
    Const DATA_COL_LIMIT = 6
    set dataRange = wsData.Range("A1").Resize(dataRows, DATA_COL_LIMIT)
    data = dataRange

    Dim varAccount As Variant, varData As Variant
    Dim dataAcct As Variant, dataData As Variant
    For s1 = 2 To 3
        varAccount = overview(s1, 1)
        For s2 = 1 To COL_LIMIT
            varData = overview(1, s2)
            For s3 = 2 To dataRows
                dataAcct = data(s3, 2)
                dataData = data(s3, 1)
                If (varAccount = datraacct) And (varData = dataData) Then
                    overview(s1, 1) = data(s3, 6)
                    Exit For
                End If
            Next s3
        Next s2
    Next s1

    '--- put the data array back on the sheet
    overviewRange = overview
End Sub


Dynamic length binary data parsing I am currently writing code for a UART communication in my firmware. I am getting an interrupt every byte received (meaning I don't get a chunk of data, 1 byte at a time but, in the sample below I store them into a chunk of data for simulating my data parsing only).

I have a dynamic message length format, as shown in the code below. May I know what is the most efficient and elegant way to parse this message? I believe my current code could be further improved.

#include <stdint.h>

typedef struct {
    uint8_t header_DLE;
    uint16_t id;
    uint8_t msg_command;
    uint16_t msg_length;
    uint8_t data[400];
    uint8_t footer_DLE;
    uint8_t footer_BCC;
} Serial_Rx_Data;

Serial_Rx_Data _rx_data;
static int _rx_index;
static uint8_t _rawData[512];

int main()
{
    uint8_t data[18];

    //Message format
    data[0] = 0x10;     //DLE
    data[1] = 0x03;     //ID
    data[2] = 0;        //ID
    data[3] = 0x03;     //Command
    data[4] = 0x03;     //Length - determines the data length
    data[5] = 0;        //Length
    data[6] = 0x41;     //Data
    data[7] = 0x42;     //Data
    data[8] = 0x43;     //Data
    data[9] = 0x03;     //DLE
    data[10] = 0xfa;    //BCC

    //Simulate getting data every byte
    for (int i = 0; i <= 10; i++) {
        _rawData[_rx_index] = data[i];

        if (_rx_index == 5) {
            _rx_data.header_DLE = _rawData[0];
            _rx_data.id = _rawData[2] << 8 | _rawData[1];
            _rx_data.msg_command = _rawData[3];
            _rx_data.msg_length = _rawData[5] << 8 | _rawData[4];
        }

        //Get the length of data
        if ((_rx_index == 6) && (_rx_data.msg_length > 0)) {
            for (int j = 0; j < _rx_data.msg_length; j++) {
                _rx_data.data[j] = _rawData[6 + j];
            }
        }

        //Get the last 2 byte
        if (_rx_index == (_rx_data.msg_length + 6 + 1)) {
            _rx_data.footer_DLE = _rawData[6 + _rx_data.msg_length];
            _rx_data.footer_BCC = _rawData[6 + _rx_data.msg_length + 1];

            //Complete parsing
            //Callback to other function
        }

        _rx_index++;
    }

    return 0;
}

Here are some things that may help you improve the quality of your code.

Don't handle message parsing in an interrupt

You don't explicitly say it, but from the context, it appears that you're attempting to parse a serially received message in an interrupt handler.  This is usually not a good idea.  An interrupt handler should be fast and do the minimum amount of work possible.  For serial message protocols, this typically means reading each character, putting it into a memory buffer (most often implemented as a circular buffer) and updating pointers.  Then the main routine that is NOT in an interrupt reads the contents of the buffer and does any message parsing/handling.

Don't parse until you have a complete message

In this case the message length appears to be 8 bytes (6 header + 2 trailer) plus the size of the enclosed data. It also appears that there is some kind of special marker for the beginning of the message.  So a quick check for the message parser would be something like this:

are there at least 8 bytes?
is the first character the message marker byte?

If the answer to either question is "no," it can't be a real message and so the data may be left in the buffer until more is received.  If the answer to both questions is "yes," then it might be a message.  The next steps might be:

does the message have at least 8+msg_length bytes?
is the checksum correct?

If the answer to both questions is "yes" then you can actually act on the message (typically by passing it to some higher layer protocol handler).

Use a state machine

The typical way to parse messages like this is to use a state machine. In this case the states might be:

waiting for message marker byte
receiving header
receiving data
receiving trailer

There are also often states for error and for timeout.

Avoid the use of "magic numbers"

This code is littered with "magic numbers," that is, unnamed constants such as 400, 512, 10, 5, etc.  Generally it's better to avoid that and give such constants meaningful names.  That way, if anything ever needs to be changed, you won't have to go hunting through the code for all instances of "5" and then trying to determine if this particular 5 means an offset into the message structure or some other constant that happens to have the same value.

Beware of buffer overflow

The allocation of a fixed 400 bytes for data might be OK, but if so, then you absolutely need to make sure that the message will fit in that space before copying it.  Failure to do so leads to the possibility of buffer overflow.


What is the fastest way to record the names of each file in a directory? I want to fill an array of strings with the name of each directory item. The issue is allocating memory. I either need to count the items before, allocate memory, then go over them again, or I will have to reallocate each iteration of the while loop. Both of these options seem slow. Surely there's a better way.
Which is faster:
char **Names;
DIR *Dir = opendir("Some dir path");
struct dirent *Entry;
unsigned int Count = 0;
while ((Entry = readdir(Dir))) {
    Count++;
}
Names = malloc(Count*sizeof(char*));
closedir(Dir);
Dir = opendir("Some dir path"); //Also, is there a better way to reset it, other than closing it and reopening?
for (int i = 0; (Entry = readdir(Dir); i++) {
    unsigned short NamLen = strlen(Entry->d_name)+1;
    Names[i] = malloc(NameLen);
    memcpy(Names[i], entry->d_name, NameLen);
}

Or
char **Names;
DIR *Dir = opendir("Some dir path");
Dir = opendir("Some dir path");
struct dirent *Entry;
for (int i = 0; (Entry = readdir(Dir); i++) {
    Names = realloc(Names, i*sizeof(char*));
    unsigned short NamLen = strlen(Entry->d_name)+1;
    Names[i] = malloc(NameLen);
    memcpy(Names[i], entry->d_name, NameLen);
}
closedir(Dir);

Please help, I'm stuck. Both of these seem less efficient than optimal, so surely there's a better way. What's the fastest way to record all the names of items in a directory to a string array? Is there a faster option aside these two?

Calling realloc in every iteration makes the code really slow. Algorithmically it is in \$\mathcal O(n^2)\$ since realloc already has complexity \$\mathcal O(n)\$. Plus, you are calling it interleaved with malloc for the name of the directory entries.
The performance of your program depends very much on the specific memory allocator. It might be very fast or very slow, probably somewhere between \$n\$ and \$n^2\$.
To fix this, you should use the well-known pattern of having a triplet (pointer, length, capacity) to represent the growing buffer of directory entries.
Ideally you define a separate struct for this, together with an append function. That way, the function that reads the directory entries doesn't have to fiddle with realloc directly.


Circular Array Rotation My code works fine, but it's takes too much time, say around 65 seconds, for large input data. If there are 50,000 numbers in the list \$a\$ and we have to rotate the numbers 45,000 times then it takes 1 minute.

How can I reduce the time it takes to print all the numbers?

Problem:
perform an operation called a right circular rotation on an array of integers [a0 , a1 , a2 , ..a(n-1)]. After performing one right circular rotation operation, the array is transformed to [a(n-1) , a0 , a1 ,..a(n-2)].

Input Format

The first line contains 3 space-separated integers, \$n\$, \$k\$
and \$q\$ respectively.
The second line contains \$n\$ space-separated integers, where each
integer \$i\$ describes array element a[i] (where \$0 \le i<n\$).
Each of the \$q\$ subsequent lines contains a single integer denoting \$m\$.

n,k,q = input().strip().split(' ') #n=no of integers in the list , k=no of rotation ,q=no of lines for integer m

n,k,q = [int(n),int(k),int(q)]
a = [int(a_temp) for a_temp in input().strip().split(' ')]

for i in range(0,k):
    a=[a.pop(n-1)]+a  #for putting the last element to the first position and del it from the last pos

for a0 in range(q):    # printing value of a at a given index
  m = int(input().strip())
  print(a[m])

Sample Input

3 2 3
1 2 3
0
1
2

Sample Output

2
3
1

Explanation

After the first rotation, the array becomes [3, 1, 2].
After the second (and final) rotation, the array becomes [2, 3, 1].
Let's refer to the array's final state as array \$b\$. For each query, we
just have to print the value of b[m] on a new line:
\$m=0\$, so we print \$2\$ on a new line.
\$m=1\$, so we print \$3\$ on a new line.
\$m=2\$, so we print \$1\$ on a new line.

You can change your array packing to use map instead.

n, k, q = map(int, input().strip().split(' '))

When popping the last element you don't need to pass an argument.
Rather than popping each element, you can perform two slices and an addition. You would also have to use the modulo operator, %, for when \$k \ge n\$.

k %= n
a = a[-k:] + a[:-k]

You don't need to strip a number, as int will do this for you.

This can get you:

n, k, q = map(int, input().strip().split(' '))
a = list(map(int, input().strip().split(' ')))
a = a[-k:] + a[:-k]
for _ in range(q):
    print(a[int(input()])

If you however want some sugar you can use collections.deque, it comes with deque.rotate that can perform the rotation in (3) for you. which can result in:

from collections import deque

n, k, q = map(int, input().strip().split(' '))
a = deque(map(int, input().strip().split(' ')))
a.rotate(k)
for _ in range(q):
    print(a[int(input()])

The above is good enough for the hackerrank challenge, however performs worse than the code above it. Rotating the array is \$O(n)\$, where the deque is \$O(k)\$. But indexing the array is \$O(1)\$, where the deque is \$O(n)\$. This means the first code block is \$O(n + q)\$ and the second is \$O(k + nq)\$. To make the second \$O(n + q)\$ all you need to do is change it to an array before the for loop, via list.

whenever you profile code, you should always take more than one timing, with more than one input. Hai Vu done neither of these.
Using my wrapper to timeit, I wrote some code to profile; deque (Peilonrayz), adding slices (mkrieger), and deleting a slice (Hai Vu), as these are the fastest functions.
However to keep things unbiased I've made a couple of adjustments:

All code will be a Python function in __main__.
All functions work correctly with any \$k\$ and when \$n \ne 0\$. (I use % to do this.)
I run each function 10000 times per timeit.
I run each timeit 10 times, and take the average and interquartile range.

This should give each and every function call the same amount of overhead.
Here is the code that I use to time the functions.

from collections import deque

from graphtimer import time, GraphTimer, flat, time
import matplotlib.pyplot as plt

def mkrieger(a, k):
    k %= len(a)
    return a[-k:] + a[:-k]

def peilonrayz(a, k):
    a = deque(a)
    a.rotate(k)
    return a

def hai_vu(a, k):
    k %= len(a)
    b = a[-k:]
    del a[-k:]
    a[0:0] = b

class Timer(GraphTimer):
    functions = [
        'mkrieger',
        'peilonrayz',
        'hai_vu',
    ]
    inputs = [
        'a = list(range(100)); k = {}'
    ]
    domain = range(1, 101, 1)
    titles = [
        '100'
    ]

if __name__ == '__main__':
    fig, axs = plt.subplots(nrows=1, ncols=1, sharex=True)
    Timer(amount=10, number=10000, stmt='fn(a, k)').plot_axes(flat(axs), show_titles=True)
    plt.show()

Which outputs:


Object-oriented RPG in Java, determining the data of a generated location I've been coding for around a month, learning through CodeHS and coding within their sandbox tool. I am currently working on a text-based explorer/RPG game in order to practice Object-Oriented Programming in Java.

I wanted to know if the current relationship between the Location and the Explore class is optimal, as I am not sure if accessing the instance variables of Location within Explore is considered to be bad practice. Also, my current method of determining whether or not a generated location is for example a stream seems inefficient, as it simply checks if it contains the strings "stream" or "river". Any help with design is greatly appreciated as well.

The Explore Class:

public class Explore{

    public Location location;
    public Player player;
    //distances from the town originated and to next town
    public int distanceFrom;
    public int distanceTo;

    public Explore(Player player){
        this.player = player;
        this.distanceFrom = 0;
        this.distanceTo = RNG.range(3, 6);
        this.location = new Location(player);
    }
    public int getDistanceFrom(){
        return distanceFrom;
    }
    public int getDistanceTo(){
        return distanceTo;
    }

    public boolean isStream(){
        return location.isStream;
    }
    public boolean canDrink(){
        return location.canDrink;
    }
    public void drink(){
        location.canDrink = false;
    }

    public String getDesc(){
        return location.toString() + " " + distanceFrom + " km away from your origin";
    }
    public Location getLocation(){
        return location;
    }

    public void advance(){
        if(distanceFrom <= 5){
            player.incrementDev(RNG.range(1, 4));
        }else{
            player.incrementDev(RNG.range(3, 6));
        }

        this.location = new Location(player);
        this.distanceFrom++;
        this.distanceTo--;
    }

    public boolean checkNearbyTown(){
        if(distanceTo <= 1 && distanceTo > -1){
            return true;
        }else{
            return false;
        }
    }
}

The Location Class:

public class Location
{
    private Player player;
    private String desc;

    public boolean isStream = false;
    public boolean isCave = false;
    public boolean canDrink = false;
    //add caves later
    //public boolean canCave = false;

    public Location(Player player){
        this.player = player;
        this.desc = RNG.locationDesc(player);

        if((this.desc.indexOf("stream") != -1) || (this.desc.indexOf("river") != -1)){
            this.isStream = true;
            this.canDrink = true;
        }
    }

    public String toString(){
        return desc;
    }
}

The RNG Class, which handles my games random events:

import java.util.*;

public class RNG{

    //Handles all random events such as pulling a number from a range or generating a name/stats
    public static int roll(int sides){
        int aRoll = (int)(Math.random() * sides + 1);
        return aRoll;
    }
    public static int range(double min, double max){
        return (int)(Math.random() * (max - min + 1) + min);
    }

    public static String locationDesc(Player player){
        int dev = player.getDev();
        //takes in a player input in order to determine their insanity, making a more twister are
        String[] desc;
        if(dev <= 75){
            desc = new String[]{"an unassuming wooded area", "an open flower field",
                "a steady stream", "the entrance of a small cave", "a small river"};
        }else{
            desc = new String[]{"Spookier areas to add later"};
        }

        int index = (int)(Math.random() * desc.length);
        return desc[index];
    }
}

Explore is a verb, so your Explore class should be called Exploration, or probably something more descriptive like PlayerStatus. And you are defining state in Location and then just re-exposing it in Explore. Merge the two classes if you notice they are bleeding. You are right to be questioning that and noticing something isn't right. What is the point of Explore? Merge it into Location.


Selecting the correct price What are your opinions on my JavaScript function? Can my code below be refactored?

var updatePrice = function () {
    var goPrice;
    var returnPrice;
    if (typeof scope.selectedGo !== 'undefined' && typeof scope.selectedReturn !== 'undefined') {
        for (var go in scope.gooffers) {
            if (scope.gooffers[go].tarifPremiere !== null && typeof scope.gooffers[go].tarifPremiere !== 'undefined' && scope.selectedGo === scope.gooffers[go].tarifPremiere.id) {
                goPrice = scope.gooffers[go].tarifPremiere.tarif;
            } else if (scope.gooffers[go].tarifSecondeFlex !== null && typeof scope.gooffers[go].tarifSecondeFlex !== 'undefined' && scope.selectedGo === scope.gooffers[go].tarifSecondeFlex.id) {
                goPrice = scope.gooffers[go].tarifSecondeFlex.tarif;
            } else if (scope.gooffers[go].tarifSecondeNoFlex !== null && typeof scope.gooffers[go].tarifSecondeNoFlex !== 'undefined' && scope.selectedGo === scope.gooffers[go].tarifSecondeNoFlex.id) {
                goPrice = scope.gooffers[go].tarifSecondeNoFlex.tarif;
            }
            if (scope.returnoffers[go].tarifPremiere !== null && typeof scope.returnoffers[go].tarifPremiere !== 'undefined' && scope.selectedReturn === scope.returnoffers[go].tarifPremiere.id) {
                returnPrice = scope.returnoffers[go].tarifPremiere.tarif;
            } else if (scope.returnoffers[go].tarifSecondeFlex !== null && typeof scope.returnoffers[go].tarifSecondeFlex !== 'undefined' && scope.selectedReturn === scope.returnoffers[go].tarifSecondeFlex.id) {
                returnPrice = scope.returnoffers[go].tarifSecondeFlex.tarif;
            } else if (scope.returnoffers[go].tarifSecondeNoFlex !== null && typeof scope.returnoffers[go].tarifSecondeNoFlex !== 'undefined' && scope.selectedReturn === scope.returnoffers[go].tarifSecondeNoFlex.id) {
                returnPrice = scope.returnoffers[go].tarifSecondeNoFlex.tarif;
            }
        }
       if (goPrice !== null && typeof goPrice !== 'undefined' && returnPrice !== null && typeof returnPrice !== 'undefined') {
                    scope.finalPrice = goPrice + returnPrice;
        }
    }
};

Variable naming is very important for readability.
Now, as some background, when I first saw your question, it had the title:

What are your opinions on my javascript function?

Titles like that are an poor choice for code review, and as a moderator, we actively change titles like that to reflect what the code does, and not what the question asker sees as being the problems in the code.
Unfortunately, when I read your code, to try to see what it does, all I could think of was goofers... it took me a few moments to understand that the code was meant to be "go offers".
camelCase to the rescue...

gooffers should be goOffers
scope.returnoffers should be scope.returnOffers

Apart from those two, all your other variables are correcly cased. The following are good:

tarifSecondeFlex
tarifPremiere
scope.selectedGo
goPrice
etc.

Note, I believe you have mixed some natural language variable names in there. Is that French? In general, when programming, you should pick a language, and stick to it. You have a mix. Since you have goPrice and selectedGo which appear to be English, I would expect tarifPremiere to be primaryPrice, etc.
Language consistency goes a long way toward readability.
DRY code
Repetitive code is often an indication that function-extraction would be useful.
Consider a function like:
function selectedTarif(component, selection) {
    if (component == null || typeof component == 'undefined' || component.id !== selection) {
        return null;
    }
    return component.tarif;
}

Then, your code can become a cascading 'falsy' test:
var updatedPrice = selectedTarif(scope.goOffers[go].tarifPremiere, scope.selectedGo)
            || selectedTarif(scope.goOffers[go].tarifSecondeFlex, scope.selectedGo)
            || selectedTarif(scope.goOffers[go].tarifSecondeNoFlex, scope.selectedGo);
if (updatedPrice) {
    goPrice = updatedPrice;
}

var updatedReturn = selectedTarif(scope.returnOffers[go].tarifPremiere, scope.selectedReturn)
            || selectedTarif(scope.returnOffers[go].tarifSecondeFlex, scope.selectedReturn)
            || selectedTarif(scope.returnOffers[go].tarifSecondeNoFlex, scope.selectedReturn);
if (updatedReturn) {
    returnPrice = updatedReturn;
}


Lazy Init if the Items property is null I was refactoring a quite large WPF project and I found some pieces of code where it can leads to error.

Ive got some services in the form

 public sealed class CurrenciesService : ContainerServiceBase<Currency>, ICurrenciesService
 {
    public override Currency GetItem(int id)
    {
        return GetItemInternal(x => x.Id == id);
    }

    public override Task Init()
    {
        return PerformInitInternal(InitCacheMessages.STR_GET_COUNTERPARTS,
      InitCacheMessages.STR_GET_COUNTERPARTS_FAILED,()=> CommonRepository.GetCurrenciesAsync());
    }
 }

where the ContainerServiceBase is something as

 public class ContainerServiceBase<T> : IContainerBase<T>
    where T : class
{
    #region Variables
    protected readonly IExceptionService ExceptionService = ServiceLocator.Default.ResolveType<IExceptionService>();
    protected readonly ICommonRepository CommonRepository = ServiceLocator.Default.ResolveType<ICommonRepository>();
    public delegate void ItemsChangedHandler(object sender, EventArgs e);

    public event ItemsChangedHandler OnItemsChanged;

    #endregion

    public virtual T GetItem(int id)
    {
        throw  new NotImplementedException();
    }

    public virtual Task Init()
    {
        return TaskEx.WhenAll();
    }

    public IList<T> Items { get; protected set; }

    protected async Task PerformInitInternal(string okMessage, string koMessage, Func<Task<IList<T>>> function)
    {
        try
        {
            var result = await function();
            if (result != null)
            {
                Items = result;
                LogTo.Information(okMessage, result.Count);

                OnItemsChanged?.Invoke(this, new EventArgs());
            }
            else
            {
                LogTo.Warning(koMessage);
            }
        }
        catch (Exception ex)
        {
            ExceptionService.HandleException(ex);
        }
    }

    protected T GetItemInternal(Func<T, bool> condition)
    {
        if (Items == null) throw new NullReferenceException("Items has not been initialized");

        var result = Items.FirstOrDefault(condition);

        return result;
    }

Currently during the Application's start I perform for each service this

     var taskPortolio = portfoliosService.Init();
        var taskInstitutes = institutesService.Init();
        var taskStatus = statusService.Init();
        var taskCounterparts = counterpartsService.Init();
        var taskCrosses = crossesService.Init();
        var taskCurrencies = currenciesService.Init();
        var taskSigns = signsService.Init();
        var taskCrossSplit = crossSplitService.Init();
        var taskDateMM = dateMmService.Init();
        var taskSignsFx = signsFxService.Init();
        var taskPortfolioTree = portfolioItemService.Init();
        var taskTags = tagService.Init();
        var taskInstitutesCustomerDesk = institutesCustomerDeskService.Init();
        var taskProvenances = provenanceService.Init();
        var taskRepoClasses = repoClassService.Init();
        var taskDatesMaturity = dateMaturityService.Init();

Those methods are initialized even if they're never used.

I was writing a unit test in the form

  [Test]
    public async Task CurrenciesLoaded()
    {
        var viewmodel = viewModelFactory.CreateViewModel<HistoricalDetailPRTViewModel>(null, null);

        var res = viewmodel.InitializeViewModelAsync();

        await res;

        Assert.NotNull(viewmodel.Currencies);
        Assert.IsTrue(viewmodel.Currencies.Count>0);

    }

and correctly I get null at viewmodel.Currencies since no CurrenciesService.Init has been called.

Can I put a check in the Item.getter to check if it's called the Init and if not call it? is this evil? consider that those init are now called async and I wish not to block the UI while it's loading

P.S. I'm using Catel as MVVM Framework

From what I can see you are calling an init function on every service so you can get data and cache it at startup.

A better way to do this would be to use the dependency injection layer for example using unity

    builder.RegisterType<CurrenciesService>()
        .As<ICurrenciesService>()
        .AutoActivate()
        .OnActivating(x => x.Instance.Init())
        .SingleInstance();

So this code activates your Currency Service, auto activates it when the container is building, when it gets activated calls init and puts it in single instance. Single instance is needed otherwise the init will be called on every request.


Non repeating character in Java Given a string s consisting of lowercase Latin Letters, find the
  first  non repeating character in s.

  Input:

  The first line contains T denoting the number of testcases. Then
  follows description of testcases. Each case begins with a single
  integer N denoting the length of string. The next line contains the
  string s.

  Output:

  For each testcase, print the first non repeating character present in
  string.    Print -1 if there is no non repeating character.

  Constraints:

1<=T<=50
1<=N<=100

  Example:

  Input :

3
5
 hello
12
zxvczbtxyzvy
6
xxyyzz

  Output :

h
c
-1

My approach:

/*package whatever //do not write package name here */

import java.io.InputStreamReader;
import java.io.IOException;
import java.io.BufferedReader;
import java.util.*;

class GFG {
    private static int firstNonRepNum (String str, int size)
        {
            Hashtable <Character, Integer> count = new Hashtable<>();
            int occurs;

            for (char ch: str.toCharArray())
                {
                    if (!count.containsKey(ch))
                        {
                            count.put(ch,1);
                        }
                    else
                        {
                            occurs = count.get(ch);
                            count.put(ch, occurs+1);
                        }
                }

           for (char ch: str.toCharArray())
            {
                int val = count.get(ch);

                if (val == 1)
                    {
                        return ch;
                    }
            }

        return -1;
        }

    public static void main (String[] args) throws IOException{
        //code
        //BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
        //String line = br.readLine();
        Scanner sc = new Scanner(System.in);

        int numTests = sc.nextInt();

        String line2;
        int size;
        String str;

        for (int i = 0; i < numTests; i++)
            {

                size = sc.nextInt();
                str = sc.next();
                if (firstNonRepNum(str,size) == -1)
                    {
                        System.out.println("-1");
                    }
                else
                    {
                        System.out.println((char)firstNonRepNum(str, size));
                    }

            }
    }
}

I have the following questions with regards to the above code:

How can I further improve my approach?
Is there a better way to solve this question?
Are there any grave code violations that I have committed?
Can space and time complexity be further improved?

Reference

Your algorithm is correct. There is no way to reduce time or space complexity, so I will give some less important suggestions.

because you know that there will only be latin letters, we can use an array instead of a hashtable and avoid the overhead. You can decide for yourself if this approach is explicit enough.

int[] counts = new int[26];
counts[ch - 'a'] += 1

firstNonRepNum() has an argument size, that it does not use.
Your indentation, while mostly consistent, is very odd. It ends up with 8 chars of indent each level. Read some style guides for other ways of indenting. In java most people indent like this.

public void func(int[] arg) {
    for (int i; arg) {
        if (i % 2) {
            even();
        } else {
            odd();
        }
    }
}

if a variable is only used in a certain scope, declare it in that scope. This helps people who are reading your code because they dont have to search for what count does.

{
    int occurs = count.get(ch);
    count.put(ch, occurs+1);
}

You call firstNonRepNum() twice. Once to check if there is a solution and once to get the result. Call it once and put the result in a variable.
Remove any unused variable declarations (i.e. String line2) or commented out code as soon as you are sure you don't need it.


Are there any improvements I can make to my implementation of the strategy pattern? I'm not sure about that if else statement in function Change().  I'm guessing there is a way to eliminate this and make better use of OOP principles.  Does anyone have any suggestions?

PS This code is functional and produces the desired result.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace ConsoleApplication1_testStrategy
{
    class Program
    {
        static void Main(string[] args)
        {
            string message = "mytext";

            Changer chng = new Changer();

            //THis is how I want the usage, I want the user to specify the string
            //and also which algorithm to invoke by specifyin the enumeration type
            string messageModified = chng.Change(message, types.type2);
        }
    }

    //ENUMERATION OF TYPES
    public enum types
    {
        type1,
        type2
    }

    //Abstact strategy
    public interface IStringModifier
    {
        string Modify(string input);
    }

    //concrete strategy 1
    class type1 : IStringModifier
    {
        public string Modify(string input)
        {
            return "hello1";
        }
    }

    //conctrete strategy 2
    class type2 : IStringModifier
    {
        public string Modify(string input)
        {
            return "hello2";
        }
    }

    //MAIN API I WANT USER TO ACCESS
    public class Changer
    {
        public Changer()
        {
        }

        private IStringModifier _stringModifier;

        public void SetStringModifier(IStringModifier stringModefier){

            this._stringModifier = stringModefier;
        }

        public string Change(string text, types type) //not sure about this part
        {
            if (type == types.type1)
            {
                SetStringModifier(new type1());
            }
            else if (type == types.type2)
            {
                SetStringModifier(new type2());
            }

            return _stringModifier.Modify(text);
        }

    }
}

You can do this more elegantly by specifying a Func delegate instead of an enum and a set of interfaces.

class Program
{
    static void Main(string[] args)
    {
        string message = "mytext";

        Changer chng = new Changer();

        string messageModified = chng.Change(message, Strategy2);
    }

    // These methods have a Func<string,string> signature
    // i.e. accept string as parameter and return a string
    static string Strategy1(string input)
    {
        return "hello1";
    }
    static string Strategy2(string input)
    {
        return "hello2";
    }
}

public class Changer
{
    public Changer()
    {
    }

    private Func<string,string> _stringModifier;

    public void SetStringModifier(Func<string,string> stringModifier)
    {
        this._stringModifier = stringModifier;
    }

    public string Change(string text, Func<string,string> stringModifier)
    {
        SetStringModifier(stringModifier); // why?

        return _stringModifier(text);
    }
}

  what was meant by the comment //why?

I would have thought that one of the following might be better.

Either, set the strategy once and use it repeatedly, like this ...

public class Changer
{
    public Changer()
    {
    }

    private Func<string,string> _stringModifier;

    public void SetStringModifier(Func<string,string> stringModifier)
    {
        this._stringModifier = stringModifier;
    }

    public string Change(string text)
    {
        return _stringModifier(text);
    }
}

... or like this ...

public class Changer
{
    public Changer(Func<string,string> stringModifier)
    {
        this._stringModifier = stringModifier;
    }

    private Func<string,string> _stringModifier;

    public string Change(string text)
    {
        return _stringModifier(text);
    }
}

Or, pass the strategy into the Change method every time, in which case there's no need to store it in as member data, like this ...

public class Changer
{
    public Changer()
    {
    }

    public string Change(string text, Func<string,string> stringModifier)
    {
        return stringModifier(text);
    }
}

  Thank you, another question is, wouldn't this allow the user to just write their own strategy and pass it to chng.Change() method?

Yes it would. That's often a good thing: it makes the class more 'flexible', so that it can be used in ways you hadn't imagined.

  And if my end goal is to make this an API, is that acceptable? Should I restrict them like I did in my original post using the enumeration?

I don't know. If you want to keep the enum, you could still use a Func delegate instead of an interface with several subclasses (and use a switch statement instead of repeated if/else).

public class Changer
{
    public Changer()
    {
    }

    private Func<string,string> _stringModifier;

    public void SetStringModifier(Types type)
    {
        switch (type)
        {
            case Types.Type1:
                this._stringModifier = Strategy1;
                break;
            case Types.Type2:
                this._stringModifier = Strategy2;
                break;
            default:
                throw new NotImplementedException();
    }

    public string Change(string text)
    {
        return _stringModifier.Modify(text);
    }

    static string Strategy1(string input)
    {
        return "hello1";
    }
    static string Strategy2(string input)
    {
        return "hello2";
    }
}


Prettify math formula in code I have a function to calculate the normal distribution in Python:

def norm_cdf(z):
  """ Use the norm distribution functions as of Gale-Church (1993) srcfile. """
  # Equation 26.2.17 from Abramowitz and Stegun (1964:p.932)

  t = 1.0 / (1+0.2316419*z) # t = 1/(1+pz) , p=0.2316419
  probdist = 1 - 0.3989423*math.exp(-z*z/2) * ((0.319381530 * t)+ \
                                         (-0.356563782* math.pow(t,2))+ \
                                         (1.781477937 * math.pow(t,3)) + \
                                         (-1.821255978* math.pow(t,4)) + \
                                         (1.330274429 * math.pow(t,5)))
  return probdist

But I have to adhere to PEP8 and 80 char margin, hence the ugly \s. How else should I prettify my code?

In mathematical form,

$$\begin{align*}
\textrm{norm_cdf}(z) = 1 - 0.3989423 e^\frac{-z^2}{2} (&1.330274429 t^5 - 1.821255978 t^4 \\
&+ 1.781477937 t^3 - 0.356563782 t^2 + 0.319381530 t )
\end{align*}$$

where

$$ t = \frac{1}{1 + 0.2316419 z}$$

Let me quote the wonderful book Numerical Recipes in C++ (but also applicable):

We assume that you know enough never to evaluate a polynomial this way:
p=c[0]+c[1]*x+c[2]*x*x+c[3]*x*x*x+c[4]*x*x*x*x;

or (even worse!),
p=c[0]+c[1]*x+c[2]*pow(x,2.0)+c[3]*pow(x,3.0)+c[4]*pow(x,4.0);

Come the (computer) revolution, all persons found guilty of such criminal behavior will be summarily executed, and their programs won't be!

(You can find the page in your edition in the analytical index, under the entry "puns, particularly bad". I love this book.)
There are two reasons not to do that: accuracy and performance. The correct way to evaluate the polynomial is like this:
-t * (0.319381530  +  t * (-0.356563782 + t * (1.781477937 + t * (-1.821255978 + 1.330274429 * t))))

And you can, of course, split at your convenience, as the newlines inside parenthesis are ignored. Remember the PEP: " The preferred place to break around a binary operator is after the operator, not before it."
-t * (0.319381530  +  t * (-0.356563782 +
    t * (1.781477937 + t * (-1.821255978 + 1.330274429 * t))))

Another alternative is to save the coefficients in a list:
coeff = [0, 0.319381530, -0.356563782, 1.781477937, -1.821255978, 1.330274429]
poly = coeff[-1]
for c in coeff[-2::-1]:
    poly *= x
    poly += c

I am doing operations in place to avoid allocating and deallocating memory, but this is only relevant if x is a NumPy array. If you are evaluating on a single number, you can just use instead the nicer expression:
poly = poly * x + coeff[i]

But I would stick with the first one because it is more general.
Of course, the result has to be multiplied by the prefactor:
return 1 - 0.3989423*math.exp(-z*z/2) * poly

Or, if you want to do it in place:
z2 = z * z # Be careful not to modify your input!
z2 *= 0.5  # Multiplication is faster than division.
np.exp(z2, out=z2)

probd = z2 * poly
probd *= -0.3989423
probd += 1
return probd

Bonus track:
If you are applying this function to large arrays (more than a thousand numbers), you may benefit from using the first technique in numexpr:
expr += '1 - 0.3989423* exp(-z * z / 2) * '
expr += '(-t * (0.319381530  +  t * (-0.356563782 +  t * '
expr += '(1.781477937 + t * (-1.821255978 + 1.330274429 * t)))))'
ne.evaluate(expr)

This will compile the expression for you and transparently use as many cores as you have.


Find the smallest element in a sorted and rotated array I was solving a question asked in interview by Uber. It says:

  Suppose an array sorted in ascending order is rotated at some pivot unknown to you beforehand. Find the minimum element in \$O(\log n)\$ time. You may assume the array does not contain duplicates.

#include <iostream>

int find_smallest(int a[], int l, int r){
    if (l ==r)
        return a[l];
    else if (l+1 == r)
        return ((a[l] < a[r]) ? a[l] : a[r]);
    else {
        int m = (l + r) / 2;
        int d1 = find_smallest(a, l, m);
        int d2 = find_smallest(a, m+1, r);
        return  ((d1 < d2) ? d1 : d2);
    }
}

int main(){
    int a[] = {5,3,2,5,6,7};
    std::cout << find_smallest(a, 0, 5);
}

Please ignore the hardcoding of values. It was just for testing.
Also, is the runtime of the code \$O(\log n)\$?

The input, 1) which doesn't contain duplicates, has been 2) sorted, 3) rotated:

int a[] = {5,3,2,5,6,7};
// removing duplicates gives
int a[] = {5,3,2,6,7};
// sorting gives
int a[] = {2,3,5,6,7};
// rotating gives (for instance):
int a[] = {5,6,7,2,3};

log(n) suggests a divide-and-conquer strategy like binary-search, as you found out by yourself. But what are you looking for in this case? Not a value, but the position where a[n] > a[n+1]. Notice that if a[n] > a[n+1], then also a[0] > a[n+1]: the "direction change" occurred in that range. If not, it occurred in the range [a[n+1], a[len(a)]).

Divide-and-conquer is trickier than it may seem because it's really easy to access the array out-of-bounds. Iterator interfaces likes those of the C++ standard library make it easier and clearer:

template <typename Iterator>
Iterator find_partition_point(Iterator first, Iterator last) {
    if (first == last) return last; // empty array
    if (std::next(first) == last) return first; // one-value array
    if (*first < *std::prev(last)) return first; // null/full rotation
    // so we have at least two elements and a change of direction
    auto pivot = first + std::distance(first, last) / 2;
    if (*pivot < *first) { // direction change in [first, pivot]
        if (*std::prev(pivot) > *pivot) return pivot;
        return find_partition_point(first, pivot);
    }
    return find_partition_point(std::next(pivot), last); // direction change in (pivot, last)
}


Largest common multiple equal or lower than max I made this small and easy function that returns the largest common multiple of the elements of ms that is equal to or lower than max:

LCMLthan = function(ms,max) {
    ms = sort(ms, decreasing =TRUE)
    max2 = c()
    for (m in ms)
    {
        max2=append(max2,m*floor(max/m))
    }
    max2=min(max2)
    while (T)
    {
        maxOld = max2
        for (m in ms)
        {
            if(max2%%m!=0){
                max2 = max2 - 1
                break
            }
        }
        if (maxOld==max2){
            return (max2)
        }
    }
}

### Examples
LCMLthan(c(3,5),52)
[1] 45
LCMLthan(c(178,124,17),520000)
[1] 375224

Looking at my code, it feels like there must have a cleaner and more efficient way to code this function. There is also probably a better way to name it too.

Looping is generally not recommended in R.
And it's not really needed here.

As @vnp pointed out in a comment,
all you need is simply calculate the least common multiple of the values in the input vector, let's call it lcm, and apply the formula lcm * (max // lcm) (with integer division).

The pracma library already has an implementation of the least common multiple. If installing that library is not an option for you, you can use this instead:

gcd <- function(a, b) {
  stopifnot(is.numeric(a), is.numeric(b))
  if (a > b) gcd(b, a)
  else if (a == 0) b
  else gcd(b %% a, a)
}

Lcm <- function(a, b) {
  stopifnot(is.numeric(a), is.numeric(b))
  a / gcd(a, b) * b
}

To calculate the least common multiples for a vector of numbers:

Lcm.many = function(values) {
  stopifnot(values)
  if (length(values) == 2) {
    Lcm(values[1], values[2])
  } else {
    Lcm.many(c(Lcm(values[1], values[2]), tail(values, -2)))
  }
}

Then with these helper functions the implementation of your function becomes much simpler:

LCMLthan = function(ms, max) {
  lcm <- Lcm.many(ms)
  lcm * (max %/% lcm)
}

Update

As @flodel pointed out in a comment,
there's actually no need for Lcm.many at all,
the call to it can be simply replaced with Reduce(Lcm, ms):

LCMLthan = function(ms, max) {
  lcm <- Reduce(Lcm, ms)
  lcm * (max %/% lcm)
}


Map strings to (const) ints I'm imagining go generate would be a good tool for this. I want to convert strings to ints to save space.

type Source int

func NewSource(s string) Source {
    switch s {
    case "Twitter":
        return Twitter
    case "Facebook":
        return Facebook
    case "Gplus":
        return Gplus
    case "Spotify":
        return Spotify
    case "Linkedin":
        return Linkedin
    case "Github":
        return Github
    case "Lastfm":
        return Lastfm
    default:
        panic(ErrUnknownSourceType)
    }
}

const (
    Twitter Source = iota
    Facebook
    Gplus
    Spotify
    Linkedin
    Github
    Lastfm
)

Shortest (shortest by you) would be indeed to use go generate. If you don't want to do that:

You have to enumerate your source names and source values to associate them, you can't avoid that.

But this enumeration and pairing can be shorter by using a map[string]Source:

var srcMap = map[string]Source{
    "Twitter":  Twitter,
    "Facebook": Facebook,
    "Gplus":    Gplus,
    "Spotify":  Spotify,
    "Linkedin": Linkedin,
    "Github":   Github,
    "Lastfm":   Lastfm,
}

func NewSource(s string) Source {
    if src, ok := srcMap[s]; ok {
        return src
    }
    panic(ErrUnknownSourceType)
}

Also note that panicing is a little "strong" reaction for an invalid source name. I would rather return an error along with the source, or return a special UnknownSrc source instead of panicing.

And while we're at it: you should exploit the zero value of Source for representing UnknownSrc and that way you don't even have to use the comma-ok idiom when checking in the map: indexing a map returns the zero value of the value type if the key is not found.

So:

const (
    UnknownSrc Source = iota // It will be 0, zero value for the underlying type (int)
    Twitter
    // ... and your other sources
)

And this way converting a source name to the Source type is one-line:

func NewSource(s string) Source {
    return srcMap[s]
}

It's just indexing a map, you don't even need a function for that.

If you would want to return an error, it could look like this:

func NewSource(s string) (Source, error) {
    if src, ok := srcMap[s]; ok {
        return src, nil
    }
    return UnknownSrc, errors.New("Invalid Source name!")
}


Getting rid of HTML file inputs once and far all They've always annoyed me, and as a matter of principle, would like to leave my HTML/CSS intact and have the various file upload plugins work with consistent behavior with any styled element.

The following script places a transparent file input over a selected input.  Note that solutions such as triggering an upload script based on another element being clicked are not supported by all browsers, so please do not recommend doing so. One deficiency of my current solution is that cursor pointer doesn't work with Chrome.  Another is that I have not addressed border radius on the new element, and will consider doing to in the future.  I am also sure there are other changes that should be made.  Please advise of recommended changes.

See https://jsfiddle.net/4noentrp/2/ for a working example.

PS.  I would think all file upload plugins should default to similar functionality.  Agree?

jQuery.fn.extend({
    makeInput: function (o) {
        var arr = [];
        this.each(function () {
            var $this = $(this);
            if ( $this.is( "input:file" ) ) {var elem=$this;}
            else {
                var position = $this.position();
                var elem = $('<input/>', $.extend({},o,{
                    type: 'file',
                    style: 'height: ' + $this.outerHeight() + 'px;width: ' + $this.outerWidth() + 'px;top: ' + position.top + 'px;left:' + position.left + 'px;margin-top:' + $this.css('marginTop') + ';position:absolute;cursor:pointer;z-index:9999;opacity:0;filter: alpha(opacity=0)'
                    }
                )).insertAfter(this);
            }
            arr.push(elem[0]);
        });
        return $(arr);
    }
});

$('.makeInput').makeInput({name:'bla'}).fileupload({
    add: function (e, data) {
        alert(data.originalFiles[0].name+' submitted');
    }
});

Related HTML and CSS is shown below, however, the primary script is the JavaScript located above.

bla bla blainlinebla bla bla

block

float

position-static

position-absolute

position-relative

a {
    border: 1px solid black;
}
p {
    width: 120px;
    border: 1px solid black;
}
#float {
    float:right;
}
#position-absolute {
    position: relative;
    left: 30px;
    top: 80px;
}
#position-relative {
    position: relative;
    left: 30px;
    top: 80px;
}
#position-static {
    position: static;
}

I know my answer comes a bit late, but I thought I'll share. I don't really get the problem you're facing. What's wrong with doing the regular styling of label, as in my example?

Answering your questions:

  @CᴴᵁᴮᴮʸNᴵᴺᴶᴬ Does it work for all browsers?

<label> was introduced in HTML4, even IE6+ can support it.

  My reason for creating this script is to allow the buttons on a jQueryUI dialog to work as file upload triggers (...)

Don't use JavaScript (and especially it's libraries) for elements and functionalities that you can create with pure (X)HTML and CSS. But if you have to do the file upload with a jQuery UI buttons or any other element and you don't want to create and style a <label>, you can simply do:

var fileInput = document.querySelector('input[type="file"]');

document.querySelector('input[type="button"]').onclick = function() {
  fileInput.click();
}
fileInput.onchange = function() {
  this.form.submit();
};

Where input[type="file"] bear display: none and input[type="button"] can be any element that we'd like to trigger file upload action. Take a peak into my working example.


Cookie value to enum Users can have a Cookie called lang that contains either the value "default=1" (English) or "default=2" (French). This cookie is used to determine their preferred language. I have a function that needs to take the cookie and return the language has an Enum. If the cookie doesn't exist, I want to return English. I cannot change the cookie value.

Language enum:

  Public Enum Language
     Undefined = 0
     English = 1
     French = 2
  End Enum

Function:

  Public Function GetLangFromCookie() As Language
     If HttpContext.Current.Request.Cookies.Get("lang") IsNot Nothing Then
        Dim langCookie = HttpContext.Current.Request.Cookies.Get("lang").Value
        Return If(langCookie(langCookie.Length - 1) = "1", Language.English, Language.French)
     End If
     Return Language.English
  End Function

I feel like this is not efficient and not secure but I don't know how to improve it. Any suggestions?

You query HttpContext.Current.Request.Cookies.Get("lang") twice. Access it once and store it in a variable.
You will get a problem if in the future you have to support more than 9 languages. Instead of accessing the last char from langCookie you should Split() langCookie by = and take the second array element. You can then use [Enum].TryParse() to get the enum.
Some horizontal spacing (new lines) would help to easier read the code.

Your code could look like so

Public Function GetLangFromCookie() As Language

    Dim langCookie = HttpContext.Current.Request.Cookies.Get("lang")

    If langCookie Is Nothing Then
        Return Language.English
    Else
        Return GetLanguage(langCookie.Value)
    End If

End Function

Private Function GetLanguage(languageCookieValue As String) As Language

    Dim splittetCookieValue As String() = languageCookieValue.Split(New String() {"="}, StringSplitOptions.RemoveEmptyEntries)

    Dim languageIdentifier As String = splittetCookieValue.LastOrDefault()

    Dim foundLanguage As Language

    ' We don't care about the success of the TryParse() call
    [Enum].TryParse(Of Language)(languageIdentifier, foundLanguage)

    ' because foundLanguage will be 0 if it doesn't succeed and will be checked here
    Return If(foundLanguage = Language.Undefined, Language.English, foundLanguage)

End Function


Core Data model for test-taking iOS app I'm building an iOS app for test-taking and I want to be sure of my model before proceeding.

I found this post very helpful and tried to implement a simplified version for Core Data.

Here are some of my assumptions:

Each User can take multiple tests
Each User has one set of answers per Test taken
Each Test has one User
Each Question has many answers

Here is my Core Data model:

Overall, what do you think of my database schema? Is it sufficient to handle a simple test-taking app?

Also, are Test_Questions and User_Answers necessary?

I could, in theory, have a relationship directly between Test andQuestion? Test -->> Question. I would like to know which is better.

Here are my model headers for Test_Questions and User_Answers:

Test_Questions.h

@class Question, Test;

@interface Test_Questions : NSManagedObject

@property (nonatomic, retain) Test *test;
@property (nonatomic, retain) NSSet *questions;
@end

@interface Test_Questions (CoreDataGeneratedAccessors)

- (void)addQuestionsObject:(Question *)value;
- (void)removeQuestionsObject:(Question *)value;
- (void)addQuestions:(NSSet *)values;
- (void)removeQuestions:(NSSet *)values;

@end

User_Answers.h

@class Answer, User;

@interface User_Answers : NSManagedObject

@property (nonatomic, retain) User *user;
@property (nonatomic, retain) NSSet *userAnswersSet;
@end

@interface User_Answers (CoreDataGeneratedAccessors)

- (void)addUserAnswersSetObject:(Answer *)value;
- (void)removeUserAnswersSetObject:(Answer *)value;
- (void)addUserAnswersSet:(NSSet *)values;
- (void)removeUserAnswersSet:(NSSet *)values;

@end

Test_Questions and User_Answers are not necessary since they do not hold any attribution and are one-to-one relationships with Test and User, respectively. It is always suspect when you have a plural table name.

However, the model fails to support the ability of a User to take multiple Tests. To fix that, User.testTaken should be replaced by User.testsTaken and made into a one-to-many relationship with Test.

Also, it seems a bit odd that a Test can only be taken by a single User, but if that's your requirement, then so be it.


Reverse "Guess the Number" program I recently got to a certain exercise in a book I am reading, and am looking for feedback. It is a program in which the user picks a number and the computer uses algorithms to guess it. If there is any way that I can improve my code, feedback is really appreciated!

#include <iostream>
#include <string>
#include <cstdlib>
#include <ctime>
using namespace std;

int main()
{
    srand(static_cast<unsigned int>(time(0))); //seed random number generator
    int compNumber = rand() % 100 + 1;

    int yourNumber;
    cout << "Please input a number between 1 and 100" << endl;
    cin >> yourNumber;

    cout << "Ok! I'm gonna try to guess it!" << endl;

    while (compNumber != yourNumber)
    {
        if (compNumber > yourNumber)
        {
            do
            {
                --compNumber;
            } while (compNumber != yourNumber);
        }

        else if (compNumber > yourNumber)
        {
            do
            {
                ++compNumber;
            } while (compNumber != yourNumber);
        }

    }

    cout << "I guessed it! Your number is " << compNumber << endl;

    return 0;

}

Sure this generates a random number from 1 -> 100.

int compNumber = rand() % 100 + 1;

BUT not all the numbers have an even probability. Assuming RAND_MAX is 32768 (a common value). Then the number 1->68 have a slightly higher probability than the number 69->100.

To get an even distribution you need to compensate for this:

int compNumber
do
{
    compNumber = rand();
}
while (compNumber > (RAND_MAX / 100 * 100));
compNumber = compNumber % 100 + 1;

Better yet learn to use the modern random number generator that is built into C++.

// I have not tested this.
// Just copied and pasted from: http://stackoverflow.com/a/19666713/14065
// This is the modern equivalent of srand()
std::random_device rd;
std::mt19937 mt(rd());
std::uniform_real_distribution<int> dist(1, 100);

// Then usage is.
// equivalent of rand().
std::cout << dist(mt) << "\n";

This does not look correct:

    if      (compNumber > yourNumber)
    {}
    else if (compNumber > yourNumber)
    {}

The conditions look the same to me.

This does not look like guessing.

        do
        {
            --compNumber;
        } while (compNumber != yourNumber);

This looks like the computer counting down until it reaches your number.


Add a newline to a String if not empty in Java This should be really simple. I'm searching for the cleanest & shortest way to add a newline to a non-null String if it is not empty, i.e. myString.isEmpty() is true. I know of a number of ways to do this, but none of them are as elegant as I would like them to.

For context, the string I'm working on will essentially be a list of key-value pairs that is filled with a loop. Every key-value-pair needs to have a newline appended after it, though not if it is the last one. An example for the end value of myString might be the following:

Boss : Julia
Worker : Louis
Intern : Tom

I could do

myString += myString.isEmpty() ? "" : "\n";

but is this really the best way to do this?

I also thought of

if(!myString.isEmpty()) myString += "\n";

Which way would be preferable, and why?
Are there better alternatives?

Every key-value-pair needs to have a newline appended after it, though not if it is the last one.

So are you interested to know how to concatenate Strings together using \n as a delimiter, or are you still interested to know how can you append \n to a non-empty String? Both have different implementations...

(using Java 8 where applicable below)

\n as a delimiter:

String toString = organizationMap.entrySet().stream()
                    .map(ThisClass::constructStringFromEntry)
                    .filter(v -> !v.isEmpty())
                    .collect(Collectors.joining("\n"));

\n as an optional suffix:

StringBuilder builder = new StringBuilder();
Iterator<Entry<String, String>> iterator = organizationMap.entrySet().iterator();
while (iterator.hasNext()) {
    Entry<String, String> entry = iterator.next();
    // some other processing...
    // time to append the current entry to builder
    String entryString = constructStringFromEntry(entry);
    builder.append(entryString);
    if (!entryString.isEmpty() && iterator.hasNext()) {
        builder.append('\n');
    }
}
String toString = builder.toString();

I am suggesting the usage of a StringBuilder here since the kind of String concatenation in a loop is usually structured, hence it can be better done relying on that rather than +. Usually, I will prefer the enhanced for-loop over using an explicit Iterator, but its usage here is required in order to avoid adding the last \n.


Performance of WordPress Plugin is currently very bad and needs to be improved I have written the following Plugin, which adds fields to the Media/Attachment Entries and returns all information in one Shortcode. This solution works, however, is pretty slow the more entries available.

I know that the problem lies in the while loop. The 20000 simply is a high number in order for all posts to be searched through.
However, I included the entire code in order to see what I'm trying to achieve:

defined( 'ABSPATH' ) or die( 'Are you ok?' );
/** ----- ADD FIELDS IN MEDIA LIBRARY ----- **/
function additionalMediaFields( $form_fields, $post ) {
    $form_fields['copyright_author'] = array(
        'label' => '&copy; Autor',
        'input' => 'text',
        'value' => get_post_meta( $post->ID, 'copyright_author', true ),
    );
    $form_fields['copyright_description'] = array(
        'label' => '&copy; Beschreibung',
        'input' => 'text',
        'value' => get_post_meta( $post->ID, 'copyright_description', true ),
    );
    $form_fields['copyright_website'] = array(
        'label' => '&copy; Website',
        'input' => 'text',
        'value' => get_post_meta( $post->ID, 'copyright_website', true ),
    );
    return $form_fields;
}
add_filter( 'attachment_fields_to_edit', 'additionalMediaFields', 10, 2 );

function storeAdditionalMediaFields( $post, $attachment ) {
    if( isset( $attachment['copyright_author'] ) )
        update_post_meta( $post['ID'], 'copyright_author', $attachment['copyright_author'] );
    if( isset( $attachment['copyright_description'] ) )
        update_post_meta( $post['ID'], 'copyright_description', $attachment['copyright_description'] );
    if( isset( $attachment['copyright_website'] ) )
        update_post_meta( $post['ID'], 'copyright_website', $attachment['copyright_website'] );
    return $post;
}
add_filter( 'attachment_fields_to_save', 'storeAdditionalMediaFields', 10, 2 );

/** ----- SHOW THEM IN ONE SHORTCODE ----- **/
function listAllCopyrightInformation ($atts, $content = null) {

    $i = 1;
    while ( $i<=20000 ) {
        $copyrightAuthor = get_post_meta($i, 'copyright_author', true );
        $copyrightDescription = get_post_meta($i, 'copyright_description', true );
        $copyrightWebsite = get_post_meta($i, 'copyright_website', true );

        if ( !empty($copyrightAuthor)) {
            if( !empty($copyrightDescription)) {
                $CopyrightEntries .= $copyrightDescription . ': ';
            }
            $CopyrightEntries .= '&copy; ';
            $CopyrightEntries .= $copyrightAuthor;
            if( !empty($copyrightWebsite)) {
                $CopyrightEntries .= ' / ' . get_post_meta($i, 'copyright_website', true) . ' || ';
            }
        }
        $i++;
    }
    $CopyrightReturn = substr($CopyrightEntries, 0, -8);
    return $CopyrightReturn;
}

add_shortcode("show-all-copyrights", "listAllCopyrightInformation");

Thanks in advance. I appreciate your effort!

If you don't need to worry about WordPress filters on getting post meta, you probably can just do this with a single SQL statement and iterate through the results, rather than relying on WordPress to get the values from the database, since that's 20,000 sql statements, plus the memory of caching all that data (which WordPress does behind the scenes when you get post meta for a post).


Create the English word for a number I cobbled together a function from various sources that takes an integer and returns it as its respective English word formatted as a string. The function is as follows:

def int2word(num, separator="-"):
    """Transforms integers =< 999 into English words

    Parameters
    ----------
    num : int
    separator : str

    Returns
    -------
    words : str
    """
    ones_and_teens = {0: "Zero", 1: 'One', 2: 'Two', 3: 'Three',
                      4: 'Four', 5: 'Five', 6: 'Six', 7: 'Seven',
                      8: 'Eight', 9: 'Nine', 10: 'Ten', 11: 'Eleven',
                      12: 'Twelve', 13: 'Thirteen', 14: 'Fourteen',
                      15: 'Fifteen', 16: 'Sixteen', 17: 'Seventeen',
                      18: 'Eighteen', 19: 'Nineteen'}
    twenty2ninety = {2: 'Twenty', 3: 'Thirty', 4: 'Forty', 5: 'Fifty',
                     6: 'Sixty', 7: 'Seventy', 8: 'Eighty', 9: 'Ninety', 0: ""}

    if 0 <= num < 19:
        return ones_and_teens[num]
    elif 20 <= num <= 99:
        tens, below_ten = divmod(num, 10)
        if below_ten > 0:
            words = twenty2ninety[tens] + separator + \
                ones_and_teens[below_ten].lower()
        else:
            words = twenty2ninety[tens]
        return words

    elif 100 <= num <= 999:
        hundreds, below_hundred = divmod(num, 100)
        tens, below_ten = divmod(below_hundred, 10)
        if below_hundred == 0:
            words = ones_and_teens[hundreds] + separator + "hundred"
        elif below_ten == 0:
            words = ones_and_teens[hundreds] + separator + \
                "hundred" + separator + twenty2ninety[tens].lower()
        else:
            if tens > 0:
                words = ones_and_teens[hundreds] + separator + "hundred" + separator + twenty2ninety[
                    tens].lower() + separator + ones_and_teens[below_ten].lower()
            else:
                words = ones_and_teens[
                    hundreds] + separator + "hundred" + separator + ones_and_teens[below_ten].lower()
        return words

    else:
        print("num out of range")

There's an easier way.
We have the num2words module which can be easily installed via pip:
pip install num2words

The advantage of this module is that it supports multiple languages:

en (English, default)
fr (French)
de (German)
es (Spanish)
lt (Lithuanian)
lv (Latvian)
en_GB (British English)
en_IN (Indian English)
no (Norwegian)
pl (Polish)
ru (Russian)
dk (Danish)
pt_BR (Brazilian Portuguese)

More, you can even generate ordinal numbers like forty-second.
A small python example for converting numbers to words using num2words looks like this:
>>> from num2words import num2words
>>> num2words(42)
forty-two
>>> num2words(42, ordinal=True)
forty-second

You can read more about what you can do using this module here

NOTE: In case somebody is wondering why this is not a code review (such as comments on the posted code), is because in the unedited question the author specifically asked if there is already a library for this.


Largest palindrome made from the product of 3-digit numbers I am given a task to write a program to find the largest palindrome made from the product of two 3-digit numbers. How can I improve this code?

def check_palindrome(s):
     """Checks whether the given string is palindrome"""

     if s == s[::-1]:
        return True

product_pal = []
for i in range (999,900,-1):
    for j in range (999,900,-1):
        product = i * j
        if check_palindrome(str(product)):
            product_pal.append(product)
            print"i =" , i , "j = ",j, "for", product
print max(product_pal)

You already got some advice on coding style. however there is a big flaw in your algorithm which is not addressed by the accepted answer.

you try to iterate downward to get an effective implementation but you got the inner loop wrong. while you expect the outer loop to do few iterations your inner loop does check relatively low numbers early. you tried to limit that by stopping the iteration at 900, a magic value without reasoning. so your implementation may give wrong results as a pair of 901*901 is much smaller than a lot of untested pairs. you need at least a check if your product is bigger than the biggest untested one 999*900.

on the other hand if we do the inner loop right all problems are gone. we use the outer loop for the lower value and the inner loop for the greater one. we do not need an arbitrary limit any more and we are quite efficient.

for i in range(999,99,-1):
    for j in range(999,i-1,-1):
        # check palindrome

again we do not want to collect all palindromes but only the biggest one. we can abort safely when we cannot get a bigger product than the current maximum one.

def is_palindrome(n):
    s = str(n)
    return s == s[::-1]

def get_biggest_palindrome():
    max_product = 0
    for i in xrange(999, 99, -1):
        if max_product >= 999*i:
            # no need to iterate further down
            break
        for j in xrange(999, i-1, -1):
            p = j * i
            if max_product >= p:
                # no need to iterate further down
                break
            if is_palindrome(p):
                max_product = p
    return max_product

I did some other minor changes:

is_palindrome - i like to name functions after what they return so the usage reads like a natural language sentence.

in python2 you should use xrange() instead of range() if you do not need a real list but just an iterator.

what you could do also:

make the magic numbers 999 and 99 constants and/or pass them as parameters. if it is about the number of digits you could define them as 10**(digits+1)-1, 10**digits-1 and pass digits as single parameter.


A regex pattern that matches all forms of integers and decimal numbers in Python The question is pretty self-explanatory. I wrote a regex pattern which should (in theory) match all possible integers and decimal numbers. The pattern is as follows:

import re

pattern = '^[+-]?((\d+(\.\d*)?)|(\.\d+))$'
re.compile(pattern)

How foolproof is this pattern? I tested out quite a few scenarios, and they all worked fine. Am I missing some edge case here? Thanks for any help.

Your expression looks just fine, maybe we would slightly modify that to:
^[+-]?((\d+(\.\d+)?)|(\.\d+))$

for failing these samples, 3., 4., for instance, just in case maybe such samples might be undesired. Other than that, you have some capturing groups that I'm guessing you'd like to keep those.

Test the capturing groups with re.finditer
import re

regex = r"^[+-]?((\d+(\.\d+)?)|(\.\d+))$"

test_str = ("0.00000\n"
    "0.00\n"
    "-200\n"
    "+200\n"
    "200\n"
    "200.2\n"
    "-200.2\n"
    "+200.2\n"
    ".000\n"
    ".1\n"
    ".2\n"
    "3.\n"
    ".")

matches = re.finditer(regex, test_str, re.MULTILINE)

for matchNum, match in enumerate(matches, start=1):

    print ("Match {matchNum} was found at {start}-{end}: {match}".format(matchNum = matchNum, start = match.start(), end = match.end(), match = match.group()))

    for groupNum in range(0, len(match.groups())):
        groupNum = groupNum + 1

        print ("Group {groupNum} found at {start}-{end}: {group}".format(groupNum = groupNum, start = match.start(groupNum), end = match.end(groupNum), group = match.group(groupNum)))

Test with re.findall
import re

regex = r"^[+-]?((\d+(\.\d+)?)|(\.\d+))$"

test_str = ("0.00000\n"
    "0.00\n"
    "-200\n"
    "+200\n"
    "200\n"
    "200.2\n"
    "-200.2\n"
    "+200.2\n"
    ".000\n"
    ".1\n"
    ".2\n"
    "3.\n"
    ".")

print(re.findall(regex, test_str, re.MULTILINE))

The expression is explained on the top right panel of this demo, if you wish to explore/simplify/modify it, and in this link, you can watch how it would match against some sample inputs step by step, if you like.

RegEx Circuit
jex.im visualizes regular expressions:


Programmatically using `exec` to load files I'm making a command line interface (CLI) role playing tool that tracks the state of the world, storing characters, monsters, etc as text files so their data can be edited directly if needed (free Vim UI yay). This means that every game loop, I need to reload the files.
Note: Direct editing implies no pickling. Also, I may eventually add functions to these files so I can use duck typing.
Since filenames are pulled into Python as strings, I felt a straightforward approach would be to use exec.
I'm concerned though because learning materials rarely discuss using this function responsibly.
Instead they warn against its use for security reasons.
Below is the directory, two test character files with simple data, and the main program.
# Directory
dnd\
    main.py
    entities\
        char01.py
        char02.py

# File: char01.py
name = 'Gandalf Potter'

# File: char02.py
name = 'Obi-wan Picard'

My solution:

For this MWE, first check whether the files are loaded. For the first loop, they won't be loaded yet. By the second loop, the files
should load and the loop will exit to avoid infinity.
Read filenames from directory.
Check if filename is in globals().
Depending on the check, either import or reload the module with
exec.

# File: main.py

import importlib
import os

def main():
    while True:
        if test_load(): return  # Run until files load
        files = get_files('entities')
        load_files(files, 'entities')

def get_files(dir):
    return (file for file in os.listdir(dir)
        if not file.startswith('__'))  # Ignore __pycache__

def load_files(files, dir):
    for f in files:
        module = f.strip('.py')
        if module in globals():
            exec(f'importlib.reload({module})')
        else:
            exec(f'from {dir} import {module}')
    modules = {k: v for k, v in locals().items()
                    if isinstance(v, type(os))}
    globals().update(modules)

def test_load():
    try:
        char01.name
        char02.name
        print('loaded')
        return True
    except NameError:
        print('not loaded')
        return False

main()

Questions:

Is there a better approach?

Using exec is frowned upon for security reasons, however I want
to start using it more often because code can be treated as strings, which allows the code to be treated as data that can be manipulated, substituted, or changed.
However, I also want to be mindful of vulnerabilities.

Is there a better way of pulling out only the modules than using if isinstance(v, type(os))?

What is everyone's opinion of using the globals() function like
this to do imports programmatically from within functions?

I used DICT.update() to add imports to globals(), however I
wanted to be fancy and use Python39's new dictionary union operator
|=. Unfortunately, lines like

globals() |= {k: v for k, v in locals().items() if isinstance(v, type(os))}

don't work because a function call like globals() can't be assigned
to. It would work if I did the following:
globals_ = globals()
globals |= {k: v for k, v in locals().items() if isinstance(v, type(os))}

but at that point, it'd be simpler to just stick with DICT.update().

I welcome any other suggestions and criticisms.

Looping
while True:
    if test_load(): return  # Run until files load

should simply be
while not test_load():
    # ...

Getting files
It's good that you're using a generator, but you should consider basing it off of pathlib instead of os. Also, it seems like you should care less whether your file starts with underscores, and more that it's a file instead of a directory. For instance:
return (
    file
    for file in Path(dir).iterdir()
    if file.is_file()
)

Dynamic loading
You're not just loading any files - you're loading arbitrary Python modules. Since your examples only show variables being initialized, I'm going to go out on a limb and say: please, please don't load Python code when you should simply be loading data, from any of the common formats - a pickle, or JSON, or CSV, or what-have-you. It's easier, faster, more secure and generally less of a headache.