SVG class with boilerplate prologue and epilogue from files I noticed this old class of mine:

class SvgDrawing:

    def __init__(self, print_layout):
        self.print_layout = print_layout

        with open(os.path.join(os.path.dirname(os.path.abspath(__file__)), 'svg_start.txt'), mode='r') as f:
            self.svg_start = f.read()

        with open(os.path.join(os.path.dirname(os.path.abspath(__file__)), 'svg_end.txt'), mode='r') as f:
            self.svg_end = f.read()

And I thought I would refactor it like this to avoid re-reading the same file every time an instance is created.

class SvgDrawing:

    with open(os.path.join(os.path.dirname(os.path.abspath(__file__)), 'svg_start.txt'), mode='r') as f:
        svg_start = f.read()

    with open(os.path.join(os.path.dirname(os.path.abspath(__file__)), 'svg_end.txt'), mode='r') as f:
        svg_end = f.read()

    def __init__(self, print_layout):
        self.print_layout = print_layout

The new code works and improves the performance, but... something is telling me that there should be a better way.

Is it correct to put code that does something more complex than just assigning constant values in the class definition?

Is it correct to put code that does something more complex than just assigning constant values in the class definition?

In general, I try to avoid it. Any code in the class definition will be run as soon as the class definition is executed – that is, as soon as somebody imports your code. So putting complex code in the class definition causes problems for importers:

If the code is slow, you slow down the rest of the program
If the code has side effects, you’re breaking the rule of least surprise – it’s quite unusual for importing a module to have side effects
If the code can throw exceptions (e.g. if you try to read svg_start.txt and it’s not there), same thing. When I import a library, the only exception I expect it might throw would be an ImportError. Throwing an OSError is generally quite unusual.

So I’d try to avoid it if you can. Only run that code when you need it – i.e., when somebody starts making instances of the class.

In this case, I’d think of maybe defining a descriptor, which caches the file contents, and then apply that to the class, like so:

class CachedFile:

    def __init__(self, path):
        self.path = path
        self.contents = None

    def __get__(self, instance, type):
        if self.contents is None:
            self.contents = open(self.path).read()
        return self.contents

class SVGDrawing:

    svg_start = CachedFile(os.path.join(os.path.dirname(os.path.abspath(__file__)), 'svg_start.txt'))
    svg_end = CachedFile(os.path.join(os.path.dirname(os.path.abspath(__file__)), 'svg_end.txt'))

    def __init__(self, *args, **kwargs):
        pass

Note that in this case, the file reads are deferred right up to the point where we need them – much more efficient than doing a read at import time. The flipside is that this code is a bit more complicated – you should decide if the performance gain is worth it.


Using std::array to implement variadic construction of class with list-of-self as subclass I have a situation with a base class for whom a container of itself is a subclass.  So a Block of Item has various ways in which it acts like an Item itself.  I want to be able to initialize Block as Block {foo, baz, bar...} in a variadic syntax.

Here are the twists:

When a Block is constructed, the parameters in the initialization list are not simply Items.  (Imagine some represent two items... while others might just be a different meaning for the literal types passed in.) To simplify the example I am suggesting that constructing an Item from a C-string literal is not available to the user... but that a Block initialization list would know what to do with that in context.
I'm trying to avoid creating std::vector in the course of the initialization, and instead building a std::array for as near-zero runtime overhead as I can get. Each element in the Block initialization should be constructed (or copy constructed) just once in the process; right in place in the array... though this array is only needed temporarily.  As far as I can tell, this rules out std::initializer_list and I must use a variadic function.

The basic idea is to create a class that has containership of an Item which is a friend of Item... here called Listable. It has its own set of constructors:

class Item {
    friend class Listable;
    friend class Block;
    Item (char const *) { std::cout << "Istring\n"; }
    Item (void *, size_t) { std::cout << "Iblock\n"; }
public:
    Item (Item const &) { std::cout << "Icopy\n"; }
    Item (float) { std::cout << "Ifloat\n"; }
};

class Listable {
    Item item;
public:
    Listable (Item const & i) : item (i) { std::cout << "Litem\n"; }
    Listable (const char * s) : item (s) { std::cout << "Lstring\n"; }
    Listable (float f) : item (f) { std::cout << "Lfloat\n"; }
};

class Block : public Item {
protected:
    Block (Listable * l, size_t c) : Item (l, c) { std::cout << "Bpointer\n"; }

    template<size_t N>
    Block (std::array<Listable, N> a) : Block (&a[0], N) { std::cout << "Barray\n"; }

public:
    template<typename... Ts>
    Block (Ts const & ...t) : Block (std::array<Listable, sizeof...(t)>{t...})
        { std::cout << "B\n"; }
};

Here is a usage, showing a string not working for the construction of an Item while the Block knows how to handle it. The goodItem is the only one that needs to be copied, as its original instance was not constructed in-place in an array slot:

#include <iostream>
#include <array>

/* #include the classes above */

int main() {
    auto goodItem = Item {10.20};
    auto goodBlock = Block {goodItem, "blue", "red", 3.04};
    /* auto misplacedItem = Item {"purple"}; */     // errors, correctly...
}

I compiled with no warnings with:

  g++ -pedantic -Wall -Wsign-conversion -Wextra -Wcast-align -Wcast-qual -Wctor-dtor-privacy -Wdisabled-optimization -Wformat=2 -Winit-self -Wlogical-op -Wmissing-declarations -Wmissing-include-dirs -Wnoexcept -Wold-style-cast -Woverloaded-virtual -Wredundant-decls -Wsign-promo -Wstrict-null-sentinel -Wstrict-overflow=5 -Wswitch-default -Wundef -Werror -Wno-unused --std=c++11 test.cpp -o test

This output is what I wanted to see:

Ifloat
Icopy
Litem
Istring
Lstring
Istring
Lstring
Ifloat
Lfloat
Iblock
Bpointer
Barray
B

Can anyone spots any problems or suggestions for improvement on this technique?

I have a few things to say about your code:

Do you intend to actually do things in the constructors instead of just displaying strings? Knowing this would help the review process. Currently, you do not store anything passed to Item, but if you intend to store things at some point, it may change things.
Instead of using &a[0] which is kind of obscure and not easy to find with a simple search query, you shoud use a.data() which clearly shows the intent.
It would probably be a good idea to pass the std::array by const reference in the constructor instead of passing it by copy. That will avoid useless copies:

template<size_t N>
Block (std::array<Listable, N> const& a) : Block (&a[0], N) { cout << "Barray\n"; }

Of course, that implies that you take const Listable* instead of Listable* at some point (but we come back to my first point: we lack information about what your code is supposed to do). I don't know what you intend to do with this, but passing a pointer to the underlying memory of a temporary doesn't feel right. This may create dangling pointers.

EDIT: since your code isn't supposed to store the pointer, then taking the std::array by copy shoud be safe. But as you noted in the comments, using an rvalue-reference parameters should be even better: it binds to the temporary, you can modify it and no copy is performed. Seems like the best solution.


Set value to multiple selector on condition I have code and set the value to hidden 0 or 1 on condition. Is there a better way to solve this?

<input type="hidden" name="type_product" value=""   class="type_product"/>
<input type="hidden" name="type_category" value=""  class="type_category" />
<input type="hidden" name="type_modifier" value=""  class="type_modifier"/>
<input type="hidden" name="type_variant" value=""   class="type_variant" />

<script>
   $(function(){
       let dataType = "product"; // category
       if (dataType == "product") {
           $(".type_product").val(1);
           $(".type_category").val(0);
           $(".type_modifier").val(1);
           $(".type_variant").val(0);
}
      if (dataType == "category") {
          $(".type_category").val(1);
          $(".type_product").val(0);
          $(".type_modifier").val(0);
          $(".type_variant").val(1);
      }
   })
 </script>

The first and most simple thing to do, in this case, is to drop that if. Duplicated code always smells:

const isProduct = dataType === "product";
$(".type_product").val(isProduct ? 1 : 0);
$(".type_category").val(isProduct ? 0 : 1);
$(".type_modifier").val(isProduct ? 1 : 0);
$(".type_variant").val(isProduct ? 0 : 1);

Note the use of === instead of ==. Note that you may write $(".type_product, .type_modifier") to reduce number of lines but I find it pretty odd because I'm aggregating selectors for objects with different content (and I'm updating their value, not their style). Just my opinion, anyway.

If this is everything you need then I'd keep it as simple as possible but what if logic is more complex? Let's introduce a function: val() accepts it as parameter:

$(".type_product").val(calculateTypeField);
$(".type_category").val(calculateProductField);

