Script for recording a country in which a company belongs I have an Apps Script that is supposed to run through the spreadsheet, and if a company belongs to a certain country (there's a country column), set the value of the region row to something (e.g. Africa).

The spreadsheet has more than 8500 rows and this is the dirty code I have.  It's working, but if there's a better way to write it, I'd appreciate any pointers:

function setRegions() {
      var mySheet = SpreadsheetApp.getActiveSpreadsheet().getActiveSheet();
      var myCompany = mySheet.getDataRange().getValues();
      var start = 2;
      for(i = 1; i < myCompany.length; i++){
        var univ = myCompany[i];
        var country_code = univ[4];
        if (country_code == "ke" ||
            country_code == "ng" ||
            country_code == "ug" ||
            country_code == "za" ||
            country_code == "sn" ||
            country_code == "gh") {
          rgMyRange = mySheet.getRange("M" + start);
          rgMyRange.setValue("AFRICA");
        }else if (country_code == "eg" ||
                 country_code == "sa" ||
                 country_code == "ma"){
          rgMyRange = mySheet.getRange("M" + start);
          rgMyRange.setValue("MENA");
        }else if (country_code == "br" ||
                 country_code == "ar" ||
                 country_code == "mx" ||
                 country_code == "pe" ||
                 country_code == "co"){
          rgMyRange = mySheet.getRange("M" + start);
          rgMyRange.setValue("LATAM");
        }else if (country_code == "my" ||
                 country_code == "th" ||
                 country_code == "id" ||
                 country_code == "ph"){
          rgMyRange = mySheet.getRange("M" + start);
          rgMyRange.setValue("APAC");
        }else if (country_code == "in") {
          rgMyRange = mySheet.getRange("M" + start);
          rgMyRange.setValue("IN");
        }
        start++;
      }
    }

TL;DR : My comments won't improve the performances much. Also, I've never used Apps Script.

This being said, here what I did : I decided to change details to make your code more concise (removing variables used only once for instance).

function setRegions() {
      var mySheet = SpreadsheetApp.getActiveSpreadsheet().getActiveSheet();
      var myCompany = mySheet.getDataRange().getValues();
      for(i = 1, start = 2; i < myCompany.length; i++, start++){
        var country_code = myCompany[i][4];
        if (country_code == "ke" ||
            country_code == "ng" ||
            country_code == "ug" ||
            country_code == "za" ||
            country_code == "sn" ||
            country_code == "gh") {
          mySheet.getRange("M" + start).setValue("AFRICA");
        }else if (country_code == "eg" ||
                 country_code == "sa" ||
                 country_code == "ma"){
          mySheet.getRange("M" + start).setValue("MENA");
        }else if (country_code == "br" ||
                 country_code == "ar" ||
                 country_code == "mx" ||
                 country_code == "pe" ||
                 country_code == "co"){
          mySheet.getRange("M" + start).setValue("LATAM");
        }else if (country_code == "my" ||
                 country_code == "th" ||
                 country_code == "id" ||
                 country_code == "ph"){
          mySheet.getRange("M" + start).setValue("APAC");
        }else if (country_code == "in") {
          mySheet.getRange("M" + start).setValue("IN");
        }
      }
    }

Then, you can get rid of the different tests on the country_code by storing what you want in a dictionary :

function setRegions() {
      var dict = {
          "ke" : "AFRICA",
          "ng" : "AFRICA",
          "ug" : "AFRICA",
          "za" : "AFRICA",
          "sn" : "AFRICA",
          "gh" : "AFRICA",
          "eg" : "MENA",
          "sa" : "MENA",
          "ma" : "MENA",
          "br" : "LATAM",
          "ar" : "LATAM",
          "mx" : "LATAM",
          "pe" : "LATAM",
          "co" : "LATAM",
          "my" : "APAC",
          "th" : "APAC",
          "id" : "APAC",
          "ph" : "APAC",
          "in" : "IN",
      };
      var mySheet = SpreadsheetApp.getActiveSpreadsheet().getActiveSheet();
      var myCompany = mySheet.getDataRange().getValues();
      for(i = 1, start = 2; i < myCompany.length; i++, start++){
        var country_code = myCompany[i][4];
        if (dict[country_code])
        {
          mySheet.getRange("M" + start).setValue(dict[country_code]);
        }
      }
    }


Golang WebSocket communication between server and app I've written a simple application that captures frames from my webcam and streams the frames via a websocket.

The stream implementation I've come up with feels weird. Networking is not my strong suit. I am wondering if the communication between the server and app can be written better.

type Stream struct {
    Frame []byte
}

func (s *Stream) ServeHTTP(w http.ResponseWriter, r *http.Request) {
    conn, err := upgrader.Upgrade(w, r, nil)
    if err != nil {
            log.Fatal(err)
    }

    //This feels weird
    //I use a ticker that ticks with the fps of the camera
    //to send the individual frames to the browser

    ticker := time.NewTicker(time.Millisecond * 33)
    for _ = range ticker.C {
            if err = conn.WriteMessage(websocket.TextMessage, s.Frame); err != nil {
                    log.Fatal(err)
            }
    }
}

s := server.Stream{}
wg := sync.WaitGroup{}
wg.Add(2)
go func() {
        http.Handle("/", &server.TemplateHandler{Filename: "index.html"})
        http.Handle("/ws", &s)
        http.ListenAndServe(":8080", nil)
}()

go func() {
        err = camera.StartStreaming()
        checkErr(err)

        for {
                frame, err := camera.ReadFrame()
                checkErr(err)

                //Also weird, but hey it works
                s.Frame = []byte(base64.StdEncoding.EncodeToString(frame))
            }
}()

wg.Wait()

You've reviewed your own code, and done a good job, actually:

//This feels weird
//I use a ticker that ticks with the fps of the camera
//to send the individual frames to the browser

ticker := time.NewTicker(time.Millisecond * 33)
for _ = range ticker.C {
        if err = conn.WriteMessage(websocket.TextMessage, s.Frame); err != nil {
                log.Fatal(err)
        }
}

Fundamentally the issue is that you've joined two separate problems in to one function, and the result is a poorly defined operation that's a bit brittle.

What you want to do is expose a channel that sends Frame instances each time one is available. This will result in code that looks more like:

for f = range s.Frames() {
        if err = conn.WriteMessage(websocket.TextMessage, f); err != nil {
                log.Fatal(err)
        }
}

Loop/range on a channel (assume s.Frames() returns a channel of Frame instances), and then send each Frame down the websocket.

This sets the channel as an "api" to the camera, and the logic is encapsulated in there.

Now, the challenge is to set up a source for that frame channel....

A separate goroutine that polls the camera for new frames, listens to a stream and breaks on each frame, or loops on a timer (like you have now), or whatever mechanism best suits managing your camera, would feed Frames in to the channel, and you've thus separated the two concerns.

You can also now have different mechanisms for sourcing frames, and still feed them in to the same method for delivering them to the websocket.


Service for generating faked objects I'm using the Bogus library to mock up some data for testing.

This is my main method demonstrating how the faking service should be used:

class Program
{
    static void Main(string[] args)
    {
        IFakerService r = new FakerService();

        var dogs = r.Generate<Dog>(10);
        var customers = r.Generate<Customer>(10);
        var cats = r.Generate<Cat>(10);
    }
}

A Dog class looks like:

public class Dog
{
    public string Name { get; set; }
    public bool IsGoodBoy => true;
}

My interface simply defines the only method the service should expose:

public interface IFakerService
{
    IEnumerable<T> Generate<T>(int count) where T : class;
}

And the service itself:

public class FakerService : IFakerService
{
    // Can we do better than <Type, object> ?
    private Dictionary<Type, object> _fakers;

    public FakerService()
    {
        _fakers = InitialiseFakers();
    }

    public IEnumerable<T> Generate<T>(int count) where T : class
    {
        var faker = GetFaker<T>();
        return faker.Generate(count);
    }

    // Types which can be faked are registered here
    private Dictionary<Type, object> InitialiseFakers()
    {
        return new Dictionary<Type, object>()
        {
            {typeof(Dog), new DogFaker().GetFaker()},
            {typeof(Cat), new CatFaker().GetFaker()},
            {typeof(Customer), new CustomerFaker().GetFaker()}
        };
    }

    private Faker<T> GetFaker<T>() where T : class
    {
        if (!_fakers.ContainsKey(typeof(T)))
        {
            throw new ArgumentException($"Type: {typeof(T).FullName} not registered with FakerService");
        }
        var faker = (Faker<T>)_fakers[typeof(T)];
        return faker;
    }
}

The interface for Faker objects:

public interface IFaker<T> where T : class
{
    Faker<T> GetFaker();
}

And one of the implementations:

public class DogFaker : IFaker<Dog>
{
    public Faker<Dog> GetFaker()
    {
        return new Faker<Dog>()
            .RuleFor(dog => dog.Name, f => f.Name.FirstName(Name.Gender.Male))
            .RuleFor(dog => dog.IsGoodBoy, f => f.Random.Bool());
    }
}

Overall I'm quite happy with how it looks - the parts that I was wondering if I could improve on are:

private Dictionary<Type, object> _fakers;

...

return new Dictionary<Type, object>()
{
    {typeof(Dog), new DogFaker().GetFaker()}
}

...

var faker = (Faker<T>)_fakers[typeof(T)];

Any advice is appreciated!

public interface IFaker<T> where T : class
{
    Faker<T> GetFaker();
}

I don't think you need this interface and the additional call to GetFaker. Instead you can derive your class from the Faker<T> and use the constructor to set everything up:

public class DogFaker : Faker<Dog>
{
    public DogFaker()
    {
        RuleFor(dog => dog.Name, f => f.Name.FirstName(Name.Gender.Male));
        RuleFor(dog => dog.IsGoodBoy, f => f.Random.Bool());
    }
}

private Faker<T> GetFaker<T>() where T : class
{
  if (!_fakers.ContainsKey(typeof(T)))
  {
      throw new ArgumentException($"Type: {typeof(T).FullName} not registered with FakerService");
  }
  var faker = (Faker<T>)_fakers[typeof(T)];
  return faker;
}

In this case it would be much clener to query the dictionary only once with TryGetValue if you are going to use the value anyway rather then doing it twice, once with ContainsKey and then with the [] indexer. At the same time you can tun it into a ternary operator ?::

private Faker<T> GetFaker<T>() where T : class
{
    return
        _fakers.TryGetValue(typeof(T), out var faker)
            ? (Faker<T>)faker
            : throw new ArgumentException($"Type: {typeof(T).FullName} not registered with FakerService");
}

Since ´InitialiseFakers´ does not require any instance data I suggest making it static. I like the way you created this initializer. Instead of letting it set the _fakers dictionary you return it as a result and assign it in the construtor. This way it's much cleaner and all the initialization-assignment stays inside the construtor so a big plus for that.

// Can we do better than <Type, object> ?
private Dictionary<Type, object> _fakers;

I'm afraid we cannot because Faker<T> isn't derived from any other collection-friendly interface. All you have is:

public class Faker<T> : IFakerTInternal, ILocaleAware, IRuleSet<T> where T : class

and this is not very useful so object is the only possible generalisation in this case. But this isn't that bad. It's an object so it doesn't cost you a penny to cast it. But sure, some general interface would be nice.


Web scraping JSON-data from API put into Dataframe The web-scraping process currently takes quite a bit of time, and I wonder if I can structure the code otherwise or improve it in any way.

Code looks like this:

import numpy as np
import pandas as pd
import requests
import json
from sklearn import preprocessing
from sklearn.preprocessing import OneHotEncoder

results_2017 = []
results_2018 = []
for game_id in range(2017020001, 2017021271, 1):
    url = 'https://statsapi.web.nhl.com/api/v1/game/{}/boxscore'.format(game_id)
    r_2017 = requests.get(url)
    game_data_2017 = r_2017.json()

    for homeaway in ['home','away']:

        game_dict_2017 = game_data_2017.get('teams').get(homeaway).get('teamStats').get('teamSkaterStats')
        game_dict_2017['team'] = game_data_2017.get('teams').get(homeaway).get('team').get('name')
        game_dict_2017['homeaway'] = homeaway
        game_dict_2017['game_id'] = game_id
        results_2017.append(game_dict_2017)

df_2017 = pd.DataFrame(results_2017)

for game_id in range(2018020001, 2018020667, 1):
    url = 'https://statsapi.web.nhl.com/api/v1/game/{}/boxscore'.format(game_id)
    r_2018 = requests.get(url)
    game_data_2018 = r_2018.json()

    for homeaway in ['home','away']:

        game_dict_2018 = game_data_2018.get('teams').get(homeaway).get('teamStats').get('teamSkaterStats')
        game_dict_2018['team'] = game_data_2018.get('teams').get(homeaway).get('team').get('name')
        game_dict_2018['homeaway'] = homeaway
        game_dict_2018['game_id'] = game_id
        results_2018.append(game_dict_2018)

df_2018 = pd.DataFrame(results_2018)

df = df_2017.append(df_2018)

Assuming you have a multi-core processor, you can use the multiprocessing module to do your scraping in parallel. Here's a good article that explains how to use it: An introduction to parallel programming using Python's multiprocessing module (you only need to read the first half of the article)

The simplest way to parallelize this would be to do your 2017 loop in a separate process from your 2018 loop. If you need it faster than that, you could further subdivide your 2017 and 2018 ranges. It also depends on how many cores you have. If you only have 2 cores, you won't benefit from dividing it into more than 2 processes (or 4 processes for 4 cores).

Other than that I can't see anything in your code structure you could change to speed it up.

Update: I'm not familiar with the API you're using, but if there's a way to make one API call that returns a list of games with their box scores instead of making a separate request for each game, that would be the best way to speed it up.

Also, according to Mathias in the comments, multiprocessing.Pool.map would be particularly helpful in this case. I'm not familiar with it, but it does look like it would be convenient for this.


Finding prime factors of a number? Okay, so just having a little fun I thought I'd see if I could write an algorithm to find all of the prime factors of a number. The solution I came up with is as follows:

class Program
{
    static void Main(string[] args)
    {
        var subject = long.MaxValue;
        var factors = new List<long>();
        var maxFactor = 0;

        Console.WriteLine("Factoring {0} ...", subject);

        var sw = new Stopwatch();
        sw.Start();

        while (subject > 1)
        {
            var nextFactor = 2;
            if (subject % nextFactor > 0)
            {
                nextFactor = 3;
                do
                {
                    if (subject % nextFactor == 0)
                    {
                        break;
                    }

                    nextFactor += 2;
                } while (nextFactor < subject);
            }

            subject /= nextFactor;
            factors.Add(nextFactor);
            if (nextFactor > maxFactor)
            {
                maxFactor = nextFactor;
            }
        }

        sw.Stop();

        var factorAnswer = 1L;
        factors.ForEach(f => factorAnswer *= f);

        Console.WriteLine("Factors: {0} = {1}",
            string.Join(" * ",
                factors.Select(i => i.ToString()).ToArray()),
            factorAnswer);
        Console.WriteLine("Max Factor: {0}", maxFactor);
        Console.WriteLine("Elapsed Time: {0}ms", sw.ElapsedMilliseconds);
    }
}

and its output is:

Factoring 9223372036854775807 ...
Factors: 7 * 7 * 73 * 127 * 337 * 92737 * 649657 = 9223372036854775807
Max Factor: 649657
Elapsed Time: 3ms

It works, and IMO awfully fast, but it's a little brute force. Is there a better way of doing it?

Other algorithms

Wikipedia has an article which lists other Factoring algorithms.

Your algorithm

Re. your algorithm, I see you're checking all odd numbers, which includes non-prime numbers such as 9.

Your sieve would be faster if you only checked prime numbers, for example by using a list like this one or this one.

Furthermore you're checking all the way to subject. Your last time through the loop would be faster if you give up as soon as nextFactor > sqrt(subject).

Your C# code

As for your C# code:

What type is nextFactor when you declare it as var nextFactor = 2;? I fear that it might be int not long.
Your code might be very slightly faster if you used the Capacity property of List.
There's something very strange (and slow) about your loop: after you find a nextFactor value such as 7, then you begin your search from 2 again! You code would be faster if you moved your long nextFactor = 2; initialization to do it only once, before/outside the while loop, and changed your nextFactor = 3; statement to nextFactor += (nextFactor == 2) ? 1 : 2;. You could then also eliminate your maxFactor variable, because the largest factor value would be left/stored in nextValue after you exit the while loop.
It might be better to use System.Numerics.BigInteger instead of long (because long.MaxValue is only about 10^18, whereas some people want to factorize larger numbers than that).


Sparking animation for Button I just came through a GitHub repo, which has an amazing animation for Button in Android.

So, I thought why not make that for web buttons too.

And I started designed that.

Here is my repo for the same.

Here is my code:

let icons = $('.magic')

for (let i = 0; i < icons.length; ++i) {
    icons[i].onclick = function (element) {
        icons[i].classList.toggle('enabled');
    }
}
.magic {
    display: inline-block;
    margin: 50px;
    position: relative;
  }

  .magic i {
    color: orange;
    filter: grayscale(100%);
    position: relative;
    animation: disable 0.5s forwards;
  }

  .enabled i {
    animation: enable 1s forwards;
  }

  .magic::before {
    content: "";
    top:calc(50% - 45px);
    left:calc(50% - 45px);
    width: 90px;
    height: 90px;
    position: absolute;
    border-color: orange;
    border-style: solid;
    border-width: 45px;
    border-radius: 50%;
    transform: scale(0);
    box-sizing: border-box;
  }

  .enabled::before {
    transition:
      transform 0.5s,
      border-width 0.5s 0.2s;
    transform: scale(1);
    border-width: 0px;
  }

  .magic::after,
  .magic i::after {
    content: "";
    position: absolute;
    top: calc(50% - 80px);
    left: calc(50% - 80px);
    height: 160px;
    width: 160px;
    background:
      radial-gradient(circle, red 50%, transparent 60%),
      radial-gradient(circle, red 50%, transparent 60%),
      radial-gradient(circle, red 50%, transparent 60%),
      radial-gradient(circle, red 50%, transparent 60%),
      radial-gradient(circle, orange 50%, transparent 60%),
      radial-gradient(circle, orange 50%, transparent 60%),
      radial-gradient(circle, orange 50%, transparent 60%),
      radial-gradient(circle, orange 50%, transparent 60%);

    background-position:
      calc(50% - 50px) calc(50% - 50px),
      calc(50% - 50px) calc(50% + 50px),
      calc(50% + 50px) calc(50% - 50px),
      calc(50% + 50px) calc(50% + 50px),
      calc(50% - 0px) calc(50% - 70px),
      calc(50% - 70px) calc(50% - 0px),
      calc(50% - 0px) calc(50% + 70px),
      calc(50% + 70px) calc(50% - 0px);
    background-size: 16px 16px;
    background-repeat: no-repeat;
    border-radius:50%;
    transform: scale(0);
  }

  .magic i:after {
    background-size: 10px 10px;
    transform: rotate(10deg) scale(0);
  }

  .enabled::after {
    transition:
      transform 0.5s 0.5s,
      opacity 0.4s 1s,
      background-size 0.4s 1s;

    transform: scale(1);
    opacity: 0;
    background-size: 0 0;
  }

  .enabled i:after {
    transition:
      transform 0.5s 0.5s,
      opacity 0.4s 0.8s,
      background-size 0.4s 0.8s;

    transform: rotate(10deg) scale(1);
    opacity: 0;
    background-size: 0 0;
  }

  @keyframes enable {
    50% {
      filter: grayscale(100%);
      transform: scale(0);
    }
    51% {
      filter: grayscale(0%)
    }
    90% {
        transform: scale(1.3)
    }
    100% {
      filter: grayscale(0%);
      transform: scale(1);
    }
  }

  @keyframes disable {
      50% {
          filter: grayscale(0%);
          transform: scale(1.3);
      }
      100% {
          transform: scale(1);
      }

  }

<link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.7.1/css/all.css">

What do you reckon? Is there anything I can improve either in the animation or in the code?

I'd like to suggest the opposite of Sᴀᴍ Onᴇᴌᴀ: Since you are using jQuery for selecting only, you can drop it altogether.

const icons = $(".magic");

becomes either

const icons = document.getElementsByClassName("magic");

or

const icons = document.querySelectorAll(".magic");

However you shouldn't use on... properties to assign event handlers. on... properties can only hold a single handler so if you or another script would attempt to assign another click handler then they'd overwrite each other. Instead use addEventListener.

Alternatively to avoid assigning seperate event handlers to each icon you could use event delegation. This means assign an single event handler to a surrounding element (or simply document) and check the target element:

document.addEventListener("click", function (event) {
  if (event.target.classList.contains("magic")) {
    event.target.classlist.toggle("enabled");
  }
};

Finally you could avoid using JavaScript altogether by using an HTML element that has the toggle functionality built in: a checkbox:

  <input type="checkbox">

Hide the actual checkbox with:

.magic-wrapper > input {
  opacity: 0;
  position: absolute;
}

(This is more accessable than just using display: none;)

And replace the selector .enabled with input:checked + .magic in the CSS.

Complete example: https://jsfiddle.net/rhy6gfn4/

A small points about the CSS:

You should select not just .enabled but use .magic.enabled, because then it's more obvious that these rules belong to the animated icons. Also "enabled" is a common class name and you don't want those styles apply to unrelated elements.

It would be a tiniest bit more performant to select the i elements using .magic > i and not just .magic i.


Concurrent task pool My use case is to dispatch multiple long-running tasks to execute concurrently. The expectation is that the tasks will be IO-bound (e.g. network requests), but importantly each task is different. This is not a question about data parallelism.

I've written a simple pool type that wraps a sync.WaitGroup and collects any errors that occur.

package gopool

import "sync"

// GoPool is a pool of goroutines with error reporting.
type GoPool struct {
    waitGroup sync.WaitGroup
    errors    chan error
}

// New initializes a new GoPool.
func New() *GoPool {
    return &GoPool{
        waitGroup: sync.WaitGroup{},
        errors:    make(chan error),
    }
}

// Run runs the specified function in a goroutine, collecting any errors that might occur.
func (p *GoPool) Run(goFuncs ...func() error) {
    p.waitGroup.Add(len(goFuncs))
    for _, goFunc := range goFuncs {
        goFunc := goFunc
        go func() {
            defer p.waitGroup.Done()
            err := goFunc()
            if err != nil {
                p.errors <- err
            }
        }()
    }
}

// Wait waits for all specified tasks in the GoPoo to complete, and returns any collected errors that occurred.
func (p *GoPool) Wait() (errors []error) {
    go func() {
        p.waitGroup.Wait()
        close(p.errors)
    }()
    for err := range p.errors {
        errors = append(errors, err)
    }
    return errors
}

Here is an example of how it might be used:

func DoThings()  {
    var thingA int
    var thingB string
    var thingC *Thing

    pool := gopool.New()
    pool.Run(func() (err error) {
        thingA, err = FetchThingA()
        return err
    })
    pool.Run(func() (err error) {
        thingB = FetchThingB()
        return nil
    })
    pool.Run(func() (err error) {
        thingC, err = FetchThingC()
        return err
    })
    errs := pool.Wait()
    if len(errs) > 0 {
        // Handle errs
    }
    // Use each of the fetched things
}

This utility seems pretty useful, but I've not seen anyone write about it. Have I missed a major problem here, or is there a better way to handle this?

Your concept is an interesting one, but it has a number of concerns in it.

The most significant issue I can see is related to the errors that can be returned. There's no way to match an error to the function that caused it.

Other concerns are that your error channel is unbuffered, which could potentially leave go-routines unnecessarily hanging around waiting for the channel to be drained.

Still, even if that's OK, there's still a usability issue. My concern here is about the requirement to manage a relatively sensitive life-cycle outside the Pool code. Your user has to know that they create a new Pool, then Run a few items, and then Wait. The order cannot be changed, and you cannot reuse a pool after Waiting for it.

My feeling is that you could use a much simpler mechanism of having a single mechanism that "does it all". The user's code would look something like:

toRun := []func() error{}
toRun = append(toRun, func() error{fmt.Println("one"); return nil});
....
errors := pool.Run(toRun...)

There's no notion in the above of the internal implementation of the pool. That's a good thing. You use basic Go mechanisms like slices to aggregate functions to run, and you use basic go parameter manipulation to expand the slice for entry.

I played around a bit, and settled on this function:

// Run will run all the supplied functions in separate Go-routines, and return any errors in the resulting slice,
// and also return a boolen ok indicator which will be true if all goroutines succeeded.
func Run(toRun ...func() error) ([]error, bool) {

    count := len(toRun)
    var wg sync.WaitGroup
    wg.Add(count)
    errors := make([]error, count, count)
    ok := true
    for i, fn := range toRun {
        // create a closure for the error index and the function
        go func(index int, fn func() error) {
            defer wg.Done()
            errors[index] = fn()
        }(i, fn)
    }
    wg.Wait()
    for _, e := range errors {
        if e != nil {
            ok = false
        }
    }
    return errors, ok
}

The Run method thus encapsulates all the sequencing logic. There's no need for an external struct. There's no need for an error channel, and the errors are indexed relative to the input functions (so you can associate an error to a function). Also, the function returns a boolean "OK" flag to be used in a similar way as other Go-like functions.

The Use-Cases I tested were things like:

func main() {

    runs := []func() error{}

    runs = append(runs, func() error {
        fmt.Println("one")
        return nil
    })
    runs = append(runs, func() error {
        fmt.Println("two")
        return nil
    })
    runs = append(runs, func() error {
        fmt.Println("three")
        return nil
    })

    fmt.Println("Run one")
    if errs, ok := Run(runs...); !ok {
        fmt.Printf("Some errors: %v\n", errs)
    } else {
        fmt.Println("OK")
    }

    runs = append(runs, func() error {
        fmt.Println("fail")
        return errors.New("Failed")
    })

    fmt.Println("Run Two")
    if errs, ok := Run(runs...); !ok {
        fmt.Printf("Some errors: %v\n", errs)
    } else {
        fmt.Println("OK")
    }

}

Note that you can see the above all running in the playground: https://play.golang.org/p/9_MrR6gvj2


PHP Session handling class I've written a custom PHP session class for handling sessions across the web app. Please review the code and point out mistakes and suggest better handling techniques.

require_once('config.php');

class Sessions {

protected $sessionID;

public function __construct(){
    if( !isset($_SESSION) ){
        $this->init_session();
    }
    //session_start();
    //$this->sessionID = session_id();
}

public function init_session(){
    session_start();
}

public function set_session_id(){
    //$this->start_session();
    $this->sessionID = session_id();
}

public function get_session_id(){
    return $this->sessionID;
}

public function session_exist( $session_name ){
    if( isset($_SESSION[$session_name]) ){
        return true;
    }
    else{
        return false;
    }
}

public function create_session( $session_name , $is_array = false ){
    if( !isset($_SESSION[$session_name])  ){
        if( $is_array == true ){
            $_SESSION[$session_name] = array();
        }
        else{
            $_SESSION[$session_name] = '';
        }
    }
}

public function insert_value( $session_name , array $data ){
    if( is_array($_SESSION[$session_name]) ){
        array_push( $_SESSION[$session_name], $data );
    }
}

public function display_session( $session_name ){
    echo '<pre>';print_r($_SESSION[$session_name]);echo '</pre>';
}

public function remove_session( $session_name = '' ){
    if( !empty($session_name) ){
        unset( $_SESSION[$session_name] );
    }
    else{
        unset($_SESSION);
        //session_unset();
        //session_destroy();
    }
}

public function get_session_data( $session_name ){
    return $_SESSION[$session_name];
}

public function set_session_data( $session_name , $data ){
    $_SESSION[$session_name] = $data;
}

}

Commented out Code

You should remove code that is commented out. If you think that you might need it in the future, think about using version control.

set_session_id()

This is only called in your commented out code. Does the user have to call it manually? If they don't, get_session_id() will return a wrong result. Maybe rewrite it like this (you don't seem to be using the field $sessionID, so you might as well get rid of it):

public function get_session_id(){
    return session_id();
}

if-else and session_exist

If you have code like this:

if( isset($_SESSION[$session_name]) ){
    return true;
}
else{
    return false;
}

you can rewrite it like this:

return isset($_SESSION[$session_name]);

Also, when you define a function like this, then use it. Instead of

if( !isset($_SESSION[$session_name])  ){

write

if(!session_exist($session_name]) ){

XSS

Using Session for XSS might be a possibility, depending on how your code is used. So in display_session I would clean the session with htmlentities to prevent XSS attacks.


Calculating the sum, of the mod Q, position multiplied, sort sequence, of Fibonacci numbers I need to write a program that uses the Fibonacci Sequence to calculate a sum.

The calculation requirement

1: Take the first n Fibonacci numbers (excluding F0)
    S1 = {F1, F2, F3 ..... , Fn}
2: Modulo each Fibonacci number by a positive value Q
    S2 = {A1, A2, A3 ..... , An}  Where Ax = Fx % Q
3: Sort the number in S2 (small to large)
    S3 = {C1, C2, C3 ..... , Cn}
4: Calculated the weighted sum:
    S = 1.C1 + 2.C2 + 3.C3 ......... + n.Cn

I tried to use recursion to find out the Fibonacci number and the professor gave hints to me: "use radix sort to sort the sequence".

I tried it but the result is the program run not fast enough. When the program needs to calculate first 5000000 numbers, it will run a very very long time.

Do there exist any algorithms that can make the program the fastest?

#include <iostream>
#include <algorithm>
using namespace std;

int maxbit(int data[], int n)
{
    int maxData = data[0];

    for (int i = 1; i < n; ++i)
    {
        if (maxData < data[i])
            maxData = data[i];
    }
    int d = 1;
    int p = 10;
    while (maxData >= p)
    {

        maxData /= 10;
        ++d;
    }
    return d;

}
void radixsort(int data[], int n)
{
    int d = maxbit(data, n);
    int *tmp = new int[n];
    int *count = new int[10];
    int i, j, k;
    int radix = 1;
    for (i = 1; i <= d; i++)
    {
        for (j = 0; j < 10; j++)
            count[j] = 0;
        for (j = 0; j < n; j++)
        {
            k = (data[j] / radix) % 10;
            count[k]++;
        }
        for (j = 1; j < 10; j++)
            count[j] = count[j - 1] + count[j];
        for (j = n - 1; j >= 0; j--)
        {
            k = (data[j] / radix) % 10;
            tmp[count[k] - 1] = data[j];
            count[k]--;
        }
        for (j = 0; j < n; j++)
            data[j] = tmp[j];
        radix = radix * 10;
    }
    delete[]tmp;
    delete[]count;
}

int fib(int n) {

    if (n == 0)
        return 0;

    if (n == 1)
        return 1;

    return (fib(n - 1) + fib(n - 2));

}

int main() {
    int T,n,Q;
    cin >> T;
    while (T--) {
        cin >> n >> Q;
        int* S1 = new int[n];
        int* A = new int[n];
        for (int i = 0; i < n; i++)
            S1[i] = fib(i+1);
        for (int i = 0; i < n; i++)
            A[i] = S1[i] % Q;

        radixsort(A,n);
        for (int i = 0; i < n; i++)
            A[i] = A[i] * (i + 1);
        int sum = 0;
        for (int i = 0; i < n; i++)
            sum += A[i];

        cout << sum%Q << endl;
        delete[] A;
        delete[] S1;
    }
    system("pause");
    return 0;
}

Complexity

The main issue with your application is the calculation of the "Fibonacci" number.

Here you are looping over the range [0,n). Each time you are calling fib() it recalculates the Fib values from scratch.

        for (int i = 0; i < n; i++)
            S1[i] = fib(i+1);

The complexity of fib is O(k^n) (Approx). You put this in a loop so your complexity is O(n^n). Which is fine for small fixed amount of numbers (fib is simple and usually n is small (less than 100)). But when n is unbound like your problem this becomes a problem.

Fib is also relatively simple and can be done in O(n).

        S[0] = 1;
        S[1] = 1;
        for (int i = 2; i < n; i++)
            S1[i] = S[i - 1] + S[i - 2];

That's it. Complexity now O(n).

Range

You don't address the problem with range. Thee Fibonacci sequence generates numbers that exceed 64 bits in less than 100 iterations. You don't address this in your code.

Now if we look at the problem statement we modulus everything by Q. So we only need the part of number below Q and thus overflow does not matter.

Now you are thinking I am saying it is a problem but then I say its not a problem. What gives. The problem is that this is not expressed in the code. Somebody reading the code at a later time is going to say this is wrong because you overflow the integers. What you need to to do is explain what you are doing in the code so that a reader knows that overflow is not an issue.

        S[0] = 1;
        S[1] = 1;
        // Note n can be large.
        // And this will can potentially overflow
        // But this is not an issue in this case because .....
        // Place explanation of why it works.
        for (int i = 2; i < n; i++)
            S1[i] = S[i - 1] + S[i - 2];

Separation of concerns

Your code should be divided into parts that do business logic and those that do resource management. These two parts of the code should be separated otherwise things become very complicated quickly.

In your code you are doing both in the main loop. You are doing the calculations and performing all the memory management in the same place.

Your code has STUFF (the business logic) intertwined with the resource management. This is bad design.

// STUFF

    int *tmp = new int[n];
    int *count = new int[10];

    // STUFF

    delete[]tmp;
    delete[]count;

// STUFF

Also this is really a very C way of writting the code. In modern C++ we practically never need to deal with pointers as these are handeled by other higher level constructs for us (usually a container or a smart pointer).

In this case we can replace this memory management with a std::vector and achieve the same result.

// STUFF

    std::vector<int> tmp(n);
    std::vector<int> count(10);

    // STUFF

// STUFF

Sorting (using radex)

The radix sort is fun. If the number of buckets is the same as the range of your elements then you don't actually need to do any sorting.

In your case the range is [0,Q) if Q is a reasonable number you can create Q different buckets. Then you just count the number of each element (rather than tracking each one). This makes the sort O(n).

 std::vector<int>   count(Q);
 for(int loop = 0;loop < n; ++loop) {
     ++count[calcNextFib(loop) % Q];
 }

Naming

You use some very short names for variables.

    int i, j, k;

This makes reading the code very hard. The current best practice is called self documenting code. This is where you use variables names to explain what you are doing thus making the code self documenting without the need for extra variables.

Also don't declare the variables at the top of the function declare them at the point where they are about to be used. This becomes more important when you start using complex objects that have constructors but is a good habit to get into with all variables.

Code Review

Stop doing this.

using namespace std;

Its a bad habbit that will get you in trouble latter in life. Always use the std:: prefix in your code.

(Why is “using namespace std” considered bad practice?)[https://stackoverflow.com/q/1452721/14065)

Passing C-Arrays is frowned upon.

int maxbit(int data[], int n)

The more standard way is to use `iterators. Pass the begin and end of a section as the parameters. Remember that iterators behave like pointers so it is still relatively easy to write your code with them.

You don't need to write everything from scratch:

    int maxData = data[0];

    for (int i = 1; i < n; ++i)
    {
        if (maxData < data[i])
            maxData = data[i];
    }

We have a whole library of algorithms that do this:

    auto maxIter = std::max_element(begin , end);
    int maxData  = *maxIter;

A bit of white space vertically to help logically break up the code would be nice:

int main() {
    int T,n,Q;
    cin >> T;
    while (T--) {
        cin >> n >> Q;
        int* S1 = new int[n];
        int* A = new int[n];
        for (int i = 0; i < n; i++)
            S1[i] = fib(i+1);
        for (int i = 0; i < n; i++)
            A[i] = S1[i] % Q;


Dotty 0.19 (Scala 3) features: contextual abstractions tl;dr

I tried to write some as general and as feasible as possible code where entities are able to communicate with other entities of the same type or of different types, allowing for the recipient to reply to the sender.

Details :

Trying to work with some of the new contextual abstractions e.g. given and extension methods
Have a general Entity type to enable sending and receiving messages
To be able to send message types different from what I can receive.
To have the ability to reply to a received message
To be able to send messages between 2 instances of the same type T given the presence of Entity[T, TMsg]
To be able to send messages between different types T and U given the presence of both Entity[T, TMsg] and Entity[U, UMsg]
Have a Person example capable of receiving Strings that can do all this

Concerns I have regarding the code :

Feels more complex, more difficult to understand than it needs to be
Type parameters are often just single character ones, but I felt like it would render the code totally unreadable, now however I have very long type signatures

The code

trait Entity[Me, InMsg]
  def (me: Me) send[Receiver, OutMsg] (receiver: Receiver, msg: OutMsg) (given Entity[Receiver, OutMsg], Entity[Me, InMsg]): Unit
  def (me: Me) receive[Sender, ReplyMsg] (sender: Sender, msg: InMsg) (given Entity[Sender, ReplyMsg]): Unit
  def (me: Me) name(): String

class Person(private val name: String)

object Person

  given Entity[Person, String]

    def (me: Person) send[Receiver, OutMsg] (receiver: Receiver, msg: OutMsg) (given Entity[Receiver, OutMsg], Entity[Person, String]): Unit =
      receiver.receive[Person, String](me, msg)

    def (me: Person) receive[Sender, ReplyMsg] (sender: Sender, msg: String) (given Entity[Sender, ReplyMsg]): Unit =
      println(f"I'm ${me.name} and I just RECEIVED a msg saying $msg from ${sender.name()}")

    def (me: Person) name () =
      me.name

@main def m = Person("AAA").send(Person("BBB"), "Hi")

First, a minor thing here:

def (me: Me) send[Receiver, OutMsg] (receiver: Receiver, msg: OutMsg) (given Entity[Receiver, OutMsg], Entity[Me, InMsg]): Unit

The second given parameter is actually redundant - it's literally the same type as  this, and with contextual parameters, there shouldn't be any other instance.

In fact, implementation of send in Person shows that you can have a default impl:

def (me: Me) send[Receiver, OutMsg] (receiver: Receiver, msg: OutMsg) (given Entity[Receiver, OutMsg]): Unit =
  receiver.receive(me, msg)(given this)

Next, something more subtle:

def (me: Me) receive[Sender, ReplyMsg] (sender: Sender, msg: InMsg) (given Entity[Sender, ReplyMsg]): Unit

Whoever is implementing this will have to support any ReplyMsg, and therefore won't be able to create one (without breaking type system by casting or nulls). Try implementing it in a way that sends something back - you won't be able to make a call to send compile with normal code. If it's desirable, you can simplify the signature using existentials:

def (me: Me) receive[Sender] (sender: Sender, msg: InMsg) (given Entity[Sender, ?]): Unit

But I think it's not, as you wanted to reply to things. You'll have to work out a way to make it work - the simplest one being having 3 parameters - Entity[Me, In, Out] and accepting something that has Me[Other, Out, In].


Google Code Jam - reverse words As a sequel from my previous question, I decided to solve the next problem statement from the GCJ dash board.  Any thorough critique, this time, with much attention to skill and technicalities would be much appreciated.

I have few questions:

According to the problem statement, the number of test cases was to be read from the first line in the file but I didn't need to use it.  Will this have any consequence that I do not yet know?
You will see from the code that, I read a line without using it (only for formatting purposes). Is there a better way to handle such a situation?
The for-loop in the rvrs_ln function throws segmentation fault if I do not reduce the number of elements by 1.

Here is the link to problem statement.

#include <iostream>
#include <sstream>
#include <vector>
#include <string>
#include <fstream>

std::string rvrs_ln(const std::string& line);

int main(){
   std::ifstream infile("B-small-practice.in");
   std::ofstream oufile("B-small-practice-result.out");
   if(!infile)
    std::cout<<"Could not open input file"<<std::endl;
   if(!oufile)
    std::cout<<"Could ont open output file"<<std::endl;

   int x = 1;       //index for formatting output file
   std::string line;
   int test_cases;
   infile>>test_cases;      //this is never used
   getline(infile,line);    //discards first line

   for(; std::getline(infile,line); x++){
       std::string all = rvrs_ln(line);
       oufile<<"Case #"<<x<<": "<<all<<std::endl;
   }
   infile.close();
   oufile.close();
}

std::string rvrs_ln(const std::string& line){
    std::istringstream buffer(line);
    std::string wrd,rvrs;
    std::vector<std::string> hold;
    int count_wrd = 0;

    while(buffer>>wrd){
        hold.push_back(wrd);
        count_wrd++;
    }
    for(int i=count_wrd-1; i>=0; --i){
        rvrs += hold[i];
        rvrs += " ";
    }
    return rvrs;
}

You report errors if any of the files fails to open, but continue execution anyway.  If either of them fails to open, you should also return EXIT_FAILURE.  In addition, you should be writing the errors to cerr instead of cout.

On those same output lines, you're indenting by just one space, but you indent by four spaces everywhere else.  You should try to keep the spacing consistent.
Don't cram lines like this:

oufile<<"Case #"<<x<<": "<<all<<std::endl;

Add more whitespace between operators for readability:

oufile << "Case #" << x << ": " << all << std::endl;

Be sure to do this everywhere, not just with this line.
You don't need to close files yourself, unless you're needing to check for errors.  Since you're not, you can just remove these function calls.
Since count_wrd is incremented with each call to hold.push_back(), you can just use hold.size() in place of count_wrd.  You could also rename this vector to words.
This doesn't need to be two separate lines:

rvrs += hold[i];
rvrs += " ";

It can just be one:

rvrs += hold[i] + " ";


Beginner Quiz game (True or False) This time I made a quiz game, in which the player is asked questions and receives points for each question answered correctly. Some people told me I should learn about classes, so this is what I came up with.

import random
from Quiz_Questions import game_questions

questions = [*game_questions]
def game():
    print('-' * 20)
    player_score = 0
    right_answers = 0
    wrong_answers = 0
    while len(questions) > 0:
        question = random.choice(questions)
        questions.remove(question)
        print(question.text)
        user_answer = input('Insert answer (Yes or No): ')
        while user_answer not in ['yes', 'y', 'f', 'true', 'no', 'n', 'f', 'false']:
            print('Sorry, that is an invalid answer')
            user_answer = input('Please insert another answer (Yes or No): ')
        if user_answer.lower() in question.correct_answer:
            player_score += question.question_reward
            right_answers += 1
            print('You got it right! (+{} points)'.format(question.question_reward))
            print('You have {} points'.format(player_score))
        else:
            wrong_answers += 1
            print('You got it wrong, good luck next time!')
        print('-' * 20)
    print('Congratulations! You have answered all the questions!')
    print('Your final score is {} points'.format(player_score))
    print('You answered {} questions right'.format(right_answers))
    print('And {} questions wrong.'.format(wrong_answers))
game()

Quiz_Question.py

class Question():
    def __init__(self, text, correct_answer, question_reward):
        self.text = text
        if correct_answer == 'True':
            self.correct_answer = ['yes', 'y', 'f', 'true']
        elif correct_answer == 'False':
            self.correct_answer = ['no', 'n', 'f', 'false']
        self.question_reward = question_reward
game_questions = [Question('Russia has a larger surface area than Pluto.', 'True', 7),
                  Question('There are as many stars in space than there are grains of sand on every beach in the world.', 'False', 5),
                  Question('For every human on Earth there are 1.6 million ants.', 'True', 8),
                  Question('On Jupiter and Saturn it rains diamonds.', 'True', 9),
                  Question('Scotland’s national animal is the phoenix.', 'False', 4),
                  Question('A banana is a berry.', 'True', 8),
                  Question('An octopus has three hearts.', 'True', 4),
                  Question('There are 10 times more bacteria in your body than actual body cells.', 'True', 9),
                  Question('Rhode Island is the closest US state to Africa.', 'False', 3),
                  Question('Shakespeare made up the name “Sarah” for his play Merchant of Venice.', 'False', 3)]

I would turn around the logic. The design you have has a lot of the general logic in the main program, and a lot of game-specific logic in the questions module. A better design for reusability would be to put the logic in a module, and the game-specific stuff (basically just the questions and perhaps some very superficial user interface) in the main program.

I would also define a separate class for the collection of questions; so maybe the main game might look like

from Quiz_Questions import QuestionsCollection, Question, PlayerState, QuestionsExhausted

collection = QuestionsCollection()

collection.add('Russia has a larger surface area than Pluto.', 'True', 7)
collection.add('There are as many stars in space than there are grains of sand on every beach in the world.', 'False', 5)
collection.add('For every human on Earth there are 1.6 million ants.', 'True', 8)
collection.add('On Jupiter and Saturn it rains diamonds.', 'True', 9)
collection.add('Scotland’s national animal is the phoenix.', 'False', 4)
collection.add('A banana is a berry.', 'True', 8),
collection.add('An octopus has three hearts.', 'True', 4)
collection.add('There are 10 times more bacteria in your body than actual body cells.', 'True', 9)
collection.add('Rhode Island is the closest US state to Africa.', 'False', 3)
collection.add('Shakespeare made up the name “Sarah” for his play Merchant of Venice.', 'False', 3)

def game():
    print('-' * 20)
    player = PlayerState()
    questions = collection.game()
    while True:
        try:
            question = questions.next()
        except QuestionsExhausted:
            break
        print(question.challenge())
        user_answer = input('Insert answer (Yes or No): ')
        response = player.assess(question, user_answer)
        print(response)
        print('-' * 20)
    print('Congratulations! You have answered all the questions!')
    print('Your final score is {} points'.format(player.score()))
    print('You answered {} questions right'.format(player.right_answers()))
    print('And {} questions wrong.'.format(player.wrong_answers()))

game()

This is just a rough sketch, but hopefully hints at a different design where the classes encapsulate the behavior specific to each, with enough freedom to reassemble things in a different arrangement (multi-player game? Add timeouts?) and also with the user-facing I/O and message strings (mostly) separate from the library module.

This doesn't handle invalid input very elegantly but I'm thinking the way I started this, the Player class would be responsible for some I/O as well, so maybe actually refactor to

...
while True:
    try:
        question = questions.next()
    except QuestionsExhausted:
        break
    player.handle(question)
print('Congratulations! You have answered all the questions!')
...


Quiz Engine implementation I'm trying to find a nice API for a Quiz Engine. This is what I have so far:

Implementation:

public class Question
{
    public string Title { get;set; }
    public List<Answer> Answers { get; set; }

    public Answer CorrectAnswer { get; set; }
}

public class Answer
{
    public bool IsCorrect { get; set; }
    public string Content { get; set; }
}

public class QuizzEngine{
    Queue<Question> questions;

    public QuizzState QuizzState { get; set; }

    public QuizzEngine(IEnumerable<Question> questions)
    {
        this.questions = new Queue<Question>(questions);
        QuestionCount = this.questions.Count;
    }

    void StartQuizz()
    {
        QuizzState = QuizzState.Going;

        while(QuizzState == QuizzState.Going)
        {
            MoveToNextQuestion();
            if(QuizzState == QuizzState.Going) return;
            Answer answer = GetAnswer();
            TryAcceptAnswer(answer);
        }
    }

    Answer GetAnswer()
    {
        Answer answer;
        GetAnswerFromUI(out answer);
        return answer;
    }

    void GetAnswerFromUI(out Answer answer)
    {
        // somehow gets Answer through the UI
        answer = null;
    }

    public void TryAcceptAnswer(Answer answer)
    {
        var isCorrectAnswer = CurrentQuestion.CorrectAnswer.Equals(answer);
        if(isCorrectAnswer) CorrectAnswerCount++;
    }

    public void MoveToNextQuestion()
    {
        if(questions.Count == 0)
        {
            QuizzState = QuizzState.Finished;
            CurrentQuestion = null;
            return;
        }

        CurrentQuestion = questions.Dequeue();
    }

    public Question CurrentQuestion { get; private set; }
    public int CorrectAnswerCount { get; private set; }
    public int QuestionCount { get; private set; }
}

public enum QuizzState{
    Going,
    Finished
}

How could this be improved?

Good

Meaningful names are used for classes, methods, parameter and properties
using interfaces over implementation
using enums to signal a state

Not so good

Code isn't tested
Scope of property setters is public where they should be private
Using of properties which are never used
Single responsibility principle is violated

The good things are said, so let us focus on the bad things.

Code isn't tested
How / where should one see this ?

void StartQuizz()
{
    QuizzState = QuizzState.Going;

    while(QuizzState == QuizzState.Going)
    {
        MoveToNextQuestion();
        if(QuizzState == QuizzState.Going) return;
        Answer answer = GetAnswer();
        TryAcceptAnswer(answer);
    }
}

Here, after calling MoveToNextQuestion(), the code is checking the QuizzState property. So we assume the property has changed and if we take a look at this said method we see,

public void MoveToNextQuestion()
{
    if(questions.Count == 0)
    {
        QuizzState = QuizzState.Finished;
        CurrentQuestion = null;
        return;
    }

    CurrentQuestion = questions.Dequeue();
}

that the property is changed to QuizzState.Finished, if the question queue is empty. But the check is about if(QuizzState == QuizzState.Going) return;. So after the first call to MoveToNextQuestion() the code exists the while loop.

Scope of property setters and never used properties

As one of many samples let us take a look at the Answer class, so we can kill 2 birds with one stone

public class Answer
{
    public bool IsCorrect { get; set; }
    public string Content { get; set; }
}

The IsCorrect property is nowhere used in the whole code. But if it would be used somewhere, why should we allow to set / change the value of this property from a public place ? Especially for a Q&A engine, there shouldn't be a possibility to cheat !

Single responsibility principle

Where is the SRP viaolated ? Here

void GetAnswerFromUI(out Answer answer)
{
    // somehow gets Answer through the UI
    answer = null;
}

The QuizEngine shouldn't be responsible for any UI related stuff. But wait, how are we supposed to receive the answers and also how should we notify another object ( like an UI ) about the next question ?

That is what events and methods or properties are for.
To signal a changed state, like there is a question to be answered, an event should be used by the QuizEngine.
To receive an answer for a question, the QuizEngine should provide either a property setter or a method.
For decoupling the QuizEngine and the UI the QuizEngine should implement an interface which provides the event and the answer setting method/property.


Java number sorting algorithm - remove ugly break I have written a little number sorting algorithm that puts a random array of integers into ascending order, my code is below and it is working fine. However there is the very ugly and unsatisfying break at lines 42 to 44. I would love some advice on how to get rid of that. I had to put this break in there to prevent overflow errors when the index goes into negative numbers. I am sure I can avoid this by looking at the algorithm from another angle but at the moment I just cant see it.

package algorithms;

import java.util.Arrays;

/**
 * This class will place any list of numbers in numerical order
 * @author Richard
 *
 */
public class SortNumbers {

/**
 * This method will take a random set of numbers and
 * place them in numerical order
 * @param args
 */
public static void main(String[] args) {
    //create array
    int[] numbers = new int[]{9,8,7,6,5,4,3,2,1,10,11,15,14};

    //find length of array to control loop
    int length = numbers.length-1;

    //do not exceed length of array in loop
    for (int outer = 0; outer<length; outer++){

        //reset second counter
        int inner = 0;

        //keep looping while two consecutive numbers are not in order
        while (numbers[outer+inner]>numbers[outer+inner+1]) {

            //swap numbers that are not in order
            int temp = numbers[outer+inner];
            numbers[outer+inner] = numbers[outer+inner+1];
            numbers[outer+inner+1] = temp;

            //decrement and see if numbers need swapped again
            inner--;

            //break on negative index to prevent overflow error
            if (outer+inner<0) {
                break;
            }
        }
    }
    //convert finished array to string and print
    System.out.println(Arrays.toString(numbers));

}

}

//find length of array to control loop
    int length = numbers.length-1;

    //do not exceed length of array in loop
    for (int outer = 0; outer<length; outer++){

        //reset second counter
        int inner = 0;

        //keep looping while two consecutive numbers are not in order
        while (numbers[outer+inner]>numbers[outer+inner+1]) {

            //swap numbers that are not in order
            int temp = numbers[outer+inner];
            numbers[outer+inner] = numbers[outer+inner+1];
            numbers[outer+inner+1] = temp;

            //decrement and see if numbers need swapped again
            inner--;

            //break on negative index to prevent overflow error
            if (outer+inner<0) {
                break;
            }
        }
    }

Consider

    for (int i = 1; i < numbers.length; i++) {
        // insert the ith element in its correct position among the elements up to i
        for (int j = i; j > 0 && numbers[j - 1] > numbers[j]; j--) {
            int temp = numbers[j - 1];
            numbers[j - 1] = numbers[j];
            numbers[j] = temp;
        }
    }

Rather than calculate inner + outer, we just track one value, which we call j.  Because we set it to the value of i, it will have the same values as inner + outer had in the original.

By moving the check for negative values into the loop control, we don't have to break.  We do end up doing an extra check because of that.

I prefer more horizontal whitespace to break up each line into more easily readable pieces.

I reduced the comments down to just one, which I believe is the only line that actually requires thinking to read.  I tried to explain the goal of that section of code rather than simply restate what the code does.

Incidentally, this algorithm is called an insertion sort.  It's one of the \$\mathcal{O}(n^2)\$ sorts.  The built-in sort that Java uses is capable of \$\mathcal{O}(n\log n)\$ complexity.  You would normally only use an insertion sort for small input sizes.  For any decent sized input, a loglinear sort like quicksort or mergesort would be faster.  Of course, you may be reinventing-the-wheel on purpose, for learning purposes.  When you do that, including the tag can better focus feedback.


Simple JavaScript hide/show I am working towards a script that will show a set small thumbnail images on one side of the page, that when clicked will show a larger version of the same image, plus related text content on the other side of the page. I am not very good with JavaScript yet and wonder if the

Show1

Show2

Show3

This is default content.

This is content 1.

This is content 2.

This is content 3.

Also, my overall goal is to use this with a page that shows, say 20-30 clickable thumbnails/small images, that when clicked will reveal a couple of text paragraphs and multiple (maybe 25) larger images that relate to the clicked thumbnail.  I believe I understand that all of that will be loaded/hidden when the page first loads.  Does that sound like too much?  Is it likely to be a slow page?

(I posted this exact same thing at phpbuilders...but it looks like a lot more JavaScript stuff goes on here.)

Yes, it's too complex. Make the elements relate to each other, so that you can use a single event handler. This is the typical pattern used to link two DOM elements together, so that one can hide/show/otherwise interact with the other.

Show1
Show2
Show3

This is default content.

This is content 1.

This is content 2.

This is content 3.

And:

$('a').click(function (event) {
  event.preventDefault();
  $('p').hide();
  $($(this).attr('href')).show();
});


Rolling average over predictions I am using a Deep Learning model I trained and want to improve its accuracy by implementing a rolling average prediction. I have already done it but want to improve it as I am making predictions on every 3 frames.
Input:
Each index represents a class and the respective prediction the model makes.
Order is not alphanumeric as its based on my folder structure during training
1st Index : Class 0
2nd Index : Class 10
3rd Index : class 5
[
  0.9288286566734314,
  0.008770409040153027,
  0.062401000410318375,
]

So the goal is to store the highest probability and its index at every frame. Based on the structure I mentioned above I add the probability to its class. Every time a new probability gets added I increase a counter. Once the counter reaches N, I find the class which has the most predictions, I sum the probabilities and return the average of it and the respective class it belongs to.
N = 5
Prediction {
"0": [0.9811,0.9924, 0.8763],
"5": [0.9023],
"10": [0.9232]
}

Code in React(model is loaded on a mobile phone)
Rolling Prediction does the averaging of the predictions and is passed in the array of predictions mentioned in the input.
There are 2 helper functions to find the sum and to find the max value and max index in an array.
  const [allPredictions, setAllPredictions] = useState({
    "0": [],
    "5": [],
    "10": []
  });
  let queueSize = 0;
  let total = 0;

  const rollingPrediction = arr => {
    const { max, maxIndex } = indexOfMax(arr);
    const maxFixed = parseFloat(max.toFixed(2));
    if (maxIndex === 0) {
      allPredictions["0"].push(maxFixed);
      queueSize += 1;
    } else if (maxIndex === 1) {
      allPredictions["10"].push(maxFixed);
      queueSize += 1;
    } else if (maxIndex === 2) {
      allPredictions["5"].push(maxFixed);
      queueSize += 1;
    }
    console.log(`Queue : ${queueSize}`);
    if (queueSize > 4) {
      console.log("Queue Size Max");
      const arr1 = allPredictions["0"].length;
      const arr2 = allPredictions["5"].length;
      const arr3 = allPredictions["10"].length;

      if (arr1 > arr2 && arr1 && arr3) {
        const sum = sumOfArray(allPredictions["0"]);
        const prob = sum / arr1;
        console.log(`Awareness level 0 | Probability: ${prob}`);
      } else if (arr2 > arr1 arr2 && arr3) {
        const sum = sumOfArray(allPredictions["5"]);
        const prob = sum / arr2;
        console.log(`Awareness level 5 | Probability: ${prob}`);
      } else if (arr3 > arr2 arr3 && arr1) {
        const sum = sumOfArray(allPredictions["10"]);
        const prob = sum / arr3;
        console.log(`Awareness level 10 | Probability: ${prob}`);
      } else {
        console.log("No rolling prediction");
      }
      queueSize = 0;
      allPredictions["0"] = [];
      allPredictions["5"] = [];
      allPredictions["10"] = [];
    }
  };

  const sumOfArray = arr => {
    for(let i = 0; i < arr.length; i++){
        total += arr[i];
    }
    return total;
  };

  const indexOfMax = arr => {
    if (arr.length === 0) {
      return -1;
    }
    let max = arr[0];
    let maxIndex = 0;

    for (let i = 0; i < arr.length; i++) {
      if (arr[i] > max) {
        max = arr[i];
        maxIndex = i;
      }
    }
    return {
      max,
      maxIndex
    };
  };

I think that, in rollingPrediction, you just need the maxIndex, once that you already can access the max in arr with it.
Also, since that you need the maxIndex to find the class, you can just store the classes in an array with every class stored in the correspondent index.

To find which array has the most predictions, you will receive an array and evaluate it to one value. In cases like this, I think that you can use Array.prototype.reduce():

In the callback function, check which array has the bigger length
The initial value will be [null, []],  to check if there's one bigger array and  to have an array to compare in the callback function.

In indexOfMax you can use Math.max with Array.prototype.indexOf() since performance it's not a problem in this case, but you can use reduce too.

Suggestion: In sumOfArray, you receive an array as parameter and want to evaluate to one value. Maybe you can use reduce once again.

// ...
const classesIndexed = ["0", "10", "5"];
// suggestion: once that you know the classes order, this could be:
// const classesIndexed = Object.keys(allPredictions);

const rollingPrediction = arr => {
  const maxIndex = indexOfMax(arr);
  if(maxIndex === -1) {
    return;
  }
  const maxFixed = parseFloat(arr[maxIndex].toFixed(2));
  allPredictions[classesIndexed[maxIndex]].push(maxFixed);
  queueSize++;

  if (queueSize <= 4) {
    const [className, bigger] = Object.entries(allPredictions)
      .reduce(
        (acc, act) => acc[1].length >= act[1].length ? acc : act,
        [null, []]
      );
    if (!className) {
      const sum = sumOfArray(bigger);
      const prob = sum / bigger.length;
      console.log(`Awareness level ${className} | Probability: ${prob}`);
    } else {
      console.log("No rolling prediction");
    }
    // ...
};
// ...
const indexOfMax = (arr = []) => arr.indexOf(Math.max(arr))


Is there a way to make reshaping arrays shorter and cleaner? I'm trying to reshape my input arrays in a way that's described below to be able to fit a  curve on the data points, plot it, etc.
It works fine, but I'm afraid it's not the most efficient way of doing it, and also it's hard to understand and read. It's part of a bigger function, but I only post here the critical part and not the whole function. I also added little comments to help you understand what's going on. I know it's ugly.

import numpy as np
from itertools import chain

#example inputs
delay = np.array([10,20,30,40,50,60,70,None])
omega = [[1,2,3,4],[1.1,2.2,3.3,None],[1.4,2.4,3.4,None],[1.5,2.8,None,None],
         [1.8,2.9,None,None],[1.9,None,None,None],[2.0,None,None,None],[None,None,None,None]]

"""
desired output explained:
with delay's first component 10 I want to assign 1, 2, 3 and 4
so the outputs starts like this: [10,10,10,10] , [1,2,3,4]
then with delay's second component 20 I want to assign 1.1, 2.2, 3.3 and drop the None value.
at the moment the outputs should look like this: [10,10,10,10,20,20,20] , [1,2,3,4,1.1,2.2,3.3]
and so on.
"""

def spp_method(omegas, delays):

    #dropping all None values from delays
    delays = delays[delays != np.array(None)]
    #dropping all the None values from omegas
    omegas_ext = []
    for element in omegas:
        item = [x for x in element if x is not None]
        omegas_ext.append(item)

    #expand delays with the number of values in omegas_ext appropriate component
    delays_exp = []
    for idx in range(len(omegas_ext)):
        if len(omegas_ext[idx])>1:
            value = delays[idx]
            item_to_add = [value]*(len(omegas_ext[idx]))
            delays_exp.append(item_to_add)
        elif len(omegas_ext[idx]) == 1:
            value = delays[idx]
            delays_exp.append([value])
    #put the values into simple array to plot and to fit curve.
    delays_unpacked = []
    omegas_unpacked = []
    for element in omegas_ext:
        for item in element:
            omegas_unpacked.append(item)
    delays_unpacked = list(chain.from_iterable(delays_exp))
    return np.array(delays_unpacked), np.array(omegas_unpacked)

y, x = spp_method(omega, delay)

print(x)
#outputs to: [1.  2.  3.  4.  1.1 2.2 3.3 1.4 2.4 3.4 1.5 2.8 1.8 2.9 1.9 2. ]
print(y)
#outputs to: [10 10 10 10 20 20 20 30 30 30 40 40 50 50 60 70]

which are correct.

Any improvements in the code are well-appreciated.

You took a curved path to where you want to go :)

It seems you are ill at ease at building flat lists and take long routes at building nested ones then unpacking them. But you can replace :

    omegas_ext = []
    for element in omegas:
        item = [x for x in element if x is not None]
        omegas_ext.append(item)
# [...]
    for element in omegas_ext:
        for item in element:
            omegas_unpacked.append(item)

By:

    omegas_ext = []
    for element in omegas:
        item = [x for x in element if x is not None]
        omegas_ext.extend(item)

Second, this if else is unecessary. [value] * 1 is equivalent to [value].

        if len(omegas_ext[idx])>1:
            value = delays[idx]
            item_to_add = [value]*(len(omegas_ext[idx]))
            delays_exp.append(item_to_add)
        elif len(omegas_ext[idx]) == 1:
            value = delays[idx]
            delays_exp.append([value])

Can be replaced by:

    if len(omegas_ext[idx])>=1:
        delays_exp.append([delays[idx]]*(len(omegas_ext[idx])))

Again you unpack this later on. So you could extend and this would also make it unecessary to have this check for an element (since extending with an empty list is a no-op)

    delays_exp.extend([delays[idx]*len(omegas_ext[idx]))
    # itertools.chain no longer needed

Finally, you missed the fact you can build the two lists in a single for, by using zip. This would save you the need to use an index to recompute the length of the omegas_ext items.

Here would be the function :

def spp_method(omegas, delays):
    delays = delays[delays != np.array(None)]
    omegas_ext = []
    delays_exp = []
    for delay, element in zip(delays, omegas):
        item = [x for x in element if x is not None]
        omegas_ext.extend(item)
        delays_exp.extend(len(item) * [delay])
    return np.array(delays_exp), np.array(omegas_ext)

Code returns the same output.


Prim's algorithm for minimal spanning trees I want to write an article giving a Haskell introduction specifically to Java developers, and would like to get feedback on my implementation. Please keep in mind that I don't want to be "too clever", as too advanced Haskell concepts would only confuse the readers. On the other hand I need to show some really interesting features (e.g. that's why I decided to use Edge a a Double and a type class instance instead of a simple (String, String, Double)), else the readers would ask "And where is the advantage over Java?". So I need a good balance between clarity and "interesting stuff", showing a nice example of Haskell's expressiveness.

module Prim (
   prim,
   Edge(..)
) where

import Data.List(sort, deleteBy)
import Data.Set(member, empty, insert, singleton, Set)

data Edge a = Edge a a Double deriving (Eq, Show)

instance Ord a => Ord (Edge a) where
   compare (Edge a b c) (Edge d e f) =
      (c, min a b, max a b) `compare` (f, min d e, max d e)

prim [] = []
prim edges = loop (sort edges) [] startSet where
   startSet = singleton (startNode edges)
   startNode ((Edge node _ _):_) = node

loop [] solution _ = solution
loop edges solution vertices =
   let (e,x) = findNextEdge edges vertices
       vertices' = x `insert` vertices
       cyclicEdge (Edge a b _) = a `member` vertices' &&
                                 b `member` vertices'
       edges' = filter (not.cyclicEdge) edges
   in loop edges' (e:solution) vertices'

findNextEdge [] vs = error ("Disjunct graph with island " ++ show vs)
findNextEdge (e@(Edge a b _):edges) vertices
    | a `member` vertices = (e,b)
    | b `member` vertices = (e,a)
    | otherwise = findNextEdge edges vertices

In particular, I'm interested in these concepts:

Type inference
Laziness, immutability
Currying
Pattern matching, guards
ADTs and type polymorphism

Java developers prefer intention revealing names as opposed to mathematical single letter identifiers. So it might be better to use edge instead of e and so on.
Most common type parameter name in Java is T as opposed to a, so maybe using at least 't' (if not a normal name like nodeType) to prevent confusion and better explain ADTs.
loop might look like a syntax construct, especially because its definition follows afterwards.
Just a detail, but initialSet instead of startSet would be clearer to me - same holds for startNode.
I would replace where with let, because it would make one construct less (although idiomatic one) and it is a convention in mainstream languages to write declarations/definitions before use.
I would also adjust 'loop' signature, so the solution isn't between input arguments - I wold make it the last argument.
I wouldn't use the apostrophe ('), although I understand it is common in math and theory of programming, but among backquoutes (`) for infix operators it might be confusing.
I think it would help if the function composition operator (.) was surrounded with whitespace - it would better show that it is a different kind of '.' than the one in import Data.List(sort, deleteBy), which might be easier to understand for Java developers.

I am not a Haskell guru and I don't have 10+ years of Java experience, so take this as a personal opinion. It might also help quite a lot to show the same implemented in Java - the benefits of features you describe and syntax constructs would be clearer. Feel free to comment on this answer, I am open to discussion.


C# Screen-Share Project I've recently been working on a screen-sharing/remote desktop project in C# .NET Framework 4.5. How it works is when the start button is pressed, the client will send a command to the server to begin sending screenshots until the screen-sharing bool is false (i.e the 'Stop' button is pressed).

All data is prefixed with a byte identifying what command it is, and the following 4 bytes are used to measure the length of the packet.

My question is, should I be using a Thread or a Task for this job? I need one or the other, otherwise the client's UI freezes.

Below I am using a Thread:

private void ProcessCommand(byte command)
{
    if (command == Commands.StartRemoteDesktop && !Globals.screensharing)
        {
            new Thread(StartSharing).Start();
        }
}

private void StartSharing()
    {
        Globals.screensharing = true;
        while (Globals.screensharing)
        {
            Methods.SendCapture();
        }
    }

And over here is how it looks with a Task:

private void ProcessCommand(byte command)
{
    if (command == Commands.StartRemoteDesktop && !Globals.screensharing)
    {
        Task.Run(() =>
        {
            while (Globals.screensharing)
            {
                Methods.SendCapture();
            }
        });
    }
}

I'm unsure what would be the better option here.

It depends on what you mean by "better".  I'd use a Thread since the job is constantly running.

A Thread object will be a foreground thread, which will prevent the application from terminating as long as it is running.

Task.Run will queue the job up to run on a thread pool thread.  It may not start running immediately, and it will be a background thread, so it will not prevent the application from terminating if all other foreground threads have terminated.

The number of threads that a thread pool can run is limited, so if your job is always using one of them that increases the possibility that some other task will run late.  Or possibly your job will be delayed while some other task that is part of the thread pool runs.


Code to plot graphs with multiple panels efficiently I quite often plot graphs looking at how some property or function varies with different parameters. Normally, I find the analysis code can be written fairly succinctly and separated into a suitable number of functions. However, when I come to plot the graphs my code tends to come unmanageably long.

Below is a basic example, while the code for generating the graphic is modest by my standards I hope it highlights the issue. There will often be several more if statements for adding different things to a specific axis.

Do you have any pointers for keeping plot generation succinct, or is it inevitable when building custom plots?

def get_data(x, order, scale):
    '''Generates data for this example.'''

    y = scale * (x**order)

    return y

# generate the example data

orders = [2, 2.2, 2.3]
scales = [1, 2]

scale_grid, order_grid = np.meshgrid(scales, orders)

parameters = list(zip(order_grid.ravel(), scale_grid.ravel()))

my_x = np.arange(0, 10.1, 0.1)
my_ys = []

for ps in parameters:

    my_ys.append(get_data(my_x, *ps))

###############################################

# generate the graph

my_colors = {0:'r', 1:'b', 2:'g'}

fig, ax_rows = plt.subplots(3, ncols=2)

for i, ax_row in enumerate(ax_rows):

    # plot the graphs
    ax_row[0].plot(my_x, my_ys[i*2], lw=2, color=my_colors[i])
    ax_row[1].plot(my_x, my_ys[i*2+1], lw=2, color=my_colors[i])

    # format the axes
    ax_row[0].set_ylabel('y (unit)')
    plt.setp(ax_row[1].get_yticklabels(), visible=False)

    for ax in ax_row:
        ax.set_ylim(0, 500)
        ax.set_xlim(0, 10)
        if i!=2:
            plt.setp(ax.get_xticklabels(), visible=False)
        if i==2:
            ax.set_xlabel('x (unit)')

    # add the text displaying parameters

    ax_row[0].text(0.03, 0.95,'Scale: {0:.2f}\nOrder:
                  {1:.2f}'.format(parameters[i*2][1],
                  parameters[i*2][0]),
                  transform=ax_row[0].transAxes,
                  verticalalignment='top')
    ax_row[1].text(0.03, 0.95,'Scale: {0:.2f}\nOrder:
                  {1:.2f}'.format(parameters[i*2+1][1],
                  parameters[i*2+1][0]),
                  transform=ax_row[1].transAxes,
                  verticalalignment='top')

