Stop Watch Application 2.1 After the tips from my previous review. I have come up with these changes:

Separation of Concerns
Use Delegates

I'm asking you guys if this looks good. Other then separate out the FileButton_Click(), WriteResultsToFile(), and PromtSaveAsFileName() functions to another class would you recommend any other things to improve?

public partial class StopWatchForm : Form
    {
    WatchState state;
    Stopwatch stopWatch;
    TimeSpan timeSpan;
    List<TimeSpan> times = new List<TimeSpan>();

    public event Action OnStart = () => { };
    public event Action OnStop = () => { };
    public event Action OnLap = () => { };

    public StopWatchForm()
    {
        InitializeComponent();
        state = WatchState.Stopped;
        stopWatch = Stopwatch.StartNew();
        stopWatch.Stop();
        ConfigureStopWatchBehavior();
        ConfigureTimerBehavior();
        ConfigureTimesBehavior();
        ConfigureViewBehavior();
        ConfigureViewStateBehavior();
    }

    private void StartAndLapButton_Click(object sender, EventArgs e)
    {
        if (state == WatchState.Stopped)
        {
            OnStart();
        } else if (state == WatchState.Started)
        {
            OnLap();
        }
    }

    private void StopButton_Click(object sender, EventArgs e)
    {
        if (state == WatchState.Started)
        {
            OnStop();
        }
    }

    private void FileButton_Click(object sender, EventArgs e)
    {
        if (!times.Any())
        {
            return;
        }
        var filename = PromtSaveAsFileName();
        if (filename == null)
        {
            return;
        }
        WriteResultsToFile(filename);
        times.Clear();
    }

    private void ConfigureStopWatchBehavior()
    {
        OnStart += stopWatch.Restart;
        OnStop += stopWatch.Stop;
    }

    void ConfigureTimesBehavior()
    {
        Action storeElapsedtime = () => times.Add(stopWatch.Elapsed);
        OnLap += storeElapsedtime;
        OnStop += storeElapsedtime;
    }

    void ConfigureViewBehavior()
    {
        OnStart += () =>
        {
            StartAndLapButton.Text = "Lap";
            this.Text = "Started";
        };

        OnStop += () =>
        {
            StartAndLapButton.Text = "Start";
            this.Text = "Stop Watch";
        };
    }

    void ConfigureViewStateBehavior()
    {
        OnStart += () => state = WatchState.Started;
        OnStop += () => state = WatchState.Stopped;
    }

    void ConfigureTimerBehavior()
    {
        OnStart += timer1.Start;
        OnStop += timer1.Stop;
    }

    private void WriteResultsToFile(string filename)
    {
        List<string> timesToString = new List<string>();
        foreach (var item in times)
        {
            TimeSpan newitem = new TimeSpan(item.Hours, item.Minutes, item.Seconds);
            timesToString.Add(newitem.ToString());
        }
        System.IO.File.WriteAllLines(filename, timesToString);
    }

    private string PromtSaveAsFileName()
    {
        var dialog = new SaveFileDialog
        {
            DefaultExt = "txt",
            Filter = "txt files (*.txt)|*.txt|All files (*.*)|*.*",
            FilterIndex = 2,
            RestoreDirectory = true
        };

        if (dialog.ShowDialog() == DialogResult.OK)
        {
            return dialog.FileName;
        }

        return null;
    }

    private void timer1_Tick(object sender, EventArgs e)
    {
        string elapsedTime;
        timeSpan = stopWatch.Elapsed;
        elapsedTime = String.Format("{0:00}:{1:00}:{2:00}", timeSpan.Hours, timeSpan.Minutes, timeSpan.Seconds);
        TimeKeeperText.Text = elapsedTime;
    }
}

This is the 3rd iteration of this code. If you would like to see the original post here is the link.

stopWatch = Stopwatch.StartNew();
stopWatch.Stop();

If you want to create a Stopwatch that's stopped, use the constructor instead of StartNew() and then immediately stopping it.

timer1

The autogenerated WinForms names don't make much sense. If you have only one timer and there is no need to name it, call it just timer.

Also, consider creating the Timer in code, so that I don't have to look at the designer to find out for example what the period of timer is (or to change it).

if (state == WatchState.Stopped)
{
    OnStart();
} else if (state == WatchState.Started)
{
    OnLap();
}

This is an unusual bracing style. The normal .Net style would be to have the else if on a new line.

TimeSpan newitem = new TimeSpan(item.Hours, item.Minutes, item.Seconds);
timesToString.Add(newitem.ToString());

What is the purpose of this code? Is it to get rid of the "milliseconds" part in the output? Are you aware that it also removes the "days" part? If you're doing this just to get the format you want, consider using a custom format string (.Net 4+ only).

string elapsedTime;

There is no reason to declare this variable early.

String.Format("{0:00}:{1:00}:{2:00}", timeSpan.Hours, timeSpan.Minutes, timeSpan.Seconds)

Why are you using two different approaches to get TimeSpans formatted in the same way (here and using the new TimeSpan() approach above)? Considering encapsulating the formatting into a common method.


Static methods or singleton? I have class that handles HTTP requests:

public final class RestHttpClient {

    /*there is no fields*/

    /**
     * @param mhttp - HTTP request that need to send
     * @return HttpResponse, that contains information about server response
     */
    public HttpResponse sendRequest(final HttpRequestBase mhttp) throws IOException {
        /*do stuff and get response from server*/

        return response;
    }

    /*there a few more helpful methods*/
}

For the one hand - there is no need in instance of class, so I can use static methods. But for another hand singleton looks more reusable. So, I'm not sure which way to choose.

What is more readable and reusable? Static or singleton? Or should I just don't care about it and allow user to create instance of class?

This:

  Or should I just don't care about it and allow user to create instance
  of class?

If you plan to write unit tests, which is recommended these days: for your class AND the code that will use it.

http://googletesting.blogspot.co.uk/2008/05/tott-using-dependancy-injection-to.html


Trigger Updating another table - Baseball Database This is my first time writing T-SQL so I'd like to get some criticism on how I wrote this trigger.

I'm building a Pitch-Tracking App that will use a relational database to track Pitches, At-Bats, Games, etc. Everything starts with tracking the individual Pitch in an individual AB, so I figured whenever tbl_PitchLog is updated, it would update that individual At Bat ID in the tbl_AtBats table. Let me know if I'm straying or if I'm on some semblance of a straight path to elegant T-SQL

CREATE TABLE [dbo].[tbl_PitchLog]
(
    [Id] BIGINT NOT NULL PRIMARY KEY IDENTITY,
    [At_Bat_ID] BIGINT NOT NULL,
    [Batter_ID] BIGINT NOT NULL,
    [Pitcher_ID] BIGINT NOT NULL,
    [Game_ID] BIGINT NOT NULL,
    [Season_ID] BIGINT NOT NULL,
    [Pitch_Type] VARCHAR(50) NULL,
    [Pitch_Speed] BIGINT NULL,
    [Pitch_X] BIGINT NULL,
    [Pitch_Y] BIGINT NULL,
    [Strike] BIT NULL,
    [Swung] BIT NULL,
    [Contact] BIT NULL,
    [Fair] BIT NULL,
    [R] BIT NULL,
    [H] BIT NULL,
    [1B] BIT NULL,
    [2B] BIT NULL,
    [3B] BIT NULL,
    [HR] BIT NULL,
    [RBI] INT NULL,
    [BB] BIT NULL,
    [SO] BIT NULL,
    [IBB] BIT NULL,
    [HBP] BIT NULL,
    [SH] BIT NULL,
    [SF] BIT NULL,
    [GIDP] BIT NULL
)

GO

CREATE TRIGGER [dbo].[Trigger_tbl_PitchLog]
    ON [dbo].[tbl_PitchLog]
    FOR DELETE, INSERT, UPDATE
    AS
    DECLARE @Strikes integer, @Balls integer
    DECLARE @Fouls integer
    DECLARE @Contact integer
    IF Strike = 1
    BEGIN
        SET @Strikes = 1;
        SET @Balls = 0;
    END
    ELSE
    BEGIN
        SET @Strikes = 0;
        SET @Balls = 1;
    END
    IF Fair = 0
    BEGIN
        SET @Fouls = 1;
    END
    ELSE
    BEGIN
        SET @Fouls = 0;
    END
    IF Contact = 0
    BEGIN
        SET @Contact = 1;
    END
    ELSE
    BEGIN
        SET @Contact = 0;
    END
    BEGIN
        SET NoCount ON
        UPDATE tbl_AtBats
        SET tbl_AtBats.PitchCount = tbl_AtBats.PitchCount + 1,
        tbl_AtBats.Strikes = tbl_AtBats.Strikes + @Strikes,
        tbl_AtBats.Balls = tbl_AtBats.Balls + @Balls,
        tbl_AtBats.Fouls = tbl_AtBats.Fouls + @Fouls,
        tbl_AtBats.Contact = tbl_AtBats.Contact + @Contact,
        tbl_AtBats.Swings = tbl_AtBats.Swings + inserted.Swung,
        tbl_AtBats.R = inserted.R,
        tbl_AtBats.H = inserted.H,
        tbl_AtBats.[1B] = inserted.[1B],
        tbl_AtBats.[2B] = inserted.[2B],
        tbl_AtBats.[3B] = inserted.[3B],
        tbl_AtBats.HR = inserted.HR,
        tbl_AtBats.RBI = inserted.RBI,
        tbl_AtBats.BB = inserted.BB,
        tbl_AtBats.SO = inserted.SO,
        tbl_AtBats.IBB = inserted.IBB,
        tbl_AtBats.HBP = inserted.HBP,
        tbl_AtBats.SH = inserted.SH,
        tbl_AtBats.SF = inserted.SF,
        tbl_AtBats.GIDP = inserted.GIDP
        WHERE tbl_AtBats.Id = inserted.At_Bat_ID;
    END

at first glance I found a couple of things that might give you issues

IF Strike = 1
BEGIN
    SET @Strikes = 1;
    SET @Balls = 0;
END
ELSE
BEGIN
    SET @Strikes = 0;
    SET @Balls = 1;
END

IIRC, you can perform 4 foul hits before you are considered "out", and that a foul hit on the first and second pitch are also counted as "Strikes" but not the third foul hit. going through this logic will give you a false "ball" count because it is a foul but not a strike.

I also noticed

IF Contact = 0
BEGIN
    SET @Contact = 1;
END
ELSE
BEGIN
    SET @Contact = 0;
END

I assumed that @Contact was your "hits" but I tried to go through the logic and I am a bit confused.  If Contact = 0 means to me that there wasn't a hit, but you set the variable to true (SET @Contact = 1;)

Again IIRC, not every contact with the ball counts as a "hit"


Check equality of an objects array property and a nested object property On my app I'm checking the equality between an objects array property and a nested object property - based on the results I return a new objects array.
The items objects array:
[{name: 'test', value: 1, category: 'a'},{name: 'test2', value: 2, category: 'b'},{name: 'test3', value: 3, category: 'c'}]

