Hide / Show panels depending of value of a dropdown According to the value of a dropdown menu I have to show/hide two panels.
For option 'derive' and 'reject' I should show the comment panel, and just for the 'derive' panel I should also show a derivation panel. There are other

This works, but how could I refactor the code to make it shorter and more efficient without losing clarity?

 $("#formTabs").on( "change", "select.actions",function(){

        var parentContainer = $(this).closest("fieldset");

        if(this.value == "reject" || this.value == "derive"){
            parentContainer.find(".commentPanel").show();
        }
        // other 3 options where the panel should not show up.
        else {
            parentContainer.find(".commentPanel").hide();
        }

        // Just for the derive option, a derivation panel must be shown.
        if(this.value == "derive"){
            parentContainer.find(".derivationPanel").show();
        } else {
            parentContainer.find(".derivationPanel").hide();
        }
    });

The only "efficiency" improvement I see is caching the result of the this.value == "derive" check to avoid evaluating the string comparison twice. The effect of this is close to nothing,
on the other hand it's good to reduce the number of duplicated literals.

".commentPanel" and ".derivationPanel" are also duplicated literals,
so it would be good to eliminate those too.

Lastly, I would recommend some minor style improvements:

Don't put a space between ( "change"
Put a space after the comma in "select.actions",function
Put a space between ){ in function(){, everywhere

Putting it together:

$("#formTabs").on("change", "select.actions", function() {

    var parentContainer = $(this).closest("fieldset");
    var commentPanel = parentContainer.find(".commentPanel");
    var derivationPanel = parentContainer.find(".derivationPanel");
    var isDerive = this.value == "derive";

    if (this.value == "reject" || isDerive) {
        commentPanel.show();
    } else {
        // other 3 options where the panel should not show up.
        commentPanel.hide();
    }

    // Just for the derive option, a derivation panel must be shown.
    if (isDerive) {
        derivationPanel.show();
    } else {
        derivationPanel.hide();
    }
});

Note that by extracting the commentPanel, derivationPanel, isDerive variables you don't lose any efficiency at all, as all the right-hand sides would be evaluated in the code no matter what. This rewritten version is better, because of the string literals now appear only once, which eliminates typos, and if you need to change something, you can do it at a single place instead of two as in the original.


Getting a key's value in a nested dictionary I just found a now deleted question on Stack Overflow and solved the problem. The OP was asking for a way to get all the values contained by all 'PLU' keys. I solved it this way but because I am a newbie in python I was wondering if it would be an optimal/better way of doing this, maybe using iterators, I don't know.

Result should be:

234
32
33
334
61

def getPLU(data):
    for item in data:
        if type(data[item]) == dict:
            getPLU(data[item])
        else :
            if item == 'PLU':
                print (data[item])

    pass

def Main():
    times = 0
    Menu = {
      'PLU' : '234',
      'Salad': {
        'salad': {
            'ceaser':{
                'PLU': '32'
            },
            'italian':{
                'PLU': '33'
            }
        }
      },
      'Dessert': {
        'cookie': {
          'PLU': '334',
          'NAME': 'cookie ',
        }
      },
      'Appetizer': {
        'extra sauce': {
          'PLU': '61',
          'NAME': 'extra sauce',
        }
      }
    }
    getPLU(data=Menu)
    #print (getPLU(Menu))

if __name__ == '__main__':
    Main()

Great job so far! I have a few suggestions for your code:

Avoid writing functions with side effects such as printed output. Side effects make a function much less reusable. Instead, you may return a generator that yields the next entry. This gives the caller maximum control over what to do with the result set: print it, iterate one by one, just get the first few items without searching the entire structure, etc.
Consider adhering more strictly to Python naming conventions. For example, functions should be lower_camel_cased. Since your function returns multiple PLUs, the function name seems more accurately written as get_plus. You can remove the pass keyword in your function and pay close attention to spacing, for example in the print (data[item]) function call and your else : block.
Clean up lines like

else :
    if item == 'PLU':
        print (data[item])

Which is logically equivalent to:

elif item == 'PLU':
    print(data[item])

Use data.items() to iterate through your dictionary's keys and values rather than only keys. This allows cleaner syntax to access values.
Make your function more general for maximum reusability. Fetching a list of values by key from a dictionary is a task that should work for any key. Why not make "PLU" a search parameter? If you want to keep the original get_plus version, write both functions and have get_plus wrap the generalized version.
isinstance may be a more accurate choice than type if you wish to allow collection subclasses of dictionary to use your function.

Here's my version for consideration:

def find_by_key(data, target):
    for key, value in data.items():
        if isinstance(value, dict):
            yield from find_by_key(value, target)
        elif key == target:
            yield value

def main():
    menu = {
      'PLU' : '234',
      'Salad': {
        'salad': {
            'ceaser':{
                'PLU': '32'
            },
            'italian':{
                'PLU': '33'
            }
        }
      },
      'Dessert': {
        'cookie': {
          'PLU': '334',
          'NAME': 'cookie ',
        }
      },
      'Appetizer': {
        'extra sauce': {
          'PLU': '61',
          'NAME': 'extra sauce',
        }
      }
    }

    for x in find_by_key(menu, "PLU"):
        print(x)

if __name__ == '__main__':
    main()


Print out a table that displays how far a cannon can shoot an object at incremental speeds and angles Does the way I am storing and printing out the data make any sense? or am I going about it all wrong. the code works great, but I'm not entirely sure it was the best way to go about it. Also, I think I can ditch the arrayList and use an array, but I'm not sure if I should.

import java.util.ArrayList;
public class CatapultTester
{
    public static void main (String[] args)
    {
        ArrayList<Catapult> cat = new ArrayList<Catapult>();

        int angleIntervals = 5;
        int speedIntervals = 5;

        int startAngle = 25;
        int startSpeed = 20;

        int NumAngles = 6;
        int NumSpeeds = 7;

        // you can add rows/columns just by changing the two ints above. (you can also change the starting points and the intervals in which they increase)

        // it should also be relatively easy to make the header adjust itself to added/removed columns, but I'll do that later.

        for(int x = 0; x < NumSpeeds; x++)
        {
            for(int y = 0; y < NumAngles; y++)
            {
                cat.add(new Catapult((y * angleIntervals) + startAngle, (x * speedIntervals) + startSpeed));
            }
        }

        // just calculates distance for every angle/speed combo.
        for(int i = 0; i < cat.size(); i++)
        {
            cat.get(i).calcDist();
        }

        System.out.println("       |                    Projectile Distance (ft.)                    ");
        System.out.println("*************************************************************************");
        System.out.println("  MPH  |  25 Deg     30 Deg     35 Deg     40 Deg     45 Deg     50 Deg  ");
        System.out.println("*************************************************************************");

        for(int x = 0, i = 0; x < NumSpeeds; x++)
        {
            double mph = cat.get(i).getMilesPerHour();

            System.out.printf("  %-8.0f", mph);

            for(int y = 0; y < NumAngles; y++, i++)
            {
                double dist = cat.get(i).getDist();

                System.out.printf("%-10.2f ", dist);
            }

            System.out.print("\n");
        }

        System.out.println("*************************************************************************");
    }
}

Here is the Catapult class

public class Catapult
{
    private double launchAngle;
    private double milesPerHour;
    private double distance;

    /**
     * Constructor for objects of class Catapult
     *
     * @param angle The angle at whitch the projectile is launched.
     *
     * @param mph The speed at whitch the projectile is launched.
     */
    public Catapult(int angle, int mph)
    {
        launchAngle = angle;
        milesPerHour = mph;
    }

    /**
     * Calulates how far the projectile travels
     */
    public void calcDist()
    {
        distance = (Math.pow(milesPerHour, 2) * Math.sin( 2 * Math.toRadians(launchAngle)) / 9.8);
    }

    /**
     * @return Returns distance.
     */
    public double getDist()
    {
        return distance;
    }

    //not used, but I added it anyway. it could be used to make a dynamic header for the output.
    /**
     * @return Returns launchAngle.
     */
    public double getLaunchAngle()
    {
        return launchAngle;
    }

    /**
     * @return Returns milesPerHour.
     */
    public double getMilesPerHour()
    {
        return milesPerHour;
    }
}

Also, sorry about the messy code.

Welcome to Code Review! You did a good job so far, but I have some style and logic suggestions for you.

Let's talk about the Catapult class first. In its current form is a good container but it has two flaws. First, you declare the constructor to accept int parameters but are storing them in double variables. Either it should be all int or should be all double, but I am assuming for the rest of this answer that you want double. The second flaw is that you calculate the distance in a separate function and not the constructor. This concept of a cached value is not the flaw, but the way you are using it, is. The only reason you would want to calculate that distance in a different location than in the constructor of Catapult is if it was a very expensive operation that might not need to be called. For this simple example, make the function private and call it in the constructor as follows:

private double angle;
private double speed;
private double distance;

// The second parameter is named "speed" to be consistent with the rest of the
// code. Its better to write a comment or documentation stating that "angle"
// is in degrees and "speed" is in miles per hour.
public Catapult(double angle, double speed) {
    this.angle = angle;
    this.speed = speed;
    this.distance = this.calculateDistance();
}

...

private double calculateDistance() {
    return speed * speed * Math.sin(2 * Math.toRadians(angle)) / 9.8;
}

public double getDistance() {
    return this.distance;
}

Also, rename the other getters to getAngle and getSpeed accordingly.

(Edit: perhaps getLaunchAngle can stay, but getMilesPerHour is not a good function name.)

Next let's talk about CatapultTester. You should move the constants that are defined at the top of main into the class definition itself. I suggest making a constructor for a CatapultTester that takes in those arguments. This way, you have the ability to make several unique CatapultTester objects. If, for example, you want the user of your library to test the Catapult in a custom way, they can build their own CatapultTester objects. Provide that ability to the user:

public class CatapultTester {
    // make sure these are reasonable defaults
    private double startAngle     = 25;
    private double startSpeed     = 20;
    private double angleInterval  =  5;
    private double speedInterval  =  5;
    private double numAngles      =  6;
    private double numSpeeds      =  7;

    public CatapultTester() {}
    public CatapultTester(double startAngle,    double startSpeed,
                          double angleInterval, double speedInterval,
                          double numAngles,     double numSpeeds)
    {
        ...  // implement custom constructor here
    }

    public void printTest() {
        ...  // put your test program here instead
    }

    ...  // add more functionality in the future?
}

The point of doing this is that you're putting all the details of the object in the class itself instead of the main program. The main function would end up looking something like this:

public static void main(String[] args) {
    new CatapultTester().printTest();
}

This clearly conveys the meaning behind your main function, without the specifics that don't need to be known by the user. Now, if you want to expand main in any way, you have a more powerful API to work with.

Now to your actual test function. There are several instances of duplicated code, note the three for loops that run over the same range. What you should do instead is just loop over the range you need and do all the computation in one step. This would also remove the need for an array for the Catapult objects.

The last for loop is enough:

System.out.println("       |                    Projectile Distance (ft.)                    ");
System.out.println("*************************************************************************");
System.out.println("  MPH  |  25 Deg     30 Deg     35 Deg     40 Deg     45 Deg     50 Deg  ");
System.out.println("*************************************************************************");

for (int speed = 0; speed < this.numSpeeds; ++speed) {
    StringBuilder output = new StringBuilder();  // StringBuilder is fast!
    for (int angle = 0; angle < this.numAngles; ++angle) {

        // build one catapult at a time and calculate all necessary values in one go
        Catapult cat = new Catapult(
            angle * this.angleInterval + this.startAngle,
            speed * this.speedInterval + this.startSpeed
        );

        // prints only once at the beginning of the angle loop
        if (angle == 0) {
            output.append(String.format("  %-8.0f", cat.getSpeed()));
        }

        output.append(String.format("%-10.2f ", cat.getDistance()));
    }

    System.out.println(output.toString());
}

System.out.println("*************************************************************************");

You will have to import java.util.StringBuilder for this functionality. This drastically reduces the number of computations and repetitions in your code. Its important to keep in mind the acronym DRY (Don't Repeat Yourself) and if you see something that happens over and over again, see if you can write it once but get the same computations out of it. It will speed up your code and make it more readable. I hope you found these suggestions helpful. Good job and good luck!


Using finally with return statement or not I was wondering if it is the proper way to always put the return statement of a function with a try-catch clause in the finally clause.

I have for example this function which returns the version of an sql server:

public String getVersion() {
        String version = new String();
        final String query = "Select VERSION()";
        Connection con = null;
        Statement stmt = null;

        try {
            Helper.println("Getting driver...");
            Class.forName(_driver);

            Helper.println("Connecting to database...");
            con = DriverManager.getConnection(_url, _user._username, _user._password);
            stmt = con.createStatement();

            Helper.println("Executing query...");
            ResultSet rs = stmt.executeQuery(query);
            rs.next();
            version = rs.getString(1);
        } catch (SQLException se) {
            Helper.printErrln("SQL Error: " + se.getErrorCode() + ' ' + se.getMessage());
        } catch (ClassNotFoundException e) {
            Helper.printErrln("Driver Error: " + e.getMessage());
        } finally {
            try {
                if (stmt != null) {
                    stmt.close();
                }
            } catch (SQLException se) {
            }
            try {
                if (con != null) {
                    con.close();
                }
            } catch (SQLException se) {
            }
            return version;
        }
    }

I return the version variable always in the finally clause in situations like these and it particularly convenient when I need to return a list because if something went wrong then I will just be left with an empty list which I prefer. For example:

public List<String> getDatabases() {
        List<String> databases = new ArrayList<>();
        Connection con = null;
        Statement stmt = null;

        try {
            Helper.println("Getting driver...");
            Class.forName(_driver);

            Helper.println("Connecting to database...");
            con = DriverManager.getConnection(_url, _user._username, _user._password);
            stmt = con.createStatement();

            Helper.println("Finding Databases...");
            ResultSet rs = con.getMetaData().getCatalogs();

            while (rs.next()) {
                databases.add(rs.getString("TABLE_CAT"));
            }
        } catch (SQLException se) {
            Helper.printErrln("SQL Error: " + se.getErrorCode() + " " + se.getMessage());
        } catch (ClassNotFoundException e) {
            Helper.printErrln("Driver Error: " + e.getMessage());
        } finally {
            try {
                if (stmt != null) {
                    stmt.close();
                }
            } catch (SQLException se) {
            }// do nothing
            try {
                if (con != null) {
                    con.close();
                }
            } catch (SQLException se) {
            }
            return databases;
        }
    }

So first of all my question is: Is what I am doing proper or can it cause problems that I have not anticipated? Also what if I wanted to encapsulate the function and have it trow the exception to the parent class for handling, how would the return be handled then?

Return statement in the finally block is almost always a very bad idea.....

The sematics of the finally block are complicated, but, if there is a return statement the try block, or in a catch block, then those will be called, and then the finally block will run, and it will change the return value.

IDE's, and the compiler, will complain about this:

Code like the following, will return 2 .... always:

private static int doSomething() {
    try {
        .... something here....
        return 0;
    } catch (Exception e) {
        return 1;
    } finally {
        return 2;
    }

}

The point is that it is very bad practice to have returns in the finally block. Don't.....

EDIT: Update to include a better solution.... Use Java try-with-resources (since Java7).

private static final String VERSION_QUERY = "Select VERSION()";
private static final String VERSION_NOT_FOUND = "";

public String getVersion() {

    try {
        Helper.println("Getting driver...");
        Class.forName(_driver);
    } catch (ClassNotFoundException e) {
        Helper.printErrln("Driver Error: " + e.getMessage());
        return VERSION_NOT_FOUND;
    }

    Helper.println("Connecting to database...");
    try (Connection con = DriverManager.getConnection(_url, _user._username, _user._password);
         Statement stmt = con.createStatement();) {

        Helper.println("Executing query...");
        try (ResultSet rs = stmt.executeQuery(VERSION_QUERY);) {
            rs.next();
            return rs.getString(1);
        }
    } catch (SQLException se) {
        Helper.printErrln("SQL Error: " + se.getErrorCode() + ' ' + se.getMessage());
        return VERSION_NOT_FOUND;
    }
}


Function to foreach over an array I made a very simple generic function to foreach over an array in C using a callback function:

#include <stddef.h>
void foreach_array(void* array, size_t size, size_t bytes_per_element, void (*callback)(void*)) {
    char* iter;
    for(iter = array; iter < (char*)array + (int)size; iter += (int)bytes_per_element) {
        callback((void*)iter);
    }
}

It can be used like:

#include <stdio.h>
void print_double(void* doubleToPrint) {
    printf("%f\n", *((double*)doubleToPrint));
}

int main() {
    double myArray[] = {3.0, 6.0, 9.0};
    foreach_array(myArray, sizeof(myArray), sizeof(*myArray), print_double);
}

Comments:

I had to use char* instead of void* for the iteration variable, Because the standard doesn't define pointer arithmetic on void*.
I typecasted the size and bytes_per_element arguments to an int because I wasn't sure if it was defined behavior to use pointer arithmetic using a size_t value.

I thought this foreach function was interesting, So I wanted to put it here for a code review.

Well, it's unavoidably quite complex to use, you need to provide 3 arguments to describe the array (start, size, element_size) and a function accepting a single element as a void*-argument:
Writing the loop manually is far simpler.

To top it of, you cannot pass any context to your callback, meaning it can only work on global state. That's all kinds of unfortunate.

Now, let's look at your implementation:
You are far too cast-happy.

The casts to int are wrong, SIZE_MAX can be bigger than INT_MAX. Anyway they are superfluous because yes you can use a size_t for pointer-arithmetic.
The cast to void* is superfluous, it would be implicitly converted.

Incorporating that all gives you:

#include <stddef.h>
void foreach_array(
    void* array,
    size_t size,
    size_t bytes_per_element,
    void (*callback)(void* context, void* element),
    void* context
) {
    for(char* iter = array; iter < (char*)array + size; iter += bytes_per_element)
        callback(context, iter);
}


Sending data through a TCP stream I've noticed that there are several ways of sending data through a TCP stream. I want to do it the fastest way in terms of latency.

One method I became aware of is with a binary writer:

using (MemoryStream ms = PrintWindow(process))
{
    writer.Write((int)ms.Length);
    writer.Write(ms.GetBuffer(), 0, (int)ms.Length);
}

And to receive it:

Image.FromStream(new MemoryStream(reader.ReadBytes(reader.ReadInt32())));

It writes the size for reference, then takes the data.

For the other method, which is a bit sensitive, it seems that it can easily fail if the image changes for some reason:

using (MemoryStream ms = PrintWindow(process))
{
    tcp.GetStream().Write(ms.GetBuffer(), 0, tcp.SendBufferSize);
}

And to receive it:

byte[] b = new byte[tt1.ReceiveBufferSize];
tt1.GetStream().Read(b, 0, tt1.ReceiveBufferSize);
Image.FromStream(new MemoryStream(b));

I've found this with trial and error.  But, as I've said, it's very sensitive and can cause an error if the image changes in a certain way.  I'm not absolutely sure why, though.

Which of these is the fastest?  In my view, writing with just the stream should be faster than with some special writer.

The error I get when using the second method seems to occur when the data is very low.  For example: a black or near-black picture. If anyone knows how to solve this, please let me know.

It seems that I need to have the TCP Send and Receive buffer at least as big as the largest file for it to work, so I've increased it by x128.  I'm not sure if that's a good idea or not, but from my understanding, the only thing it does is allocate more RAM.  Then again, I think it's probably because I set the byte size to the buffer size.

If you are looking for ways to optimize your application - you are clearly looking at the wrong place, at least in my opinion. BinaryWriter is a simple wrapper around actual stream. All it does is it converts simple types to byte arrays and writes those to stream. Ofc it is slower, as any other wrapper, but not that much slower. In real life this difference can be ignored.

If writing to NetworkStream is your main performance issue, then consider wrapping it with BufferedStream.

If it isnt, and you are simply curious... then you can always use disassembler to see whats inside BinaryWriter.Write method for yourself.


Do they have the POWER? I'm working on a city building simulation game, and I have reached the point where I need to check whether tiles are connected to a power source.  I decided on a flood fill for this, since it seemed simple enough to implement.  For reference, I used this flood fill algorithm, and I also found this question on code review about flood fill in java.  My code is largely similar to the code in those links, but it is slightly different because I am not filling an image with color but rather marking tiles connected to power plants as isPowered.

I know that recursion is dangerous, and the efficiency of the code may be suspect.  With the map mostly filled up, this method gets called a couple of thousand times per cycle. With just a snaky path, about 200 times.

Here is an example to show that the algorithm is working:

The visible tiles are powered.

Here is the code:

private void calculatePower() {
    for (Tile tile : this.tilesForSort) {
        tile.hasBeenSearched = false;
    }
    for (Tile tile : this.tilesForSort) {
        if (tile.getBuildingType() == IsoTileType.POWER) {
            this.powerFill(tile.getPosition().x, tile.getPosition().y);
        }
    }
}
public void powerFill(int x, int y) {

    if (this.isNotInsideGrid(x, y)) {
        return;
    }

    Tile tile = this.levelData[x][y];

    if (tile.hasBeenSearched || tile.zoneType.getLevel() < 1) {
        return;
    }

    tile.isPowered = true;
    tile.hasBeenSearched = true;

    this.powerFill(x - 1, y);
    this.powerFill(x + 1, y);
    this.powerFill(x, y - 1);
    this.powerFill(x, y + 1);
}
private boolean isNotInsideGrid (int x, int y) {
    return x < 0 || y < 0 || x >= World.WORLD_WIDTH || y >= World.WORLD_HEIGHT;
}

It seems that Tile is a key element in your game model.
On the other hand, a field like Tile.hasBeenSearched sounds something technical, not something that belongs to conceptual model.
You probably don't wake up in the morning thinking, "hm, it will be nice to give the Tile class a hasBeenSearched attribute". This field is an implementation detail of the flood-fill algorithm, and I don't think it fits well into Tile. It will be better to use a Set<Tile> searched.
This field could be encapsulated within a FloodFiller class,
that takes tilesForSort and returns a collection of tiles for which the caller can set isPowered to true.
The advantage of this approach will be that you can reuse FloodFiller for other purposes, say, to actually flood your city.

This call is a bit tedious, with the duplicated tile.getPosition():

powerFill(tile.getPosition().x, tile.getPosition().y);

It would be good to create a helper powerFill(tile.getPosition()) that forwards to powerFill(int, int).

The name isNotInsideGrid is a bit clumsy. How about isOutsideGrid ?

Perhaps something like this might work well:

public class FloodFill {
    interface World {
        Set<Position> getEligibleNeighbors(Position position);
    }

    interface Position {
    }

    public static Set<Position> floodFill(World world, Position start) {
        Set<Position> searched = new HashSet<>();
        floodFill(world, searched, start);
        return searched;
    }

    private static void floodFill(World world, Set<Position> searched, Position position) {

        if (searched.contains(position)) {
            return;
        }

        searched.add(position);

        for (Position neighbor : world.getEligibleNeighbors(position)) {
            floodFill(world, searched, neighbor);
        }
    }
}

The idea is:

It is the responsibility of the World implementation to return the valid neighbors of a position that are eligible for filling:

x +- 1, y +- 1, or whatever coordinate system you use
Excluding positions outside of the world's boundary
Excluding positions where zoneType.getLevel() < 1
Any other custom logic you might need

The FloodFill class doesn't need to know anything else about the world, and doesn't need to know what "filling" really means

This gives you the flexibility to reuse the flood fill logic to other purposes, without polluting the rest of your game model with specific knowledge catering to the flood fill process.


Validating JavaScript origins I need to write a function in Python that makes sure the user entered a valid JavaScript origin. If I understand it correctly, the origin includes the scheme, hostname and port (port and scheme might be implicit, defaulting to 80 and http respectively), so would this be a correct way to validate it?

import urlparse

def validate_javascript_origin(origin):
    parsed = urlparse.urlsplit(origin)

    if parsed.scheme and parsed.scheme not in ["http", "https"]:
        raise ValueError("Only the http and https url schemes are supported.")

    if not parsed.netloc:
        raise ValueError("The origin must include a hostname.")

    if parsed.path or parsed.query or parsed.fragment:
        raise ValueError("The origin must not contain a path, query string or fragment.")

The origin will be used to pass as the preferredOrigin to window.postMessage.

The main thing I'm worried about is that I'm not sure how credentials in the url (username:password@example.com) are handled.

Going to http://username@frederikcreemers.be, and getting location.originin javascript returns http://frederikcreemers.be, so the origin doesn't include credentials. Would it be sufficient to add a condition like this to the function above:

if "@" in parsed.netloc:
    raise ValueError("The origin must not contain credentials.")

First off, easy task, add a docstring to your function, validate_javascript_origins, and describe this function, and its arguments, preferably in detail.
Finally, if you're worried about input like username@site.end, you should add the check for the @ character. If it isn't valid input, or the input is interpreted in the wrong way, you should most definitely add this check.


How to get the Split value from collection? I have the following key value pair in an array, and am trying to extract and load them into a collection.

The below code is working but it can be optimized using Linq:

string _data = "Website=url:www.site1.com,isdefault:true,url:www.site2.com,isdefault:true";

List<WebSiteAddress> _websiteList = new List<WebSiteAddress>() ;
WebSiteAddress _website = new WebSiteAddress();

string[] _websiteData = _divider[1].Split('=');
string[] _WebsiteKeyValuePair = _websiteData[1].Split(',');

for (int j = 0; j < _WebsiteKeyValuePair.Length; j++)
{
    string key = _WebsiteKeyValuePair[j].Split(':')[0];
    string value = _WebsiteKeyValuePair[j].Split(':')[1];

    if (key.ToLower() == "url")
    {
        _initWebsite.Url = value;
    }
    else if (key.ToLower() == "isdefault")
    {
        _website.IsDefault = Convert.ToBoolean(value);
        _websiteList.Add(_website);
    }
}

Naming
I don't get the _ underscore prefix. This typically identifies a private field (albeit controversial). I'm pretty sure there's a consensus about simple camelCasing for local identifiers.
The name _websiteList is a bad one, for two reasons:

It's of type WebSiteAddress, which has words Web and Site; the name isn't following the established casing convention, should be _webSiteList.
It's tying the variable to its type. Would the name still make sense if the type was WebSiteAddress[]?

A better name would be, simply, webSiteAddresses.
Typing
If you want to use LINQ, you're targeting .NET 3.5+. Therefore, you can use implicit typing to improve the readability (and possibly the maintainability) of your code (although that's possibly personal preference).
Consider:

var webSiteData = _divider[1].Split('=');

The SO answer gives you a nice LINQ-oriented approach.


Advent of Code 2018 day 1 part 2: find the first repeated number after some increases and decreases Here is the problem:

Part Two
You notice that the device repeats the same frequency change list over and over. To calibrate the device, you need to find
the first frequency it reaches twice.
For example, using the same list of changes above, the device would
loop as follows:
Current frequency  0, change of +1; resulting frequency  1.
Current frequency  1, change of -2; resulting frequency -1.
Current frequency -1, change of +3; resulting frequency  2.
Current frequency  2, change of +1; resulting frequency  3.
(At this point, the device continues from the start of the list.)
Current frequency  3, change of +1; resulting frequency  4.
Current frequency  4, change of -2; resulting frequency  2, which has already been seen.
In this example, the first frequency reached twice is 2. Note that your device might need to repeat its list of frequency changes many times before a duplicate frequency is found, and that duplicates might be found while in the middle of processing the list.
Here are other examples:
+1, -1 first reaches 0 twice.
+3, +3, +4, -2, -4 first reaches 10 twice.
-6, +3, +8, +5, -6 first reaches 5 twice.
+7, +7, -2, -7, -4 first reaches 14 twice.
What is the first frequency your device reaches twice?

And here is my solution:
module Main where

import           Data.List.Split

parseString :: Integer -> String -> Integer
parseString acc s =
  let num = read $ tail s :: Integer
      operator = head s
  in if operator == '+'
       then acc + num
       else acc - num

findFreq :: Integer -> [Integer] -> [String] -> (Integer, Integer, [Integer])
findFreq curr acc [] = (0, curr, acc)
findFreq curr acc (x:xs) =
  let next = parseString curr x
  in if next `elem` acc
       then (next, 0, [])
       else let f = acc ++ [next]
            in findFreq next f xs

findRepeatingFrequency :: Integer -> [Integer] -> [String] -> Integer
findRepeatingFrequency init nums xs =
  let (found, acc, lst) = findFreq init nums xs
  in if found > 0
       then found
       else findRepeatingFrequency acc lst xs

main :: IO ()
main = do
  file <- readFile "input.txt"
  let input = filter (not . null) $ splitOn "\n" file
  print (findRepeatingFrequency 0 [] input)

This is seriously slow.
Can anyone give me pointes as to how this can be done better?
I'm also keen to know specifically why this is so slow.

Small things

Data.List.Split

I don't see the need for this module. I see what you're going for with splitOn "\n", however Haskell has a function in Prelude called lines:

words :: String -> [String]

Prelude> lines "test\nphrase, please\nignore"
["test","phrase, please","ignore"]

parseString

I think it's cleaner to instead first parse your input into an Integer format and then use [Integer] everywhere instead of [String]. This simplifies your parsing and also gives you a more general function. Note that my implementation is a partial function.

parseInt :: String -> Integer
parseInt (op:s) =
  case op of
    '+' -> read s
    '-' -> (-1) * read s
    -- There is a hole here: this will error if 'op'
    -- (the first character) isn't '+' or '-'

Return values

The type of findFreq is

findFreq :: Integer -> [Integer] -> [String] -> (Integer, Integer, [Integer])

I see no reason why it can't return a single Integer. Perhaps you returned all values to debug initially, but once that's done you should switch back.

It also seems to me like you were using 0 as an "error value," which is appropriate in other languages but generally frowned upon in Haskell. In this case, you can instead use Maybe to indicate failure and change your type signature to

findFreq :: Integer -> [Integer] -> [String] -> Maybe Integer

Like I mention later, this isn't necessary since we can condense and fix some logic, but I would recommend using Maybe instead in the future.

curr and acc

You do some switching around with your variables curr and acc which confused me a bit. I'd keep acc as the accumulation value for your frequency everywhere and call the list of visited values something like seenFreqs or prevFreqs.

Correctness

Repeating frequencies of 0

Your code currently assumes the repeated frequency is the first nonzero repeated frequency. Thus, it fails for the +1, -1 test case. You could change the code to

findRepeatingFrequency :: Integer -> [Integer] -> [String] -> Integer
findRepeatingFrequency init nums xs =
  let (found, acc, lst) = findFreq init nums xs
  in found

to accommodate that.

This breaks your way of going through the list again if you don't find a repeat the first time, but fortunately there's a less obfuscated way to avoid your checking and continue: you can use cycle, which creates an infinite list consisting of the input list repeated infinitely.

cycle :: [a] -> [a]

Prelude> take 20 $ cycle [1,2,3,4]
[1,2,3,4,1,2,3,4,1,2,3,4,1,2,3,4,1,2,3,4]

You can call findRepeatingFrequency on cycle input to prevent having to do the cycling yourself in the function.

Efficiency

Things go wrong for efficiency in findFreq. There are two problem points that make your code asymptotically inefficient.

First, in the following line

  in if next `elem` acc

elem is an O(n) operation. You're calling it for each of the n elements in your input list, meaning that your function is at least O(n^2) (and it turns out that this is the final complexity).

I checked the number of iterations required for my sample input and it took 142991 iterations to find a repeated frequency. An O(n^2) runtime is going to require about 10 billion iterations for the lookups alone. Ouch.

Second is a more insidious mistake that is easy to overlook. In this line,

   else let f = acc ++ [next]

Appending to a list is an O(n) operation. Lists in Haskell are implemented as linked lists, and appending to the back of one cannot be amortized to O(1) like one might in Python's list or Java's ArrayList. You need to travel all the way to the back to add in a link.

Fixing the second issue actually isn't very hard since you don't care about the ordering of the list. You can switch it to

   else let f = next : acc

to return to O(1) inserts.

Fixing the lookups, however, requires a change of data structure.

Introducing Data.Set

Data.Set provides unordered sets with O(log n) lookup and insert time. Yeah, it's O(log n), but the total runtime for me when I checked the implementation was less than a second.

I'm including a sample implementation of day 1 below that I've tested and confirmed on my inputs if you want to compare. However, you said you wanted pointers, so here's the pointers I'll give.

You can keep your code mostly the same (although I'd recommend making the style changes I suggested)
You will want to use two functions from Data.Set: member and insert.
Your end result will look a lot like a fold but with some differences in end conditions (kind of like findFreq)

Sample implmentation

Finally, here's a sample implementation.

module Main where

import Data.Set (Set)
import qualified Data.Set as S

-- This is a partial function
parseInt :: String -> Integer
parseInt (op:s) =
  case op of
    '+' -> read s
    '-' -> (-1) * read s
    -- There is a hole here, assuming valid input

findRepeatingFrequency :: Integer -> Set Integer -> [Integer] -> Integer
findRepeatingFrequency acc seen (x:xs) =
  if acc `S.member` seen
     then acc
     else findRepeatingFrequency (acc + x) (S.insert acc seen) xs

partOne :: [Integer] -> Integer
partOne = sum

partTwo :: [Integer] -> Integer
partTwo ints = findRepeatingFrequency 0 S.empty $ cycle ints

main :: IO ()
main = do
  file <- readFile "input.txt"
  let input = filter (not . null) $ words file
  let ints = map parseInt input
  print $ partOne ints
  print $ partTwo ints


C++14-ish code for creating q-grams of a string I want to return an iterable collection of all q-grams of a provided string. This means all sub-words of length Q. Let Q be 3 for example:

normalized as an already whitespace-stripped and upcased word or string. PREFIX3 and SUFFIX3 are constant strings ^^ and$$ that are put surrounding the string before splitting. That is a common trick to get more q-words.

For the word INDEX the resulting q-grams are therefore:

^^I  ^IN  IND  NDE  DEX  EX$  X$$

My version uses several C++14 constructs. This is intended, because the code is part of a lesson for teaching about C++14.

Is there anything I could do better, especially with respect to C++14-ish code?

vector<string> qwordify(const string& normalized) const
{
    auto word = PREFIX3 + normalized + SUFFIX3;
    vector<string> result {};
    auto left = word.cbegin();
    auto right = word.cbegin() + Q; // valid, because |"^^"|+|"$$"| => 3.
    for( ; right <= word.end(); ++left, ++right) {
        result.emplace_back(left, right);
    }
    return result;
}

Here are the things I considered especially:

Despite of returning a value, result is not copied, because of RVO.
Lots of auto, which I like.
Use of cbegin() and  cend(), because I declared auto word and not const auto word. I could have done, but for this demonstration I liked to show the c-variants.
Use of emplace_back, which really saves a copy here, correct?

I feel like some places could be improved, maybe. The iterators maybe? Getting rid of the loop in favor of an algorithm? Maybe even not returning a full vector at all, but only an iterable proxy-object, but that would too long a shot for this small example, I think.

Things I like:

(Mostly) a pretty neatly packaged, self-contained component.
Written primarily to emphasize simplicity

and let the compiler deal with making it fast

Things I don't like so well:

Q, PREFIX3, and SUFFIX3 are apparently globals (or maybe class variables?) so I can't see what they really are.
Subtle interaction: PREFIX3 and SUFFIX3 both (seem to) implicitly assume that Q = 3.
qword has a well-known meaning in Intel assembly language. Unless you're sure this will only be used by an audience that immediately knows what you intend, another name might be better.
Written non-generically, for no particularly good reason.
It almost seems like qwordify is really two separate things shoved together: a generic "generate all the results in a sliding window", and a specialized "generate all the qwords from a string".
If you can use the new string_view, it would be a good choice here.

Ignoring point 6 (since it's not really C++14), we could end up with code on this order:

// the generic algorithm:
template <class InIt, class OutIt, class size_type>
void window(InIt b, InIt e, OutIt d, size_type len) {
    for (auto s = std::next(b, len); s != e; ++s, ++b, ++d)
        *d = { b, s };
}

// The piece specifically for q-grams
std::vector<std::string> generate_qgram(std::string const &in, size_t q) {
    std::string input = std::string(q-1, '^') + in + std::string(q, '$');

    std::vector<std::string> words;

    window(input.begin(), input.end(), std::back_inserter(words), q);
    return words;
}

// and a little demo code:
int main() {
    auto ret = generate_qgram("INDEX", 3);

    std::copy(ret.begin(), ret.end(), std::ostream_iterator<std::string>(std::cout, "\n"));
}


Check if a string has HTML in C# I need a function to determine whether or not a string has HTML in it or not so that I can know whether I'm dealing with a plain-text format or HTML format.

It seems simple enough in C#, using HTMLAgilityPack.  Recursively go through the tree of nodes, and if any are an element node (or comment too) then we say "Yes, it's HTML"

public static class HTMLUtility
{
    public static bool ContainsHTMLElements(string text)
    {
        HtmlDocument doc = new HtmlDocument();
        doc.LoadHtml(text);

        bool foundHTML =  NodeContainsHTML(doc.DocumentNode);
        return foundHTML;
    }

    private static bool NodeContainsHTML(HtmlNode node)
    {
        return node.NodeType == HtmlNodeType.Element
            || node.NodeType == HtmlNodeType.Comment
            || node.ChildNodes.Any(n => NodeContainsHTML(n));
    }
}

Am I missing anything?  Thanks!

I've had to do this exact thing before, your way is absolutely fine but I went for the other way round - checking if it was all text:

private static bool HtmlIsJustText(HtmlNode rootNode)
{
    return rootNode.Descendants().All(n => n.NodeType == HtmlNodeType.Text);
}

Then you have your public method as:

public static bool ContainsHTMLElements(string text)
{
    HtmlDocument doc = new HtmlDocument();
    doc.LoadHtml(text);
    return !HtmlIsJustText(doc.DocumentNode);
}

I think that makes the code slightly more concise.

I'd argue that your class should really be called HtmlUtility as per guidelines

  The PascalCasing convention, used for all identifiers except parameter names, capitalizes the first character of each word (including acronyms over two letters in length), as shown in the following examples:

  PropertyDescriptor
  HtmlTag


React Filter library This is my first small react app. I got a little experience in Vue.js before, but I wan't to learn the basics and best practices of react.js too. So, this is a small library app (containing link tipps with title, desc, topics) that shows all library items with the possibility to filter them by clicking on specific keywords. Pretty easy, but I want to know from you if I coded some bad practices or anti patterns or sth. like that.

main.js

import React from 'react'
import ReactDOM from 'react-dom'
import Library from './components/Library.jsx'
const libraryContainer = document.getElementById('library')
ReactDOM.render(<Library />, libraryContainer)

Library.jsx

import React from 'react'
import Filters from './Filters.jsx'
import LibraryItem from './LibraryItem.jsx'
import SmoothScroll from 'smooth-scroll'

const libraryContainer = document.getElementById('library')
const scroll = new SmoothScroll('a[href*="#"]', { offset: 100 })

export default class Library extends React.Component {
  state = {
    lib: [],
    filteredLib: [],
    topics: [],
    filter: '',
  }

  filterLib = () => {
    if (this.state.filter === '') return this.setState({ filteredLib: this.state.lib })

    this.setState(
      {
        filteredLib: this.state.lib.filter(item => {
          if (item.topics) return item.topics.includes(this.state.filter)
        }),
      },
      () => scroll.animateScroll(libraryContainer),
    )
  }

  handleFilters = topic => {
    const topicsClone = JSON.parse(JSON.stringify(this.state.topics))
    topicsClone.forEach(item => {
      if (item.id === topic.id) {
        item.active = !item.active
        item.active ? null : (topic.name = '')
      } else {
        item.active = false
      }
    })
    this.setState({ topics: topicsClone, filter: topic.name }, () => {
      this.filterLib()
    })
  }

  componentDidMount() {
    let topicsArr = []

    fetch('/api')
      .then(res => res.json())
      .then(data => {
        this.setState({ lib: data.lib, filteredLib: data.lib })
        data.topics.map((val, key) => {
          topicsArr.push({ name: val, id: key, active: false })
        })
        this.setState({ topics: topicsArr })
      })
      .catch(err => console.log(err))
  }

  render() {
    if (this.state.filteredLib.length > 0) {
      return (

          <Filters topics={this.state.topics} handler={this.handleFilters} />

              {this.state.filteredLib.map(item => {
                return <LibraryItem key={item._id} {...item} />
              })}

      )
    } else {
      return
Wir konnten leider nichts finden :(

    }
  }
}

Filters.jsx

import React from 'react'

export default function Filters(props) {
  const handleClick = topic => {
    props.handler(topic)
  }

  return (

Filter

        {props.topics.map(topic => {
          return (

 handleClick(topic)}>
              {topic.name}

          )
        })}

  )
}

LibraryItem.jsx

import React from 'react'

export default function LibraryItem(props) {
  return (

{props.type}

{props.title}

{props.teaser}

  )
}

Here is a review of the code. While some my comments / suggestions are related to JavaScript and not to react specifically, these do reflect common practices in React code.

I put a live version with the changes on codesandbox. I created a small API to have some data for the fetch request. I also added a css file to test the active behavior for the selected filter topics.

Library.jsx

componentDidMount():

It is a common practice in react to use the map function to create a new array. Also, you can change the code so that setState is only called once (for conciseness):

componentDidMount() {
  fetch("https://demo5925186.mockable.io")
    .then(res => res.json())
    .then(data => {
      const topics = data.topics.map((name, index) => ({
        name,
        id: index,
        active: false
      }));

      this.setState({
        lib: data.lib,
        filteredLib: data.lib,
        topics
      });
    })
    .catch(err => console.log(err));
}

render()

It can make your JSX easier to read if you extract properties from the state on the first line using object destructuring assignment:

const { filteredLib, topics } = this.state;

It seems like you would always want to show the Filters component (the topic selection), even if filteredLib.length is equal to 0.

render() {
    const { filteredLib, topics } = this.state;
    return (

        <Filters topics={topics} handleClick={this.handleFilters} />
        {filteredLib.length === 0 ? (

Wir konnten leider nichts finden :(

        ) : (

              {filteredLib.map(item => (
                <LibraryItem key={item._id} {...item} />
              ))}

        )}

    );
}

handleFilters

I changed state.filter to state.selectedTopic and the input argument name from topic to newTopic.

Similarly to componentDidMount, the newTopics array can be created with map (you could convert the ternary to an if-else statement). You can use the spread operator on each object to create a new object.

It seems like state.filter would never be an empty string because handleFilters always sets state.filter to whichever topic was selected.  To fix this, you can compared the new selected topic to the existing one. If they are the same, the new selected topic should be set to an empty string.

handleFilters = newTopic => {
    const { topics, selectedTopic } = this.state;

    const newTopics = topics.map(
      topic =>
        topic.id === newTopic.id
          ? { ...topic, active: !topic.active }
          : { ...topic, active: false }
    );

    // If the new selected topic is same as the old one
    // We will set state.selectedTopic to ""
    const newSelectedTopic =
      newTopic.name === selectedTopic ? "" : newTopic.name;

    this.setState(
      {
        topics: newTopics,
        selectedTopic: newSelectedTopic
      },
      () => {
        this.filterLib();
      }
    );
  };

Filters.jsx

It is common to define functional components with const, and the parameters can be destructured in the function definition.

There is no need for the handleClick method. You can change the name of props.handler to props.handleClick, and call it directly.

const Filters = ({ topics, handleClick }) => (

Filter

      {topics.map(topic => (

 handleClick(topic)}
        >
          {topic.name}

      ))}

);

export default Filters;


Which is the most correct pattern for using Stack.Pop? There at least 2 patterns of using Stack.Pop

private Stack<T> availableResources;
...
if (availableResources.Count > 0)
{
  resource = availableResources.Pop();
  ...
}

private Stack<T> availableResources;
...
try
{
  resource = availableResources.Pop();
  ...
}
catch (InvalidOperationException e)
{
  ...
}

I'd like to ask people, which of 2 is preferable? If possible, argument your answer.

Program defensively and use option 1.

Since the Stack class provides the ability to check whether it contains anything before trying to Pop a value from it, you should do the check and avoid using the exception for flow control.

If you want to avoid doing the if check throughout your application, you could create a TryPop extension.

public static class StackExtensions
{
    public static bool TryPop<T>(this Stack<T> stack, out T value)
    {
        if (stack.Count > 0)
        {
            value = stack.Pop();
            return true;
        }

        value = default(T);
        return false;
    }
}

It could be used in the following way:

T resource;
if (availableResources.TryPop(out resource))
{
    // use `resource` for something
}

It's worth noting that this approach is only suitable if the Stack is not accessed concurrently! - If it is, and you are using .NET 4.0 onwards then you should use ConcurrentStack, otherwise create your own concurrent stack which encapsulates a Stack and manages access to it with locks.


Working with Locales in Swing (with change at runtime)? I thought about the best way to implement localisation with runtime  in Swing.

I currently solve the problem like that:

JMenu menuData = new JMenu("Data");
    menuData.setName("mainframe.menu.data"); // property key
    localeChangedListener.add(menuData);

The LocaleChangedListener:

public class SwingLocaleChangedListener implements LocaleChangedListener {

    private ArrayList<AbstractButton>   abstractButtons;

    @Override
    public void localeChanged(ResourceBundle rb) {
        logger.info("Locale changed to '" + rb.getLocale() + "'");
        for (AbstractButton b : abstractButtons) {
            b.setText(rb.getString(b.getName()));
            b.setComponentOrientation(ComponentOrientation.getOrientation(rb.getLocale())); //EDIT: Line added
        }

    }

    public boolean add(AbstractButton b) {
        initAbstractButtons();
        return abstractButtons.add(b);
    }

    private void initAbstractButtons() {
        if (abstractButtons == null) {
            this.abstractButtons = new ArrayList<AbstractButton>();
        }
    }
}

And the registration of the Listener:

public class GuiBundleManager {

    private String                  filePrefix  = "language.lang";
    private ResourceBundle          rb         = null;
    private LocaleChangedListener   listener    = null;

    private static GuiBundleManager instance    = null;

    private GuiBundleManager() {
        setLocale(Locale.getDefault());
    }

    public String getString(String key) {
        return rb.getString(key);
    }

    public String[] getStringArray(String key) {
        return rb.getStringArray(key);
    }

    public Locale getLocale() {
        return rb.getLocale();
    }

    public void setLocale(Locale l) {
        rb = ResourceBundle.getBundle(filePrefix, l);
        if (listener != null) {
            listener.localeChanged(rb);
        }
    }

    public LocaleChangedListener getLocaleChangedListener() {
        return listener;
    }

    public void setLocaleChangedListener(LocaleChangedListener listener) {
        this.listener = listener;
        if (listener != null) {
            listener.localeChanged(rb);
        }
    }

    public static GuiBundleManager get() {
        if (instance == null) {
            instance = new GuiBundleManager();
        }
        return instance;
    }
}

Another way I'm thinking of is using Component.setLocale() combined with an PropertyChangedListener:

public abstract class GUIComponentFactory {

    public JLabel createLocalisedJLabel(final String key) {
        final JLabel label = new JLabel(GuiBundleManager.get().getString(key));
        label.addPropertyChangeListener("locale", new PropertyChangeListener() {

            @Override
            public void propertyChange(PropertyChangeEvent e) {
                label.setText(GuiBundleManager.get().getString(key));
                label.setComponentOrientation(ComponentOrientation.getOrientation(e.getNewValue())); //Edit: Line added
                for(Component c : getComponents()){
                    c.setLocale(e.getNewValue());
                }
            }
        });
                return label;
    }
    .
    .
    .
}

Consider applyComponentOrientation(), which recursively "Sets the ComponentOrientation property of this component and all components contained within it." Examples may be found here.


Parsing fixed-width format with J I have a fixed-width input format. The widths are known to me. Suppose I am told that the first field is 20 characters, the second is 10, the third is 12. The total width of a row should therefore be 42 characters. I have a working solution:

parseFW =: dyad def '(+/ (0,+/\x) ="(0 1) i. (+/x)) <;.1 y'"(_ _1)

My plan here is to use the dyadic form of cut (i.e. ;.) with a boolean array to indicate the fret locations. I'm going to get the rolling sum of the field widths to find where the indices of the frets and then I'm going to reduce that to a boolean array by looking for those indices.

This works:

   input =. noun define
NAME                STATE     TELEPHONE
John Smith          WA        418-Y11-4111
Mary Hartford       CA        319-Z19-4341
Evan Nolan          IL        219-532-c301
)

   $ input
171

  (20 10 12) parseFW >cutLF input
┌────────────────────┬──────────┬────────────┐
│NAME                │STATE     │TELEPHONE   │
├────────────────────┼──────────┼────────────┤
│John Smith          │WA        │418-Y11-4111│
├────────────────────┼──────────┼────────────┤
│Mary Hartford       │CA        │319-Z19-4341│
├────────────────────┼──────────┼────────────┤
│Evan Nolan          │IL        │219-532-c301│
└────────────────────┴──────────┴────────────┘

      $ (20 10 12) parseFW >cutLF input
4 3

My concerns are:

In my head, I feel like this shouldn't take so much code to accomplish and I'm missing something
Is it idiomatic? I'm new to J and would like to know
I. :^ _1 looks like it would do the work I need to convert the rolling sum to a boolean array of fret locations, but it doesn't seem to work (domain error). Am I misusing it?
Efficiency isn't a huge concern of mine, but I would like to know if I'm missing a "special code" opportunity here with this approach

For reference, in case anyone stumbles across this page later, and as discussed in your thread here.

Something like this would do the trick:

parseFW=: (i.@{:@$@] e. 0,+/\@[) <;.1"1 ]

(Though when I test against your input, I'm tripped up by a leading space on each line of your 'input' value, which does not match the specific width values you have used here. Need to also deal with that issue somehow. I would recommend either manually removing those spaces, or dropping the first character on each row before handing off to either implementation of parseFW).

That said, to get an exact workalike to your parseFW (throws an error because the record size is wrong, rather than slicing at the "wrong" column positions on the larger records), you could do this:

parseFW=: (i.@(+/) e. 0,+/\)@[ <;.1"1 ]


VBA save multiple records code, 2 for loops or change to 1 for loop with if statement inside I am becoming more critical of the code I write. This is leading me to questions  like "Could I write my code to be more efficient?" Of course the answer to that is a resounding YES. So, I posit the question to those who know much more than I do. What could I do to make this more efficient?

The purpose of this code is to take data from an Access form and save it into two (or three if a condition is met) tables.

Note: I originally asked a form of this question on Stack Overflow.

Private Function SaveSerials()
Dim rs As DAO.Recordset
Dim rs2 As DAO.Recordset
Dim rs3 As DAO.Recordset
Dim qry As QueryDef
Dim i as Long
If Not BasicInclude.DebugMode Then On Error GoTo Error_Handler Else On Error GoTo 0
    Dim out As String: out = Validate(Me.Name, msclblstyle)
    If out = "" Then
        Set qry = dbLocal.QueryDefs("qryDeletefromLabelTemp")
        qry.Parameters(0) = "%1" & Me.txtOperator.Value & "%"
        qry.Execute
        Set rs = dbLocal.TableDefs("dbo_Part_Serial_Number").OpenRecordset(dbOpenDynaset, dbSeeChanges)
        Set rs2 = dbLocal.TableDefs("dbo_Label_Trace_Temp").OpenRecordset(dbOpenDynaset, dbSeeChanges)
        For i = 1 To Me.lstData.ListCount - 1
            rs.AddNew
            rs2.AddNew
            rs.Fields("Part_No").Value = Me.txtPart_No.Value
            rs2.Fields("PartNumber").Value = Me.txtPart_No.Value
            rs.Fields("Label_Date").Value = Me.txtLabel_Date.Value
            rs.Fields("Serial_No").Value = Me.lstData.Column(0,i)
            rs2.Fields("PartSerialNumber").Value = Me.lstData.Column(0,i)
            rs.Fields("Operator").Value = Me.txtOperator.Value
            rs2.Fields("Operator").Value = "%1" & Me.txtOperator.Value & "%"
            rs.Fields("Work_Order").Value = Me.cboWork_Order.Value
            rs2.Fields("WorkOrder").Value = Me.cboWork_Order.Value
            rs.Fields("Date_Code").Value = Me.txtDate_Code.Value
            rs2.Fields("Date_Code").Value = Me.txtDate_Code.Value
            rs.Fields("Notes").Value = Me.txtNotes.Value
            rs.Fields("Flex_Circuit_Date_Code").Value = Me.txtFC_Date_Code.Value
            rs.Fields("Flex_Circuit_Int_Date_Code").Value = Me.txtFCI_Date_Code.Value
            rs.Fields("Substrate_Date_Code").Value = Me.txtSubstrate_Date_Code.Value
            rs.Fields("SMF_Date_Code").Value = Me.txtSMF_Date_Code.Value
            rs.Fields("r").Value = Me.chkR.Value
            rs.Update
            rs2.Update
        Next
        If Me.txtPart_No Like "PS*" Then
            Set qry = dbLocal.QueryDefs("qryClear_tbl_BarcodeList")
            qry.Parameters(0) = Environ(GetEnvironString(esComputerName))
            qry.Execute
            Set rs3 = dbLocal.TableDefs("tbl_BarcodeList").OpenRecordset(dbOpenDynaset, dbSeeChanges)
            For i = 1 To Me.lstData.ListCount - 1
                rs3.AddNew
                rs3.Fields("Part_No").Value = Me.txtPart_No.Value
                rs3.Fields("Host").Value = Environ(GetEnvironString(esComputerName))
                rs3.Fields("Serial_No").Value = Me.lstData.Column(0,i)
                rs3.Update
            Next
            DoCmd.OpenReport "rpt_BarcodeList", acViewPreview
        End If
    SaveSerials = 0
    Else
        MsgBox "Please fill out the following fields: " & vbCrLf & out, vbCritical, "Error"
        SaveSerials = 1
    End If
Error_Exit:
set rs = Nothing
set rs2 = Nothing
set rs3 = Nothing
Exit Function
Error_Handler:
    MsgBox "The following error has occured" & vbCrLf & vbCrLf & _
           "Error Number: " & Err.Number & vbCrLf & _
           "Error Source: SaveSerials" & vbCrLf & _
           "Error Description: " & Err.Description _
           , vbOKOnly + vbCritical, "An Error has Occured!"
           SaveSerials = -1
Resume Error_Exit
End Function

On Error GoTo 0 is used in conjunction with On Error Resume Next to reset any errors that may occur between the two statements.

For this reason,  On Error GoTo 0 in the ↓ line below ↓ should be changed to On Error Resume Next.

If Not BasicInclude.DebugMode Then On Error GoTo Error_Handler Else On Error GoTo 0

I would rewrite the Validation statement removing the else clause.  This will make the code easier to read.

If out <> "" Then
    MsgBox "Please fill out the following fields: " & vbCrLf & out, vbCritical, "Error"
    SaveSerials = 1
    Goto Error_Exit:
End If

Setting objects to nothing is rarely necessary with today's garbage collectors.

set rs = Nothing
set rs2 = Nothing
set rs3 = Nothing

Recordset should be closed.

rs.Close
rs2.Close
rs3.Close

Grouping your recordset field will improve the readability of the code, make it easier to spot missing fields, and just makes sense.

For i = 1 To Me.lstData.ListCount - 1
    'Add record to dbo_Part_Serial_Number
    rs.AddNew
    rs.Fields("Part_No").Value = Me.txtPart_No.Value
    rs.Fields("Label_Date").Value = Me.txtDate_Created.Value
    rs.Fields("Serial_No").Value = Me.lstData.Column(i)
    rs.Fields("Operator").Value = Me.txtOperator.Value
    rs.Fields("Work_Order").Value = Me.cboWork_Order.Value
    rs.Fields("Date_Code").Value = Me.txtDate_Code.Value
    rs.Fields("Notes").Value = Me.txtNotes.Value
    rs.Fields("Flex_Circuit_Date_Code").Value = Me.txtFC_Date_Code.Value
    rs.Fields("Flex_Circuit_Int_Date_Code").Value = Me.txtFCI_Date_Code.Value
    rs.Fields("Substrate_Date_Code").Value = Me.txtSubstrate_Date_Code.Value
    rs.Fields("SMF_Date_Code").Value = Me.txtSMF_Date_Code.Value
    rs.Fields("r").Value Me.chkR.Value
    rs.Update

    'Add record to dbo_Label_Trace_Temp
    rs2.AddNew
    rs2.Fields("PartNumber").Value = Me.txtPart_No.Value
    rs2.Fields("PartSerialNumber").Value = Me.lstData.Column(i)
    rs2.Fields("Operator").Value = "%1" & Me.txtOperator.Value & "%"
    rs2.Fields("WorkOrder").Value = Me.cboWork_Order.Value
    rs2.Fields("Date_Code").Value = Me.txtDate_Code.Value
    rs2.Update
Next

You may consider extracting code  related to the three recordset into their own subroutines but it is not necessary.

It is hard to discern the meaning of the return value of SaveSerials (e.g. -1, 0, 1) without studying the code. Consider using an Enum.


Count Words in Quotes Fetched from Website I have solved the following exercise and would like to get some feedback on my implementation.

  Create a function that uses futures to parallelize the task of downloading random quotes from https://www.braveclojure.com/random-quote using (slurp "http://www.braveclojure.com/random-quote"). The futures should update an atom that refers to a total word count for all quotes. The function will take the number of quotes to download as an argument and return the atom’s final value. Keep in mind that you’ll need to ensure that all futures have finished before returning the atom’s final value.

(defn quote-word-count
  [quote-count]
  (->> quote-count
      (range)
      (map (fn [_] (fetch-quote)))
      (pmap (comp count-words deref))
      (apply merge-with +)
      (atom)))

(defn fetch-quote
  []
  (future (slurp "https://www.braveclojure.com/random-quote")))

(defn update-word
  [word-counts word]
  (update word-counts word (comp inc zero)))

(defn zero
  [number]
  (if (= nil number) 0 number))

(defn count-words
  [quote]
  (let [cleaned-quote (clean-input quote)]
    (reduce update-word {} (clojure.string/split cleaned-quote #" "))))

(defn clean-input
  [input]
  ((comp remove-commas remove-author clojure.string/lower-case) input))

(defn remove-author
  [quote]
  (clojure.string/replace quote #".\n-- .*\n" ""))

(defn remove-commas
  [quote]
  (clojure.string/replace quote #"," ""))

Things I am not sure about:

the range then map to create the calls
the top-down order of functions
the atom. the exercise says to return an atom but I don't understand why I would need that

Any feedback is appreciated.

For quote-word-count, there's a few things to note.

I think this is a good use of ->>. Personally, I would have started the threading with (range quote-count) instead of splitting that though. From my experience, needlessly elongating the thread call just hurts readability. I also recommend maintaining the same type of object being threaded all the way down. I find it makes the thread much easier to understand. If you're threading collections like you are here, I'd prefer that every object being threaded is a collection (whereas quote-count is a number).

It isn't a good idea to use map to run side effects (like starting a future task). map doesn't run its effect immediately, so the futures aren't started when (map (fn [_] (fetch-quote))) runs, they're started when the pmap begins requesting elements. I'd switch to mapv which is strict, so the future's are started right away.

I don't think that mapping a range is the right approach here though; which is hinted at by your use of a wrapper (fn [_]) function. repeatedly would be a better candidate for this situation. It's lazy though, so I'd probably add a call to vec after it to realize it right away.

Your zero is actually unnecessary. fnil can be used here instead:

(update word-counts word (fnil inc 0)))

I wouldn't have parameters called quote ideally. quote is a rather important built-in, and it's a good idea to avoid shadowing built-ins.

I'd probably use ->> in count-words:

(defn count-words
  [quote]
  (let [cleaned-quote (clean-input quote)]
    (->> (clojure.string/split cleaned-quote #" ")
         (reduce update-word {}))))

I personally find that to be much more readable than the nested version.

I'll note too that Clojure has the built-in frequencies that can do the work here:

(defn count-words
  [quote]
  (let [cleaned-quote (clean-input quote)]
    (->> (clojure.string/split cleaned-quote #" ")
         (frequencies))))

In clean-input, I wouldn't use comp. I think it would be much neater using ->:

(defn clean-input
  [input]
  (->> input
       (clojure.string/lower-case)
       (remove-author)
       (remove-commas)))

I really only like comp when it's being passed to another function. Writing out ((comp g f) x) just seems like an overly complicated way of applying x to f then g.

Your functions are in reverse order for some reason. I'm not sure where you're running this that's allowing for functions to be used before they're declared.

Personally, I like having the parameter vector on the first line unless I have a doc string. I find the functions look noisy otherwise.

You're writing clojure.string/ a fair amount. You may want to require that module to avoid verbose, qualified function calls. While you're at it, you might as well add a ns macro call, since each file should really have one anyways:

(ns your.file.path.here
  (:require [clojure.string :as s]))

Now, clojure.string/split can be written as s/split.

And for 3.:

  The function will take the number of quotes to download as an argument and return the atom’s final value.

It wants you to return the value of the atom, not the atom itself. They want you to update an atom inside the futures; although I don't think that's necessary, or even preferable.

Here's the final code I ended up with:

(ns your.file.path.here
  (:require [clojure.string :as s]))

(defn remove-author [quote]
  (s/replace quote #".\n-- .*\n" ""))

(defn remove-commas [quote]
  (s/replace quote #"," ""))

(defn clean-input [input]
  (->> input
       (s/lower-case)
       (remove-author)
       (remove-commas)))

(defn count-words [quote]
  (let [cleaned-quote (clean-input quote)]
    (->> (s/split cleaned-quote #" ")
         (frequencies))))

(defn fetch-quote []
  (future (slurp "https://www.braveclojure.com/random-quote")))

(defn quote-word-count [quote-count]
  (->> (repeatedly quote-count fetch-quote)
       (vec)  ; To ensure futures start before pmap begins processing chunks
       (pmap (comp count-words deref))
       (apply merge-with +)))


Sieve of Eratosthenes optimization I have applied some optimizations like storing only odd values
and starting to mark off from square of the number.

Can it be optimized further?

bool * isPrime = new bool [n/2];

for (int i=0; i < n/2; ++i)
    isPrime [i] = true;

cout<< 2 << "\n";

for (int i = 3; i < n; i += 2) {

    if (isPrime [i / 2]) {

        cout<< i << "\n";

        for (int j = i * i; j < n; j += 2 * i)
            isPrime [j / 2] = false;
    }
}

You can go further by not processing multiples of 3 together with multiples of 2 (even numbers, which you already not process). For stepping on numbers not multiple of 2 and 3, you should take steps of size 2 and 4 alternatively. 5 (+2) 7 (+4) 11 (+2) 13 (+4) 17 ...

This way you will also save space (from n/2 to n/3). You can change your loops like below:

for (int i = 5, t = 2; i < n; i += t, t = 6 - t) {

    if (isPrime [i / 3]) {

        cout<< i << "\n";

        for (int j = i * i, v = t; j < n; j += v * i, v = 6 - v)
            isPrime [j / 3] = false;
    }
}

a little hint is that if don't want to print the prime numbers and want just to generate isPrime array, you can change your outer loop condition to i*i<n, because composite numbers are turned to false by their factor less than their square root.

Another good optimization is that you can use a single bit for storing isPrime flag of each number. This way you can save space by a factor of 1/8, and what is great about this method is that you are increasing your locality of reference and your code will use cache better (when I first wrote sieve by this optimization, amazingly I achieved about 20% speed improvement).


IEnumerable extension method that ingests SqlCommand and returns query results The following ingests a preexisting SqlCommand and returns the values.

A couple caveats right now.

1 ) Type properties must be named identical to the SQL column

2) properties must be given a custom [SQLColumn] attribute.

The attribute contains a string which I plan to use later for variation in property names while keeping column designation.

    public static IEnumerable<T> FromCommand<T>(this IEnumerable<T> b, SqlCommand command)
    {

        using (command.Connection)
        {

            command.Connection.Open();

            using (var reader = command.ExecuteReader())
            {

                var readerColumns = new HashSet<string>(Enumerable.Range(0, reader.FieldCount).Select(reader.GetName));
                var properties = typeof(T).GetProperties().Where(o => o.IsDefined(typeof(SQLColumn)) && readerColumns.Contains(o.Name)).ToList();

                while (reader.Read())
                {

                    object OUTPUT = Activator.CreateInstance(typeof(T));

                    foreach (var item in properties)
                    {
                        item.SetValue(OUTPUT, reader[item.Name] is DBNull ? null : reader[item.Name]);
                    }

                    yield return (T)Convert.ChangeType(OUTPUT, typeof(T));
                }
            }
        }
    }

As requested, here is an example of the class utilizing the extension method.

public class MyType
{

    #region Properties Block

    [SQLColumn]
    public Guid GuidColumn { get; set; }

    [SQLColumn]
    public int IntColumn { get; set; }

    [SQLColumn]
    public bool BitColumn { get; set; }

    #endregion Properties Block

    #region Select Block

    [Description("Selects and returns all rows.")]
    public static IEnumerable<MyType> SelectAll()
    {

        SqlCommand command = new SqlCommand(QueryRepository.SELECT_MyType_ALL, Conn.ConnectionString) { CommandTimeout = 0 };

        return new List<MyType>().FromCommand(command);

    }

    [Description("Selects and returns rows filtered by GuidColumn.")]
    public static IEnumerable<MyType> SelectFiltered(Guid GuidColumn)
    {

        SqlCommand command = new SqlCommand(QueryRepository.SELECT_MyType_GuidColumn, Conn.ConnectionString) { CommandTimeout = 0 };

        command.Parameters.Add("@GuidColumn", SqlDbType.UniqueIdentifier).Value = GuidColumn;

        return new List<MyType>().FromCommand(command);

    }

    #endregion Select Block

}

You are never using b in the method. Why make it an extension method if you are going to completely ignore the first argument? In order to use this code, you need to create a new instance that is immediately thrown away. That is unneeded waste.

Instead, you can just specify the generic type in the function call.

public static IEnumerable<T> FromCommand<T>(SqlCommand command)
{
    // ...
}

[Description("Selects and returns all rows.")]
public static IEnumerable<MyType> SelectAll()
{
    SqlCommand command = new SqlCommand(QueryRepository.SELECT_MyType_ALL, Conn.ConnectionString) { CommandTimeout = 0 };

    return FromCommand<MyType>(command);
}

The expression that assigns to properties is over 100 characters. That doesn't count the indentation or the variable declaration. That is going to be hard to follow. I suggest that you extract the lambda being passed to Where() into a static function with a descriptive name. That way I don't need to think about what is being filtered out.

You are adding extra blank lines after opening curly brackets and before some closing curly brackets. This is just inflating the file size and not helping readability.


Finding expiring/expired documents This code is to find expiring/expired documents. It was included in one of the models, say Document.

Is this good approach of using constant variables which holds the some data and used within this mixin only? Is there any better approach I could use instead the constants?

require 'active_support/concern'

module Expirable
  extend ActiveSupport::Concern

  BEFORE_EXPIRY_DURATION =
    eval(APP_CONFIG.timings.notification_of_expired.before_expiry)

  EXPIRING_DURATION =
    (BEFORE_EXPIRY_DURATION.ago..Date.today)

  included do
    scope :expirables,
      -> { where("expiry_date IS NOT NULL") }
    scope :ready_to_expire,
      -> { expirables.where(expiry_date: EXPIRING_DURATION) }
    scope :expired,
      -> { expirables.where("DATE(expiry_date) < ?", Date.today) }
  end

  module ClassMethods
  end

  def ready_to_expire?
    expiry_date.present? && !expired? && expiry_date.to_date.in?(EXPIRING_DURATION)
  end

  def expired?
    expiry_date.present? && (expiry_date.to_date < Date.today)
  end

end

Looks good except for your use of constants.

Constant variables is probably the biggest oxymoron I have ever heard. The whole point of a constant is that it does not change - the very name variable implies that it is prone to change.

A proper use of a constants is to hold immutable data where the value is clearly understandable by just reading the code:

# A very contrived example
class Week
  DAYS = [:monday, :tuesday, :wednesday, :thursday,
          :friday, :saturday, :sunday]
end

The key point here is that if the value is not known at before the class is evaluated it should be treated as an extreme code smell.

Here is  better but long example of how constants should be properly used: ActiveSupport::Timezone on Github.


Updating a .csv file I have a CSV file, call it csv_file. It has the following content:

Username, Password
name1, pass1
name2, pass2
...

I also have a dictionary, call it mydict. It has the following content:

mydict = {
    "name2" : "pass2",
    "name3" : "pass3"
     ...
}

I want to update my CSV file to now include name3, pass3, since those aren't in the CSV file but they are in the dictionary.

What's the most efficient, pythonic way of doing this?

Right now, here's what I have, but I don't think it's very efficient:

with open(csv_file, 'rb') as infile, open(new_csv_file, 'wb') as outfile:

     r = csv.DictReader(infile)
     w = csv.DictWriter(outfile, r.fieldnames)
     w.writeheader()

     temp_dict = {row['Username'] : row['Password'] for row in r}

     for k in mydict:
          if k.key not in temp_dict:
               temp_dict[k] = mydict[k]

     for value in temp_dict:
          w.writerow({'Username' : value, 'Password' : temp_dict[value]})

I'm sure there's something I can do to make this better. Any suggestions?

There's no better way than creating a temporary dictionary to quickly update the contents of the entire file the way you want. However you can speed things by not using csv.DictReader and csv.DictWriter because they require building a separate temporary dictionary for each row processed.

Here's a more efficient version based on that supposition that also effectively updates the file "in-place". Note that the order of the rows in the file will be changed as a result of storing them temporarily in the dictionary. If that's important, use a collections.OrderedDict instead.

Also noteworthy is that it would be even more efficient to use @user3757614's suggestion, and instead do a less complicated mydict.update(temp_dict) (and then write mydict.items() out as the updated version of the file). If you want to preserve mydict, just make a copy of it first and then update that with temp_dict's contents.

import csv
import os

mydict = {
    "name2" : "pass2",
    "name3" : "pass3"
#     ...
}

csv_file = 'users.csv'  # file to be updated
tempfilename = os.path.splitext(csv_file)[0] + '.bak'
try:
    os.remove(tempfilename)  # delete any existing temp file
except OSError:
    pass
os.rename(csv_file, tempfilename)

# create a temporary dictionary from the input file
with open(tempfilename, mode='rb') as infile:
    reader = csv.reader(infile, skipinitialspace=True)
    header = next(reader)  # skip and save header
    temp_dict = {row[0]: row[1] for row in reader}

# only add items from my_dict that weren't already present
temp_dict.update({key: value for (key, value) in mydict.items()
                      if key not in temp_dict})

# create updated version of file
with open(csv_file, mode='wb') as outfile:
    writer = csv.writer(outfile)
    writer.writerow(header)
    writer.writerows(temp_dict.items())

os.remove(tempfilename)  # delete backed-up original


Searching the number with the biggest amount of odd divisors in an interval I wrote this program for school (first year in college) and it works, but it's too slow. I wondered if anyone could help me optimize this a bit, because I've been trying and trying, but it just won't get better.

If I choose a lower limit of 5000 and an upper limit of 10000 it my program 10 times longer than my professors code (I did not see his code).

This is for school, but I won't get a grade for this; I'm not even obligated to make it. I just really want to improve.

def askLowerLimit():
    lowerLimit = int(input())
    while lowerLimit <= 0:
        print()
        lowerLimit = int(input())
    return lowerLimit

def askUpperLimit(lowerLimit):
    upperLimit = int(input())
    while upperLimit < lowerLimit:
        print()
        upperLimit = int(input())
    return upperLimit

def amountOddDivisorsOf(number):
    amountOddDivisors = 1
    divisor = 2
    if number % 2 != 0:
        divisor = 3
    for i in range(3, number//divisor +1, 2):
        if number % i == 0:
            amountOddDivisors += 1
    return amountOddDivisors

def numberWithMostOddDivisors(lowerLimit, upperLimit):
    numberWithMostDivisors = lowerLimit
    amountOfNumberWithMostDivisors = amountOddDivisorsOf(numberWithMostDivisors)
    for x in range(lowerLimit + 1, upperLimit + 1):
        amountOfCurrentNumber = amountOddDivisorsOf(x)
        if amountOfCurrentNumber > amountOfNumberWithMostDivisors:
            numberWithMostDivisors = x
            amountOfNumberWithMostDivisors = amountOfCurrentNumber
    return numberWithMostDivisors

def biggestAmmountOddDivisorsInInterval():
    lowerLimit = askLowerLimit()
    upperLimit = askUpperLimit(lowerLimit)
    numberWithMostD = numberWithMostOddDivisors(lowerLimit, upperLimit)
    amountOfOddDivisorsOfNumber = amountOddDivisorsOf(numberWithMostD)
    print("Number with most odd divisors in interval [{0} {1}] is {2}:".format(lowerLimit, upperLimit, numberWithMostD))
    print("{0} has {1} odd divisors!".format(numberWithMostD, amountOfOddDivisorsOfNumber))

biggestAmmountOddDivisorsInInterval()

The problem should be somewhere in amountOddDivisorsOf(number) or numberWithMostOddDivisors(lowerLimit, upperLimit).

Bug

Your code isn't always correct:

>>> amountOddDivisorsOf(15)
3

Should be 4.

Coding style

The Python style guide recommends snake_case over camelCase. Also, many of your names are just WAY too long. You're taking up too much space without providing new information.

There's also a lot of repetitiveness. askLowerLimit() and askUpperLimit() shouldn't be separate functions. You should have one function prompting for a value, and then separately assert 0 < lower < upper.

Finding the most divisors

numberWithMostOddDivisors is extremely verbose. We can do much better. The max function takes a keyword argument named key that gives you a function. So as a first go, it's as easy as:

def most_odd_divisors(lo, hi):
    return max(range(lo, hi+1), key=num_odd_divisors)

But this is unsatisfactory, as then you have to recalculate how many odd divisors it has. So instead, we can use:

def most_odd_divisors(lo, hi):
    with_divs = ((i, num_odd_divisors(i)) for i in range(lo, hi+1))
    return max(with_divs, key=operator.itemgetter(1))

And that'll give you both the most odd divisors and the value:

lo = ask_limit()
hi = ask_limit()
assert hi > lo > 0
num, divisors = most_odd_divisors(lo, hi)

Better way of counting

Rather than manually counting each divisor, remember the number of factors formula. If \$n = \prod_p p^i\$, then the sum of the factors is \$\prod_p (i+1)\$, so the number of odd factors if \$\prod_{p \neq 2} (i+1)\$.

So let's find the prime factorization first. We'll just yield pairs of prime numbers and their exponents:

def prime_factors(n):
    for i in itertools.count(2):
        if i*i > n:
            break

        exp = 0
        while n%i == 0:
            n //= i
            exp += 1

        if exp > 0:
            yield i, exp

    if n > 1:
        yield n, 1

The advantage here is that we're not repeating a lot of work. Once you know that a number is divisible by 3 and 5, you know it'll be divisible by 15 too. This way, doing the bare minimum amount of divisibility checking - which isn't cheap!

We can use that to:

def num_odd_divisors(n):
    prod = 1
    for p, e in prime_factors(n):
        if p != 2:
            prod *= e+1
    return prod

Counting from 5,000 to 10,000, this is roughly 11x faster than your implementation. So I suspect your professor did something along these lines.


Using XML from web service with switch Very simple requirements here:

connect to web service
check if account number exists
if it exists update it
if it does not exist create the account

I have accomplished this with the code below I am wondering if this is efficient and secondly if there is a better way of doing this.

 protected void Button1_Click(object sender, EventArgs e)
 {
    //authentication header
    com.x.w_1.www.WebService ws = new com.x.w_1.www.WebService();
    com.dat_e_baseonline.w_1.www.AuthenticationHeader header = new com.x.w_1.www.AuthenticationHeader();
    header.UserName = "";
    header.Password = "";
    ws.AuthenticationHeaderValue = header;

    string email = TextBox1.Text.ToString();

    //Are they a subscriber?
    XmlNode result = ws.getAccountIDbyEmail(email, "247");

    Label1.Text = result.OuterXml;

    foreach (XmlNode x in result.FirstChild)
    {
        string m = x.InnerText;
        string account = result.LastChild.InnerText;

        switch (m)
        {

            case "100":
                //update the account
                ws.Subscribe("x", "3", account);
                Label1.Text = "account exists";
                break;
            case "200":
                //create an account
                ws.createAccount("x", "5", email, "x", "x", "x", "01-01-1998", "1", "yes");
                Response.Redirect("/thankyou.aspx");
                break;

        }
    }

For reference here is the XML I get back when I send an email to be updated:

<getAccountID:name xmlns:getAccountID="http://x">
<getAccountIDResultCode xmlns="">100</getAccountIDbyEmailResultCode>
<getAccountIDResultDescr xmlns="">Account found.</getAccountIDResultDescr>
<accountID xmlns="">12345</accountID>
</getAccountIDbyEmail:name>

I would move the operations on the web service out to their own class:

public class AccountRepository
{
    private readonly void _webService;

    public AccountRepository()
    {
        //authentication header

        var header = new AuthenticationHeader
                              {
                                  header.UserName = "";
                                  header.Password = "";
                              }

        _webService = new WebService
                              {
                                   ws.AuthenticationHeaderValue = header;
                              }
    }

    public IEnumerable<AccountResult> SearchByEmail(string email)
    {
         return ConvertXmlToResultsList(_webService.getAccountIDbyEmail(email, "247"));
    }

    private IEnumerable<AccountResult> ConvertXmlToResultsList(XmlDocument xmlDocument)
    {
         // logic to convert xml to account results
    }

    public void UpdateAccount(/* parameters */)
    {
         _webService.Subscribe(/* parameters */);
    }

    public void CreateAccount(/* parameters */)
    {
         _webService.createAccount(/* parameters */
    }
}

Doing this gets rid of most of the housekeeping code out of the event handler.

I would also consider creating two classes: Account, and SearchResult.

Account should be obvious, it contains all the functionality and data that an account requires.  This would be instantiated in the Repository on the search, and also in the event handler when you need to create a new Account.

The SearchResult class I would use to return the results of the search.  It would contain the Account from the search, and the status of the search.  You could also put stuff like error checking, or anything else you see as being relevant.  You will also see a ResultCode variable type.  ResultCode is an enum that contains all of the possible codes returned from the search.

public enum ResultCode
{
     Error,
     UpdateAccount,
     CreateAccount,
}

public class SearchResult
{
    private readonly Account _account;
    private readonly ResultCode _code;

    public SearchResult(Account account, int resultCode)
    {
         _account = account;
         _code = DetermineCode(resultCode);
    }

    private static ResultCode DetermineCode(int resultCode)
    {
         switch(resultCode)
         {
              case 100:
                  return ResultCode.UpdateAccount;
              case 200:
                  return ResultCode.CreateAccount;
         }

         return ResultCode.Error;
    }
}

The final result of all this is an event handler that looks like:

protected void Button1_Click(object sender, EventArgs e)
{

     var results = _accountRepository.SearchByEmail(TextBox1.Text);

     foreach (var result in results)
     {
         switch (result.ResultCode)
         {
             case ResultCode.UpdateAccount:
                 result.Account.Update(/* parameters */);
                 _accountRepository.Update(result.Account);

                 Label1.Text = "account exists";
                 break;
             case ResultCode.CreateAccount:
                 var account = new Account("x", "5", email, "x", "x", "x", "01-01-1998", "1", "yes");

                 _accountRepository.Create(account);

                 Response.Redirect("/thankyou.aspx");
                 break;

         }
     }
 }

There are a few more points I'd like to make.

Calling .ToString() on a .Text property of a TextBox is counterproductive.  TextBox.Text returns a string, no need to make it a string.

In your code, I would move ResultStatus magic numbers into constants.  This will better describe what they represent.  Of course, if you take my code suggestions, no need to worry about this.

I don't know who wrote the web service, but the method names follow java naming conventions, not C#.  This is just a btw, ignore if the web service is written in Java.

Finally, there is quite a bit more clean-up to do on the code, even what I have posted, but this is a good start.  Hopefully you can pick up some tips from it.

Cheers.


Use a Java Array like a List I'm saving string values in an array one at a time so I cannot initialize the array with the values all at once. After I'm done with the array I need to pass it to another class that is expected a String array.

Normally I would use a List but the end result is expecting an Array so I stuck with the Array. I'm having second thoughts though because maybe it would have better performance if I used a List then when I'm finished filling up the List I could use List.toArray and get the Array that way? Here is majority of the Array code I wrote that takes one argument at a time and adds it to the Array,

public String[] addArgument(String[] arguments, String arg) {

    String[] temp = new String[arguments.length + 1];

    for(int i=0; i<arguments.length; i++)
        temp[i] = arguments[i];

    temp[arguments.length] = arg;

    arguments = temp;

    temp = null;

    return arguments;
}

You should use the List#toArray method, you want to use the generic method, so not the one returning Object[], for this to work you need to supply an array of the correct size, such that it can store the data there.

List<String> list = ...;
String[] = list.toArray(new String[list.size()]);

Note that the array need not be the same size, there are two cases to consider:

If array.length < list.size(), then it will create a new array and return that one.
Else, it will put the data in the input array.


update object in list Q: I have a Bank class containing multiple loan accounts (LoanAccount class). I've create a LoanAccountService that have the CRUD functionalities. My concerns are about how I implemented the update functionality.

Bank

public class Bank {
    private List<LoanAccount> loanAccounts;
}

Loan account

public class LoanAccount {
    private String id;
    private Integer numberOfInstallments;
    private LoanAccountType type;
    private Date creationDate;
    private BigDecimal loanAmount;
}

Service

public class LoanAccountService{

    private Bank bank;

    public LoanAccountService(Bank bank) {
        this.bank = bank;
    }

    public LoanAccount update(LoanAccount loanAccount) {
        Optional<LoanAccount> account = bank.getLoanAccounts()
                .stream()
                .filter(la -> la.getId().equals(loanAccount.getId()))
                .findAny();
        if (account.isPresent()) {
            account.get().setCreationDate(loanAccount.getCreationDate());
            account.get().setLoanAmount(loanAccount.getLoanAmount());

        account.get().setNumberOfInstallments(loanAccount.getNumberOfInstallments());
            account.get().setType(loanAccount.getType());
        } else {
            throw new IllegalArgumentException("The object does not exist.");
        }
        return loanAccount;
    }
}

When the method update is called with a LoanAccount containing an id that already exists in loanAccounts list, I want to update the existing object with the object loanAccount given as parameter.

Above is my implementation, but I feel like there should be better ways to do it.

I would suggest following:

Place operations to the objects that contain data for this operation. It will improve encapsulation and allow more flexible code reuse.
Also it will be easy to test such implementation because of small methods.
Use more convenient structure. In this case Map<String, LoanAccount> (map id to loanAccount) instead of List<LoanAccount>

As you can see in this case service contains only required logic (how to react on loan absence). It is easy to read, test and understand.

LoanAccountService.java

public class LoanAccountService{

    private Bank bank;

    public LoanAccountService(Bank bank) {
        this.bank = bank;
    }

    public LoanAccount update(LoanAccount loanAccount) {
        if (!bank.updateLoanAccount(loanAccount)) {
            throw new IllegalArgumentException("The object does not exist.");
        }
        return loanAccount;
    }
}

Bank.java

@Getter
@Setter
public class Bank {
    private Map<String, LoanAccount> loanAccounts;

    public boolean updateLoanAccount(LoanAccount loanAccount) {
        LoanAccount loan = loanAccounts.get(loanAccount.getId());
        if (loan != null) {
            loan.update(loanAccount);
            return true;
        }
        return false;
    }
}

LoanAccount.java

@Getter
@Setter
public class LoanAccount {
    private String id;
    private Integer numberOfInstallments;
    private LoanAccountType type;
    private Date creationDate;
    private BigDecimal loanAmount;

    public void update(LoanAccount loanAccount) {
        this.setCreationDate(loanAccount.getCreationDate());
        this.setLoanAmount(loanAccount.getLoanAmount());
        this.setNumberOfInstallments(loanAccount.getNumberOfInstallments());
        this.setType(loanAccount.getType());
    }
}

If you can't use this approach, you could made a small refactoring of the service (less code and "if conditions" are good things):

public LoanAccount update(LoanAccount loanAccount) {
    LoanAccount account = bank.getLoanAccounts()
            .stream()
            .filter(la -> la.getId().equals(loanAccount.getId()))
            .findAny()
            .orElseThrow(() ->  new IllegalArgumentException("The object does not exist."));
    account.setCreationDate(loanAccount.getCreationDate());
    account.setLoanAmount(loanAccount.getLoanAmount());
    account.setNumberOfInstallments(loanAccount.getNumberOfInstallments());
    account.setType(loanAccount.getType());
    return account;
}

Also I think you should return updated loan account.


Using graphics to draw on form, in C# I have a form, which I want to draw on-top some shapes. To be exact, rectangles and lines.

This is the "Redraw" method. I call this method every 100ms from a different thread.

public void Redraw()
    {
        g.FillRectangle(b, new Rectangle(13, 13, (int)drawMapX + 2, (int)drawMapY + 2)); /* Draw background */

        try
        {
            foreach (Location drawPoint in points)
            {
                g.DrawLine(new Pen(y), new Point((int)Math.Round(masterpos.X), (int)Math.Round(masterpos.Y)), new Point((int)Math.Round(drawPoint.X), (int)Math.Round(drawPoint.Y)));
                g.FillRectangle(r, drawPoint.X, drawPoint.Y, 2, 2);

            }
            g.FillRectangle(gr, masterpos.X, masterpos.Y, 2, 2);

            g.DrawString(Math.Round(pos.X).ToString() + " " + Math.Round(pos.Y), drawFont, gr, masterpos.X + 2, masterpos.Y - 20, centered);
        }
        catch { }
    }

Where "g" is

  Graphics g = Application.OpenForms["Client"].CreateGraphics();

Nothing too interesting about it, draws some shapes in some given places, and some strings

My question: Is my thread safe and memory-leak proof?

void CreateDrawingThread() {new Thread(() => {
            Thread.CurrentThread.IsBackground = true;
            do
            {
                try
                {

                    Thread.Sleep(100);
                    Redraw();
                }
                catch(Exception ex) { Console.WriteLine(ex.ToString()); return; }
            } while (true);

        }).Start();}

Because as you can see, I don't do anything like g.Flush every redraw, I was wondering if it stores old "draws" in memory, or if I'm doing something in a cpu costraining way.

new Pen(y)

This should be disposed.

Graphics g = Application.OpenForms["Client"].CreateGraphics();

I wonder why are you getting the Graphics object like this? To redraw something you should actually handle the Paint event or override the OnPaint method where you get a ready to use Graphics object from the system. And of course like @Cody Gray said, you need to dispose a graphics object acquired this way.

Those try/catch blocks are suspicious too. There is nothing that could throw an exception. If this runs every 100ms there is no room for exception handling. This would be very bad for the performance.