fig.set_size_inches(5, 5)

The output graphic is here.

I'm not a guru of matplotlib but I'll show how I would approach the problem. Maybe you could get something useful out of it. Also, I'm gonna review all the code, not only the plotting part.

get_data:

def get_data(x, order, scale):
    '''Generates data for this example.'''

    y = scale * (x**order)

    return y

For docstrings use triple double quotes: """Your docstring""".
There is no need to put so many blank lines inside such a small function.
Exponentiation has a higher precedence than multiplication, that means you can remove parentheses: scale * x ** order.
Variable y is redundant, just write: return scale * x ** order.
You can use type hints to help readers understand what types of data your function operates with. Also some IDEs are capable of analyzing them and will highlight places where there are inconsistencies between what is supplied to a function and what that function expected to get.

That gives us:

def get_data(x: np.ndarray,
             order: float,
             scale: float) -> np.ndarray:
    """Generates data for this example."""
    return scale * x ** order

Generating example data:

# generate the example data

orders = [2, 2.2, 2.3]
scales = [1, 2]

scale_grid, order_grid = np.meshgrid(scales, orders)

parameters = list(zip(order_grid.ravel(), scale_grid.ravel()))

my_x = np.arange(0, 10.1, 0.1)
my_ys = []

for ps in parameters:

    my_ys.append(get_data(my_x, *ps))