And so on. Those functions can be as complex as required (and you introduce some separation between your view and its view model. For even more complex cases you may also have a dictionary:

{
    'product': calculateProductField
}

To use together with a data-field attribute to map the function to the function which calculates it:

<input type="hidden" name="type_product" value="" data-field="product" />

(of course you need to write few JavaScript lines to do this _mapping__)


Making code elegant, clean and optimized I have some piece of code that I already optimized, however I found that there is an edge case that I should handle and I am unable to make it elegant.
char* data = get_data_from_queue(data);
Album album;
get_album(data, &album);
if (get_album_vals(album) <= 0) {
    printf("Album doesn't exist, will try to create it");
    return should_create_album(album);
}
do_stuff_with_data(&data);

Now, when I don't find album vals, under certain conditions (< 0.01%), it may be a golden album.
If it is a golden album, under certain conditions (<0.01%), it may need a second chance.
This is the modified code that should keep same behavior, apart from the second chance for golden album:
char* data = get_data_from_queue(data);
Album album;
get_album(data, &album);
if (get_album_vals(album) <= 0) {
    int is_golden_album = 0;
    ret = should_create_album(album);
    if (!ret && album_might_be_golden(album)) {
        GoldenAlbum golden_album;
        if (get_golden_album(data, &golden_album)
        {
            ret = should_create_album(golden_album);
        } else {
            is_golden_album = 1;
        }
    }
    if (is_golden_album > 0)
    {
        return ret;
    }
}
do_stuff_with_data(&data);

However the new code is awful and I am not sure how to make it more elegant and optimized and I have spent way too much time on this.
Anyone have a suggestion?

The inelegance lies in the way you're storing your data.  You haven't normalized it, so you have to check multiple places for what should be the same information.
Why are golden albums in a different list?  The fact that you need to check that list too, should be an indication that those two lists should be the same.  Just like the artists, producers, and other information about the album, whether or not it's golden (and indeed, how many times, if I understand golden album status correctly) is an attribute of the data.
You don't spend much time justifying why GoldenAlbum and Album are different types. Without knowing much about the data you're modeling, it's hard to be sure exactly what your data attempts to describe.  So maybe I'm misunderstanding what 'golden' means for an 'album'.  But if you have only one Album object and it has a is_golden attribute, you can avoid continuing to implement checks for both types of album in any code that interacts with the data by album name.


A C++ program to check if a string is a pangram I'm new to C++ and decided to implement a simple program that checks if a string is a pangram (i.e. the string contains all the English alphabet letters). It's my first program in C++ and I have some background with interpreted programming languages like Python and R.
I want to know what are the common beginners mistakes I'm making in this implementation, whether I'm following good practices, what I should avoid etc.
The Code
#include<iostream>
#include<string>
#include<vector>
#include <algorithm>

bool isPangram(std::string str) {
    std::transform(str.begin(), str.end(), str.begin(), ::toupper); // Transforming a string to upper case
    std::vector<bool> hash_table(26, false); // a hash_table that maps the  index of a letter from the alphabet
                                             // and a boolean saying if the string contains this element
    int index;
    for(auto c: str){
        if(isalpha(c))
            // Checks if the element in the string is in the alphabet
            index = c - 'A';
            hash_table[index] = true;
            std::cout << index << std::endl;

    };

    if(std::any_of(hash_table.begin(), hash_table.end(), [](bool v) { return !v; })) {
        // if any of the items in the array evaluates to false
        // means that the string don't contain all the elements
        // in the alphabet,hence, it's not a pangram

        return false;
    }
    return true;
};

int main() {
    std::string str = "We promptly judged antique ivory buckles for the next prize";
    if (isPangram(str)){
         std::cout << "Yes" << std::endl;
     }
     else {
         std::cout << "No" << std::endl;
     };
     return 0;
}
```

Here are a number of things that may help you improve your program.
Make sure you have all required #includes
The code uses toupper but doesn't #include <cctype>.
Eliminate spurious semicolons
The code has a number of spurious semicolons immediately after closing braces.  They don't bother the compiler but they will bother any other programmer who looks at your code.
Check your if statements for proper braces
It appears from the indentation in the if statement in the isPangram routine is not what you intended to write.  The only thing that is currently associated with the if statement is index = c - 'A'; -- all of the lines after that are executed in every case.
Initialize variables before use
The index variable, as mentioned above, may or may not actually get initialized to anything as it's currently written.  That leads to undefined behavior of your program, which is not a good thing.  Best practice is to initialize variables when they are declared.
Return boolean values directly
The isPangram ends with this strange construct:
if(std::any_of(hash_table.begin(), hash_table.end(), [](bool v) { return !v; })) {
    return false;
}
return true;

Here's a much more direct way to do that:
return std::all_of(hash_table.begin(), hash_table.end(), [](bool v){return v;});

Similarly, within main, I'd write this:
std::cout << (isPangram(str) ? "Yes" : "No" ) << '\n';

Consider a more efficient approach
Consider that the the transform must necessarily visit every letter.  We then make another pass through with the main loop.  It is possible to do everything in a single pass.
Don't use std::endl unless you really need to flush the stream
The difference between std::endl and '\n' is that std::endl actually flushes the stream. This can be a costly operation in terms of processing time, so it's best to get in the habit of only using it when flushing the stream is actually required. It's not for this code.
Eliminate return 0 at the end of main
When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so it is not necessary to explicitly write that.  Some people prefer to do so for style reasons, but it's important to know that it's not required by the standard.
Consider using std::spanstd::accumulate
If your compiler is C++20 capable, this would be a good use of std::accumulate (not std::span as I originally wrote).  This version is faster than anything so far.
// helper function for isPangram_accum
constexpr uint_fast32_t charToMask(const unsigned char c){
    // can't use non-const toupper in constexpr function
    unsigned index{(c | 0x20u) - 'a'};
    return index < 26 ? ~(1u << index) : ~0;
}

bool isPangram_accum(const std::string& str) {
    uint_fast32_t present{(1u << 26) - 1};
    constexpr auto letterMask = [](uint_fast32_t collection, char ch) {
      if (collection)
        collection &= charToMask(ch);
      return collection;
    };
    return std::accumulate(str.begin(), str.end(), present, letterMask) == 0;
}


Looping through columns in Entity Framework Is there a cleaner/more efficient way to loop through the columns in EF implicitly than the way written below?

static void Main(string[] args) {
    using (var db = new someDbContext()) {
        var query = from p in db.someTable
                    select new {
                        column1 = p.column1
                        column2 = p.column2
                    };

        var columnAccessors = CreateAccessors(query.FirstOrDefault());

        foreach (var row in query) {
            foreach (var col in columnAccessors) {
                var val = col(row);
                //Do something with val here.
            }
        }
    }
}

static Func<T, object>[] CreateAccessors<T>(T source = default(T)) {
    var propertyAccessors = typeof(T).GetProperties(BindingFlags.Instance | BindingFlags.Public)
                .Where(p => p.CanRead)
                .Select((p, i) => new {
                    Index = i,
                    Property = p,
                    Accessor = CreatePropertyAccessor<T>(p)
                })
                .ToArray();

    return propertyAccessors.Select(p => p.Accessor).ToArray();
}

static Func<T, object> CreatePropertyAccessor<T>(PropertyInfo prop) {
    var param = Expression.Parameter(typeof(T), "input");
    var propertyAccess = Expression.Property(param, prop.GetGetMethod());
    var castAsObject = Expression.TypeAs(propertyAccess, typeof(object));
    var lambda = Expression.Lambda<Func<T, object>>(castAsObject, param);
    return lambda.Compile();
}

I've tried simply doing a foreach (var col in row) before but I know that it won't work because row doesn't contain a definition for GetEnumerator and I'm not sure how one would go about implementing a solution that would do so generically for something like this.

I was working on implementing a DataReader and came across this page: http://www.developerfusion.com/article/122498/using-sqlbulkcopy-for-high-performance-inserts/. The thought occurred to me that I could modify the CreatePropertyAccessors segments to loop through columns and so I came up with the above. Problem being I'm just not sure that it's a "good" solution.

Edit:

With one small change the following is possible as well:

var query = from p in db.someTable
            select new SomeModel {
                item1 = p.column1,
                item2 = p.column2
            };

var columnAccessors = CreateAccessors<SomeModel>();

So I had a look and made a few modifications, see below with comments.

// Made the method generic, the constaint is required by DbSet
static void LoopThroughColumns<T>(Func<someDbContext, DbSet<T>> getTable)
    where T : class
{
    using (var db = new someDbContext())
    {
        // Selecting columns exlicitly was unnecessary
        var query = getTable(db);
        var columnAccessors = CreateAccessors<T>();

        foreach (var row in query)
        {
            foreach (var col in columnAccessors)
            {
                var val = col(row);
            }
        }
    }
}

// Parameter is unnecessary as you never used it
static Func<T, object>[] CreateAccessors<T>()
{
    // Index and Property values weren't being used
    // ToArray() was an unnecessary conversion
    var propertyAccessors = typeof(T).GetProperties(BindingFlags.Instance | BindingFlags.Public)
                .Where(p => p.CanRead)
                .Select((p, i) =>  CreatePropertyAccessor<T>(p));

    return propertyAccessors.Select(e => e).ToArray();
}

static Func<T, object> CreatePropertyAccessor<T>(PropertyInfo prop)
{
    var param = Expression.Parameter(typeof(T), "input");
    var propertyAccess = Expression.Property(param, prop.GetGetMethod());
    var castAsObject = Expression.TypeAs(propertyAccess, typeof(object));
    var lambda = Expression.Lambda<Func<T, object>>(castAsObject, param);
    return lambda.Compile();
}

Usage is a little different now, you would call it like this:

LoopThroughColumns(e => e.someTable);

To be extra-super cool (ie. generic) you could even pass in an Action<object> and have that define what is done with val, then derive different methods on that one. Below I'm creating a PrintColumnValues method which calls LoopThroughColumns with an specific Action<object> to perform.

static void LoopThroughColumns<T>(Func<someDbContext, DbSet<T>> getTable, Action<object> actionOnObject)
    where T : class
{
    using (var db = new someDbContext())
    {
        var query = getTable(db);
        var columnAccessors = CreateAccessors<T>();

        foreach (var row in query)
        {
            foreach (var col in columnAccessors)
            {
                actionOnObject(col(row));
            }
        }
    }
}

static void PrintColumnValues<T>(Func<someDbContext, DbSet<T>> getTable)
    where T : class
{
    LoopThroughColumns(getTable,
        new Action<object>(e =>
        {
            Console.WriteLine(e);
        }));
}

Usage

PrintColumnValues(e => e.someTable);


Scrolling in Horizontal ScrollView losing selection When I chose an element from this HorizontalScrollView, I was setting focus to that element by calling:

else if (v.getParent() == candidatesScrollView.getChildAt(0))
{
    Button candidateButton = (Button) v;
    v.requestFocusFromTouch();
    v.setSelected(true);
            (...)
}

Here is part of my XML:

<HorizontalScrollView
        android:id="@+id/CandidatesHorizontalScrollView"
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:layout_above="@+id/linearLayout2"
        android:clickable="false"
        android:focusable="false"
        android:focusableInTouchMode="false"
        android:visibility="gone" >

    <LinearLayout
        android:layout_width="match_parent"
        android:layout_height="match_parent"
        android:clickable="false"
        android:focusable="false"
        android:focusableInTouchMode="false"
        android:orientation="horizontal" >

        <Button
            android:id="@+id/horizontalscrollview1_button1"
            android:layout_width="wrap_content"
            android:layout_height="wrap_content"
            android:text="Button"
            android:textSize="25sp" />

        (...)
        // 11 more buttons
    </LinearLayout>

</HorizontalScrollView>

After that, when I scrolled the list without choosing other element, I was losing focus of previously selected element. I made some research about this topic, but there was no solution that could work for me.  Finally, after almost two weeks I have created some solution by myself.

I would like you to review it and tell me what can I improve on.

I created a custom HorizontalScrollView class inside and have overridden the onTouchEvent() method. I don't think this is optimal way of doing that, because in that case I have to do calculations every time I move even one pixel.  For example, if I add toast.show() to the below method, it will try to show as many toast as many I moved pixels (If I move by 10 pixels, it will try to show 10 Toast). Anyway, it works for me and the selection and focus are being kept. Please help me modify this code to make finally a good answer for that known issue:

@Override
public boolean onTouchEvent(MotionEvent ev)
{
    int i = 0;
    Button button = null;
    for (; i < 11; i++)
    {
        button = (Button)((LinearLayout)getChildAt(0)).getChildAt(i);
        if(button.isSelected())
            break;
    }
    super.onTouchEvent(ev);
    button.setSelected(true);
    button.requestFocusFromTouch();

    return true;
}

To be sure that the above code will work, you need to have only one selected item in your HorizontalScrollView at a time, i.e when you press different Button, you need to make the previous one setSelected(false).

I'm not familiar with Android, so just some generic notes:

int i = 0;
Button button = null;
for (; i < 11; i++)

I think the most developers more familiar with the following (equal and shorter) form:

for (int i = 0; i < 11; i++)

11 and 0 are magic numbers. Use a named constant instead of them which explains the meaning of these numbers (and probably removes some duplication and makes maintenance easier).
I think MotionEvent variable could deserve a little bit longer name for better readability. (Clean Code by Robert C. Martin, Avoid Mental Mapping, p25)
You could increase the abstraction level and readability a little bit with a new method whose name contains the purpose of the loop:

@Override
public boolean onTouchEvent(MotionEvent event) {
    Button button = getSelectedButton();
    super.onTouchEvent(event);
    button.setSelected(true);
    button.requestFocusFromTouch();

    return true;
}

private Button getSelectedButton() {
    LinearLayout linearLayout = (LinearLayout) getChildAt(0);
    for (int i = 0; i < 11; i++) {
        Button button = (Button) linearLayout.getChildAt(i);
        if (button.isSelected()) {
            return button;
        }
    }
    throw new IllegalStateException("no button selected");
}

Note that the Button variable get a smaller scope and the linearLayout local variable.

(Clean Code by Robert C. Martin, One Level of Abstraction per Function, p36; Clean Code by Robert C. Martin, G19: Use Explanatory Variables, p296; Effective Java, Second Edition, Item 45: Minimize the scope of local variables; Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)
If you return null the button.setSelected() will throw a NullPointerException (or if you don't find any selected button in the original code). I guess having zero selected button is not a valid state of the application. If it somehow happens I'd throw an exception immediately (instead of setting null as the Button reference).

private Button getSelectedButton() {
    for (int i = 0; i < 11; i++) {
        Button button = (Button) ((LinearLayout) getChildAt(0)).getChildAt(i);
        if (button.isSelected())
            return button;
    }
    throw new IllegalStateException("no button selected");
}

(The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)
super.onTouchEvent(event);
button.setSelected(true);

Are you sure that you need setSelected(true) here? It seems that it's already selected before the super call (the for loop returns a selected button) but super.onTouchEvent() might unselect it. If super doesn't change this state you could remove the button.setSelected call.


Using composition or inheritance to enhance this linked list Problem Statement:

  A method called insertEnd() exists, but it runs in linear time, because every time it is called, it walks down the list to find the end.  Without changing the meaning of this method or any other, modify the representation of a SList and whatever methods are necessary to make insertEnd() run in constant time. Your SList class will need to continually maintain a record of the last (tail) SListNode in an SList, and all SList's methods will have to ensure that this record stays current.

SList class & SListNode class are already provided in the assignment as shown below:

SListNode

/* SListNode.java */
class SListNode {
  Object item;
  SListNode next;

  /**
   *  SListNode() (with two parameters) constructs a list node referencing the
   *  item "obj", whose next list node is to be "next".
   */

  SListNode(Object obj, SListNode node) {
    item = obj;
    this.next = node;
  }

  /**
   *  SListNode() (with one parameter) constructs a list node referencing the
   *  item "obj".
   */

  SListNode(Object obj) {
    this(obj, null);
  }
}

SList

/* SList.java */

  public class SList {

  protected SListNode head;
  protected int size;

  /**
   *  SList() constructs an empty list.
   **/

  public SList() {
    size = 0;
    head = null;
  }

  /**
   *  insertFront() inserts item "obj" at the beginning of this list.
   *  @param obj the item to be inserted.
   **/

  public void insertFront(Object obj) {
    head = new SListNode(obj, head);
    size++;
  }

  /**
   *  insertEnd() inserts item "obj" at the end of this list.
   *  @param obj the item to be inserted.
   **/

  public void insertEnd(Object obj) {
    if (head == null) {
      head = new SListNode(obj);
    } else {
      SListNode node = head;
      while (node.next != null) {
        node = node.next;
      }
      node.next = new SListNode(obj);
    }
    size++;
  }
}

Below is the solution for the given problem:

TailList

/* TailList.java */

public class TailList extends SList{
    private SListNode tail;

    public TailList(){
        tail = null;
    }

    /**
     *  insertEnd() inserts item "obj" at the end of this list.
     *  @param obj the item to be inserted.
     **/

    public void insertEnd(Object obj) {
        if(this.size > 0){
            SListNode node  = new SListNode(obj);
            this.tail.next = node;
            this.tail = node;
        }else{
            this.head = this.tail = new SListNode(obj);
        }
        this.size++;
    }

    /**
     *  insertFront() inserts item "obj" at the beginning of this list.
     *  @param obj the item to be inserted.
     **/

    public void insertFront(Object obj){
        super.insertFront(obj);
        if(size == 1){
            tail = head;
        }
    }
}

My question:

Does TailList class inheritance relation with SList implementation class looks fine(within same package)? How do I know whether SList class is specifically designed for inheritance, as mentioned below?

  It is safe to use inheritance within a package, where the subclass and the superclass implementations are under the control of the same programmers. It is also safe to use inheritance when extending classes specifically designed and documented for extension
  (Item 17). Inheriting from ordinary concrete classes across package boundaries,
  however, is dangerous. As a reminder, this book uses the word “inheritance” to mean implementation inheritance (when one class extends another). The problems discussed in this item do not apply to interface inheritance (when a class implements an interface or where one interface extends another).

Considering that…

You have full access to the source code of the original SList class
To make inheritance work, you had to edit SList.java to change head and size from private to protected access
The assignment asks you to "modify the representation of SList"
The resulting class performs the same task as the original code, adding no new functionality (just a large performance optimization for insertEnd() with a negligible cost to insertFront())

I see no reason to use either inheritance or composition.  Just edit SList.java directly to modify its implementation.  The resulting SList class should be fully substitutable with the original, just with better performance characteristics.


Graceful thread shutdown with std::move I have a thread as part of a class that access the class's data members. The class has a move constructor that moves some of the old class's data members into the new object being constructed. Before moving the class members of the old object, I need to shutdown its thread. I do this by calling thread_shutdown in the move constructor. I would like to know if this is the most elegant way to achieve thread-safety.

#include <chrono>
#include <iostream>
#include <memory>
#include <thread>

class B {
public:
  int x;
};

class A {
  std::unique_ptr<B> b_ptr;
  volatile bool thread_shutdown = false;
  std::thread t;
  void run() {
    while (!thread_shutdown) {
      int x = b_ptr->x;
    }
  }

public:
  A() : b_ptr(std::make_unique<B>()), t(std::thread(&A::run, this)) {}
  // A(A &&a) : b_ptr(std::move(a.b_ptr)), t(std::thread(&A::run, this)) {}
  A(A && a) {
    a.shutdown_thread();
    b_ptr = std::move(a.b_ptr);
    t = std::thread(&A::run, this);
  }
  ~A() { shutdown_thread(); }
  void shutdown_thread() {
    thread_shutdown = true;
    if (t.joinable()) {
      t.join();
    }
  }
};

int main() {
  std::unique_ptr<A> a_ptr = std::make_unique<A>();
  a_ptr = std::make_unique<A>(std::move(*a_ptr));
}

If I replace the move constructor definition by the commented line in the code, I get a segmentation fault in the second line of main because the old thread is trying to access b_ptr->x, but b_ptr is NULL as it has been moved to the new object's b_ptr. Thus, I cannot rely on the destructor of class A to shutdown the thread.

So, the question is: What is the best way to handle thread shutdown before some of the objects accessed by it are moved? I don't like having to call shutdown_thread in the move constructor.

volatile bool thread_shutdown = false;

This is definitely not correct. volatile is not for thread safety. What you want here is atomic<bool>.

Other than that, I'd say everything else looks technically kosher. The thing is, the semantics of your move operations will be weird now. For example, if I write:

auto func(A a) {}

auto a = A{};
func(std::move(a));

What I expect to happen is that a is cheaply moved into the function argument (and thus, into the function). Instead what happens here is that I've triggered a thread death plus the creation of a whole new thread. So much for moves being cheap - in fact, copying might be cheaper because it would create a new thread, but not destroy the old one.

If you want to define a constructor that steals data from another object, and creates an entirely new thread to use that data in, that's fine. But using the move constructor for that seems unwise. This is probably a job for a tagged constructor:

A() :
    b_ptr{std::make_unique<B>()}
{
    // I wouldn't rely on order of data members to guarantee the thread
    // is started after the data is set.
    t = std::thread{&A::run, this};
}

A(steal_data_t, A&& a)
{
    a.shutdown_thread();
    b_ptr = std::move(a.b_ptr());
    t = std::thread{&A::run, this};
}

A(A&&) noexcept = default; // well, you'd need to handle moving the atomic, I believe

// Usage:
auto a1 = A{};
auto a2 = std::move(a1); // this invalidates a1, as usual
auto a3 = A{steal_data, std::move(a2)}; // this invalidates a2, as expected

You could avoid some repetition with private helper functions.


Generating smooth numbers in Haskell I wrote a smooth function that returns a list of all smooth numbers below a certain limit from a list of prime:

repeatMul init limit = takeWhile (<=limit) . scanl (*) init . repeat

smooth limit = (`inner` 1)
    where inner [] init = [init]
          inner (x:xs) init = repeatMul init limit x >>= inner xs

For instance:

main = print . sort . smooth 20 $ [2, 3, 5]
-- [1,2,3,4,5,6,8,9,10,12,15,16,18,20]

It works fine, but I wonder if the inner function could be written in a different way. I've been trying to find a high order function to achieve the same purpose, but I can't wrap my head around it.

Also I'm new to haskell, so any general comment is welcome.

Extra question: I have another version of smooth that returns a set instead of a list:

import Data.Set (singleton, unions)

repeatMul limit x = takeWhile (<=limit) . iterate (*x)

smooth limit xs = foldr f singleton xs $ 1
    where f x c = unions . map c . repeatMul limit x

Is it possible to simplify this as well even though the Set type does not instantiate the Monad type class?

smooth limit = foldlM (`repeatMul` limit) 1

Intermediate steps:

smooth limit = (`inner` 1)
    where inner [] = pure
          inner (x:xs) = flip (`repeatMul` limit) x >=> inner xs

smooth limit = (`inner` 1)
    where inner = foldr (>=>) pure . map (flip (`repeatMul` limit))

smooth limit xs = (foldr (>=>) pure $ map (flip (`repeatMul` limit)) xs) 1

smooth limit = foldl (>>=) (pure 1) . map (flip (`repeatMul` limit))

smooth limit = foldlM (flip id) 1 . map (flip (`repeatMul` limit)) -- did I mention foldlM has its argument order as stupid as repeatMul? :P

Extra answer:

There is no need for the no-duplicates bookkeeping of sets - every natural has a unique prime factorization. Anyway, here as a fold from the right, if repeatMul produced a set:

smooth limit = foldr (foldMap . repeatMul limit) (singleton 1)

Note that this also works for the list case with [1] in place of singleton 1.


Calculating the results of bit operations I have a simple program that calculates and outputs the results of all the bit operators. I'm planning to send it to my friend and to explain some stuff about them to him. Before I do that, I want to know if I can make any improvements to my program.

internal class Program
{
    private static int a;
    private static int b;
    private static byte byteOperation;
    private static short int16Operation;
    private static int int32Operation;
    private static long int64Operation;

    private static void Main()
    {
        while (true)
        {
            bool numberA = int.TryParse(Console.ReadLine(), out a);
            bool numberB = int.TryParse(Console.ReadLine(), out b);
            if (!numberA || !numberB) Console.WriteLine("Input can be only numbers !");
            else
            {
                Console.WriteLine();
                byteOperation = (byte) (a & b);
                int16Operation = (short) (a & b);
                int32Operation = a & b;
                int64Operation = a & b;
                WriteLine("Binary AND", "&");

                byteOperation = (byte) (a | b);
                int16Operation = (short) (a | b);
                int32Operation = a | b;
                int64Operation = a | b;
                WriteLine("Binary OR", "|");

                byteOperation = (byte) (a ^ b);
                int16Operation = (short) (a ^ b);
                int32Operation = a ^ b;
                int64Operation = a ^ b;
                WriteLine("Binary XOR Operator", "^");

                byteOperation = (byte) (a << b);
                int16Operation = (short) (a << b);
                int32Operation = a << b;
                int64Operation = a << b;
                WriteLine("Binary Left Shift", "<<");

                byteOperation = (byte) (a >> b);
                int16Operation = (short) (a >> b);
                int32Operation = a >> b;
                int64Operation = a >> b;
                WriteLine("Binary Right Shift", ">>");

                byteOperation = (byte) (a << b | a >> 8 - b);
                int16Operation = (short) (a << b | a >> 16 - b);
                int32Operation = a << b | a >> 32 - b;
                int64Operation = a << b | a >> 64 - b;
                WriteLine("Circular Shift", "<< | >>", "<<", "|", ">>");
            }
        }
    }

    private static void WriteLine(string text, string ch)
    {
        Console.ForegroundColor = ConsoleColor.Green;
        Console.WriteLine(text + " : " + ch);
        Console.WriteLine();
        Console.ForegroundColor = ConsoleColor.Red;
        Console.WriteLine("Byte : {0} " + ch + " {1} = {2}", a, b, byteOperation);
        Console.WriteLine("Int16 : {0} " + ch + " {1} = {2}", a, b, int16Operation);
        Console.WriteLine("Int32 : {0} " + ch + " {1} = {2}", a, b, int32Operation);
        Console.WriteLine("Int64 : {0} " + ch + " {1} = {2}", a, b, int64Operation);
        Console.WriteLine();
    }

    private static void WriteLine(string text, string chFull, string ch1, string ch2, string ch3)
    {
        Console.ForegroundColor = ConsoleColor.Green;
        Console.WriteLine(text + " : " + chFull);
        Console.WriteLine();
        Console.ForegroundColor = ConsoleColor.Red;
        Console.WriteLine("Byte : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 8 - {1}" + " = {2}", a, b, byteOperation);
        Console.WriteLine("Int16 : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 16 - {1}" + " = {2}", a, b, int16Operation);
        Console.WriteLine("Int32 : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 32 - {1}" + " = {2}", a, b, int32Operation);
        Console.WriteLine("Int64 : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 64 - {1}" + " = {2}", a, b, int32Operation);
        Console.WriteLine();
    }
}

private static int a;
private static int b;
private static byte byteOperation;
private static short int16Operation;
private static int int32Operation;
private static long int64Operation;

That whole chunk of static instance fields should be passed around as parameters.

They're only static so that you can use them from static context. static is like plague, a virus; it creeps and expands its tentacles across your entire code base if you let it.

Since this is just a learning exercise, I'll let it slip this time and won't say a thing about all the static methods being called from Main. But the fields shouldn't exist.

Now this is confusing:

bool numberA = int.TryParse(Console.ReadLine(), out a);
bool numberB = int.TryParse(Console.ReadLine(), out b);

numberA and numberB are Boolean variables, but if you don't pay attention, it's very easy - WAY too easy, to glance at the code and think they're numbers... that's what their names say!

if (!numberA || !numberB) Console.WriteLine("Input can be only numbers !");
else

That's a very confusing and dangerous way to write conditional code. Always use braces around scopes. - this would be much, much, much clearer and less error-prone:

 if (!numberA || !numberB)
 {
     Console.WriteLine("Input can be only numbers !");
 }
 else

What you really need, is a separate method that gets the two numbers and only returns once you have valid input. You also need a way to break out of the loop and sanely quit the program! How many concerns is that? How many methods should it be written with?

private static Tuple<int,int> PromptForTwoNumbers()
{
    var value1 = PromptForValidNumber();
    if (value1 == null)
    {
        return null;
    }

    var value2 = PromptForValidNumber();
    if (value2 == null)
    {
        return null;
    }

    return Tuple.Create(value1, value2);
}

private static int? PromptForValidNumber()
{
    while (true)
    {
        Console.WriteLine("Please enter a number:");

        int value;
        var input = Console.ReadLine();
        if (IsUserTiredOfThis(input))
        {
            return null;
        }

        if (int.TryParse(input, out value))
        {
            return value;
        }

        Console.WriteLine("Input must be a number!");
    }
}

private static bool IsUserTiredOfThis(string input)
{
    return string.IsNullOrEmpty(input) || input == "quit"
}

Now, all xxxxOperation variables are misnamed. They're not operations, they're results.

Your WriteLine method is also confusing: until you get to the actual implementation, there's no way of telling that method is going to actually be writing 7 lines to the console.

The way you're writing is awkward:

    Console.WriteLine("Byte : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 8 - {1}" + " = {2}", a, b, byteOperation);
    Console.WriteLine("Int16 : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 16 - {1}" + " = {2}", a, b, int16Operation);
    Console.WriteLine("Int32 : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 32 - {1}" + " = {2}", a, b, int32Operation);
    Console.WriteLine("Int64 : {0} " + ch1 + " {1} " + ch2 + " {0} " + ch3 + " 64 - {1}" + " = {2}", a, b, int32Operation);

Don't mix string-formatting and + concatenations. And name things for Pete's sake! "ch2" rings like "channel 2" in my mind, and tells me nothing about what that thing is supposed to be.

Your Int64 stuff is operating on Int32's. In fact, none of your operations are operating with the types they're claiming to be:

byteOperation = (byte) (a | b);

Should be:

byteResult = (byte)((byte)a | (byte)b); // yuck!

You're just working off Int32's and casting the result to a given type. Which means... you're probably missing an unchecked scope around these operations to let them overflow without blowing everything up.

Or was overflowing your "clean way out" of the app?


Recursively printing equations I did this program and I need to make sure I did it right. The answers come out to be right but I want to make sure nothing is wrong with the coding.

class RecursiveMethods
{
    RecursiveMethods()          //default constructor
    {
    }

    public int fOf(int x)
    {
        if (x <= 10)                        //the base case
        {
            System.out.println(x + " <= 10, therefore ... f(" + x + ") = -5");
            return -5;
        }
        else
        {
            System.out.println(x + " > 10, therefore ... f(" + x + ") = f(" + x + " - 3) + 2 = f(" + (x -3) + ") + 2");
            return fOf(x-3) + 2;
        }
    }
}

public class RecursionMethodTester
{
    public static void main(String[] args)
    {
        int x;
        RecursiveMethods rMethods = new RecursiveMethods();

        System.out.println("---------------------------------");
        System.out.println("       f(x - 3) + 2    if x >  10");
        System.out.println("f(x) = ");
        System.out.println("       -5              if x <= 10");
        System.out.println("---------------------------------");
        System.out.println();

        x = 20;
        System.out.println("Example 1:  x = " + x);
        System.out.println("f(" + x + ") = " + rMethods.fOf(x));
        System.out.println();

        x = 19;
        System.out.println("Example 2:  x = " + x);
        System.out.println("f(" + x + ") = " + rMethods.fOf(x));
        System.out.println();

        x = 18;
        System.out.println("Example 3:  x = " + x);
        System.out.println("f(" + x + ") = " + rMethods.fOf(x));
        System.out.println();
    }
}

As mentioned by @Cyrille fOf is not a good method name.

I disagree with the use of more comments, if you name your function well enough, you shouldn't need comments.  Comments should be used to explain hard to understand or confusing code, not describe what is going on.

In java the opening { is usually placed at the end of the line where the code block starts.  i.e.

if (somethingIsTrue) {

You can get rid of the else in your fOf function.  It is implied with the return in the if.

You don't need to specify the default constructor.  It does nothing, so it is just clutter.

I'm also not happy with having the System.out.println in your class.  Off of the top of my head, I would create an reporting structure, which would include an interface and at least one class.  Something like

interface Reporter {

    void reportLine(string message);
}

Then you could implement it like

public class ConsoleReporter implements Reporter {

    void reportLine(string message) {
        System.out.println(message);
    }

}

and your implementation.  Notice the constructor now does something, so my point about removing it is not valid in this case.

class RecursiveMethods {

   private Reporter reporter;

   RecursiveMethods(Reporter reporter)          //default constructor
   {
       this.reporter = reporter;
   }

   public int fOf(int x)
   {
       if (x <= 10)                        //the base case
       {
           reporter.reportLine(x + " <= 10, therefore ... f(" + x + ") = -5");
           return -5;
       }

       reporter.reportLine(x + " > 10, therefore ... f(" + x + ") = f(" + x + " - 3) + 2 = f(" + (x -3) + ") + 2");

        return fOf(x-3) + 2;
   }
}

public static void main(String[] args)
{
    Reporter reporter = new ConsoleReporter();
    int x;
    RecursiveMethods rMethods = new RecursiveMethods(reporter);

    // Change all System.out.writeLn to reporter.reportLine
}

Doing this will allow you to output to different mediums, for instance, you could create FileReporter, which would output to a file, or StreamingReporter, which would output to a stream of some sort.  It just makes your code more versatile.  You could also add a method like reportString, which would write an entire formatted string.


Proxy evasion browser in Python This code works. It is designed to retrieve website data directly, parse it, and open it as normal. I will develop it further later. Right now I want to know if there is any way to improve efficiency, such as not using a file etc. I would also like to know if my code could be made more pythonic (this would include PEP standards), or incidences where concatenation would be acceptable to readability. Here is the code:

import urllib.request
from tkinter import *
import webbrowser
import os

proxy_handler = urllib.request.ProxyHandler(proxies=None)
opener = urllib.request.build_opener(proxy_handler)

def navigate(query):
    response = opener.open(query)
    html = response.read()
    return html

def parse(junk):
    ugly = str(junk)[2:-1]
    lines = ugly.split('\\n')
    return lines

while True:
    url = input("Path: ")
    dump = navigate(url)

    content = parse(dump)
    with open('cache.html', 'w') as f:
        for line in content:
            f.write(line)

    webbrowser.open_new_tab(os.path.dirname(os.path.abspath(__file__))+'/cache.html')

1. Make sure your variable names are both short and to the point, using a parameter called junk is pointless. Also, this could cause conflicts if you ever write other code and import this.

def parse(junk):
    ugly = str(junk)[2:-1]
    lines = ugly.split('\\n')
    return lines

2. urllib is outdated. Unless there is a specific reason you need to use urllib, you should use urllib2. Urllib2 is faster and more simple.

import urllib.request

3. This part looks a bit messy:
webbrowser.open_new_tab(os.path.dirname(os.path.abspath(__file__))+'/cache.html')
Perhaps separate into multiple lines for readability.


Embed MD5 digest in a directory path name My boss needs me to embed the MD5 digest in a file path, but the problem is MD5 contains escape characters.

I've already taught about a uc%duc%d... format, but it also makes lengthy file names. In Linux it's not a problem since the Linux kernel could handle them.

char * uc_encode_md5(const char * password)
{
  char digest_output[1024];
  MD5_CTX md5_ctx ;
  MD5_Init(&md5_ctx);
  MD5_Update(&md5_ctx,password,strlen(password));
  MD5_Final((unsigned char*) digest_output,&md5_ctx);
  char* ret = (char*) malloc(500);
  int len=0;
  int i ;
  for(i=0;i<16;i++)
  {
   len+= sprintf(&ret[len],"uc%d",(unsigned char)digest_output[i]);
  }

  return ret;
}

Any better way than this? Since my just boss shared what to do, but not how to do it: what's the best practice here?

How about #ifdef's?  I could suggest more formats on here and let him choose when I get back to him later tonight.

For example:

char * uc_encode_md5(const char * password)
{
#ifdef _UC_PATH_ENCODE_
  char digest_output[1024];
  MD5_CTX md5_ctx ;
  MD5_Init(&md5_ctx);
  MD5_Update(&md5_ctx,password,strlen(password));
  MD5_Final((unsigned char*) digest_output,&md5_ctx);
  char* ret = (char*) malloc(500);
  int len=0;
  int i ;
  for(i=0;i<16;i++)
  {
   len+= sprintf(&ret[len],"uc%d",(unsigned char)digest_output[i]);
  }
#endif /* endif _UC_PATH_ENCODE_ */
  return ret;
}

MD5 is simply a 128 bit number.

Convert the number into a string that is the ASCII representation of the number. Note: Usually this is done as hex number. If you make sure it is 0 padded it will be 32 characters long (as each 8 bits is converted into two hex digits).

This is how I would do it:

static_assert(sizeof(int) == 4, "Figure it out with other sizes");

int*  data = reinterpret_cast<int*>(ret);
int   len  = 128 / (sizeof(int) * 8);

for(int i=len-1; i >= 0; --i)
{
    std::cout << std::setfill('0') << std::setw(8) << std::hex << data[i];
}


Entity Database Add/Update method What would be the most elegant way to refactor the following method?

 public void AddOrUpdateDesignCVT(DesignInformation designInformation,
                            CommonTransformerProperties commonTransformerProperties,
                            SecondaryWindings secondaryWindings,
                            CapacitorVoltageDivider capacitorVoltageDivider,
                            IntermediateVoltageTransformer intermediateVoltageTransformer,
                            SpecialDesignInformation specialDesignInformation,
                            TransformerLimits transformerLimits)
    {

        GenerateWindingInstructions(designInformation);

        Design design;
        using (var context = new DesignContext())
        {

            if (!DesignExist(designInformation.DesignID))
            {

                context.DesignInformation.Add(designInformation);
                context.CommonTransformerPropeteries.Add(commonTransformerProperties);
                context.SecondaryWindings.Add(secondaryWindings);
                context.CapacitorVoltageDivider.Add(capacitorVoltageDivider);
                context.IntermediateVoltageTransformer.Add(intermediateVoltageTransformer);
                context.SpecialDesignInformation.Add(specialDesignInformation);
                context.TransformerLimits.Add(transformerLimits);
                context.SaveChanges();
                return;
            }
        }

            using (var newcontext = new DesignContext())
            {
                design = GetDesingByNameFromDatabase(newcontext, designInformation.DesignID, designInformation.Technology);

                UpdateSecondaryWindings(design, newcontext, designInformation, secondaryWindings);

                UpdateSpecialWires(design, newcontext, designInformation, specialDesignInformation);

                newcontext.SaveChanges();
            }
            int dbId = design.DesignInformation.DataBaseID;

            design.DesignInformation = designInformation;
            design.DesignInformation.DataBaseID = dbId;

            design.CommonTransformerProperties = commonTransformerProperties;
            design.CommonTransformerProperties.DataBaseID = dbId;

            design.SecondaryWindings = secondaryWindings;
            design.SecondaryWindings.DataBaseID = dbId;

            design.CapacitorVoltageDivider = capacitorVoltageDivider;
            design.CapacitorVoltageDivider.DataBaseID = dbId;

            design.IntermediateVoltageTransformer = intermediateVoltageTransformer;
            design.IntermediateVoltageTransformer.DataBaseID = dbId;

            design.SpecialDesignInformation = specialDesignInformation;
            design.SpecialDesignInformation.DataBaseID = dbId;

            using (var newcontext = new DesignContext())
            {
                newcontext.Entry(design.CommonTransformerProperties).State = System.Data.Entity.EntityState.Modified;
                newcontext.Entry(design.SecondaryWindings).State = System.Data.Entity.EntityState.Modified;
                newcontext.Entry(design.CapacitorVoltageDivider).State = System.Data.Entity.EntityState.Modified;
                newcontext.Entry(design.IntermediateVoltageTransformer).State = System.Data.Entity.EntityState.Modified;
                newcontext.Entry(design.SpecialDesignInformation).State = System.Data.Entity.EntityState.Modified;
                newcontext.Entry(design.DesignInformation).State = System.Data.Entity.EntityState.Modified;
                newcontext.SaveChanges();
                return;
            }
    }

1) Possibly better structure

Since information from DesignInformation, CommonTransformerProperties, ..., TransformerLimits objects seem to be semantically linked, it should be grouped into a single object type (maybe a Transformer). The EF model should be similar to the class below (greatly simplified).

class Transformer
{
   int TransformerId { get; set; }

   DesignInformation designInformation;
   CommonTransformerProperties commonTransformerProperties,
   SecondaryWindings secondaryWindings,
   CapacitorVoltageDivider capacitorVoltageDivider,
   IntermediateVoltageTransformer intermediateVoltageTransformer,
   SpecialDesignInformation specialDesignInformation,
   TransformerLimits transformerLimits
}

The real gain here is that you can have a single ID to be generated, but you have to change child types to link them to the parent. I.e. DesignInformation, CommonTransformerProperties, ..., TransformerLimits should also have a TransformerId specified. Persistence code should be simpler:

    using (var newcontext = new DesignContext())
    {
        design = GetDesingByNameFromDatabase(newcontext, designInformation.DesignID, designInformation.Technology);

        UpdateSecondaryWindings(design, newcontext, designInformation, secondaryWindings);

        UpdateSpecialWires(design, newcontext, designInformation, specialDesignInformation);

        newcontext.SaveChanges();
    }

    int dbId = design.DesignInformation.DataBaseID;

    design.DesignInformation = designInformation;
    design.DesignInformation.DataBaseID = dbId;

    design.CommonTransformerProperties = commonTransformerProperties;
    design.CommonTransformerProperties.DataBaseID = dbId;

    design.SecondaryWindings = secondaryWindings;
    design.SecondaryWindings.DataBaseID = dbId;

    design.CapacitorVoltageDivider = capacitorVoltageDivider;
    design.CapacitorVoltageDivider.DataBaseID = dbId;

    design.IntermediateVoltageTransformer = intermediateVoltageTransformer;
    design.IntermediateVoltageTransformer.DataBaseID = dbId;

    design.SpecialDesignInformation = specialDesignInformation;
    design.SpecialDesignInformation.DataBaseID = dbId;

should be:

    using (var newcontext = new DesignContext())
    {
        var newTransformer = new Transformer();
        newcontext.Transformer.Add(newTransformer);

        design = GetDesingByNameFromDatabase(newcontext, designInformation.DesignID, designInformation.Technology);
        UpdateSecondaryWindings(design, newcontext, designInformation, secondaryWindings);
        UpdateSpecialWires(design, newcontext, designInformation, specialDesignInformation);

        newTransformer.DesignInformation = designInformation;
        newTransformer.CommonTransformerProperties = commonTransformerProperties;
        // ...

        newcontext.SaveChanges();
    }

EF will automatically push generated identifier to all children entities.

2) Database context usage

Database context should not be disposed so often. Usually Unit of work pattern is used to simplify its usage and also group everything that must be saved together (i.e. in a transaction). More details can be found here and here.

This will spare you from passing the context for calls like:

        design = GetDesingByNameFromDatabase(newcontext, designInformation.DesignID, designInformation.Technology);
        UpdateSecondaryWindings(design, newcontext, designInformation, secondaryWindings);
        UpdateSpecialWires(design, newcontext, designInformation, specialDesignInformation);

Those functions can use the context by just calling DesignContext.Instance method where the method would look like this:

    public static Instance
    {
        get
        {
            // Dirty (non thread-safe) check
            if (UnitOfWorkStore.GetData(DATACONTEXT_KEY) == null)
            {
                lock (objSync)
                {
                    // Thread-safe check
                    if (UnitOfWorkStore.GetData(DATACONTEXT_KEY) == null)
                    {
                        var context = new DesignContext();
                        UnitOfWorkStore.SetData(DATACONTEXT_KEY, context);
                    }
                }
            }
            return (DesignContext)UnitOfWorkStore.GetData(DATACONTEXT_KEY);
        }
    }

An improvement is to using Dependency Injection and to have the context injected. The whole process is explained in great detail here. Of course, other DI frameworks - such as Ninject - can be used.


FFT Frequency Shifter Extravaganza I've been writing a frequency shifter in C++ for an Audio Unit plugin. The best way for me to do this while keeping a large amount of other algorithms working in the same plugin was by writing a separate header file with a full class definition that could handle all things related to the frequency shift. After a few problems with my code were cleared up in this question, I decided to go look for some optimizations and better functionality. I didn't get very far, so here we are. For any people answering my question, I ask that you address the following:

Please keep the arguments to the processBlock function as they are. I am not good enough to in any way revise how audio is supplied to the processing function.
If you've got a better FFT algorithm (you probably do), please implement it! I used an elementary function from Rosetta Code that didn't even utilize all the fancy bitshifting/reversing that it could be. I also know that Apple offers various vDSP functions for FFT, but I am even more clueless on how to implement those.
Of course, optimizations would be nice since the function is called several times a second.
I always appreciate a great lecture on code conventions, especially in C++.

Thanks in advance for any answers. The following is from FrequencyShifter.h:

#include <complex>
#include <valarray>

typedef std::complex<double> Complex;
typedef std::valarray<Complex> CArray;

class FrequencyShifter {
float sampleRate;
public:
    FrequencyShifter() {

    }
    void setSampleRate(float inSampleRate) {
        sampleRate = inSampleRate;
    }
    void fft(CArray& x)
    {
        const size_t N = x.size();
        if (N <= 1) return;

        // divide
        CArray even = x[std::slice(0, N/2, 2)];
        CArray  odd = x[std::slice(1, N/2, 2)];

        // conquer
        fft(even);
        fft(odd);

        // combine
        for (size_t k = 0; k < N/2; ++k)
        {
            Complex t = std::polar(1.0, -2 * PI * k / N) * odd[k];
            x[k    ] = even[k] + t;
            x[k+N/2] = even[k] - t;
        }
    }
    void processBlock(float *inBlock, const int inFramesToProcess, float scale) {
        //inFramesToProcess is the amount of samples in inBlock
        Complex *copy = new Complex[inFramesToProcess];
        for (int frame = 0; frame<inFramesToProcess; frame++) {
            copy[frame] = Complex((double)inBlock[frame], 0.0);
        }
        CArray data(copy, inFramesToProcess);
        fft(data);
        const float freqoffsets = 2.0 * PI/ inFramesToProcess;
        const float normfactor  = 2.0 / inFramesToProcess;
        for (int frame = 0; frame<inFramesToProcess; frame++) {
            inBlock[frame] = 0.5 * data[0].real();
            for (int x = 1; x < data.size() / 2; x++) {
                float arg = freqoffsets * x *f rame*scale;
                inBlock[frame] += data[x].real()*cos(arg) - data[x].imag()*sin(arg);
            }
            inBlock[frame] *= normfactor;
        }
    }
};

The main thing that I feel you should change is the heap allocations inside the processBlock(). With realtime processing like this audio application you never want to call new/delete within time critical code, either yourself (or indirectly like adding a vector, vector.resize() etc. etc.) The reason it's unpredictable how long a new/delete will take.

You have two lines that call new either directly/indirectly

Complex *copy = new Complex[inFramesToProcess];
CArray data(copy, inFramesToProcess); // indirect call of new

The solution is to create work areas beforehand as a member variable. This is created to be the right size at setup time before playback then the processBlock() can write to it as it wants.

So data (now renamed fftData) is outside as a member variable...

Edit: better solution below!

#include <complex>
#include <valarray>

typedef std::complex<double> Complex;
typedef std::valarray<Complex> CArray;

class FrequencyShifter {

float sampleRate;
const int guessInFramesToProcess;
CArray fftData;

public:
    FrequencyShifter()
        : guessInFramesToProcess {512}, // best guess at what the num samples per block will be
                                        // or is there a prepareToPlay function that you can
                                        // send in the blocksize before user hits play?
          fftData {Complex(0.0, 0.0), inFramesToProcess}
    {
    }
    void setSampleRate(float inSampleRate) { // something like this but for blocksize???
        sampleRate = inSampleRate;
    }
    void fft(CArray& x)
    {
        const size_t N = x.size();
        if (N <= 1) return;

        // divide
        CArray even = x[std::slice(0, N/2, 2)];
        CArray  odd = x[std::slice(1, N/2, 2)];

        // conquer
        fft(even);
        fft(odd);

        // combine
        for (size_t k = 0; k < N/2; ++k)
        {
            Complex t = std::polar(1.0, -2 * PI * k / N) * odd[k];
            x[k    ] = even[k] + t;
            x[k+N/2] = even[k] - t;
        }
    }
    void processBlock(float *inBlock, const int inFramesToProcess, float scale) {

        /* now there are no heap allocations in the time-critical processBlock */

        fillFftData(inBlock, inFramesToProcess, fftData);       // function that declares intent

        fft(fftData);

        const float freqoffsets = 2.0 * PI/ inFramesToProcess;  // below is quite hard to figure out
        const float normfactor  = 2.0 / inFramesToProcess;      // your intent. maybe functions
                                                                // like below?
        for (int frame = 0; frame<inFramesToProcess; frame++) {
            inBlock[frame] = 0.5 * fftData[0].real();
            for (int x = 1; x < fftData.size() / 2; x++) {
                float arg = freqoffsets * x *frame*scale;
                inBlock[frame] += fftData[x].real()*cos(arg) - fftData[x].imag()*sin(arg);
            }
            inBlock[frame] *= normfactor;
        }
    }
private:
    // probably good to make little functions like this in your processBlock
    // so you can easier debug later. here's an example
    void fillFftData(float* inBlock, const int inFramesToProcess, CArray& data) {
        for (int frame = 0; frame<inFramesToProcess; frame++) {
            data[frame] = Complex(static_cast<double> (inBlock[frame]), 0.0);
        }                       // static cast ugly but preferred to c-style casts
    }                           // for better compiler checking
};

Now the issue is with how do you tell in advance what the blocksize is going to be i.e. inFramesToProcess ?

The way the code stands now, it may not work because we need some way to let the host tell us the blocksize before we actually start processing the audio in processBlock().

We need this because now the fftData needs to be fully resized the same as the blocksize (and filled with zeros) before during the setup time.

Is there a function that you can hook into that will tell your plugin what the expected blocksize will be?

(Sorry I'm a JUCE user I don't know the specifics of what framework you're using but for example in JUCE there's a function called prepareToPlay(int sampleRate, int expectedBlockSize) that gets called before processing the audio with processBlock(), this would be a perfect place to resize and zero fftData).

Hopefully you can figure this bit out!

Some smaller points

Not sure you need to copy into the buffer then into the fftData? Seems unnecessary. (If you need it, again, allocate the buffer beforehand).
If you split your code into small functions with good names that state the purpose, then it's easier to come back to your code in a few months time :)
Try and avoid C-style casts in C++ as the compiler 'knows' what's going on better with a static_cast.

The main point is really the heap allocations in the processBlock(). Only after it might be time to look at implementing and benchmarking the FFT performance vs. Apple Accelerate vDSP :)

There are other things you should avoid in time critical code too, for more info see this section of this talk...

CppCon 2015: Timur Doumler “C++ in the Audio Industry”

EDIT: Just overcompensate the FFT data buffer.

Sorry I wasn't thinking clearly about the issue previously! The simplest way around this issue is to make fftData larger than you could possibly need i.e. larger than the largest possible block size, then if a smaller block size is actually dealt with in processBlock(), it will only access the start of the buffer. Note how we are now sending the block size through to fft() and now iterating over currentInFramesToProcess (since fftData.size() is overcompensated).

#include <complex>
#include <valarray>

typedef std::complex<double> Complex;
typedef std::valarray<Complex> CArray;

class FrequencyShifter {

float sampleRate;
const int largestPossibleInFramesToProcess;
CArray fftData;

public:
    FrequencyShifter()
        : largestPossibleInFramesToProcess {2048}, // best guess, could add more for safety
          fftData {Complex(0.0, 0.0), largestPossibleInFramesToProcess}
    {
    }
    void setSampleRate(float inSampleRate) {
        sampleRate = inSampleRate;
    }
    void fft(CArray& x, int currentInFramesToProcess)
    {
        const size_t N = currentInFramesToProcess;
        if (N <= 1) return;

        // divide
        CArray even = x[std::slice(0, N/2, 2)];
        CArray  odd = x[std::slice(1, N/2, 2)];

        // conquer
        fft(even);
        fft(odd);

        // combine
        for (size_t k = 0; k < N/2; ++k)
        {
            Complex t = std::polar(1.0, -2 * PI * k / N) * odd[k];
            x[k    ] = even[k] + t;
            x[k+N/2] = even[k] - t;
        }
    }
    void processBlock(float *inBlock, const int inFramesToProcess, float scale) {

        /* now there are no heap allocations in the time-critical processBlock */

        fillFftData(inBlock, inFramesToProcess, fftData);       // function that declares intent

        fft(fftData, inFramesToProcess);

        const float freqoffsets = 2.0 * PI/ inFramesToProcess;  // below is quite hard to figure out
        const float normfactor  = 2.0 / inFramesToProcess;      // your intent. maybe functions
                                                                // like below?
        for (int frame = 0; frame < inFramesToProcess; frame++) {
            inBlock[frame] = 0.5 * fftData[0].real();
            for (int x = 1; x < inFramesToProcess / 2; x++) {
                float arg = freqoffsets * x * frame * scale;
                inBlock[frame] += fftData[x].real()*cos(arg) - fftData[x].imag()*sin(arg);
            }
            inBlock[frame] *= normfactor;
        }
    }
private:
    // probably good to make little functions like this in your processBlock
    // so you can easier debug later. here's an example
    void fillFftData(float* inBlock, const int inFramesToProcess, CArray& data) {
        for (int frame = 0; frame < inFramesToProcess; frame++) {
            data[frame] = Complex(static_cast<double> (inBlock[frame]), 0.0);
        }                       // static cast ugly but preferred to c-style casts
    }                           // for better compiler checking
};

This would get you by for now.

The 'best practise' would be to make your FFT size independent of whatever the processBlock() size is. That way if the user changes the soundcard block settings, they will still hear the same thing. i.e. lock FFT to size 1024, soundcard block size processBlock() variable 64-512

This is accomplished with a circular buffer of size 1024, you fill this up sample by sample with input, then when it's full you call fft() on those 1024 samples.

Hope these are all helpful tips to get started!


How can I make this tax calculator cleaner? I recently created this Income Tax calculator based off your filing status (Single, or Married Filing Jointly) and which tax bracket you fall into for the 2020 tax year, taking into account the Standard deduction. I know the code is a little robust, but the math checks out as compared to several other tax calculators. I first posted it on Stack Overflow, which I now know was not the place for this, but please give any constructive advice you have.

How can I make this cleaner, as in, accomplish the same task with less code? Or if you have a different/more efficient way of writing any of this, I'm also interested.
Update: Thank you everyone for your advice and input! I'll be taking this back to the whiteboard and making the suggested changes as I learn more. Your time and advice is appreciated, immensely!

marital_status = input("Filing Status (M / S): ")
household_income = int(input("household income: "))

if marital_status == "M":
    standard_deduction = "24800"
    # marrital standard deduction
elif marital_status == "S":
    standard_deduction = "12400"
    # single standard deduction

taxable_income = int(household_income) - int(standard_deduction)

married_bracket_caps = [19750, 80250, 171050, 326600, 414700, 622050]
    # the above rates are the max dollar amount in the Married-filing-joint tax bracket(m/j)
single_bracket_caps = [9875, 40125, 85525, 163300, 207350, 518400]
    # the above rates are the max dollar amount in the filing Single tax bracket

if marital_status == "M":
    bracket = married_bracket_caps
elif marital_status == "S":
    bracket = single_bracket_caps
#if M is entered, the future tax rates will abide by the m/j tax rates; #if
    # S is entered than the taxt rates will abide by the Filing Single tax rates.
    #   ie. enter: M, the first tax rate will be "19750", computed as

if taxable_income <= int(bracket[0]):
    tax = taxable_income * .1
elif taxable_income <= int(bracket[1]):
    tax = (int(bracket[0]) * .1)+(.12*(taxable_income - int(bracket[0])))
elif taxable_income <= int(bracket[2]):
    tax = (int(bracket[0]) * .1)+(.12*(int(bracket[1]) - int(bracket[0])))+(.22 * (taxable_income - int(bracket[1])))
elif taxable_income <= int(bracket[3]):
    tax = (int(bracket[0]) * .1)+(.12*(int(bracket[1]) - int(bracket[0])))+(.22 * (int(bracket[2]) - int(bracket[1])))+(.24 * (taxable_income - int(bracket[2])))

print("taxable income: ", taxable_income)
print("owed in Federal tax: ", tax)

Note: The brackets for this calculator only go up four brackets because if I didn't want to do all the way up if I was going to need to rewrite it anyway. Again, thanks!

You have 1 reputation point, and you make no use of functions or complex data structures. Because of this, I'm going to assume you're a beginner to coding.

Congratulations, beginner, on finishing a program and on submitting it for review! You will find that you can learn a huge amount from reviews, probably more stuff faster than by any other method.

Control & Data structures

You don't show any loops or functions, so I won't suggest using them. Yet.

PEP 8

I will suggest that you take a look at PEP-8. It's "THE standard" for writing Python code, so the sooner you start drinking the Kool-Aid, the sooner you'll stop getting criticized for not following it.

CAPS are for constants

One thing you'll find in PEP 8 is the notion of different letter-case conventions for different kinds of name. Variable names that change are generally in what is called snake_case, while "variables" that don't change (that are called "constants" despite the fact that you could change them) are spelled using ALL_CAPS.

In your code, you have some symbols that meet these criteria. Specifically, married_bracket_caps and single_bracket_caps are constants, and should be spelled that way.

You can argue about whether other symbols, that are initialized based on a single choice and never change later, are constant or not. And people do. ;-)

Error Checking

You don't check for errors. At this point, with no way to loop and try again, your only option would be to exit the program. But you should still get in the habit of validating your inputs, especially inputs that come from those pesky humans. Humans are notorious for providing bad inputs. You have this:

if marital_status == "M":
    standard_deduction = "24800"
    # marrital standard deduction
elif marital_status == "S":
    standard_deduction = "12400"
    # single standard deduction

Add this:

else:
    print("Eat flaming death, vile Terry!")
    sys.exit(1)

(Or some other message, if you'd like.)

Don't Repeat Yourself

There is a currently-popular "principle" of programming called "DRY" -- short for "Don't Repeat Yourself." A good programmer has dry code, dry socks, and a dry wit.

In your case, you are repeating a test:

if marital_status == "M":
    standard_deduction = "24800"
...

taxable_income = int(household_income) - int(standard_deduction)

if marital_status == "M":
    bracket = married_bracket_caps
...

Instead of repeating yourself, move the check down until you have all the data you need, and then do the test one time:

if marital_status == "M":
    bracket = MARRIED_BRACKET_CAPS
    standard_deduction = MARRIED_STANDARD_DEDUCTION

DRY your code, redux

Also, you have this:

elif taxable_income <= int(bracket[1]):
    tax = (int(bracket[0]) * .1)+(.12*(taxable_income - int(bracket[0])))

This is bad because it repeats a lot, and because it puts too much work in a single expression. Let's look at it:

tax = bracket-0-cap * some rate + (income - bracket-0-cap) * some other rate

You can dry this out by eliminating a lot of the calls. For example, why are you calling int() on numbers that YOU put into YOUR array as integers? They're already ints, you don't need to do that!

No Magic Numbers

What's .1? What's .12? Those are called magic numbers and they're bad.

Use constants (which you now know should be in ALL_CAPS) to replace these. Or use a lookup of an array (which is constant). Like:

tax = RATE_0 * income

or

tax = TAX_RATES[0] * income


AES-128 Encryption/Decryption of NSData I recently found an Objective-C extension on NSData that encodes and decodes data with AES-128.  I made an attempt to port this code to Swift 2.2:

NSData+AES.swift

extension NSData {

    struct AES {
        static let privateKey = "DefaultPrivateKey"
    }

    func AES128EncryptedDataWithKey(key: String = AES.privateKey) -> NSData? {
        return AES128Operation(CCOperation(kCCEncrypt), key: key)
    }

    func AES128DecryptedDataWithKey(key: String = AES.privateKey) -> NSData? {
        return AES128Operation(CCOperation(kCCDecrypt), key: key)
    }

    private func AES128Operation(operation: CCOperation, key: String) -> NSData? {
        var keyPtr = [CChar](count: kCCKeySizeAES128 + 1, repeatedValue: 0)
        key.getCString(&keyPtr, maxLength: keyPtr.count, encoding: NSUTF8StringEncoding)

        let dataLength = self.length
        let bufferSize = dataLength + kCCBlockSizeAES128
        let buffer = malloc(bufferSize)

        var numBytesEncrypted = 0
        let cryptStatus = CCCrypt(operation, CCAlgorithm(kCCAlgorithmAES128), CCOptions(kCCOptionPKCS7Padding), keyPtr, kCCBlockSizeAES128, nil, self.bytes, dataLength, buffer, bufferSize, &numBytesEncrypted)

        if cryptStatus == CCCryptorStatus(kCCSuccess) {
            // bytesNoCopy will transfer ownership to the NSData object returned, so we don't need to explicitly call free here
            return NSData(bytesNoCopy: buffer, length: numBytesEncrypted)
        }

        // If the operation failed, we never transferred memory ownership, so free the buffer
        free(buffer)

        return nil
    }
}

A few notes:

I keep a default key as a private struct in the extension so the operation function can be called without arguments for encoding/decoding with a default key.
No need to call bzero anymore since we init the buffer with a repeatedValue of 0.
In my first pass of this in Swift, I accidentally was passing in the size of the array (pointer) as the maxLength parameter in getCString (Stack Overflow question here).  Is there any way I could make the code clearer so I don't do that in the future?
Is it necessary to malloc a buffer?  Can I use a Swift [Int8] instead?
According to my boss in a passive comment, AES-128 is "known to be compromised."  How complex would it be to refactor this code to use AES-256 or (192) bit encryption?

Disclaimer: I am not an expert on cryptography or security, so please
take everything with respect to these topics with a grain (or more) of
salt. Note also that there are ready-made crypto libraries (such as
RNCryptor) so you might want
to use one of those.

I do not see the advantage of a default key – only the risk that you use
it for some critical task. I would suggest to remove the default value.

CCCrypt() in encryption mode can only fail if the provided buffer is
too small, or if padding is disabled. Both cases cannot occur in your
code. Therefore there is no need to make the return value of
the encryption method an optional:

func AES128EncryptedDataWithKey(key: String) -> NSData {
    return AES128Operation(CCOperation(kCCEncrypt), key: key)!
}

Your getCString() code is quite clear. There are various methods
to convert a C string to a byte buffer. Here are two alternatives
which don't require the address-of operator &:

var keyData = Array(key.utf8)
if keyData.count < kCCKeySizeAES128 {
    keyData += Repeat(count: kCCKeySizeAES128 - keyData.count, repeatedValue: 0)
}

or

let keyData = key.dataUsingEncoding(NSUTF8StringEncoding)!.mutableCopy() as! NSMutableData
if keyData.length < kCCKeySizeAES128 {
    keyData.length = kCCKeySizeAES128 // fills with zeros
}
// Use `keyData.bytes` to access the bytes.

(Note that these unwrappings/casts cannot fail.) Any of these methods
is fine, use whatever you feel most confident with.

The allocated buffer is "a bit" too large, the required size for
PKCS7 padding is the "next multiple" of the block size:

let bufferSize = (self.length/kCCBlockSizeAES128 + 1) * kCCBlockSizeAES128

The most reliable method is to determine the required buffer size by calling CCCrypt() with an empty buffer:

var bufferSize = 0
let status = CCCrypt(operation, CCAlgorithm(kCCAlgorithmAES128), CCOptions(kCCOptionPKCS7Padding), keyData, kCCBlockSizeAES128, nil, self.bytes, self.length, nil, 0, &bufferSize)
assert(status == CCCryptorStatus(kCCBufferTooSmall))

According to the CCCrypt documentation, this requires only a
"minimal runtime penalty."

Yes, you can use a Swift array instead of malloc():

var buffer = [UInt8](count:bufferSize, repeatedValue: 0)
// ...
return NSData(bytes: buffer, length: dataOutAvailable)

var numBytesEncrypted is misleading, it can be the number of
encrypted or decrypted bytes. CCCrypt() uses dataOutAvailable
for that parameter.

As I understand it from Wikipedia: AES,
AES 128/192/256 differ only in the size of the key, which means
that you have to replace kCCKeySizeAES128 by kCCKeySizeAES192
or kCCKeySizeAES256. (But as I said, I am not an expert on this
topic.)

You have chosen CBC (cipher block chaining) mode and not
ECB mode (as in the referenced Objective-C code) – which is good. But you should also
add an additional parameter to provide an
initialization vector., or the
possibility to generate a random one.
 Currently the corresponding parameter is
nil so that a block of zeros is used.
See – for example – Why use an Initialization Vector (IV)? for more information.


Creation of a temp file on Windows I implemented a test that reads data from temp file on Windows.
I originally had an issue that I solved with the help of this answer

def test_read(self):
    """Test read from file"""
    import tempfile
    import os
    try:
        with tempfile.NamedTemporaryFile(delete=False) as temp:
            content = 'Some data'
            temp.write(content.encode('utf-8'))
            temp.flush()
            self.assertEqual(content, b.read(Path(temp.name)))
    finally:
        os.remove(temp.name)

How can I do this better? Is it ok to have try...finally in the test?

b is a custom module.
It reads data from temp file.

def read(path):
    """Reads file content from FS"""
    if path in VIRTUAL_FS:
       return VIRTUAL_FS[path]
    with open(path.as_posix(), 'r') as file:
       return file.read()

In the try block, temp might not get assigned, so temp.name would be an error. But this is an extremely unlikely scenario.

You seem to be going out of your way to defeat the natural clean-up capability of the named temporary file:

  If delete is true (the default), the file is deleted as soon as it is closed.

Since the file is opened via a with block, it should be closed automatically at the end of the block, and automatically deleted as well, if you let it.  Therefore, you shouldn't need the try...finally at all, and you can remove the delete=False parameter from NamedTemporaryFile:

def test_read(self):
    """Test read from file"""
    import tempfile

    with tempfile.NamedTemporaryFile() as temp:
        content = 'Some data'
        temp.write(content.encode('utf-8'))
        temp.flush()
        self.assertEqual(content, b.read(Path(temp.name)))

When you open a file for reading, you can omit the r in here:

with open(path.as_posix(), 'r') as file:
   return file.read()

Finally, b is terrible name. It doesn't tell anything about the module.


Finding the positive numbers count in an ECMAScipt array All I am trying to achieve is better use of built-in ES functions.

var arr = [0, 1, 2, 3, -1, -2, -3];

function countPositiv(p, c, i, a)
{
  if(c > 0){
    return p+1;
  } else {
    return p;
  }
}

var positiv = arr.reduce(countPositiv, 0);

console.log(positiv);

I don't think there is much to review, it looks ok. I would just do it inline, and there is no need to list arguments you are not using:

var positiv = arr.reduce(function(acc, x) {
  return x > 0 ? acc + 1 : acc;
}, 0);


Mergesort performance in C I have two questions about my code:

Are there any performance improvements in this implementation or generally (register int ?) that I could make?
What could I improve in my coding style?

void mergesortArray(int data[], int amount){

    if(amount == 1) return;

    //Precomputing values
    int sizeint = sizeof(int);
    int amountLeft = amount / 2;
    int amountRight = (amount % 2 == 0) ? amountLeft : (amountLeft + 1);

    //Splitting the array in right and left
    int *left = calloc(amountLeft, sizeint);
    int *right = calloc(amountRight, sizeint);
    if(left == NULL || right == NULL) return;

    //Copying the splitted content
    memcpy(left, data, amountLeft * sizeint);
    memcpy(right, data + amountLeft, amountRight * sizeint);

    //Recursive sorting the splitted arrays
    mergesortArray(left,amountLeft);
    mergesortArray(right,amountRight);

    //Merging the numbers
    int *pos1 = &left[0];
    int *pos2 = &right[0];
    int i = 0;
    for(i = 0; i < amount; i++) {
        if(*pos1 <= *pos2) {
            data[i] = *pos1;
            if (pos1 == &right[amountRight - 1])
                break;
            if(pos1 == &left[amountLeft - 1])
                pos1 = &right[amountRight - 1];
            else
                pos1++;
        }
        else {
            data[i] = *pos2;
            if(pos2 == &right[amountRight - 1])
                pos2 = &left[amountLeft - 1];
            else
                pos2++;
        }
    }
}

The register keyword will do nothing. Current day compilers are smart enough to find a good register allocation themselves without you running in its way.

Why calloc? you initialize the values through memcpy anyway so no need for the zero initialization, malloc will work fine. Speaking of you need to free what you allocate otherwise you'll get a memory leak.

You should only need to allocate a single extra array for the merge.

When the amount left to sort is small (amount < 5) you can switch over to another sort that is more efficient for small numbers like insertion sort.

Finally brace position on your else is a bit odd:

    if(*pos1 <= *pos2) {

    }
    else {

    }

I'd expect either the opening braces to also be on its own line or else to be on the same as the closing brace of the if.


Change example.jpg to example-color.jpg on hover I wrote a script that would change example.jpg to example-color.jpg (.png, .gif would work too) on mouse hover and changes it back on mouse leave. I'm making a portfolio, and I have examples of my work in black and white thumbnails, on hover they switch to colored ones.

I'm just wondering if I did this in a redundant or 'wrong' way. is it poor how I have functions in variable, and to call it as such?

$(function () {
        var hoverImg = function() {
            var imgSrc = $(this).attr('src');
            var newSrc = imgSrc.slice(0, -4) + '-color.jpg';
            var isColor = imgSrc.split("-").pop();
            var notColor = (imgSrc.slice(0, - 10) +'.jpg');

            if ( isColor !== 'color.jpg' ) {
                $(this).attr('src', newSrc);
            } else {
                $(this).attr('src', notColor);
            }
         }
        var captionImg = function() {
            $(this).children('.gallery-caption').toggleClass('up');
         }

  $('.gallery').on('mouseenter mouseleave', '.gallery-item img', hoverImg);
 $('.gallery').on('mouseenter mouseleave', '.gallery-item', captionImg);

});

On another approach, you could do spriting. One problem you'll encounter with that approach, where you change src is that the image will take some time to load before the hover image appears. That's annoying. With sprites, you have one image just positioned differently on a container. There's no loading that will happen on hover.

See this example. And the best part is... no JS!

HTML:

CSS:

/* Class for setting the image */
.image {
    background-image: url("http://www.alexmourer.com/wp-content/uploads/2013/09/css-sprite.png");
    display: block;
    width: 77px;
    height: 77px;
}

/* class for positioning to the right image (x and y) */
.image.twitter{
   background-position: 0 0;
}

.image.facebook{
   background-position: 308px 0;
}

/* class to offset the image on hover, in this case, up */
.image:hover{
    background-position-y: -154px;
}


Finds a Character based on distance and health I need to improve the efficiency of my program and, using the profiler, have narrowed the problem down to 2 key areas, but I am having trouble coming up with ways to make the program run better.

Based on my profiler's report, it seems to be telling me that my if functions are inefficient. Whats a better way to achieve a better result?

Character* FindAttackTarget() const
        {
            float weakestHp = FLT_MAX;
            Character* weakestEnemy = nullptr;
            uint64_t weakestCharId = INT64_MAX;

            //Only attack characters that are within attack range
            auto& gameChars = m_pGame->m_gameCharacters;

            for (size_t i = 0; i < gameChars.size(); ++i)
            {
                auto& c = gameChars[i];
                if (Location.Dist(c.GetLocation()) <= AttackRange &&
                    c.HP > 0 &&
                    c.Team != Team)
                {
                    //We want the weakest with the lowest ID number - this keeps consistent results when re-playing the same part of the game (eg. after a load game)
                    if (c.HP < weakestHp || (c.HP == weakestHp && c.ID < weakestCharId))
                    {
                        weakestEnemy = &gameChars[i];
                        weakestHp = c.HP;
                        weakestCharId = c.ID;
                    }
                }
            }

            return weakestEnemy;
        }

The tests c.HP > 0 and c.Team != Team are probably blazingly fast tests.  Location.Dist(c.GetLocation()) <= AttackRange probably involves the square-root of the sum of the squares of the difference of coordinates in two or three dimensions. Plus, GetLocation() may involve memory allocation and/or copying constructors.  It is by far the slowest test, yet you are testing it first!  Take advantage of the short-circuit logical and operators by reordering the tests so the fastest tests are done first, so the slowest test may not even need to be executed, resulting in faster execution.

Since all conditions must pass before you update weakestEnemy, you could even test whether or not the target passes the “weakest with lowest ID” test before checking the attack range.

Bonus: the square-root can be avoided; simply compute the square distance, and compare against the \$AttackRange^2\$ (computed outside of the loop) for another speed gain.