The itemList object:
{testnum1: {name: 'test123', category: 'a'}, testnum2: {name: 'test234', category: 'b'}

I need to check the equality between the category properties and return the following objects array:
[{name: 'test', itemCategory: 'testnum1'},{name: 'test2', itemCategory: 'testnum2'}]

This is what I did, any idea on how to make it more readable?
const getCategorizedItems = items => {
  const itemListCategories = Object.keys(itemList)
  const categorizedItems= items.map(item=> {
    const itemCategory = itemListCategories.find(category =>
      item.category === itemList[category].category
    )
    if (itemCategory ) return {name: item.name, category: itemCategory }
  })
  .filter(f => f)
  return categorizedItems
}

Short review;

Unless you are writing inline functions, you should stick to using function
Using semicolons is preferred, it takes skill and knowledge to avoid the pitfalls of not using semicolons
You assign in 1 statement the value of categorizedItems and return it in the next, you might as well return immediately
In essence you do a find within a map, not too fast, I would create a lookup structure before the map
itemList seems like a terrible name, I would call it categories

I would counter-propose the following;

const items = [{name: 'test', value: 1, category: 'a'}, {name: 'test2', value: 2, category: 'b'}, {name: 'test3', value: 3, category: 'c'}];
const categories = {testnum1: {name: 'test123', category: 'a'}, testnum2: {name: 'test234', category: 'b'}};

function getCategorizedItems(items, categories){
  //Desired output; [{name: 'test', itemCategory: 'testnum1'},{name: 'test2', itemCategory: 'testnum2'}]
  const categoriesMap = Object.fromEntries(Object.entries(categories).map((entry) => [entry[1].category, entry[0]]));
  return items.map(item => ({name: item.name, itemCategory:categoriesMap[item.category]})).filter(item => item.itemCategory);
}

console.log(getCategorizedItems(items, categories));


Spotfire: Creating filtering relationships by passing filters between tables I am using the code below to pass filter selections from table A to table B in Spotfire. I am doing this to create a filtering relationship between the tables, but skipping the creation of a traditional Spotfire table relationship, as this is not advisable in this setting.

The code is associated with a document property which itself is driven by a simple data function, following the instructions found here.

After it is triggered, the code finds a specific filter in table A. It reads the content and creates a list. It checks the length of the list, and if greater than zero, it takes the filter object and passes it to the matching filter in table B. If nothing is selected (len = 0), then the matching filter in table B is reset. The process is repeated for multiple filters.

As far as I can tell it works on my machine. But even as a complete beginner, I can tell it is poorly written. I would appreciate suggestions and feedback on how I can make it better.

from Spotfire.Dxp.Application.Filters import *
from Spotfire.Dxp.Application.Visuals import VisualContent
from System import Guid

#Get the active page and filterPanel
page = Application.Document.ActivePageReference
filterPanel = page.FilterPanel

##########################reasons#############################################
#get information for the first filter
theFilter = filterPanel.TableGroups[0].GetFilter("Reason")
lbFilter = theFilter.FilterReference.As[ListBoxFilter]()
#get information for the second filter
theFilter2 = filterPanel.TableGroups[1].GetFilter("Reason")
lb2Filter = theFilter2.FilterReference.As[ListBoxFilter]()

#check if any selections are made in the first filter
n = []
for value in lbFilter.SelectedValues:
    n.append(value)
if len(n)>0:
    lb2Filter.IncludeAllValues = False
    lb2Filter.SetSelection(lbFilter.SelectedValues)
#if no selections are made, reset the second filter
else:
    lb2Filter.Reset()

#########################Type#####################################################
theFilterPT = filterPanel.TableGroups[0].GetFilter("Type")
lbFilterPT = theFilterPT.FilterReference.As[ListBoxFilter]()

theFilter2PT = filterPanel.TableGroups[1].GetFilter("Type")
lb2FilterPT = theFilter2PT.FilterReference.As[ListBoxFilter]()

n = []
for value in lbFilterPT.SelectedValues:
    n.append(value)
if len(n)>0:
    lb2FilterPT.IncludeAllValues = False
    lb2FilterPT.SetSelection(lbFilterPT.SelectedValues)
else:
    lb2FilterPT.Reset()

from ... import * is discouraged by a large amount of people.

Currently it's easy to see what comes from Filters, as you only have one. But if you have two or more than it just becomes impossible to understand.
Your code doesn't look Pythonic, unfortunately this is because Spotfire has violated PEP 8 making anything that uses it look, to my eyes, ugly.

Whilst I'm not a fan of the style you are using, I would like to congratulate you on sticking to one style. Well done.
If the comments help you then keep them. However if you wrote them to help us then I would recommend that you remove them.
You have lots of un-needed variables; page, theFilter, theFilterPT.
Functions are the bread and butter of programming. And your code is in some dire need of functions.

In your code we see that you have the following definitions:

  #get information for the first filter
theFilter = filterPanel.TableGroups[0].GetFilter("Reason")
lbFilter = theFilter.FilterReference.As[ListBoxFilter]()
#get information for the second filter
theFilter2 = filterPanel.TableGroups[1].GetFilter("Reason")
lb2Filter = theFilter2.FilterReference.As[ListBoxFilter]()

  theFilterPT = filterPanel.TableGroups[0].GetFilter("Type")
lbFilterPT = theFilterPT.FilterReference.As[ListBoxFilter]()

theFilter2PT = filterPanel.TableGroups[1].GetFilter("Type")
lb2FilterPT = theFilter2PT.FilterReference.As[ListBoxFilter]()

We can see clearly that only two things change between them; [0] and [1], and "Reason" and "Type".
I personally think filterPanel.TableGroups[0] shouldn't be in the function and we can just pass the filter in the function. This can result in:

def getGroup(tableGroup, filter):
    return (
        tableGroup
            .getFilter(filter)
            .FilterReference
            .As[Filters.ListBoxFilter]()
    )

After this we can see you've written something else twice too.

  n = []
for value in lbFilter.SelectedValues:
    n.append(value)
if len(n)>0:
    lb2Filter.IncludeAllValues = False
    lb2Filter.SetSelection(lbFilter.SelectedValues)
#if no selections are made, reset the second filter
else:
    lb2Filter.Reset()

Firstly creating n can be simplified simply as list(lbFilter.SelectedValues).
You don't need to use len(n)>0 as just if n: does that check, with a list, for you.
Finally I find it easier to have the smaller code block first, and so flipped the if.

def filterGroups(tableGroups, filter):
    group1 = getGroup(tableGroups[0], filter)
    group2 = getGroup(tableGroups[1], filter)

    if not list(group1.SelectedValues):
        group2.Reset()
    else:
        group2.IncludeAllValues = False
        group2.SetSelection(group1.SelectedValues)

Finally I would wrap everything in a main function and you'd get the following:

import System
from Spotfire.Dxp.Application import Filters, Visuals

def getGroup(tableGroup, filter):
    return (
        tableGroup
            .getFilter(filter)
            .FilterReference
            .As[Filters.ListBoxFilter]()
    )

def filterGroups(tableGroups, filter):
    group1 = getGroup(tableGroups[0], filter)
    group2 = getGroup(tableGroups[1], filter)

    if not list(group1.SelectedValues):
        group2.Reset()
    else:
        group2.IncludeAllValues = False
        group2.SetSelection(group1.SelectedValues)

def main():
    tableGroups = (
        Filters
            .Application
            .Document
            .ActivePageReference
            .FilterPanel
            .TableGroups
    )
    filterGroups(tableGroups, "Reason")
    filterGroups(tableGroups, "Type")

if __name__ == '__main__':
    main()


Checking for parentheses balance in C A recent assignment asks me to create a program that checks for parentheses balance in a given string. As I can't find any duplicate questions that have code written in C, I decided to post another one.

I tried to make a stack that stores currently opened / unclosed parentheses and checks with each character in array, if it is unbalanced the program flags and depending on the flag boolean prints different value.

Is this the best way? Are there any situations where this program will print incorrect outputs? How can I make it take less time if the input is extremely long?

This is the code:

#include<stdio.h>
int main(){
    int pilepointer=-1,flag=0,i;
    char line[10001],pile[10001];
    gets(line);
    for(i=0;line[i]!=NULL;i++){
        switch(line[i]){
        case '(':
            pilepointer=pilepointer+1;
            pile[pilepointer]='(';
            break;
        case '[':
            pilepointer=pilepointer+1;
            pile[pilepointer]='[';
            break;
        case '{':
            pilepointer=pilepointer+1;
            pile[pilepointer]='{';
            break;
        case ')':
            if(pile[pilepointer]=='('){
                pile[pilepointer]==NULL;
                pilepointer=pilepointer-1;
                break;
            }
            else{
                flag=1;
                break;
            }
        case ']':
            if(pile[pilepointer]=='['){
                pile[pilepointer]==NULL;
                pilepointer=pilepointer-1;
                break;
            }
            else{
                flag=1;
                break;
            }
        case '}':
            if(pile[pilepointer]=='{'){
                pile[pilepointer]==NULL;
                pilepointer=pilepointer-1;
                break;
            }
            else{
                flag=1;
                break;
            }
        }
    }
    if(flag==1){
        printf("No\n");
    }
    else{
        printf("Yes\n");
    }
    return 0;
}

Bugs

The loop header is wrong:

for(i=0;line[i]!=NULL;i++){

Here, you want to scan until you find a byte with ASCII value 0 (also called an ASCII NUL, and written in C as '\0').  However, you wrote NULL, which is supposed to represent a null pointer (a pointer to memory address 0).

In practice, you might get away with confounding the two, since they are both zero-like in nature, but it is not semantically correct.
The result of this comparison is thrown away:

pile[pilepointer]==NULL;

Did you intend to write pile[pilepointer] = NULL?  That wouldn't be quite right either, since you've confused NULL and '\0' again, as above.  In any case, that statement was unnecessary.
Stack underflow is possible.  If the input is simply ")", for example, you will immediately try to peek at the top of an empty stack (pile[-1]), resulting in out-of-bounds memory access.

Style

Flag variables suck; flag variables named flag are even worse.  At the least, rename it to something meaningful, such as mismatch_found.
When you do

flag = 1;
break;

You are breaking out of the case.  However, you need not bother analyzing the rest of the line.
Pushing and popping are clumsily expressed.  I would write:

char pile[10001];
int pile_size = 0;

To push:

pile[pile_size++] = '(';

To pop:

if (!pile_size || pile[--pile_size] != '(') {
    // Mismatch detected
}

You can coalesce all of the popping cases, if you push the expected matching closing delimiter when you encounter each opening delimiter.

for (char *c = line; *c && !mismatch_found; c++) {
    switch(*c) {
    case '(':
        pile[pile_size++] = ')';
        break;
    case '[':
        pile[pile_size++] = ']';
        break;
    case '{':
        pile[pile_size++] = '}';
        break;
    case ')':
    case ']':
    case '}':
        if (!pile_size || pile[--pile_size] != *c) {
            mismatch_found = 1;
        }
        break;
    }
}
printf((pile_size == 0 && !mismatch_found) ? "Yes\n" : "No\n");


Factorials, loops, break and professional code The program below correctly calculates factorials, until the limit for an unsigned long long integer is exceeded.

My question has to do with loops and exiting loops. The program uses one infinite while loop and the break statement to exit 2 while loops.

Do working, professional programmers handle loops this way? If not, how would they write and exit these loops?

/* factorial.c

   program to calculate factorials until the limit for an unsigned long long
   integer is exceeded

   20! =    2,432,902,008,176,640,000
   limit = 18,446,744,073,709,551,615
   21! =   51,090,942,171,709,440,000
*/

#include <stdio.h>
#include <limits.h>

unsigned long long int factorial (int i)
{
    // function returns 0 when limit exceeded

    int j;
    unsigned long long int result;

    if ( i == 0 )
        result = 1;
    else {
        result = i;

        j = i - 1;

        while ( j >= 1 ) {
            if ( result <= ULLONG_MAX / j )
                result *= j;
            else {
                result = 0;
                break;
            }

            --j;
        }
    }

    return result;
}

int main (void)
{
    int i;
    unsigned long long int number;

    printf ("\nTABLE OF FACTORIALS\n\n");
    printf (" n            n!\n");
    printf ("-- ------------------------\n");

    i = 0;

    while ( 1 ) {
        number = factorial (i);
        if ( number == 0 )
            break;
        printf ("%2i %24llu\n", i, number);
        ++i;
    }

    return 0;
}

The other reviews have covered most items, so I'll just add a few.

Use constant string concatenation

The current code includes this:

printf("\nTABLE OF FACTORIALS\n\n");
printf(" n            n!\n");
printf("-- ------------------------\n");

But we don't really need to make 3 separate calls to printf here.  Instead, let the compiler automatically concatenate string literals together so we can write this:

printf("\nTABLE OF FACTORIALS\n\n"
         " n            n!\n"
         "-- ------------------------\n");

I'd also suggest that puts would be a more appropriate call than printf since all we're printing is a string literal.

Be careful with signed vs. unsigned

If you don't intend for your code to handle negative numbers, I'd suggest that you indicate this clearly by changing the function signature to this:

unsigned long long factorial(unsigned i)

Minimize the scope of variable

In modern C, we don't tend to declare all variables at the top of each function.  Instead, they're declared as they're needed and in as small a scope as possible, as in the following suggestion:

Prefer for to while loops

In the main code, the while(1) suggests that the loop continues forever but it really doesn't.  In fact, we increment i each time and exit when the factorial code detects an overflow.  Why don't we just write it that way instead of making the reader work so hard?

int main(void)
{
    printf("\nTABLE OF FACTORIALS\n\n"
             " n            n!\n"
             "-- ------------------------\n");

    unsigned long long number;

    // factorial() returns 0 on overflow
    for (unsigned i; (number = factorial(i)); ++i) {
        printf ("%2u %24llu\n", i, number);
    }
}

Simplify your code

The current factorial code has the same problem with its loop, which is that it's not made clear when the loop ends.  Also, the code for detecting overflow is much more complex than it needs to be.  Instead, we can simply note that when the multiplication results in overflow, the resulting product is less than the previous value.  Using that, I'd write the code like this:

// return the factorial of i or
// 0 if multiplication overflows
unsigned long long factorial(unsigned i)
{
    if (i == 0) {
        return 1;
    }
    unsigned long long res;
    for (res = i--; i; --i) {
        unsigned long long prev = res;
        res *= i;
        if (res < prev) { // oveflow?
            return 0;
        }
    }
    return res;
}

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments:  "I didn't know that." or "That's bad advice!"  My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard.  For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

  [...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

  If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea.  We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function.  Reasons against omitting seem to boil down to "it looks weird".  If, like me, you're curious about the rationale for the change to the C standard read this question.  Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!)  In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.


JS scope of member functions I was writing a program that used a bit of recursion with objects, and it wasn't working at all. After debugging I realized that JS's evil scope was playing nasty tricks.

To spare you guys a wall of text code, I wrote a smaller program:

//Class definition and framework:
TestClass=function(label,level){
    this.label=label;
    this.level=level;

}

TestClass.prototype.recur=function(){
   r=Math.random(); //initialise a local variable
   document.write(this.label+":"+r+"
");
   if(this.level==0){return;} //exit after this if level==0, we don't want to blow up the call stack
   x=new TestClass("inner1",0)
   x.recur();
   document.write(this.label+":"+r+"
"); //Check if r has changed
   y=new TestClass("inner2",0)
   y.recur();
   document.write(this.label+":"+r+"
"); //check if r has changed
}

//Run this:
a=new TestClass("outer",1);
a.recur();

Output:

outer:0.5542382076382637
inner1:0.7689833084587008
outer:0.7689833084587008
inner2:0.24465859192423522
outer:0.24465859192423522

As one can see, the value of r in the outer function has changed. From this, I see that JS joins the scope of a function with a function (recursively) called in itself. This seems slightly icky and illogical.

My own solution to this was to make every locally initialized variable a member variable. In this example, I would replace all instances of r with this.r.

Then, since I like my objects to be clean, I wrote a garbage collector. Final code would be somewhat like:

//Class definition and framework:
TestClass=function(label,level){
    this.label=label;
    this.level=level;

}
TestClass.allowedProperties=["label","level"] // do NOT blow these up during garbage collection
TestClass.prototype.recur=function(){
   this.r=Math.random(); //initialise a local variable
   document.write(this.label+":"+this.r+"
");
   if(this.level==0){return;} //exit after this if level==0, we don't want to blow up the call stack
   x=new TestClass("inner1",0)
   x.recur();
   document.write(this.label+":"+this.r+"
"); //Check if r has changed
   y=new TestClass("inner2",0)
   y.recur();
   document.write(this.label+":"+this.r+"
"); //check if r has changed
}

//Garbage collector:
TestClass.prototype.cleanup=function(){
    for(x in this){
        if(TestClass.allowedProperties.indexOf(x)==-1){
            delete this[x];
        }
    }
}

//Run this:
a=new TestClass("outer",1);
a.recur();
a.cleanup();

This manages the job reasonably well, and cleans up after itself. But I feel that this is a rather inelegant way to do it. Is there a better, elegant way to deal with this form of evil javascript scoping?

"From this, I see that JS joins the scope of a function with a function (recursively) called in itself."

No, it doesn't.  You created a global variable called "r", not a method variable, and so there is only one "r", and it is accessible to all your JavaScript code.

Thou shalt always use var to declare a variable.


Playing the game of Chaos According to Rosetta Code:

The Chaos Game is a method of generating the attractor of an iterated
function system (IFS). One of the best-known and simplest examples
creates a fractal, using a polygon and an initial point selected at
random.
Task
Play the Chaos Game using the corners of an equilateral triangle
as the reference points. Add a starting point at random (preferably
inside the triangle). Then add the next point halfway between the
starting point and one of the reference points. This reference point
is chosen at random.
After a sufficient number of iterations, the image of a Sierpinski
Triangle should emerge.

Here is my solution in Python:
"""This module performs the Chaos Game."""

from random import choice
from PIL import Image

def midpoint(p1, p2):
    """
    Takes 2 points and calculates the midpoint.
    Args:
        p1 (real, real): A point in space.
        p2 (real, real): Another point in space.
    Returns:
        (real, real): The midpoint between p1 and p2.
    """
    return (p1[0] + p2[0]) / 2, (p1[1] + p2[1]) / 2

def translate(point, screen_size):
    """
    Takes a point and converts it to the appropriate coordinate system.
    Note that PIL uses upper left as 0, we want the center.
    Args:
        point (real, real): A point in space.
        screen_size (int): Size of an N x N screen.
    Returns:
        (real, real): Translated point for Pillow coordinate system.
    """
    return point[0] + screen_size / 2, point[1] + screen_size / 2

def image_rep(point, img_size):
    """
    Returns a representation of the point that can be used by Pillow.
    Args:
        point (real, real): A point in space.
        img_size (int): The size of an N x N image
    """
    # Convert the point to the correct coordinate system.
    translated = translate((point[0], point[1]), img_size)
    # Make it an integer so Pillow can use it.
    return int(translated[0]), int(translated[1])

def play_chaos(guiders, seed, img, steps=1000):
    """
    Play the Chaos Game:
    1. Have a collection of n (ordered) "guider" points.
    2. Start with a random seed some where on the screen.
    3. Roll die that has n sides
    4. Make a new point that is halfway between the appropriate gudier and the
       previous point.
    5. Repeat the game from the new point
    Args:
        guiders ([(real, real)]): List of guider points
        seed (real, real): Initial seed, be sure to be within the screens range
        img (Image): Image to write spiral to.
        steps (int): The amount of times the process is repeated.
        (default: 1000)
    """
    prev_point = seed
    for _ in range(steps):
        # Select the next guider to move closer to
        guider = choice(guiders)
        new_point = midpoint(prev_point, guider)
        img.putpixel(image_rep(new_point, img.size[0]), 1)
        prev_point = new_point

def main():
    """
    Main method, start of the program. Creates an image and populates it with
    chaos.
    """
    IMAGE_SIZE = 300, 300
    img = Image.new('1', IMAGE_SIZE)
    SEED = (0.0, 0.0)
    GUIDERS = [(-100.0, -100.0),
               (100.0, -100.0),
               (0.0, 100.0)]
    play_chaos(GUIDERS, SEED, img)
    img.save('chaos.png')

if __name__ == '__main__':
    main()

midpoint()

This function could be written more elegantly using zip(), and it would work for any number of dimensions. (I suspect it might be slower, though.)

def midpoint(p1, p2):
    """
    Find the midpoint of two cartesian points.
    """
    return tuple((c1 + c2) / 2 for c1, c2 in zip(p1, p2))

translate() and image_rep()

Since translate() gives a result in Pillow coordinates, I think that the result should be quantized as integers. You should round to the nearest integer rather than truncating.

Pillow coordinates increase towards the right and towards the bottom.  Conventionally, cartesian coordinates increase towards the right and towards the top.  I would therefore incorporate a reflection of the vertical axis in the translation.

I don't see why the image must be square.

You can just pass point instead of packing it into a new tuple.

translated = translate(point, img_size)

I suggest hiding these two helper functions inside a plotting routine. (See below.)

play_chaos()

I would split this into the mathematical concept and its visualization.

def chaos_points(attractors, start_point):
    attractor_stream = (random.choice(attractors) for _ in itertools.count())
    return itertools.accumulate(itertools.chain([start_point], attractor_stream), midpoint)

def plot(img, points):
    half_width, half_height = img.size[0] / 2, img.size[1] / 2
    to_pillow = lambda p: (int(round(half_width + p[0])), int(round(half_height - p[1])))
    for point in points:
        img.putpixel(to_pillow(point), 1)

Demo

Your GUIDERS don't form an equilateral triangle as suggested.

def main():
    """
    Create an image and populate it with chaos.
    """
    IMAGE_SIZE = 300, 300
    SEED = (0.0, 0.0)
    GUIDERS = [(-100.0, -50 * 3**.5),
               (+100.0, -50 * 3**.5),
               (   0.0, +50 * 3**.5)]
    img = Image.new('1', IMAGE_SIZE)
    plot(img, itertools.islice(chaos_points(GUIDERS, SEED), 1000))
    img.save('chaos.png')

if __name__ == '__main__':
    main()


Callback pattern for handling socket response I am in the process of learning go and am coming from a PHP, JS, and Nodejs background.

I created a package that is a client which connects to a socket server and processes the data received from the server.  I am using a callback pattern and am wondering if this is frowned upon in the Go community.  Here are the relevant code snippets:

// SocketClient allows handles the socket connection to a server
type SocketClient struct {
    Host string
    Path string
    conn *websocket.Conn
}

// SocketResponse function callback for data from socket
type SocketResponse func(res []byte, err error)

// Connect create the connection with the host
func (sc SocketClient) Connect(cb SocketResponse) {
    interrupt := make(chan os.Signal, 1)
    signal.Notify(interrupt, os.Interrupt)

    u := url.URL{Scheme: "wss", Host: sc.Host, Path: sc.Path}

    var err error
    sc.conn, _, err = websocket.DefaultDialer.Dial(u.String(), nil)

    if err != nil {
        cb(nil, err)
    }

    defer sc.conn.Close()

    done := make(chan struct{})

    // anonymous function call
    go func() {
        defer sc.conn.Close()
        defer close(done)
        for {
            _, message, err := sc.conn.ReadMessage()
            if err != nil {
                cb(nil, err)
            }
            cb(message, nil)
        }
    }()

    // ...
}

And then in another file I actually initialize SocketClient and call Connect...

sc.Connect(func(res []byte, err error) {
    // handle the response
})

I am using a callback pattern and am wondering if this is frowned upon in the Go community.

"callback pattern" shouldn't be frowned upon (they are used in "first-class functions" for instance: https://blog.golang.org/first-class-functions-in-go-and-new-go https://dave.cheney.net/2016/11/13/do-not-fear-first-class-functions)

However regarding your code, I don't think that it looks like idiomatic go code.

Don't just check errors, handle them gracefully

sc.conn, _, err = websocket.DefaultDialer.Dial(u.String(), nil)

if err != nil {
    cb(nil, err)
}

You pass the err to your callback, but you then continue inside the Connect function: if you couldn't dial, why do you continue?

Never start a goroutine without knowing how it will stop

go func() {
    defer sc.conn.Close()
    defer close(done)
    for {
        _, message, err := sc.conn.ReadMessage()
        if err != nil {
            cb(nil, err)
        }
        cb(message, nil)
    }
}()

The inner for loop will never stop (so will your goroutine).

One possibility would be to change it like this:

// SocketResponse function callback for data from socket
type SocketResponse func(res []byte)

// Connect create the connection with the host
func (sc SocketClient) Connect(cb SocketResponse) error {
    u := url.URL{Scheme: "wss", Host: sc.Host, Path: sc.Path}

    var err error
    sc.conn, _, err = websocket.DefaultDialer.Dial(u.String(), nil)

    if err != nil {
        return err
    }

    defer sc.conn.Close()
    for {
        _, message, err := sc.conn.ReadMessage()
        if err != nil {
            return err
        }
        cb(message)
    }
}

Some properties of this code :

on dial error, it stops
it reads all the messages until one error is encountered
it blocks (but go sc.Connect() is easy to write)

Some possible evolutions:

to continue even in case of errors, you could return a channel of errors (and send all errors encountered)
also use a channel to report the received messages
to control when the Connect method should stop, you could use a context or split your method in 3 parts: Connect, ReadMessage & Close


Unity C# - Radar works but performance is horrible (Update & LateUpdate) Hello I need help to optimize this code. I have a radar and it gets the enemies from a list and adds an image to their location to track them. I use Update to remove the images and LateUpdate to add the images. Is there any way I can do this without costing performance. I have a list of all the Enemies. I have a pool of over 40 images to use as tracking for the radar. Enemy is always moving. Any ideas?
private void Update()
{
    if (statsView_Panel.activeInHierarchy)
    {
        if (showIndicators_Enemies)
        {
            if (!enemyListHolder.activeInHierarchy)
            {
                enemyListHolder.SetActive(true);
            }

            Remove_PositionIndicatorOnEnemies();
        }
    }
}

private void LateUpdate()
{
    if (showIndicators_Enemies)
    {
        if (!enemyListHolder.activeInHierarchy)
        {
            enemyListHolder.SetActive(true);
        }

        Add_PositionIndicatorOnEnemies();
    }
}

private void Add_PositionIndicatorOnEnemies()
{
    //  Do if ListHolder is Active
    if (enemyListHolder.activeInHierarchy)
    {
        //  Run if 1 or more Enemies
        if (EnemiesManager.Count > 0)
        {
            //  Check All Enemies
            for (int i = 0; i < EnemiesManager.Count; i++)
            {
                //  Check All Indicators
                for (int s = 0; s < enemyIndicList.Count; s++)
                {
                    //  Choose Indicator that is not active *****BUG: Will Add All Deactivated Indicators to Active Enemies*****
                    if (enemyIndicList[s].gameObject.activeInHierarchy == false)
                    {
                        //  Activate Indicator and Reposition
                        enemyIndicList[s].gameObject.SetActive(true);
                        Vector3 aLocatorPos;
                        aLocatorPos = theCam.WorldToScreenPoint(satManager.GetByIndex(i).transform.position);
                        aLocatorPos.z = 0;
                        enemyIndicList[s].transform.position = aLocatorPos;

                        //  Break from Loop
                        break;
                    }
                }
            }
        }
    }
}

private void Remove_PositionIndicatorOnEnemies()
{
    //  Do if ListHolder is Active
    if (enemyListHolder.activeInHierarchy)
    {
        //  Run if 1 or more Enemies
        if (EnemiesManager.Count > 0)
        {
            //  Check All Indicators
            for (int s = 0; s < enemyIndicList.Count; s++)
            {
                //  Choose Indicator that is active
                if (enemyIndicList[s].gameObject.activeInHierarchy == true)
                {
                    //  Deactivate Indicator
                    enemyIndicList[s].gameObject.SetActive(false);
                }
            }
        }
    }
}

Let's review the code from bottom to top.
Remove_PositionIndicatorOnEnemies

Having an underscore in a method name doesn't fit with the .NET naming guidelines.
By reverting the first if condition you can return early if enemyListHolder.activeInHierarchy is false which saves you one level of indentation.
The next if condition can be omitted because that check is done inside the loop condition.
Instead of using a for loop I would prefer a foreach loop and would restrict the items to only them where activeInHierarchy is true by a Where() clause.

Implementing the mentioned changes will lead to
private void RemovePositionIndicatorOnEnemies()
{
    if (!enemyListHolder.activeInHierarchy) { return; }

    foreach (var enemy in enemyIndicList.Where(e=> e.gameObject.activeInHierarchy))
    {
        enemy.gameObject.SetActive(false);
    }
}

Add_PositionIndicatorOnEnemies
Here, the first three points from above apply as well.

The inner for loop can be replaced by FirstOrFDefault() which is more readable.

Implementing the mentioned changes will lead to
private void Add_PositionIndicatorOnEnemies()
{
    if (!enemyListHolder.activeInHierarchy) {return; }

    for (int i = 0; i < EnemiesManager.Count; i++)
    {
        var enemy = enemyIndicList.FirstOrDefault(e=> !e.gameObject.activeInHierarchy);
        if (enemy != null)
        {
            enemy.gameObject.SetActive(true);
            Vector3 aLocatorPos = theCam.WorldToScreenPoint(satManager.GetByIndex(i).transform.position);
            aLocatorPos.z = 0;
            enemy.transform.position = aLocatorPos;
        }
    }
}

LateUpdate

Again returning early will save one level of indentation.

Implementing the mentioned changes will lead to
private void LateUpdate()
{
    if (!showIndicators_Enemies) { return; }

    if (!enemyListHolder.activeInHierarchy)
    {
        enemyListHolder.SetActive(true);
    }

    Add_PositionIndicatorOnEnemies();
}

Update

Here we can save two levels of indentation by reverting and combining the first two if conditions.

Implementing the mentioned changes will lead to
private void Update()
{
    if (!statsView_Panel.activeInHierarchy || !showIndicators_Enemies) { return; }

    if (!enemyListHolder.activeInHierarchy)
    {
        enemyListHolder.SetActive(true);
    }

    Remove_PositionIndicatorOnEnemies();
}


Convert a list of sets into the minimum list of non-intersecting sets I have a list of sets. The same items may appear in multiple sets.

I want to transform this into a new list of sets where:

Each item only appears once in the entire list of sets.
For each set in this new list, on every set in the old list, the new set will either be a subset or will not intersect at all.

I want this new list of sets to contain the minimum number of sets it can while still satisfying the above two requirements.

For example, this list

abcd
befg
cehi

Would turn into

ad
b
c
e
fg
hi

This seems like a pretty generic problem. Is there a name for it?

Here's my implementation. It requires the comparison of each set with each other set, so I guess this has a performance \$O(k \cdot n^2)\$ where \$n\$ is the count of lists, and \$k\$ is the average length of each set.

Are there any efficiency issues that could be fixed easily?

public static class SetUtilities
{
    public static IEnumerable<IEnumerable<T>> MinimumNonIntersectingSets<T>(this IEnumerable<IEnumerable<T>> sets)
    {
        List<List<T>> disjointSets = new List<List<T>>();
        // Special case: if there is a null set we have to add it first
        if (sets.Any(s => !(s.Any())))
        {
            disjointSets.Add(new List<T>());
            sets = sets.Where(s => s.Any());
        }

        foreach (IEnumerable<T> newSet in sets)
        {
            List<T> disjointCopy = newSet.ToList();
            int i = 0;
            while(i < disjointSets.Count())
            {
                var intersection = disjointSets[i].Intersect(disjointCopy).ToList();

                if ((intersection.Count() == disjointCopy.Count()) && (intersection.Count() == disjointSets[i].Count())) // They are equal
                {
                    disjointCopy.Clear();
                    break;
                }
                if (intersection.Any())
                {
                    disjointSets[i] = disjointSets[i].Except(intersection).ToList();
                    disjointSets.Insert(++i, intersection);
                    disjointCopy = disjointCopy.Except(intersection).ToList();
                }
                ++i;
            }
            if(disjointCopy.Any())
            {
                disjointSets.Add(disjointCopy);
            }
        }
        return disjointSets;
    }
}

I guess this has a performance \$O(k \cdot n^2)\$ where \$n\$ is the count of lists, and \$k\$ is the average length of each set.

You shouldn't guess the time complexity, you should carefully analyze your code, so let's do that:

The empty test check is \$\mathcal{O}(n)\$.
The foreach is over all sets in the input, that's \$n\$.
The while loop is over all sets in the output so far. The number of iterations will form something similar to arithmetic progression, with maximum at \$m\$, the number of sets in the output. The average is going to be \$\approx m/2\$, so this part is going to contribute \$\mathcal{O}(m)\$.

(A formal proof would require proving that “something similar to arithmetic progression” will actually contribute that, but this is not a formal proof)
Calculating intersection and complements of sets is going to be \$\mathcal{O}(k)\$.
Inserting in the middle of a List of length \$\mathcal{O}(m)\$ is \$\mathcal{O}(m)\$.

Put together, we have \$\mathcal{O}(n + n \cdot m \cdot (k + m)) = \mathcal{O}(n \cdot m \cdot (k + m))\$.

That Insert annoyingly increases the complexity, but we can easily get rid of it either by using LinkedList (and walking it instead of using indexes) or by Adding at the end of the List (and making sure we don't iterate over the sets that were just added). This will bring us to \$\mathcal{O}(n \cdot m \cdot k)\$.

So, how big is \$m\$? Your estimate indicates you assumed it's \$\mathcal{O}(n)\$, but even your example shows that it can be larger than \$n\$. So, can it actually grow beyond \$\mathcal{O}(n)\$? Turns out, it can: when each of the \$n\$ sets has exactly one element in common with each other set*, then in the output, each of the \$n(n-1)/2\$ elements has to be in its own set, so the length of the output is \$\mathcal{O}(n^2)\$.

This means, that the total worst case complexity of your algorithm is \$\mathcal{O}(n^3  \cdot k)\$

* Or, in the language of graph theory, the sets are sets of incident edges for vertexes of a complete  graph on \$n\$ vertices.

For some inputs, your output contains lots of empty sets that don't belong there. For example, try it on new[] { new[] { 1 }, new[] { 1, 2 }, new[] { 1, 2, 3 } }.

// Special case: if there is a null set we have to add it first

No, you don't need a special case for the empty set. It doesn't intersect with anything, so both your requirement will be still fulfilled, even if you don't include it in the output.

.Count()

I think it's better to use the Count property instead of the Count() extension method, to make it clear that it's an \$\mathcal{O}(1)\$ operation, not \$\mathcal{O}(n)\$.


Converting Roman numerals to integers Roman numerals are represented by seven different symbols: I, V, X, L,
  C, D and M.

Symbol       Value
I             1
V             5
X             10
L             50
C             100
D             500
M             1000

  For example, two is written as II in Roman numeral, just two one's
  added together.  Twelve is written as, XII, which is simply X + II.
  The number twenty seven is written as XXVII, which is XX + V + II.

  Roman numerals are usually written largest to smallest from left to
  right. However, the numeral for four is not IIII. Instead, the number
  four is written as IV. Because the one is before the five we subtract
  it making four. The same principle applies to the number nine, which
  is written as IX. There are six instances where subtraction is used:

  I can be placed before V (5) and X (10) to make 4 and 9.
  X can be placed before L (50) and C (100) to make 40 and 90.
  C can be placed before D (500) and M (1000) to make 400 and 900.

  Given a Roman numeral, convert it to an integer. Input is guaranteed
  to be within the range from 1 to 3999.

  Example 1:
  Input: "III"
  Output: 3

  Example 2:
  Input: "IV"
  Output: 4

  Example 3:
  Input: "IX"
  Output: 9

  Example 4:
  Input: "LVIII"
  Output: 58
  Explanation: L = 50, V= 5, III = 3.

  Example 5:
  Input: "MCMXCIV"
  Output: 1994
  Explanation: M = 1000, CM = 900, XC = 90 and IV = 4.

My function in Python to accomplish this task:

def romanToInt(self, s: str) -> int:
    checks = {"IV":-2,"IX":-2, "XL":-20, "XC":-20, "CD":-200, "CM":-200}
    values = {"I":1, "V":5,"X":10, "L":50,"C":100, "D":500,"M":1000}
    sum = 0

    for i in s:
        sum += values[i]

    for i in range(len(s) - 1):
        combine = str(s[i] + s[i + 1])
        if combine in checks:
            sum += checks[combine]

    return sum

I'd love to get some feedback. I'm not sure if I should change the variable name from sum to something else, as I'm overwriting a python function. However, I don't use that function inside of my function, so does it matter?

Additionally, should I be converting the string to a list? Strings are immutable and I'm pulling apart and creating a new string with the combine variable, so I'm not sure if that's something I should be concerned about with regards to time complexity.

Don't use sum as a variable.  Especially because you want to use it with your approach!

sum = 0
for i in s:
    sum += values[i]

could become:

number = sum(values[i] for i in s)

s[i] is a string.  s[i] + s[i+1] is a string.  There is no need to use str() to convert what is already a string to a string.  But s[i:i+2] is easier still.

With 3999 as the maximum, the longest Roman numeral string would be 15 characters  ("MMMDCCCLXXXVIII").  Your last loop would iterate 14 times, taking two letter substrings.

There are only 6 two letter negative combinations.  You could instead search for those 6 patterns:

number += sum(value for key, value in checks.items() if key in s)

This is 6 \$O(N)\$ substring searches, instead of N \$O(1)\$ lookups.  Time complexity is unchanged, but this latter approach may be faster for many strings.  (It certainly will be slower for 1 character strings!)

checks and values are constants.  You should not recreate them every time you call the function.  Make them global, initialized once.

PEP-8:

Variables and functions should be snake_case, not camelCase, so romanToInt should be roman_to_int.

s  is too short to be descriptive.  roman might be better.

self?  This should be a function, not a method in a class.  If it is in a class, then use @staticmethod, since it doesn't use self (or cls) at all.

Result:

SUBTRACTIVE = {"IV": 2, "IX": 2, "XL": 20, "XC": 20, "CD": 200, "CM": 200}
VALUES = {"I": 1, "V": 5, "X":10, "L": 50, "C": 100, "D": 500, "M": 1000}

def roman_to_int(roman: str) -> int:

    number = sum(VALUES[ch] for ch in roman)
    number -= sum(val for key, val in SUBTRACTIVE.items() if key in roman)

    return number

Although I'm not a fan of CHECKS, but a better concise name is escaping me.  Now using SUBTRACTIVE as suggested by JollyJoker.

Speed Tests

Execution time for parsing all Roman numerals from I to MMMCMXCIX, repeated 100 times, in seconds:

somya_agrawal   1.288
ajneufeld       0.737
jg              1.521
jg2             1.324
maarten_bodewes 2.186
maarten_bodewes 1.653 (decode only)

It wasn't clear whether J.G. was using local variable or global variables for checks and values in their solution, so I coded it both ways.

Speed Test Code

import time

def create_roman(value):
    roman = ""
    for letters, val in (("M", 1000),
                         ("CM", 900), ("D", 500), ("CD", 400), ("C", 100),
                         ("XC", 90), ("L", 50), ("XL", 40), ("X", 10),
                         ("IX", 9), ("V", 5), ("IV", 4), ("I", 1)):
        while value >= val:
            roman += letters
            value -= val
    return roman

ROMAN_NUMERALS = [create_roman(value) for value in range(1, 4000)]

def somya_agrawal(s: str) -> int:
    checks = {"IV":-2,"IX":-2, "XL":-20, "XC":-20, "CD":-200, "CM":-200}
    values = {"I":1, "V":5,"X":10, "L":50,"C":100, "D":500,"M":1000}
    sum = 0

    for i in s:
        sum += values[i]

    for i in range(len(s) - 1):
        combine = str(s[i] + s[i + 1])
        if combine in checks:
            sum += checks[combine]

    return sum

CHECKS = {"IV": 2, "IX": 2, "XL": 20, "XC": 20, "CD": 200, "CM": 200}
VALUES = {"I": 1, "V": 5, "X":10, "L": 50, "C": 100, "D": 500, "M": 1000}

def ajneufeld(roman: str) -> int:
    number = sum(VALUES[ch] for ch in roman)
    number -= sum(val for key, val in CHECKS.items() if key in roman)

    return number

def jg(s: str) -> int:
    checks = {"IV":-2,"IX":-2, "XL":-20, "XC":-20, "CD":-200, "CM":-200}
    values = {"I":1, "V":5,"X":10, "L":50,"C":100, "D":500,"M":1000}

    result = sum(values[i] for i in s)
    result += sum(checks.get(s[i : i + 2], 0) for i in range(len(s) - 1))

    return result

checks = {"IV":-2,"IX":-2, "XL":-20, "XC":-20, "CD":-200, "CM":-200}
values = {"I":1, "V":5,"X":10, "L":50,"C":100, "D":500,"M":1000}

def jg2(s: str) -> int:

    result = sum(values[i] for i in s)
    result += sum(checks.get(s[i : i + 2], 0) for i in range(len(s) - 1))

    return result

def time_test(f):
    start = time.perf_counter()
    for _ in range(100):
        for roman in ROMAN_NUMERALS:
            f(roman)
    end = time.perf_counter()
    print(f"{f.__name__:15} {end-start:5.3f}")

if __name__ == '__main__':
    for f in (somya_agrawal, ajneufeld, jg, jg2):
        time_test(f)


JS/jQuery - Sort data by year An AJAX request gives me an object, containing dates and values:

"Values":{"2017-05-01T23:59:27":100.0,"2017-05-02T23:59:27":100.0,"2017-05-03T23:59:27":41.25,"2017-05-04T23:59:27":100.0,...}

The aim is to split this array by year, get all values of a same year together.

The current code is:

var dataPerYear = [];
function addToData(array, nameOfObject, dataToInsert) {
    //Check if the year is already in the array
    var object = $.grep(array, function (e, i) {
        return e.name == nameOfObject;
    });
    // If so, add new data to the correct object
    if (object.length > 0) {
        object[0].values.push(dataToInsert)
    } else
    // If not, create a new object, and add it to the array.
    {
        array.push({name: nameOfObject, values: [dataToInsert]})
    }
}

$(document).ready(function () {
        $.post("myEndPoint", function (data) {
            $.each(data,function (key, val) {
                    addToData(dataPerYear, new Date(key).getFullYear(), val);
            });
        });
});

How can I improve this code, with regards to readability / maintenability first, performance then (not a lot of data, so not a big concern)?

Instead of an array, and having to do a $.grep to check if the year is already in the array, consider using an object. That way, it's a key-value pair, and it's easier and faster to look up. You can convert the data to the final array later on.

Consider using promises instead of callbacks. It's much more flexible and allows better flow control than callbacks.

nameOfObject is misleading when all it is is just the year. Name it to year instead.

There's probably little sense breaking out that function unless something else uses it. It's better to inline operations in this case, making the code easier to trace.

Be wary of the use of new Date. Every time you do that, you create a date object. Since your timestamps are uniformly formatted, you might want to consider slicing off the first 4 characters and use it as year.

Here's an alternative approach. Probably not the most performant, but probably is easier to wrap your head around. Imagine that it's just transformation of data from one form to the other.

$(document).ready(function () {
  $.post("myEndPoint").then(data => {

    // {"2017-05-01T23:59:27":100.0, ... }
    // to
    // { "2017": [100.0, ...], ... }
    const dataByYear = Object.keys(data).reduce((c, t) => {
      const year = timestamp.slice(0, 4);
      c[year] = c[year] || [];
      c[year].push(data[t]);
      return c;
    }, {});

    // { "2017": [100.0, ...], ... }
    // to
    // [ { name: "2017", values: [100.0, ...] }, ... ]
    const dataPerYear = Object.keys(dataByYear).map(year => {
      return { name: year, values: dataByYear[year] }
    });

  })
});


JavaScript/PHP file uploader I'm writing a JavaScript/PHP file uploader and I want to start this project off on the right track. I've started with this, a JavaScript Uploader object which will handle Ajax calls, display progress, show error messages, etc. I'm just hoping to get some feedback on the way I'm setting up my scripts.

function Uploader(method) {

if(arguments.callee._singleton) return arguments.callee._singleton;
var _this = arguments.callee._singleton = this;

_this.opts = {
    form:"#uploader",
    filePath:"public/uploads/",
    errorCodes:{
        1:"No file/folder selected",
        2:"Invalid file/folder type",
        3:"File/folder exceeds upload size"
    }
}

_this.methods = {

    init: function(options) {
        for(var o in options) {
            _this.opts[o] = options[o];
        }

        _this.methods.setup();
    },

    setup: function() {

    }

}

if(_this.methods[method]) return _this.methods[method].apply(this, Array.prototype.slice.call(arguments, 1));
else if(typeof method === "object" || !method) return methods.init.apply(this, arguments);

}

Why not use an existing ajax file upload library, like Fine Uploader?
As for your code.
1)
I think you meant to return _this.methods.init.apply(this, arguments);.
2)
You should rewrite Uploader to not use arguments.callee for two reasons; it makes your code harder to read and for the browser to optimize.


How to reorganize event listeners in javascript I often find myself writing code as follows...

var startX, startY, dragging = false, element = document.getElementById("whatever");

var start = function(e){
  dragging = true;
  startX = e.pageX;
  startY = e.pageY;
  element.style.opacity = 0.5;
}

var move = function(e){
  if (!dragging) return;
  var deltaX = startX - e.pageX;
  var deltaY = startY - e.pageY;
  //apply to element
}

var end = function(e){
  dragging = false;
  element.style.opacity = 1;
}

element.addEventListener("mousedown", start, false);
document.addEventListener("mousemove", move, false);
document.addEventListener("mouseup", end, false);

Now, my problem is really basic, it feels to me that the functions should be auxiliary and belong to the bottom part of my program, but I don't know how to attach the functions to the event listeners without declaring them first. How do I restructure my code so that it is more evident what I'm going to do with these functions?

You could simply declare the functions inline, like:

element.addEventListener("mousedown", function (e){
  dragging = true;
  startX = e.pageX;
  startY = e.pageY;
  element.style.opacity = 0.5;
}, false);

And likewise for the other 2 functions.

But that can get messy in a hurry. Especially because startX, startY and dragging would still be outside.

So instead you could put the handler in an object literal, just to keep them separated:

var dragEvents, element = document.getElementById("whatever"),

dragEvents = (function () {
  var dragging = false, startX, startY;
  return {
    start: function (e){
      dragging = true;
      startX = e.pageX;
      startY = e.pageY;
      element.style.opacity = 0.5;
    },

    move: function (e){
      if (!dragging) return;
      var deltaX = startX - e.pageX;
      var deltaY = startY - e.pageY;
      //apply to element
    },

    end: function(e){
      dragging = false;
      element.style.opacity = 1;
    }
  };
}());

element.addEventListener("mousedown", dragEvents.start, false);
document.addEventListener("mousemove", dragEvents.move, false);
document.addEventListener("mouseup", dragEvents.end, false);

I've wrapped it in an immediately invoked function so the dragging-related vars can be kept in there too.

However, might as well go the extra step and put everything in a function, instead of relying on element being a closure:

function makeDraggable(elementId) {
  var startX,
      startY,
      dragging = false,
      element = document.getElementById(elementId);

  function start(e){
    dragging = true;
    startX = e.pageX;
    startY = e.pageY;
    element.style.opacity = 0.5;
  }

  function move(e){
    if (!dragging) return;
    var deltaX = startX - e.pageX;
    var deltaY = startY - e.pageY;
    //apply to element
  }

  function end(e){
    dragging = false;
    element.style.opacity = 1;
  }
  element.addEventListener("mousedown", start, false);
  document.addEventListener("mousemove", move, false);
  document.addEventListener("mouseup", end, false);
}

The last suggestion I'd make would be to skip the dragging variable in favor of simply adding the move and end handlers on mouse down, and removing them on mouse up, like so:

function makeDraggable(elementId) {
  var startX,
      startY,
      element = document.getElementById(elementId);

  function move(e){
    var deltaX = startX - e.pageX;
    var deltaY = startY - e.pageY;
    //apply to element
  }

  function end(e){
    element.style.opacity = 1;
    element.removeEventListener("mousedown", move, false);
    element.removeEventListener("mousedown", end, false);
  }

  element.addEventListener("mousedown", function (e){
    startX = e.pageX;
    startY = e.pageY;
    element.style.opacity = 0.5;
    document.addEventListener("mousemove", move, false);
    document.addEventListener("mouseup", end, false);
  }, false);
}


Updating a clock display with the time Originally, I was going to post a question about how to condense four repetitive functions into a single function, but I wound up figuring it out myself before I could even post it.

However, the original idea that I had was to break down some of the repetitive sections into their own function and use those new sub-functions as part of the main one. I'm still not sure how I would have been able to make that work if I hadn't figured out the bigger picture.

function GetClock(){
    var d=new Date().addHours(1);
    var nday=d.getDay(),nmonth=d.getMonth(),ndate=d.getDate(),nyear=d.getYear();
    if(nyear<1000) nyear+=1900;
    var nhour=d.getHours(),nmin=d.getMinutes(),nsec=d.getSeconds(),ap;

    if(nhour===0){ap=" ";nhour=12;}
    else if(nhour<12){ap=" ";}
    else if(nhour===12){ap=" •";}
    else if(nhour>12){ap=" •";nhour-=12;}

    if(nhour<=9) nhour="0"+nhour;
    if(nmin<=9) nmin="0"+nmin;
    if(nsec<=9) nsec="0"+nsec;

    document.getElementById('clockbox').innerHTML=""+tday[nday]+", "+tmonth[nmonth]+" "+ndate+", "+nyear+" "+nhour+":"+nmin+":"+nsec+""+ap+"";
}

This is one of the sections that I was originally thinking about splitting off into its own function:

if(nhour===0){ap=" ";nhour=12;}
else if(nhour<12){ap=" ";}
else if(nhour===12){ap=" •";}
else if(nhour>12){ap=" •";nhour-=12;}

These were my two feeble attempts to make it work:

function ampm() {
    if(nhour===0){ap=" ";nhour=12;}
    else if(nhour<12){ap=" ";}
    else if(nhour===12){ap=" •";}
    else if(nhour>12){ap=" •";nhour-=12;}
}

And then this:

function ampm(nhr,ampm) {
    if(nhr===0){ampm=" ";nhr=12;}
    else if(nhr<12){ampm=" ";}
    else if(nhr===12){ampm=" •";}
    else if(nhr>12){ampm=" •";nhr-=12;}
}

But they both failed due to (what I'm assuming are) issues with scope that I'm not sure how to solve.

Theres a lot of things here.

Disclaimer:

Using time stuff in JavaScript native is very hard and doesn't guarantee the cross browser. Also there are 300 of implementations of the same thing, plz don't make a new well. Please use a library like http://momentjs.com

First of all, is a little hard to read so, I started, formatting the code:

    function GetClock(){
        var d = new Date().addHours(1);
        var nday= d.getDay(),
            nmonth= d.getMonth(),
            ndate= d.getDate(),
            nyear= d.getYear();

        if(nyear<1000)
            nyear += 1900;

        var nhour= d.getHours(),
            nmin= d.getMinutes(),
            nsec= d.getSeconds(),
            ampm;

        if(nhour === 0) {
            ampm = " ";
            nhour = 12;
        } else if (nhour<12) {
            amPmSymbol =  " ";
        } else if(nhour===12) {
            ampm= " •";
        } else if(nhour>12) {
            ampm = " •";
            nhour-= 12;
        }

        if(nhour<=9)
            nhour = "0" + nhour;

        if(nmin<=9)
            nmin = "0" + nmin;

        if(nsec<=9)
            nsec = "0" + nsec;

        document.getElementById('clockbox').innerHTML =
            tday[nday] + ", " +
            tmonth[nmonth] + " " +
            ndate+", " +
            nyear + " " +
            nhour + ":" +
            nmin + ":" +
            nsec + "" + ampm + "";
    }

So, i can see some issues and repeated code on this.

This function doesn't return anything. So, why is called as Get?, I recommend use updateTime
I can split date format, from hour/time format and am/pm symbol (I guess).
also, I eliminate the unused/usefulness variables from the clock creation. Now all additional functions return strings, so, can be concatenate with the template.

function updateTime(){
    var aDate = new Date().addHours(1);
    var ndate = aDate.getDate(),
        nyear = aDate.getYear() ? ;

    if(nyear<1000)
        nyear += 1900;

    document.getElementById('clockbox').innerHTML =
        tday[aDate.getDay()] + ", " +
        tmonth[aDate.getMonth()] + " " +
        ndate + ", " +
        nyear + " " + getTimeAsFormatString(aDate);
}

function getTimeAsFormatString(aDate) {
    var hour = aDate.getHours(),
        mins = aDate.getMinutes(),
        secs = aDate.getSeconds();

    if(hour === 0) {
        hour = 12;
    } else if(hour>12) {
        hour-= 12;
    }

    hour = hour<= 9  ? "0" + hour : hour;
    mins = mins<= 9  ? "0" + mins : mins;
    secs = secs <= 9  ? "0" + secs : secs;

    return hour + ":" + mins  + ":" + secs +  "" + getAmPmSymbol(hour) + "";

}

function getAmPmSymbol(hour) {
    return (hour >= 12) ? " •" : " ";
}


QNetworkReply network reply timeout helper Since Qt still does not support to set timeouts on QNetworkRequest objects, I wrote this little wrapper class:

/**
Usage:
new QReplyTimeout(yourReply, msec);

When the timeout is reached the given reply is closed if still running
*/
class QReplyTimeout : public QObject {
  Q_OBJECT
public:
  QReplyTimeout(QNetworkReply* reply, const int timeout) : QObject(reply) {
    Q_ASSERT(reply);
    if (reply) {
      QTimer::singleShot(timeout, this, SLOT(timeout()));
    }
  }

private slots:
  void timeout() {
    QNetworkReply* reply = static_cast<QNetworkReply*>(parent());
    if (reply->isRunning()) {
      reply->close();
    }
  }
};

You can use it in a very simple fire-and-forget manner:

QNetworkAccessManager networkAccessManger;
QNetworkReply* reply = networkAccessManger.get(QNetworkRequest(QUrl("https://www.google.com")));
new QReplyTimeout(r, 100);

If the call to Google does not finish in 100ms, it is aborted. And since the QReplyTimeout class is parented to the QNetworkReply, it will be destroyed automatically.

Review the code for any pitfalls, memory leaks, invalid casts and if it's generally in a good style.

Here are my thoughts:

It's a simple class that allocates quite a few times on the heap. You could minimize the number of implicit allocations it does. The QTimer::singleShot creates a temporary QObject instance and allocates a bunch on the heap. You can avoid it by handling a single-shot timer explicitly using timerEvent. You also avoid the need to set up any connections that way. A QObject does about two allocations when the first connection is added to it.
Use Qt-5 style connections, but that doesn't apply anymore in light of #1 above.
Add a static helper method that uses this class to set a timeout on a network request. The fact that you need to allocate ReplyTimeout is an implementation detail of sorts that could be abstracted out that way.
Check if the reply is running before you set a timeout on it. Perhaps it was an incorrect/impossible request at the moment it was submitted to the manager and it was immediately finished.
You're not supposed to name your classes with a Q prefix. It's reserved for Qt.

The code below is portable across Qt 4 and Qt 5:

class ReplyTimeout : public QObject {
  Q_OBJECT
  QBasicTimer m_timer;
public:
  ReplyTimeout(QNetworkReply* reply, const int timeout) : QObject(reply) {
    Q_ASSERT(reply);
    if (reply && reply->isRunning())
      m_timer.start(timeout, this);
  }
  static void set(QNetworkReply* reply, const int timeout) {
    new ReplyTimeout(reply, timeout);
  }
protected:
  void timerEvent(QTimerEvent * ev) {
    if (!m_timer.isActive() || ev->timerId() != m_timer.timerId())
      return;
    auto reply = static_cast<QNetworkReply*>(parent());
    if (reply->isRunning())
      reply->close();
    m_timer.stop();
  }
};

Use:

QNetworkAccessManager networkAccessManger;
QNetworkReply* reply =
  networkAccessManger.get(QNetworkRequest(QUrl("https://www.google.com")));
ReplyTimeout::set(reply, 100);


Concatenating results in a comma separated list I am trying to list all of the employees linked to a certain 'Interaction' I want the employees listed on one line with comma's separating them, I have the following query which works but is very slow.

SELECT CONVERT(VARCHAR(254), COALESCE(STUFF((
SELECT ', ' + LTRIM(RTRIM(REPLACE(emp1.Full_Name,CHAR(9),'')))
FROM Employee emp1 WITH (nolock)
JOIN Rn_Int_Employee rie WITH (nolock) on emp1.Employee_Id = rie.Employee_Id
WHERE rnint.Rn_Interactions_Id = rie.Rn_Interactions_Id
GROUP BY emp1.Full_Name
ORDER BY emp1.Full_Name
FOR XML PATH (''),TYPE ).value('text()[1]', 'nvarchar(max)'), 1, 2, '')
,STUFF((
SELECT ', ' + LTRIM(RTRIM(REPLACE(emp1.Full_Name,CHAR(9),'')))
FROM Employee emp1 WITH (nolock)
WHERE rnint.Rn_Create_User=emp1.Rn_Employee_User_Id
GROUP BY emp1.Full_Name
ORDER BY emp1.Full_Name
FOR XML PATH (''),TYPE ).value('text()[1]', 'nvarchar(max)'), 1, 2, '')))
Employees
FROM Rn_Interactions rnint

Does anyone have any recommendations for a more efficient way of doing this?

Concatenating strings from rows in to a single field is an age-old quandary among the SQL Server community unfortunately.

In SQL Server 2017, Microsoft introduced the STRING_AGG function which has performance benefits over the "stuff XML" method you're using above. It also looks a lot cleaner, which is nice. Example:

SELECT
    i.Rn_Interaction_Id,
    STRING_AGG(e.Full_Name, ', ') WITHIN GROUP (ORDER BY e.Full_Name) as Employees
FROM Rn_Interactions i
    LEFT JOIN Rn_Int_Employee ie ON i.Rn_Interactions_Id = ie.Rn_Interactions_Id
    LEFT JOIN Employee e1 ON ie.Employee_Id = e1.Employee_Id
GROUP BY Rn_Interaction_Id

(Haven't actually tested that, but it shows you how you might use STRING_AGG)

If you haven't got and can't get SQL Server 2017 though, tough luck unfortunately. One solution might be to tackle the root of the problem: Create a field on the Rn_Interactions table that is meant to store this string, then re-write the string to the table every now and then (maybe when your app saves the interaction or changes the employees it's linked to). Then when you want a list of employees, it's right there already in the table, rather than having SQL re-generate it every time you select. It's far from a normalised and perfect solution, but hey, life is neither of those things.


Coordinates Struct for Flow Field Pathfinding... am I doing this right? I am slowly creating a flow field pathfinding solution for Unity3D.  I'm trying to follow Elijah Emerson's description, which appears in the book 'Game AI Pro' (volume 1).
The map is divided into sectors, and each sector houses three fields: cost, integration, flow, which need each other to calculate the flow field.
At one point I got fed up trying to reference locations in space between sectors and fields, so created a Coordinates object to handle this task.  But I'm not sure if I've created an object which makes good sense for an optimal solution?  I initially created a Struct, because the object is a data type which holds just world X and Y, along with a reference to the world this belongs to.
Because of this, I didn't want to load the object with more data fields, but I did need to reference various things described above.  So I added get-only properties which call data from elsewhere.  But is this fast and optimal?  I don't know.  I know there's an inevitable trade between memory and processing, but I just don't know what makes sense here for a fast solution.
using UnityEngine;

public struct Coordinates
{
    public readonly FlowFieldWorld world;
    public readonly int worldX, worldY;
    public int SectorX { get { return worldX % world.SectorSize; } }
    public int SectorY { get { return worldY % world.SectorSize; } }
    public FlowFieldSector Sector { get { return world.Sectors[worldX / world.SectorSize, worldY / world.SectorSize]; } }
    public CostTile Cost { get { return Sector.CostField[SectorX, SectorY]; } }
    public IntegrationTile Integration { get { return Sector.IntegrationField[SectorX, SectorY]; } }
    public FlowTile Flow { get { return Sector.FlowField[SectorX, SectorY]; } }
    public bool IsWalkable { get { return Cost.Value != 255; } }
    public readonly static Coordinates zero = new Coordinates(0, 0, null);
    public Vector3 Vector3 { get { return new Vector3(worldX, 0, worldY); } }

    public Coordinates(int worldX, int worldY, FlowFieldWorld world)
    {
        this.worldX = worldX;
        this.worldY = worldY;
        this.world = world;
    }

    public Coordinates(Coordinates coordinates)
    {
        this.worldX = coordinates.worldX;
        this.worldY = coordinates.worldY;
        this.world = coordinates.world;
    }

    public static bool operator ==(Coordinates left, Coordinates right)
    {
        return left.worldX == right.worldX && left.worldY == right.worldY;
    }

    public static bool operator !=(Coordinates left, Coordinates right)
    {
        return left.worldX != right.worldX || left.worldY != right.worldY;
    }

    public override bool Equals(object obj)
    {
        return obj is Coordinates coordinates && Equals(coordinates);
    }

    public bool Equals(Coordinates other)
    {
        return worldX == other.worldX && worldY == other.worldY;
    }

    public override int GetHashCode()
    {
        int hashCode = 1845962855;
        hashCode = hashCode * -1521134295 + worldX.GetHashCode();
        hashCode = hashCode * -1521134295 + worldY.GetHashCode();
        return hashCode;
    }

}

I have no knowledge about Unity3d, so here are some general comments.
If world.SectorSize is constant throughout the session, you can change some of the properties to readonly fields and calculate them in the constructor:
public readonly int SectorX;
public readonly int SectorY;
public readonly FlowFieldSector Sector;
...

public Coordinates(int worldX, int worldY, FlowFieldWorld world)
{
  this.worldX = worldX;
  this.worldY = worldY;
  this.world = world;
  SectorX = worldX % world.SectorSize;
  SectorY = worldY % world.SectorSize;
  Sector =  world.Sectors[worldX / world.SectorSize, worldY / world.SectorSize];
}

Doing so with Sector, Cost, Integration and Flow requires of cause that these objects are reference types and not replaced - if they are updated while the Coordinates instance lives. It is probably just a micro optimization - but everything counts?

public bool Equals(Coordinates other)
{
  return worldX == other.worldX && worldY == other.worldY;
}

This is actually an implementation of IEquatable<Coordinates> so you can add this interface to the inheritance list:
public struct Coordinates : IEquatable<Coordinates>

In respect to the DRY-principle you could do the following changes:
The copy constructor can call the other constructor:
public Coordinates(Coordinates source) : this(source.worldX, source.worldY, source.world)
{
}

The comparison operators can call Equals(Coordinates other):
public static bool operator ==(Coordinates left, Coordinates right)
{
  return left.Equals(right);
}


Stored procedure to write in a file The following stored procedure allows me to write in a file stored on my SQL Server:

CREATE PROCEDURE [dbo].[spWriteToFile]
(
  @PATH_TO_FILE nvarchar(MAX),
  @TEXT_TO_WRITE nvarchar(MAX)
)
AS
BEGIN
  DECLARE @OLE int
  DECLARE @FileID int

  EXECUTE sp_OACreate 'Scripting.FileSystemObject', @OLE OUT
  EXECUTE sp_OAMethod @OLE, 'OpenTextFile', @FileID OUT, @PATH_TO_FILE, 8, 1
  EXECUTE sp_OAMethod @FileID, 'WriteLine', NULL, @TEXT_TO_WRITE
  EXECUTE sp_OADestroy @FileID
  EXECUTE sp_OADestroy @OLE
END

In order to grant all the needed permissions, I had to execute the following query:

sp_configure 'show advanced options', 1;
GO
RECONFIGURE;
GO
sp_configure 'Ole Automation Procedures', 1;
GO
RECONFIGURE;
GO

Here is an exemple to execute the stored procedure:

-- NB: The folder "D:\test\" has to be created before
EXEC dbo.spWriteToFile N'D:\test\t1.txt', N'Hello World !'

My problem is that I have to run this stored procedure a lot and therefore I have some performance issues. Is there something I can improve in my implementation to improve performance ? Is there another way to write in a file using SQL Server that could be faster ? If not, I think I'll have to try to call it less often.

NB: I'm using SQL Server Express Edition 2014 in case it could be relevant to my issue.

As discussed in comments with @scsimon and @Milney, I've created the following table:

CREATE TABLE [dbo].[file_write]
(
  [ID] bigint NOT NULL IDENTITY(1,1) ,
  [PATH_FILE] nvarchar(500) NOT NULL ,
  [TEXT_FILE] nvarchar(500) NOT NULL ,
  [DATE_WRITE] datetime NOT NULL ,
  PRIMARY KEY ([ID])
)

After that, i've written a stored procedure in order to insert into this table using only PATH_FILE and TEXT_FILE as parameters. This procedure replaces the one I had written in my question.

CREATE PROCEDURE [dbo].[insertFileWrite]
(
  @PATH_FILE nvarchar(500),
  @TEXT_FILE nvarchar(500)
)
AS
BEGIN
  DECLARE @NOW datetime = CURRENT_TIMESTAMP

  INSERT INTO dbo.file_write(PATH_FILE, TEXT_FILE, DATE_WRITE)
         VALUES (@PATH_FILE , @TEXT_FILE , @NOW)
END

Finally, i've written a second stored procedure in order to write the lines that are in my table. This stored procedure is executed every 5 minutes by a scheduled task on the server.

CREATE PROCEDURE [dbo].[writeAllIntoFile]
AS
BEGIN
  DECLARE @PATH_FILE nvarchar(500)
  DECLARE @PATH_FILE_PREV nvarchar(500)
  DECLARE @TEXT_FILE nvarchar(500)
  DECLARE @ID bigint

  DECLARE @OLE int
  DECLARE @FileID int

  BEGIN TRANSACTION t_writeAllIntoFile
  BEGIN TRY

    DECLARE c_writeFile CURSOR FOR
      SELECT PATH_FILE,
             TEXT_FILE,
             ID
      FROM dbo.file_write
      ORDER BY PATH_FILE ASC,
               DATE_WRITE ASC

    OPEN c_writeFile
    FETCH NEXT FROM c_writeFile INTO @PATH_FILE, @TEXT_FILE, @ID

    SET @PATH_FILE_PREV = @PATH_FILE

    EXECUTE sp_OACreate 'Scripting.FileSystemObject', @OLE OUT
    EXECUTE sp_OAMethod @OLE, 'OpenTextFile', @FileID OUT, @PATH_FILE, 8, 1

    WHILE @@FETCH_STATUS = 0
    BEGIN
      IF (@PATH_FILE <> @PATH_FILE_PREV)
      BEGIN
        EXECUTE sp_OADestroy @FileID
        EXECUTE sp_OADestroy @OLE

        EXECUTE sp_OACreate 'Scripting.FileSystemObject', @OLE OUT
        EXECUTE sp_OAMethod @OLE, 'OpenTextFile', @FileID OUT, @PATH_FILE, 8, 1
      END

      EXECUTE sp_OAMethod @FileID, 'WriteLine', NULL, @TEXT_FILE
      DELETE FROM dbo.file_write WHERE ID = @ID

      SET @PATH_FILE_PREV = @PATH_FILE
      FETCH NEXT FROM c_writeFile INTO @PATH_FILE, @TEXT_FILE, @ID
    END

    CLOSE c_writeFile
    DEALLOCATE c_writeFile

    EXECUTE sp_OADestroy @FileID
    EXECUTE sp_OADestroy @OLE

    COMMIT TRANSACTION t_writeAllIntoFile
  END TRY
  BEGIN CATCH
    ROLLBACK TRANSACTION t_writeAllIntoFile

    IF (SELECT CURSOR_STATUS('global','c_writeFile')) >= -1
    BEGIN
      IF (SELECT CURSOR_STATUS('global','c_writeFile')) > -1
      BEGIN
        CLOSE c_writeFile
      END

      DEALLOCATE c_writeFile
    END

    RETURN -999
  END CATCH
END

Whis this solution, I don't have to open and close my file everytime I want to write one line into it. Since writing is now running on its side, performances have been a lot improved.


Merge two UIImages into one I have merge two images into one. I already implemented it with some help from the internet but it takes around 2,5s. I'm testing on the simulator so let's take it as a reference.

I currently use UIGraphicsBeingImageContext. Is there any faster way to achieve it?

extension UIImage {
    func overlayWith(image: UIImage, posX: CGFloat, posY: CGFloat, topImageSize: CGSize,
                     combinedImage: @escaping (UIImage) -> Void) {
        DispatchQueue.global(qos: .userInteractive).async {
            let newWidth = self.size.width < posX + image.size.width ? posX + image.size.width : self.size.width
            let newHeight = self.size.height < posY + image.size.height ? posY + image.size.height : self.size.height
            let newSize = CGSize(width: newWidth, height: newHeight)

            UIGraphicsBeginImageContextWithOptions(newSize, false, 0.0)
            self.draw(in: CGRect(origin: CGPoint.zero, size: self.size))
            image.draw(in: CGRect(origin: CGPoint(x: posX, y: posY), size: topImageSize))
            let newImage = UIGraphicsGetImageFromCurrentImageContext()!
            UIGraphicsEndImageContext()

            DispatchQueue.main.async {
                combinedImage(newImage)
            }
        }
    }
}

On a simulator with the original code, it takes about 1.52s on my machine.

Since the base image won't get resized (self.size is passed in self.draw(in:)), and its alpha channel is always 1, I could gain at least 200ms by using the following :

self.draw(at: CGPoint.zero, blendMode: .copy, alpha: 1)


Morse code converter I wanted an uncomplicated project to get myself started again. Although this works I was wondering if it's the best approach, especially when it comes to validation parts? It seems to me that I'm potentially looping quite a while, and this may not be the best or even common practice. I was hoping the CR community could enlighten me on this matter, as well as any other shortcomings this suffers from, as it has graciously done thus far.

public enum Morse {
    MC_1(".----", '1'), MC_2("..---", '2'),MC_3("...--", '3'),

    MC_4("....-", '4'), MC_5(".....", '5'), MC_6("-....", '6'),

    MC_7("--...", '7'), MC_8("---..", '8'), MC_9("----.", '9'),

    MC_0("-----", '0'), MC_A(".-", 'A'), MC_B("-...", 'B'),

    MC_C("-.-.", 'C'), MC_D("-..", 'D'), MC_E(".", 'E'),

    MC_F("..-.", 'F'), MC_G("--.", 'G'), MC_H("....", 'H'),

    MC_I("..", 'I'), MC_J(".---", 'J'), MC_K("-.-", 'K'),

    MC_L(".-..", 'L'), MC_M("--", 'M'), MC_N("-.", 'N'),

    MC_O("---", 'O'), MC_P(".--.", 'P'), MC_Q("--.-", 'Q'),

    MC_R(".-.", 'R'), MC_S("...", 'S'), MC_T("-", 'T'),

    MC_U("..-", 'U'), MC_V("...-", 'V'), MC_W(".--", 'W'),

    MC_X("-..-", 'X'), MC_Y("-.--", 'Y'), MC_Z("--..", 'Z'),
    MC_SPACE("/", ' ');

    public final String morse;
    public final char code;

    Morse(String morse, char code) {
        this.morse = morse;
        this.code = code;
    }
}

This contains the Main class:

import java.util.Scanner;

public class MorseCodeConverter {
    public static void main(String[] args) {
        Scanner input = new Scanner(System.in);
        System.out.print("Enter String to be converted to Morse Code: ");
        String userInput = input.nextLine();
        System.out.println(morseConversion(userInput));
    }

    public static String morseConversion(String s) {
        String ms = s.toUpperCase().trim(); // modified string
        StringBuilder result = new StringBuilder();

        for (int i = 0; i < ms.length(); i++) {
            isValid(ms.charAt(i));

            for (Morse c : Morse.values()) {
                if (ms.charAt(i) == ' ' && ms.charAt(i - 1) == ' '); // Ignore consecutive spaces
                else if (ms.charAt(i) == c.code) {
                    result.append(c.morse);
                    if (i != ms.length() - 1) { result.append((" ")); }
                }
            }
        }
        return result.toString();
    }

    public static boolean isValid(char c) {
        for (Morse m : Morse.values()) {
            if (c == m.code) return true;
        }
        throw new IllegalArgumentException("Invalid Entry: " + c + ". Non-space input may only be: 0-9, A-Z, or a-z");
    }
}

MC_1(".----", '1'),
...

This is pretty repetitive. Unfortunately, you can't call the object simply 1, but you can do better:

MC_1(".----"),
...
Morse(String morse) {
    this.morse = morse;
    this.code = name().charAt(name().length - 1);
}

This assumes, you can use all needed chars as a part of the name, which is not the case. So let's handle it by adding another constructor (your original and using

MC_SPACE("/", ' ');

just like you did. But I'm not sure at all, if using an enum is the way to go. What about using a simple Map<String, char> and another the other way round or better a BiMap?

Encapsulate it in a class Codec and let the class do more work.

public static String morseConversion(String s) {
    String ms = s.toUpperCase().trim(); // modified string

As this is nothing but a cleanup of s, I'd simply modify it. There's a rule about never modifying arguments, but it's wrong. Ask yourself:

What harm can happen by cleaning up s directly?
What harm can happen by having both s and sm?

The answers should make it perfectly clear in this case.

for (int i = 0; i < ms.length(); i++) {
    isValid(ms.charAt(i));

According to the name, isValid is a predicate. It returns a boolean to be used elsewhere. Your method should be called checkIsValid or alike. Or make it to a predicate and use checkArgument.

Validation is fine, but not here. What you do is determining if the following loop will succeed. So you're doing all the work twice.

for (Morse c : Morse.values()) {
     if (ms.charAt(i) == ' ' && ms.charAt(i - 1) == ' ');

This test is independent of c, so it does not belong to the loop.

What's worse: if (..); else if is a really terrible way if writing conditions. It's pretty confusing. If you need something like this, write

     if (ms.charAt(i) == ' ' && ms.charAt(i - 1) == ' ') {
         // Do nothing. Ignoring consecutive spaces.
     } else if (ms.charAt(i) == c.code) {

  It seems to me that I'm potentially looping quite a while, and this may not be the best or even common practice.

That's exactly what a Map is for. Feed it with the char. and it gives you the String. No loop needed.


Parsing huge xml file with lxml.etree.iterparse in python After solving the error in SO (like suggested) I return now for codereview. :-)

The task is to parse a huge file dblp.xml (~800 MB) presented by DBLP. The records in this huge file do look for example like this or this. In particular:

<?xml version="1.0" encoding="ISO-8859-1"?>
<!DOCTYPE dblp SYSTEM "dblp.dtd">
<dblp>
    record_1
    ...
    record_n
</dblp>

I wrote some code, that shall get me ech tag of some records (will bes tored in a database).

I adapted this approach from an article from IBM developerWorks which refers to the article Incremental Parsing on effbot.org. Is this the correct approach for this task? Or is there a better way?

import sys
import os
import MySQLdb
from lxml import etree

def fast_iter2(context, cursor):
    # Available elements are:   article|inproceedings|proceedings|book|incollection|phdthesis|mastersthesis|www
    elements = set(['article', 'inproceedings', 'proceedings', 'book', 'incollection', 'phdthesis', "mastersthesis", "www"])
    # Available tags are:       author|editor|title|booktitle|pages|year|address|journal|volume|number|month|url|ee|cdrom|cite|
    #                           publisher|note|crossref|isbn|series|school|chapter
    childElements = set(["title", "booktitle", "year", "journal", "ee"])

    paper = {} # represents a paper with all its tags.
    authors = []   # a list of authors who have written the paper "together".
    paperCounter = 0
    for event, element in context:
        tag = element.tag
        if tag in childElements:
            if element.text:
                paper[tag] = element.text
                # print tag, paper[tag]
        elif tag == "author":
            if element.text:
                authors.append(element.text)
                # print "AUTHOR:", authors[-1]
        elif tag in elements:
            paper["element"] = tag
            paper["mdate"] = element.get("mdate")
            paper["dblpkey"] = element.get("key")
            # print tag, element.get("mdate"), element.get("key"), event
            if paper["element"] in ['phdthesis', "mastersthesis", "www"]:
                pass # throw away "unwanted" records.
            else:
                populate_database(paper, authors, cursor)
            paperCounter += 1
            print paperCounter
            paper = {}
            authors = []
            # if paperCounter == 100:
            #     break
            element.clear()
            while element.getprevious() is not None:
                del element.getparent()[0]
    del context

def main():
        cursor = connectToDatabase()
        cursor.execute("""SET NAMES utf8""")

        context = etree.iterparse(PATH_TO_XML, dtd_validation=True, events=("start", "end"))
        fast_iter(context, cursor)

        cursor.close()

if __name__ == '__main__':
    main()

The name fast_iter2 seems to lack much of a connection with the function is actually doing.

Rather then having your two sets be local variables, I suggest putting them as global constants.

Calling them elements and childElements is kinda generic. I suggest something more specific

Your loop is a bit awkward. Rather then collecting them in that iterative format, just grab everything when looking at the element tags. This makes the code filling up paper/authors more straightforward.

Rather then having an empty if block and using the else block, invert the logic.

I suggest moving the element clearing code into its own function. This helps make the function actual task clearer avoiding the question of how to clear.

The del context line is useless. Del doesn't destory the object, it merely removes the object from the current context. If this was the only context in which object was refereed to then its reference count would drop to zero. However, that's not the case here as the calling function will keep it alive anyways. Even if it did, the memory savings aren't worth worrying about since the whole script is about to end anyways.

My reworking of your code, no testing has been done on it:

    CATEGORIES = set(['article', 'inproceedings', 'proceedings', 'book', 'incollection', 'phdthesis', "mastersthesis", "www"])
    SKIP_CATEGORIES = set(['phdthesis','mastersthesis', 'www'])
    DATA_ITEMS = ["title", "booktitle", "year", "journal", "ee"]

    def clear_element(element):
        element.clear()
        while element.getprevious() is not None:
            del element.getparent()[0]

    def fast_iter2(context, cursor):
        # Available elements are:   article|inproceedings|proceedings|book|incollection|phdthesis|mastersthesis|www
        # Available tags are:       author|editor|title|booktitle|pages|year|address|journal|volume|number|month|url|ee|cdrom|cite|
        #                           publisher|note|crossref|isbn|series|school|chapter

        paperCounter = 0
        for event, element in context:
            if element.tag in CATEGORIES:
                authors = [author.text for author in element.findall("author")]
                paper = {
                    'element' : element.tag,
                    'mdate' : element.get("mdate"),
                    'dblpkey' : element.get('key')
                }
                for data_item in DATA_ITEMS:
                    data = element.find(data_item)
                    if data is not None:
                        paper[data_item] = data

                if paper['element'] not in SKIP_CATEGORIES:
                    populate_database(paper, authors, cursor)

                paperCounter += 1
                print paperCounter

                clear_element(element)

EDIT

How to avoid an explicit counter:

    CATEGORIES = set(['article', 'inproceedings', 'proceedings', 'book', 'incollection', 'phdthesis', "mastersthesis", "www"])
    SKIP_CATEGORIES = set(['phdthesis','mastersthesis', 'www'])
    DATA_ITEMS = ["title", "booktitle", "year", "journal", "ee"]

    def clear_element(element):
        element.clear()
        while element.getprevious() is not None:
            del element.getparent()[0]

    def extract_paper_elements(context):
        for event, element in context:
             if element.tag in CATEGORIES:
                   yield element
                   clear_element(element)

    def fast_iter2(context, cursor):
        for paperCounter, element in enumerate(extract_paper_elements(context)):
                authors = [author.text for author in element.findall("author")]
                paper = {
                    'element' : element.tag,
                    'mdate' : element.get("mdate"),
                    'dblpkey' : element.get('key')
                }
                for data_item in DATA_ITEMS:
                    data = element.find(data_item)
                    if data is not None:
                        paper[data_item] = data

                if paper['element'] not in SKIP_CATEGORIES:
                    populate_database(paper, authors, cursor)

                print paperCounter