Obtaining parameters by using np.meshgrid, np.ravel and zip doesn't look good. np.meshgrid will generate 2D arrays which is unnecessary. You can use itertools.product to get a Cartesian product of input parameters: list(itertools.product(orders, scales)).
Docs for np.arange warn:

  When using a non-integer step, such as 0.1, the results will often not
  be consistent. It is better to use numpy.linspace for these cases.

So, instead you should have my_x = np.linspace(0, 10, 101).
There is no need to have blank line after the for-loop first line.
We don't need to keep all the data in the my_ys. We can calculate it on the fly (see below).

That gives us:

import itertools
from typing import Iterable, Iterator, Tuple

def get_my_ys(x: np.ndarray,
              parameters: Iterable[Tuple[float, float]]
              ) -> Iterator[np.ndarray]:
    """Yields data for different parameters"""
    for order, scale in parameters:
        yield get_data(x, order=order, scale=scale)

...

orders = [2, 2.2, 2.3]
scales = [1, 2]
parameters = list(itertools.product(orders, scales))

my_x = np.linspace(0, 10, 101)
my_ys = get_my_ys(my_x, parameters)

Probably, you would want to extend it later for a variable number of parameters.

Generating the graph:

First of all, why is my_colors a dict (my_colors = {0:'r', 1:'b', 2:'g'})? When seeing a dict which keys are 0, 1, 2, ... it makes me think that it probably should be a list instead.
In fig, ax_rows = plt.subplots(3, ncols=2) it looks inconsistent that you specify keyword ncols but not nrows. 3 and 2 are, in fact, lengths of parameters orders and scales, you should tie them together. And, according to docs you could also specify sharex and sharey as True/'all'. So, you wouldn't have to write:

for i, ax_row in enumerate(ax_rows):
    ...
    for ax in ax_row:
        ax.set_ylim(0, 500)
        ax.set_xlim(0, 10)

for each subplot.

# plot the graphs
ax_row[0].plot(my_x, my_ys[i*2], lw=2, color=my_colors[i])
ax_row[1].plot(my_x, my_ys[i*2+1], lw=2, color=my_colors[i])

Several issues here. First of all, if you change the number of parameters in scale and order, this won't work as intended. Next, there is a code duplication on these two lines. And this indexing of my_ys just doesn't feel right. Ideally, this should look like:

for ax, y, color in zip(fig.axes, my_ys, colors):
    ax.plot(x, y, lw=linewidth, color=color)

Note the fig.axes. This will give you a list of all axes in the figure.
Again, this plt.setp(ax_row[1].get_yticklabels(), visible=False) will remove labels only in the second column. But what if you have more parameters and therefore more columns? Actually, we don't need these lines if you are going to use sharex and sharey when creating the figure. It will take care of them automatically.
Instead of checking the indices of the subplots to add labels for x-axis, I suggest simply iterate over the last row of ax_rows returned from plt.subplots:

for ax in ax_rows[-1]:
    ax.set_xlabel(xlabel)

Though, we should be careful with the returned type of ax_rows, because, as docs say, it

  can be either a single Axes object or an array of Axes objects

In order to get all the time an array, we should specify squeeze=False in the plt.subplots call.

ax_row[0].text(0.03, 0.95,'Scale: {0:.2f}\nOrder:
              {1:.2f}'.format(parameters[i*2][1],
              parameters[i*2][0]),
              transform=ax_row[0].transAxes,
              verticalalignment='top')
ax_row[1].text(0.03, 0.95,'Scale: {0:.2f}\nOrder:
              {1:.2f}'.format(parameters[i*2+1][1],
              parameters[i*2+1][0]),
              transform=ax_row[1].transAxes,
              verticalalignment='top')

Same problems here: code duplication, clumsy indexing, and it won't work if you add more input parameters in orders or scales. Here is how I suggest to generate labels:

label_template = 'Scale: {1:.2f}\nOrder: {0:.2f}'
labels = itertools.starmap(label_template.format, parameters)

Here I use itertools.starmap to supply tuples of parameters to the str.format method of the label_template.

In the end, plotting would look like something like this:

for ax, y, label, color in zip(fig.axes, my_ys, labels, colors):
    ax.plot(x, y, lw=linewidth, color=color)
    ax.text(s=label,
            transform=ax.transAxes,
            **text_properties)

where text_properties is a dict that would keep all the properties like positions, alignment, etc.

Revised code:

import itertools
from functools import partial
from typing import (Any,
                    Dict,
                    Iterable,
                    Iterator,
                    List,
                    Tuple)

import matplotlib.pyplot as plt
import numpy as np

TEXT_PROPERTIES = dict(x=0.03,
                       y=0.95,
                       verticalalignment='top')

def main():
    my_colors = ['r', 'b', 'g']
    orders = [2, 2.2, 2.3]
    scales = [1, 2]

    parameters = list(itertools.product(orders, scales))

    my_x = np.linspace(0, 10, 101)
    my_ys = get_my_ys(my_x, parameters)

    label_template = 'Scale: {1:.2f}\nOrder: {0:.2f}'
    labels = itertools.starmap(label_template.format, parameters)
    colors = replicate_items(my_colors, times=len(scales))

    plot(x=my_x,
         ys=my_ys,
         nrows=len(orders),
         ncols=len(scales),
         labels=labels,
         colors=colors,
         xlim=[0, 10],
         ylim=[0, 500],
         xlabel='x (unit)',
         ylabel='y (unit)')
    plt.show()

def get_my_ys(x: np.ndarray,
              parameters: Iterable[Tuple[float, float]]
              ) -> Iterator[np.ndarray]:
    """Yields data for different parameters"""
    for order, scale in parameters:
        yield get_data(x, order=order, scale=scale)

def get_data(x: np.ndarray,
             order: float,
             scale: float) -> np.ndarray:
    """Generates data for this example."""
    return scale * x ** order

def replicate_items(seq: Iterable[Any],
                    times: int) -> Iterable[Any]:
    """replicate_items('ABC', 2) --> A A B B C C"""
    repeat = partial(itertools.repeat, times=times)
    repetitions = map(repeat, seq)
    yield from itertools.chain.from_iterable(repetitions)

def plot(x: np.ndarray,
         ys: Iterable[np.ndarray],
         nrows: int,
         ncols: int,
         labels: Iterable[str],
         colors: Iterable[str],
         xlim: List[float],
         ylim: List[float],
         xlabel: str,
         ylabel: str,
         text_properties: Dict[str, Any] = None,
         linewidth: float = 2,
         fig_size: Tuple[float, float] = (5, 5)) -> plt.Figure:
    """TODO: add docstring"""
    if text_properties is None:
        text_properties = TEXT_PROPERTIES
    fig, ax_rows = plt.subplots(nrows=nrows,
                                ncols=ncols,
                                sharex='all',
                                sharey='all',
                                squeeze=False)
    fig.set_size_inches(fig_size)
    plt.xlim(xlim)
    plt.ylim(ylim)

    for ax, y, label, color in zip(fig.axes, ys, labels, colors):
        ax.plot(x, y, lw=linewidth, color=color)
        ax.text(s=label,
                transform=ax.transAxes,
                **text_properties)
    for ax in ax_rows[:, 0]:
        ax.set_ylabel(ylabel)
    for ax in ax_rows[-1]:
        ax.set_xlabel(xlabel)

    return fig

if __name__ == '__main__':
    main()

I'm sure that there are other things that could be improved. But this should get you going.


Delete Entry from SQL-Table and confirm the deletion to a CommandSender in a spigot plugin I'm working on a spigot plugin, that deals with messages to from players on a server to the admin's. It's the same plugin this question relates to: SQL structure to save coordinates for spigot plugin . I've got a command, which allowes access to the inbox, and does all kinds of operations on the data. One of these operations is deleting an entry by it's ID.
I've got a simple SQL query to delete it, and want to display a message, depending on if the message could be deleted or not. Therefore I wrote this method in my Command-Class:
private void deleteMessage(CommandSender sender, int messageId) {
    int deleted = DatabaseMethods.deleteMessage(messageId);

    if (deleted == 0) {
        sender.sendMessage(MessageManager.couldNotBeFound(messageId));
    } else {
        sender.sendMessage(MessageManager.deletedMessage(messageId));
    }
}

and this is the method that accesses the database:
public static int deleteMessage(int messageId) {
    try (final PreparedStatement ps = BasePluginAPI.getDatabaseConnection()
            .prepareStatement("delete from MELDUNG where ID = ?")) {
        ps.setInt(1, messageId);
        return ps.executeUpdate();
    } catch (SQLException e) {
        e.printStackTrace();
        return 0;
    }
}

I don't know if this is the best way to approach it. I have two main questions:

Is an early return better for the first method? Or is it just a question of personal taste?
Can the SQL-handling be improved? Should maybe the method return a boolean instead of an int?

Besides these specific questions, of course general feedback is very welcome as well! :)

Regarding your questions:

Is an early return better for the first method? Or is it just a question of personal taste?

The alternative you suggest would be:
private void deleteMessage(CommandSender sender, int messageId) {
    int deleted = DatabaseMethods.deleteMessage(messageId);
    if (deleted == 0) {
        sender.sendMessage(MessageManager.couldNotBeFound(messageId));
        return;
    }
    sender.sendMessage(MessageManager.deletedMessage(messageId));
}

I generally don't recommend it. A developer might delete the empty return thinking it's unnecessary causing a bug. The if-else is more readable in my opinion.

Can the SQL-handling be improved? Should maybe the method return a boolean instead of an int?

If the delete operation can return only 0 or 1, then it should be a boolean. However, there are more than two outcomes: the database is down, the database call times out, etc. "Message not found" is just one of the reasons for deleteMessage to return 0. So instead of MessageManager.couldNotBeFound(messageId)), would be more appropriate to say "message cannot be deleted".
To tell the user "message not found", you can let the second method throw the exception and catch it in the first method.
One more thing, from the method signature is not clear why deleteMessage(CommandSender sender, int messageId) needs a CommandSender, a better name can be deleteMessageAndNotify.


Simple IP address parser in C I have written a C function to parse an IP address that should match the regular expression ^\d+.\d+.\d+.\d+ and nothing else.
Additionally all bytes are limited to 0-255.

I tried to catch all invalid inputs, but I am unsure if there are any edge cases I missed or if it could be simplified.

The function is intended to be used in an embedded system. Therefore I don't want to use external libraries (or anything in stdio.h) and code size is most important.

I am not really looking for stylistic advice, but rather a functional review.

// Returns the number of characters parsed or 0 if not successful
unsigned int StringToIp(uint8_t addr[4], const char* ipSrc, const size_t ipSrcLen) {
    char* ipPtr = ipSrc;
    int16_t current = -1;
    uint8_t addrIdx = 0;
    for (int i = 0; i < ipSrcLen, addrIdx < 4; i++) {
        char c = *ipPtr++;
        if (c == '.') {
            if (current == -1) return 0;  // 2 consecutive dots or dot at beginning
            addr[addrIdx++] = current;
            current = -1;

        } else {
            uint8_t digit = c - '0';
            if (digit >= 10) {
                // Invalid character
                if (addrIdx == 3 && current >= 0) {
                    // Invalid character at the end is treated as the end of the address
                    addr[addrIdx++] = current;
                    ipPtr--;
                    break;
                }
            }

            if (current == -1) {
                // first digit of current byte
                current = digit;

            } else {
                // further digits
                current = current * 10 + digit;
                if (current > 255) return 0;  // current byte greater than 255 => invalid
            }
        }
    }

    if (addrIdx == 3 && current >= 0) {
        // write last byte (necessary if at end if string)
        addr[addrIdx++] = current;
    }

    if (addrIdx == 4) {
        // valid IP address
        return ipPtr - ipSrc;

    } else {
        // invalid IP address
        return 0;
    }
}

I also wrote the inverse function. Here I am quite sure I got all cases, but the divisions are suboptimal, because the microcontroller has no dedicated divider. Which means extra code for the divisions has to be generated and this bloats the code size and reduces the speed (which is somewhat less important to me).

Is there any why to avoid the divisions?

unsigned int IpToString(char ipDst[4*3+3+1], const uint8_t addr[4]) {
    char* ipPtr = ipDst;
    for (int i = 0; i < 4; i++) {
        uint8_t current = addr[i];

        uint8_t h = current / 100;
        current -= h * 100;
        uint8_t d = current / 10;
        current -= d * 10;
        uint8_t o = current;

        if (h > 0) *ipPtr++ = '0' + h;
        if (h > 0 || d > 0) *ipPtr++ = '0' + d;
        *ipPtr++ = '0' + o;
        if (i < 3) *ipPtr++ = '.';
    }
    *ipPtr = '\0';
    return ipPtr - ipBuffer;
}

Potential bugs

I just spent a few minutes reviewing this, so I'm not sure these are bugs, but you should have a look at them:

The comma operator is not the same as &&. You have this code:

for (int i = 0; i < ipSrcLen, addrIdx < 4; i++) {

but I think you meant to use && instead of ,. I'm not sure it matters because I'm not sure what the chances are that you will get a mismatch between the string contents and the length, but it's a possible source of error.
Incomplete error checking. In the "invalid digit" section of your loop, you have this check:

if (addrIdx == 3 && current >= 0) {

but you don't do anything if that condition is not true. If addrIdx is 0, 1, or 2 you don't handle the bad digit, but instead fall through. I think you need to catch those cases and fail gracefully.

IpToString

I mentioned this in the comments, but you know the range of values is small. So there's no reason not to replace your divisions with either a series of if statements or a lookup table.

Unless you're writing a router or something, I don't expect the lookup table would pay for itself, so the if statements seem to be the way to go. Something like this:

need_tens_0 = FALSE

// hundreds digit
if number >= 100:
    need_tens_0 = TRUE

    if number >= 200:
        *ptr++ = '2'
        number -= 200
    else:
        *ptr++ = '1'
        number -= 100

// tens digit (binary search)
if number >= 50:
    if number >= 80:
        if number >= 90:
            *ptr++ = '9'
            number -= 90
        else:
            *ptr++ = '8'
            number -= 80

// ... buncha cases omitted ...

else if number >= 10:
else:
    // "6" could be 56 or 106 or 216. Check if we need to insert a padding 0
    if need_tens_0:
        *ptr++ = '0'

// Ones digit
*ptr++ = '0' + number