Populating a ListView with Items I populate a ListView with Items. Each Item has data attached to its .Tag property.

Over time, I have needed to handle many cases where something would be null or missing or corrupt or whatever.

And so was born this beast:

private void PlayRecording()
{
    if (list.Items.Count >= 1)
    {
        if (list.SelectedItems.Count >= 1)
        {
            if (((Item)list.SelectedItems[0].Tag).Attachments.Count >= 1)
            {
                if (((Item)list.SelectedItems[0].Tag).Attachments[0].AudioStreams.Count >= 1)
                {
                    if (((Item)list.SelectedItems[0].Tag).Attachments[0].AudioStreams[0].AudioStream != null)
                    {
                        byte[] recording = ((Item)list.SelectedItems[0].Tag).Attachments[0].AudioStreams[0].AudioStream;

                        if (recording != null)
                        {
                            using (MemoryStream memoryStream = new MemoryStream(recording))
                            {
                                if (memoryStream != null)
                                {
                                    soundPlayer.Stream = memoryStream;
                                    soundPlayer.Play();

                                    AudioState = AudioStates.Playing;
                                }
                                else status.Text = "The Memory Stream returned is null. The stream may be corrupt or non-existent.";
                            }
                        }
                        else status.Text = "Notes is unable to play the recording.";
                    }
                    else status.Text = "The selected item's Audio Stream is null. The recording may be corrupt or missing.";
                }
                else status.Text = "The selected item does not contain any recordings to play.";
            }
            else status.Text = "The selected item does not contain any attachments.";
        }
        else status.Text = "You haven't selected an item in the list.";
    }
    else status.Text = "There are no items in the list.";
}

Yeah, sure, I could just get rid of the if's and else's and replace ((Item)list...) with Item item = list... and wrap it all in a single Try{}Catch{}, but I really do hate using Try{}Catch{} unless there is something I am not expecting. And to be honest, using Try{} Catch{} makes me feel lazy and like I'm not doing it properly.

Is there any way I can improve this code and make it a little smaller (while still handling all of the cases where I would need to convey a message)?

Yeah, sure, I could just get rid of the if's and else's and replace ((Item)list...) with Item item = list... and wrap it all in a single Try{}Catch{}, but I really do hate using Try{}Catch{} unless there is something I am not expecting. And to be honest, using Try{} Catch{} makes me feel lazy and like I'm not doing it properly.

That is true. The best exception is the exception which will be avoided.

That beeing said, I need to tell you that you are using the arrow antipattern here.

How about reversing the conditions by using guard clauses and return early like

if (list.Items.Count == 0)
{
    status.Text = "There are no items in the list.";
    return;
}
if (list.SelectedItems.Count == 0)
{
    status.Text = "You haven't selected an item in the list.";
    return;
}

........

or maybe add a method which returns a string for a not satisfied condition and an empty string for a satisfied condition like so you can place these guard clauses in a separate method.

If I read recording I think about a boolean. Maybe it is only me, but I would rename the variable to audioContent or recordedContent to be more clear.

Avoiding braces {} for single instruction ifand else can lead to serious problems. I would like to encourage you to always use them.

Btw, your method is doing too many things and is therefor violating the single responsibiliy principle. The method is validating conditions, updating the GUI and also playing the audio stream.


Login functionality with enum or bool I'm re-designing the login functionality for our application, and the first thing I came across is something a previous developer wrote a while back. There's a simple Login class that just contains 2 strings, username and password, and an enum LoginStatus:

public enum LoginStatus
{
    LoggedIn,
    LoggedOut
}

I'm thinking it would be better to just use a bool here, but maybe there's something more here that I'm not understanding?

In my opinion, it is slower and more confusing to read:

if (login.LoginStatus == LoginStatus.LoggedIn)
{
    ...
}

It would seem a lot better to just write:

if (login.loggedIn) { ... }

or

if (!login.loggedIn) { ... }

I already tested it in my branch and changed everything accordingly. Everything still works normally, so the new Login class looks like this:

public class Login
{
    public string username { get; set; }
    public string password { get; set; }
    public bool loggedIn { get; set; }
}

Is there any specific reason one would want to use enum over bool in this instance? Performance, readability, ease of future expansions, or any other overhead my young mind may not be seeing?

I find enums are ofte easier to use then booleans. At some point you probably will want to search for users so consider this:

User FindUserByStatus(LoginStatus status)

and its usage

userSearch.FindUserByStatus(LoginStatus.LoggedIn)

vs

User FindUserByStatus(bool loginStatus)

and its usage

userSearch.FindUserByStatus(true);

but even with the enum you could do

if (login) { .. }

by adding an implicit cast to the Login

public static implicit operator bool(Login login) => login.Status == LoginStatus.LoggedIn;


Localized Enum in C# 6 I had to jump through some hoops to get a localized enum working. I went through dozens of SO articles that never actually answered the relevant conundrum with a working solution. So I made my own:

I have two resource files: LocalizedStrings.resx and LocalizedStrings.es-MX.resx
With two corresponding lines:

GENDER_FEMALE   Female

GENDER_MALE     Male

... and ...

GENDER_FEMALE   Mujer

GENDER_MALE     Hombre

… respectively.

I have an Enum Extension:

public static class EnumExtensionMethods
{
    public static string GetDisplay(this Enum enumValue)
    {
        object[] attr = enumValue.GetType().GetField(enumValue.ToString())
            .GetCustomAttributes(typeof(DisplayAttribute), false);

        return attr.Length > 0
           ? ((DisplayAttribute)attr[0]).GetName()
           : enumValue.ToString();
    }
}

My actual Enum:

public enum Gender
{
    [Display(ResourceType = typeof(LocalizedStrings), Name = "GENDER_MALE")]
    Male = 1,
    [Display(ResourceType = typeof(LocalizedStrings), Name = "GENDER_FEMALE")]
    Female = 0
}

I load the values into a Winforms ComboBox’s items, (I could not get binding it to an entity to work) with the OnLoad event:

foreach (Gender value in Enum.GetValues(typeof(Gender)))
{
    GenderComboBox.Items.Add(new KeyValuePair<string, Gender>(value.GetDisplay(), value));
}

GenderComboBox.DisplayMember = "Key";
GenderComboBox.ValueMember = "Value";
GenderComboBox.SelectedIndex = 1;

Finally, I have to set the entity value with this event:

private void GenderComboBox_SelectedIndexChanged(object sender, EventArgs e)
{
    if (!GenderComboBox.Focused)
        return;

    if (GenderComboBox.SelectedIndex == -1)
        return;

    KeyValuePair<string, Gender> gender = (KeyValuePair<string, Gender>) GenderComboBox.SelectedItem;

    Person person = (Person)PersonBindingSource.Current;

    person.Gender = gender.Value;
}

All of the previous code just to get a Localized Enum. Is there not a better way?

You really shouldn't mix event and data-binding together.

The proper way of updating value from UI, would be using ComboBox.DataBindings.Add(Binding). And, not patching it via ComboBox.SelectedSomethingChanged.

Here is how I would do it :

// mock model
var person = new Person { Gender = Gender.Female };;

// mock data source
var personBindingSource = new BindingSource();
personBindingSource.DataSource = new List<Person> { person };

// initialize
var combo = new ComboBox();
// you may want to set DropDownStyle to ComboBoxStyle.DropDownList
// from the designer to disable text input.
//
// choose one of the two below :
//    combo.DisplayMember = "Value";
//    combo.ValueMember = "Key";
//    combo.DataSource = EnumHelper.GetEnumDefinitions<Gender>();
combo.BindTo<Gender>();

// databind
combo.DataBindings.Add(new Binding(
    nameof(combo.SelectedValue), personBindingSource, nameof(person.Gender), false, DataSourceUpdateMode.OnPropertyChanged));

public static class EnumHelper
{
    public static IList GetEnumDefinitions<TEnum>()
        where TEnum : struct, IConvertible
    {
        var type = typeof(TEnum);
        if (!type.IsEnum)
            throw new ArgumentException($"'{type}' is not an enum type.");

        return type
            .GetFields(BindingFlags.Public | BindingFlags.Static)
            .ToDictionary(x => (TEnum)x.GetValue(null), x =>
                x.GetCustomAttribute<DisplayAttribute>()?.GetName() ?? x.GetValue(null).ToString()
            )
            .ToList();
    }

    public static void BindTo<TEnum>(this ComboBox combo)
        where TEnum : struct, IConvertible
    {
        combo.DisplayMember = "Value";
        combo.ValueMember = "Key";
        combo.DataSource = EnumHelper.GetEnumDefinitions<TEnum>();
    }
}


Optimize a parallelized program to optimize train schedules This loops through a vector of vector of A and adds a smaller array to a bigger one at a specified position+random shift.  Then it is checked if it is a better solution, and if yes, it is stored, else it is discarded.  I have already parallelized it.

Can this be improved to run faster?

I am not sure if a vector<vector<struct>> is a good idea, or maybe there is a better memory management to use better cache effects.  The struct is currently global and since it is only readable by the threads all threads use the same instance.

I am using C++11 threads.

Description of what the program does:

We have a big array, rand_val, and we have many small arrays, Leistungskurve. We can add these  array to rand_val at a fixed position + random shift. We want to find a valid position for each array so the max element in rand_val is minimized. In the array, rand is stated which position we have chosen.

#include <iostream>
#include <string>
#include <vector>
#include <time.h>
#include <cmath>
#include <iterator>

#include <algorithm>

#include <fstream>
#include <stdlib.h>
#include <sstream>

#include <stdlib.h>

#include <thread>
#include <random>

using namespace std;

struct Teilstrecke {
  int StreckenNr;
  int Fahrzeit;
  int Haltezeit;
  int FruehesteAbfahrt;
  int SpaetesteAbfahrt;
  int Abfahrtsintervall;
  double *Leistungskurve;
};

struct Zug {
  int ZugNr;
  int AnzahlStrecken;
  vector<Teilstrecke> Teilstrecken;
};

vector<Zug> Zuege;

int Planungsintervall;
int AnzahlZuege;
int AnzahlSchattenzuege;

inline double maxArray(double *arr, int size)
{
  double max = 0;
  int pos = 0;
  for (int i = 0; i < size; i++)
  {
    if (arr[i] > max)
    {
        max = arr[i];
        pos = i;
    }
  }
  return pos;
}

inline void addArray(double *arr1, double *arr2, int size2, int pos)
{
  for (int i = 0; i < size2; i++)
  {
    arr1[pos+i] += arr2[i];
  }
}

inline void addArray(double *arr1, Teilstrecke ts, int shift)
{
    addArray(arr1, ts.Leistungskurve, ts.Fahrzeit*600+1, (ts.FruehesteAbfahrt+shift)*600);
}

inline void randomSolution(int *rand, double *rand_val);

inline int randomNumber(int low, int high) // has to be replaced to c++11 random
{
  return rand() % (high - low + 1) + low;
}

void wrapper()
{
  int size = Planungsintervall*600+1;
  int *rand = new int[AnzahlSchattenzuege];
  fill_n(rand, AnzahlSchattenzuege, 0);
  double *rand_val = new double[size];
  fill_n(rand_val, size, 0);
  int max_iter = 1000000;

  double best_max = 1e20;
  int *rand_best = new int[AnzahlSchattenzuege];
  for (int i = 0; i < max_iter; i++)
  {
    randomSolution(rand, rand_val);
    double max = maxArray(rand_val, size);
    //cout << max << '\n';
    if (max < best_max)
    {
      best_max = max;
      memcpy(rand_best, rand, sizeof(int)*AnzahlSchattenzuege);
    }
    fill_n(rand, AnzahlSchattenzuege, 0);
    fill_n(rand_val, size, 0);
  }
  cout << best_max <<  ' ' << '\n';
}

inline void randomSolution(int *rand, double *rand_val)
{
  int pos2 = 0;
  for (int i = 0; i < AnzahlZuege; i++)
  {
    for (int j = 0; j < Zuege.at(i).AnzahlStrecken; j++)
    {
      Teilstrecke ts = Zuege.at(i).Teilstrecken.at(j);
      int k = randomNumber(0, ts.Abfahrtsintervall);
      rand[pos2+k] = 1;
      addArray(rand_val, ts, k);
      pos2 += ts.Abfahrtsintervall + 1;
    }
  }
}

void read_input(char *read_file);

int main(int argc, char** argv)
{
  read_input((char*)argv[1]);
  vector<thread> threads;
  for (int i = 0; i < 7; i++)
  {
    threads.push_back(thread(wrapper));
  }
  wrapper();
  for (int i = 0; i < 7; i++)
  {
    threads.at(i).join();
  }
}

Profiling

It seems that all work is done in addArray.

Do not use using namespace std as it could cause clashing issues, which will introduce bugs.  More info about that here.
This is very unorganized:

#include <iostream>
#include <string>
#include <vector>
#include <time.h>
#include <cmath>
#include <iterator>

#include <algorithm>

#include <fstream>
#include <stdlib.h>
#include <sstream>

#include <stdlib.h>

#include <thread>
#include <random>

Steps to improve this:

Make sure there aren't any libraries that you aren't currently using or have repeated.
Make sure you aren't using any C libraries (which end with .h).  They should be replaced with the equivalent C++ library so that everything will be in the std namespace.
Sort them in a certain order for organization, such as alphabetically.  This will make it easier to locate certain libraries during maintaining.

Assuming every library is still being used, this is what it could look like:

#include <algorithm>
#include <cmath>
#include <cstdlib>
#include <ctime>
#include <fstream>
#include <iostream>
#include <iterator>
#include <random>
#include <sstream>
#include <string>
#include <thread>
#include <vector>

This is one #include shorter because you've included <stdlib.h> twice.  Had this list been organized from the beginning, this wouldn't have been likely to happen.
Do not use global variables:

vector<Zug> Zuege;

int Planungsintervall;
int AnzahlZuege;
int AnzahlSchattenzuege;

They are accessible throughout the entire file or program, and can possibly be modified without your knowledge.  This can greatly hurt maintainability and also introduce bugs.  You should keep them in local scope, such as in a function, and pass them to other functions as needed.  This will make it much easier to keep track of their locality and determine how they're used.
The inline keyword likely won't offer you any benefit here.  The compiler is free to decide what should really be inlined and can ignore any explicit use of it.  It merely serves as a hint.
There is absolutely no need to call std::rand() when you're using C++11 and have already included <random>.  You should utilize this library and ditch std::rand() altogether.

Regarding the usage of std::rand() itself, you don't even call std::srand(), meaning you'll get the same random values with each call to std::rand().  Have you not even noticed this?
Why are you mixing std::vectors and dynamically-allocated arrays?  Vectors are the same type of array, but much more specialized.  You should ditch all the arrays and just use vectors (or any suitable storage container).  You end up passing these arrays (specifically the pointer to their first respective elements), which can get very nasty in C++, especially because memory-management will now be in your hands.  Just utilize storage containers and let them handle the dirty work for you (which is very necessary, as you fail to deallocate memory with delete, causing the whole program to leak memory).  They will also allow you to ditch the size variables since storage containers already keep track of their sizes (accessible via size()).
Since you use argv[1] in main() as the only command line argument, I assume that you're only expecting one additional argument.

In order to make sure that this argument is provided, check the value of argc at the start, before using argv[1].  If it's less than 2, terminate the program.  You may also print an error message to inform the user of this error.

if (argc < 2)
{
    std::cerr << "A filename argument is required!";
    return EXIT_FAILURE;
}

To sum it up:

This is quite broken and messy.  I'm sure there are other issues besides the ones I've mentioned, but this should address many of them.  Before worrying about optimization, I'd recommend cleaning this up and making sure it runs smoothly and correctly.


Entity Framework Skip and Take too slow The following code achieves what I require, reading 10000 rows at a time:

var query = repository.Subscribers.ToList().Skip(numberToSkip).Take(numberInBatch);

Is there an alternative which will make this faster?

Here's the full code:

protected static string ContactUrl = BaseUrl + "/api/contact";

protected static int NumberInBatch = 10000;
protected static int NumberToSkip = 10000;
protected static int NumberToCountUpto = 100000;

public static string GetSubscribers(int numberToSkip, int numberInBatch)
    {
        using (var repository = new SubscriberEntities())
        {
            var index = 0;
            string output = null;
            var query = repository.Subscribers.ToList().Skip(numberToSkip).Take(numberInBatch);

            foreach (var subscriber in query)
            {
                var telephone = ContactHelper.GetTelephoneFromEmail(subscriber.EmailAddress);

                string requestCreateContact = RequestHelper.CreateContact(subscriber.EmailAddress, telephone);
                if (index > 0) output = output + ",";
                output = output + (requestCreateContact);

            }
            return output;
        }
    }

    //Batch Create
    public static void SendBatchCreate()
    {

        for (int i = NumberToSkip; i < NumberToCountUpto; i += NumberInBatch)
        {
            var requestCreateMultipleContacts = GetSubscribersAndCreateContacts(i, NumberInBatch);

            RequestHelper.SubmitPostRequest(requestCreateMultipleContacts, ContactUrl);
        }
        Console.ReadKey();
    }

I would first suggest logging the database operation to see what SQL is executed.  EF may not be running the SQL you think it is.

The red flag I see is that you call ToList before the Skip and Take.  Generally in LINQ statements, ToList forces execution immediately.

This suggests your EF code is pulling every record from the table up front, building a list, and then applying the Skip/Take to the list.  If that's the case, the logged SQL should show a select statement without any filtering.

I suspect what you really want to do is run the ToList after the Skip and Take.  This should apply the filtering inside the executed SQL, but the logging will show you for sure.


Find if value exists using SQL True/False Return Aim: To Assess if one, or more, examples of a value exists in a Database in the quickest time as I only needs a True/False result.

The variable is Alphanumeric.

Question: Is this the quickest and best way to do this?

Public Function PrcCheckIfValueExists(vVariable As String) As String
        'Here we check if the Value Exists in the database
        Try
            Dim ConnectionString As String = System.Configuration.ConfigurationManager.ConnectionStrings("SQLLocal").ToString()
            Using connection = New SqlConnection(ConnectionString)
                Using command As New SqlCommand("SELECT TOP 1 ID FROM TblTable WHERE ID= '" & vVariable & "'", connection)
                    connection.Open()
                    Dim result = command.ExecuteScalar()
                    connection.Close()
                    If result = "" Then
                        Return False
                    Else
                        Return True
                    End If
                End Using
            End Using

        Catch ex As Exception
            Return False
        End Try

    End Function

This code is open to injection. Consider
what happens if I pass sometext'; Drop Table TblTable; -- into
that variable. Use a proper parameterized query instead.
The SQL statement itself is about as efficient as it can be to my
knowledge. A count would add extra overhead. Using an Exists might
garner you a negligible amount of performance at the cost of
readability. (Exists wouldn't need to return any value at all
except what you tell it.)
You could simplify your return statement by using IsNullOrEmpty()

Instead of:

                If result = "" Then
                    Return False
                Else
                    Return True
                End If

You could just use this:

Return Not String.IsNullOrEmpty(result)


Searching for a word in a list of tuple extracted from a db via SQL Python 3.6 and Sqlite3.

I have a database with something like 300K entries, each entry has several fields (date, username, etc) and a "text" one.  I have a list of 200 words and for each word i have to return the number of entry in the database with the word in the "text" field.
Currently, it takes around 1 second per word to do that. It's fine for 200 words but the list and the db will grow up in the future so i would like to optimize the speed.
Here is how i proceed for now (I won't paste the entire code which is a bit too long but important part but if you need more i'll do it !).

I extract the data from the db with this line and i obtain a list of tuple, each tuple is a row in the db :

db = cursor.execute("select * from mydata").fetchall()

Then I loop on the list of word and i'm looking for the word in the db list like this (row[6] is my "text" field):

for word in wordList:
    for row in db:
        hasFound = re.compile(r"\b"+word.lower()+"\\b")
        if hasFound.search(row[6]):
            compteur += 1
    dicoVolume[word] = compteur

Is there a way to search faster ?

You could try doing the filtering at the database level. Something like:

sql = "SELECT COUNT(*) FROM mydata WHERE myTextColumn MATCH %s"
for word in wordList:
    db = cursor.execute(sql, word).fetchall()
    dicoVolume[word] = db[0][0]

This would have the same word boundary caveats mentioned by Ludisposed, but it would save retrieving the entire table. Whether it's faster will probably depend on the relative sizes of the table and the word list, and if the column has an index it can use.

Edit: To use full text indexes with SQLite, it seems you need to create the table as an FTS table, and then do your searches using the MATCH command:

CREATE VIRTUAL TABLE mydata USING fts3(content TEXT);
INSERT INTO mydata VALUES ('some test data');
SELECT count(*) FROM mydata WHERE content MATCH 'test';

SQL Fiddle: http://sqlfiddle.com/#!7/fd6c5/8


Tornado PyMongo product wishlist I am writing a Tornado app in which a user can maintain his products wishlist. There are two collections, users and products. As the names imply, the users collections will contain users info and the products collections will contain the URLs.

Typically a user document in users collection will contain the following fields:

email_id
name
tracked_products

The tracked_products here is a list which maintains the wishlist. It contains the ObjectId mapped to URLs in products collection. To make it clear, when two users add the same product URL, in their respective user documents instead of containing the URL, they will have the ObjectId of this URL, which is the ID of the URL in the products collection.

To find tracked_products of a specific user:

    search_user = user_db.find_one({'email_id': user_email})
    search_user['tracked_products']

above will return to me list of ObjectIds.

To add a product URL to the user's wishlist:

product_doc = product_db.find_one({'url': url})
if not product_doc:
    # code needs to be added here to get product name from url
    product_id = str(product_db.insert({'product_name': 'iPad', 'url': url})
else:
    product_id = product_doc['_id']
user_db.update({'email_id': user_email}, {'$addToSet': {'tracked_products': ObjectId(product_id)}})

The following is the code when user wants to remove a URL from his wishlist. Do note that he will send the ObjectId instead of the URL, he intends to remove. This may seem less intuitive. The user will actually never know the ObjectIds, but the page rendered to him which displays the wishlist, will make call when user wants to delete the URL (am I clear here?):

user_db.update({'email_id': user_email}, {'$pull': {'tracked_products': ObjectId(product_id)}})

And here is my actual Tornado app code. I am not using any templating as of now, will add that later. Also ignore the repeated user_db = self.application.db.users statement which is present in all handlers (which should not, may app can have this variable). And one more thing: I access the current logged in user's email ID every time by a cookie.

Is there any way to avoid this? Can I have some global variable which stores user email ID every time the user is logged in?

class ProductsDisplayHandler(BaseHandler):
    @tornado.web.authenticated
    def get(self):
        user_email = self.get_secure_cookie('trakr')
        user_db = self.application.db.users
        search_user = user_db.find_one({'email_id': user_email})
        self.write(str(search_user.get('tracked_products', None)))

class ProductAddHandler(BaseHandler):
    @tornado.web.authenticated
    def get(self, url):
        user_email = self.get_secure_cookie('trakr')
        user_db = self.application.db.users
        product_db = self.application.db.products
        product_doc = product_db.find_one({'url': url})
        if not product_doc:
            product_id = str(product_db.insert({'product_name': 'iPad', 'url': url}))
        else:
            product_id = product_doc['_id']
        user_db.update({'email_id': user_email}, {'$addToSet': {'tracked_products': ObjectId(product_id)}})

class ProductDelHandler(BaseHandler):
    @tornado.web.authenticated
    def get(self, product_id):
        user_email = self.get_secure_cookie('trakr')
        user_db = self.application.db.users
        user_db.update({'email_id': user_email}, {'$pull': {'tracked_products': ObjectId(product_id)}})
        self.redirect('/products')
        return

You can try editing your BaseHandler like this

class BaseHandler(tornado.web.RequestHandler):
    def prepare(self):
        self.user_email = self.get_secure_cookie('trakr')
        self.user_db = self.application.db.users

and then call the  following in your other Handlers

self.userdb.update({'email_id': self.user_email},

Basically the prepare function is called right at the beginning of the request
for more information you can look at the tornado docs


Autofill a table based on the input provided in a userform Good morning,

I have the following code:

Option Explicit

 Private Sub CommandButton1_Click()
 Dim LastRowA As Long, LastRowB As Long, LastRowC As Long, lastrowD As Long, LastrowE As Long,
 LastrowF As Long, LastrowG As Long, LastrowH As Long, LastrowI As Long, LastrowK As Long, LastrowL
 As Long
 Dim ws As Worksheet

 Set ws = Sheets("Sheet1")

LastRowA = ws.Range("A" & Rows.Count).End(xlUp).Row + 1 'Finds the last blank row
LastRowB = ws.Range("B" & Rows.Count).End(xlUp).Row + 1
LastRowC = ws.Range("C" & Rows.Count).End(xlUp).Row + 1
lastrowD = ws.Range("D" & Rows.Count).End(xlUp).Row + 1
LastrowE = ws.Range("E" & Rows.Count).End(xlUp).Row + 1
LastrowF = ws.Range("F" & Rows.Count).End(xlUp).Row + 1
LastrowG = ws.Range("G" & Rows.Count).End(xlUp).Row + 1
LastrowH = ws.Range("H" & Rows.Count).End(xlUp).Row + 1
LastrowI = ws.Range("I" & Rows.Count).End(xlUp).Row + 1
LastrowK = ws.Range("K" & Rows.Count).End(xlUp).Row + 1
LastrowL = ws.Range("L" & Rows.Count).End(xlUp).Row + 1

ws.Range("A" & LastRowA).Value = JobID.Text 'Adds the TextBox3 into Col A & Last Blank Row
ws.Range("B" & LastRowB).Value = Surveyor.Text
ws.Range("C" & LastRowC).Value = DateBox.Text
ws.Range("D" & LastRowC).Value = AddressBox.Text
ws.Range("E" & LastrowE).Value = CityBox.Text
ws.Range("F" & LastrowF).Value = PostcodeBox.Text
ws.Range("G" & LastrowG).Value = THPBox.Text
ws.Range("H" & LastrowH).Value = ChamberBox.Text
ws.Range("I" & LastrowI).Value = ORFibreBox.Text
ws.Range("K" & LastrowK).Value = ("=I" & LastrowI & "*5")

 End Sub

which helps me to autofill the table columns based on the user form.

The code is valid, but I am wondering how can I write it smarter. Is it possible at all?

Whenever you find yourself writing a block of nearly identical code, I recommend creating a common function that simplifies and isolates that functionality. The advantage is that you now have a single location for the logic and can easily make a change that is consistent for each instance you need to use that logic. In your case, you are repeatedly finding the last row in a column and adding a value to the first empty row in that column. So create a separate routine for that.

You have three tricky parts. The first is that you're not always adding a value. In at least one case, you want to add a formula. For this, I created a simple enumeration type as an optional parameter. The second is you wanted to use the row index in your formula, so that row index value is returned from the function to use if needed. The last tricky bit is adding a value to a column that is completely empty. You can't blindly always add one to the lastRow value because you'll end up skipping that top row. This isn't always a problem since many folks reserve the first row for a column header, but the function takes this into account to be thorough.

Option Explicit

Private Enum DataType
    DataValue
    FormulaValue
End Enum

Private Sub CommandButton1_Click()
    AppendToColumn Sheet1, "A", JobID.Text  'Adds the TextBox3 into Col A & Last Blank Row
    AppendToColumn Sheet1, "B", Surveyor.Text
    AppendToColumn Sheet1, "C", DateBox.Text
    AppendToColumn Sheet1, "D", AddressBox.Text
    AppendToColumn Sheet1, "E", CityBox.Text
    AppendToColumn Sheet1, "F", PostcodeBox.Text
    AppendToColumn Sheet1, "G", THPBox.Text
    AppendToColumn Sheet1, "H", ChamberBox.Text
    Dim lastRowI As Long
    lastRowI = AppendToColumn(Sheet1, "I", ORFibreBox.Text)
    AppendToColumn Sheet1, "K", ("=I" & lastRowI & "*5"), FormulaValue
End Sub

Private Function AppendToColumn(ByRef ws As Worksheet, _
                                ByVal columm As Variant, _
                                ByVal value As Variant, _
                                Optional ByVal kindOfValue As DataType = DataValue) As Long
    '--- copies the given value to the first empty cell in the
    '    specified column. the "columm" value can be either numeric
    '    or alphabetic. RETURNS the index of the last row
    Dim colIndex As Long
    Dim lastRow As Long
    Dim firstEmptyRow As Long
    With ws
        '--- quick conversion to make sure we have a numeric column index
        colIndex = IIf(IsNumeric(columm), columm, .Cells(1, columm).column)
        lastRow = .Cells(.Rows.Count, colIndex).End(xlUp).Row
        '--- if the column is completely empty, the first empty row is 1,
        '    otherwise it's one row down from the last row
        firstEmptyRow = IIf(IsEmpty(.Cells(lastRow, colIndex)), 1, lastRow + 1)
        Select Case kindOfValue
        Case DataValue
            .Cells(firstEmptyRow, colIndex).value = value
        Case FormulaValue
            .Cells(firstEmptyRow, colIndex).Formula = value
        End Select
    End With
    AppendToColumn = lastRow
End Function


Remove all occurrences of a digit from a number I need to remove all occurrences of a digit from a number in a recursive function with this prototype:

unsigned rem(unsigned num, unsigned dig);

This is my solution:

#include <stdio.h>

// just returns 10 to the power of n
int power(int n)
{
    if (n==0)
        return 1;

    int res=1;

    for (int i=0; i<n; i++)
        res *= 10;

    return res;
}

static int i=0;

unsigned rem(unsigned num, unsigned dig)
{
    int tmp = power(i);

    if (num == 0)
        return 0;
    else
    {
        if (num%10 != dig)
        {
            i++;
            return (num%10)*tmp + rem(num/10, dig);
        }
        else
            return rem(num/10, dig);
    }

}

int main()
{
    unsigned n, res;

    scanf("%u", &n);

    res = n;

    for (int j=2; j<10; j+=2)
    {
        res = rem(res, j);
        // reset
        i=0;
    }

    printf("%u without even numbers: %u\n", n, res);
    return 0;
}

It works but I've read a lot about how a global static variable is not a good solution, so I was thinking if there is a way without it.

I've had other solutions (with a local static variable), but all fail when I need to call the function more than once.

Also, the only reason for using a static variable, is to keep tract of what the weight of the next digit of the result.

Yes, the code can be simplified quite a bit, and in the process the static/global variable eliminated.

At least to me, an obvious implementation would be something like this:

unsigned rem(unsigned num, unsigned dig)
{
    if (num == 0)
        return num;

    unsigned digit = num % 10;

    unsigned temp = rem(num / 10, dig);

    if (digit != dig)
        temp = temp * 10 + digit;

    return temp;
}

So we basically split the number into the least significant digit, and everything else. We do the recursive call on the "everything else" part. Then we look at the least significant digit. If it's to be removed, we just leave it off the result. If it's not to be removed, we multiply the return value by 10, then add our digit on as the least significant again.

Of course, variations in syntax are possible. For example, you could use something like:

temp = rem(num / 10, dig);

return dig != digit ? temp * 10 + digit : temp;

That makes it shorter in terms of the number of lines of code, but it's essentially identical in terms of the real work being done.


Calculate the nth Fibonacci number, mod m I was trying to output F(n) mod m to find the remainder of Fibonacci number, where F(n) could be a very big number:

# Uses python3
n, m =  [int(x) for x in input().split()]

def fib(n):
    a = [0, 1]
    if (n <=1):
        return n
    else:
        for i in range(1, n):
            a.append((a[-1] + a[-2])%m)
        return a[i+1]
print (fib(n))

As I typed in 239 1000, it returns 161 as the remainder, which is correct. But with bigger inputs eg. 2816213588 239, it seems to exceed time limits, what can I do to improve the code?

Sorry, I am giving no code (I am on a phone and I don't know Python), but be aware that if m^2 is way lower than n, you could use the fact that your function gets periodical with a period maximally m^2 (as both a[-1] and a[-2] can gain m different values).

You could test in your for loop if/when you reached your period (if a[-2]==0 and a[-1]==1) and if so, variable i would indicate your period. Then you could simply grab a[n%(i-2)] as the answer, if I am not mistaken.

By the way, shouldn't the for loop range begin with 2?


Centering an item over an image with absolute positioning and flexbox Here's how I'd usually center align an item with absolute positioning:

img {
 width: 100%;
 height: auto;
}

.cat {
 position: relative;
}

.center {
 position: absolute;
 top: 50%;
 left: 50%;
 transform: translate(-50%, -50%);
 z-index: 9999;
 background: #fff;
 padding: 12px;
}

a,
a:visited,
a:hover,
a:focus {
 color: #000;
 text-decoration: none;
}

 I want to center this

I've recently discovered that you can do the same with flexbox, like so:

img {
 width: 100%;
 height: auto;
}

.cat {
 position: relative;
 display: flex;
 justify-content: center;
 align-items: center;
}

.center {
 position: absolute;
 z-index: 9999;
 background: #fff;
 padding: 12px;
}

a,
a:visited,
a:hover,
a:focus {
 color: #000;
 text-decoration: none;
}

 I want to center this

Is there any benefit to using the flexbox solution? A major downside I'm seeing is browser compatibility. I can't find a solution that purely uses flexbox.

Is there any benefit to using the flexbox solution?

The principal benefit will be more concise, easier-to-maintain code.

That's not to say there's anything wrong with position: absolute (there isn't). Just that with display: flex you can achieve more with less.

That is, once you have declared justify-content and align-items on a flex parent, you don't necessarily need to apply any further positioning to flex-children at all - they will automatically display in the right position, regardless of browser viewport size and dimensions.

  IE11 isn’t playing ball.

True: https://caniuse.com/#feat=flexbox

But IE11 is from October 2013.

Even Microsoft says IE is not a browser and no-one should be using it:

https://www.zdnet.com/article/microsoft-security-chief-ie-is-not-a-browser-so-stop-using-it-as-your-default/

Flex Example:

.cat {
  display: flex;
  justify-content: center;
  align-items: center;
  width: 340px;
  height: 180px;
  background: url('https://hackernoon.com/hn-images/1*mONNI1lG9VuiqovpnYqicA.jpeg') 0 0 / 100% 100%;
}

.center {
  background: rgba(255, 255, 255, 0.6);
  padding: 12px;
}

  I want to center this


What are the downsides of my Mergesort implementation? Could I ask you to evaluate my implementation of mergesort?

I think that it's consistent with \$\mathcal{O}(n \log n)\$ complexity but has a downside that I create a new result array at each step of recursion.

How do you think it can be improved?

public int[] mergesort(final int[] input) {
    return mergesort(input, 0, input.length - 1);
}

private int[] mergesort(final int[] input, final int left, final int right) {
    final int[] result;
    if (left < right) {
        final int m = (right - left) / 2 + left;
        final int[] leftPart = mergesort(input, left, m);
        final int[] rightPart = mergesort(input, m + 1, right);
        result = new int[leftPart.length + rightPart.length];
        int i = 0, j = 0;
        while (i < leftPart.length && j < rightPart.length) {
            if (leftPart[i] <= rightPart[j]) {
                result[i + j] = leftPart[i++];
            } else {
                result[i + j] = rightPart[j++];
            }
        }
        while (i < leftPart.length) {
            result[i + j] = leftPart[i++];
        }
        while (j < rightPart.length) {
            result[i + j] = rightPart[j++];
        }
    } else {
        result = new int[]{input[left]};
    }
    return result;
}

First things first: Why are you returning a new array instead of directly sorting the input array? If we create 2 temporary arrays (left and right part) and then merge them back into the input array we don't need to return a new one (and can optimise later on).

private static void mergesort(int[] input, final int left, final int right) {
    //quick note here, I flipped your if check so we can return earlier.
    //This makes it easier to follow the complete flow
    if (left >= right) {
        return;
    }
    //edge cases handled before this point. Can continue with actual algorithm after.
    final int m = (right - left) / 2 + left;
    mergesort(input, left, m);
    mergesort(input, m + 1, right);

    int[] leftPart = new int[m - left + 1];
    for(int i = 0; i < leftPart.length-1;i++){
        leftPart[i] = input[left + i];
    }
    int[] rightPart = new int[right - m];
    for(int i = 0; i<rightPart.length-1;i++){
        rightPart[i] = input[m + i];
    }

    int i = 0, j = 0;
    while (i < leftPart.length && j < rightPart.length) {
        if (leftPart[i] <= rightPart[j]) {
            input[i + j] = leftPart[i++];
        } else {
            input[i + j] = rightPart[j++];
        }
    }
    while (i < leftPart.length) {
        input[i + j] = leftPart[i++];
    }
    while (j < rightPart.length) {
        input[i + j] = rightPart[j++];
    }
}

Now that we sort the input array directly theres one major memory optimisation we can do. We no longer need to store the right half in a temporary array anymore. Instead we can directly copy from the input array since the "current" spot is always smaller or equal than the index of the right part we're handling.

private static void mergesort(int[] input, final int left, final int right) {
    if (left >= right) {
        return;
    }

    final int m = (right - left) / 2 + left;
    mergesort(input, left, m);
    mergesort(input, m + 1, right);

    int[] leftPart = new int[m - left+1];
    //note: arraycopy is usually more efficient than manually copying an array.
    System.arraycopy(input, left, leftPart, 0, leftPart.length);

    int current = left;
    int currentLeft = 0;
    int currentRight = m+1;

    while (currentLeft < leftPart.length && currentRight <= right) {
        if (leftPart[currentLeft] <= input[currentRight]) {
            input[current++] = leftPart[currentLeft++];
        } else {
            input[current++] = input[currentRight++];
        }
    }
    while (currentLeft < leftPart.length) {
        input[current++] = leftPart[currentLeft++];
    }
    //no need to handle remaining right part, as it's already in the input array
}

Some extra notes:

I renamed some variables to make it more clear what they mean. Even though i and j are common enough for a simple loop, it wasn't obvious enough anymore once the right index started from halfway the part of the array that needed to be sorted.

I made the method static. It doesn't depend on any state of the class it's in so could be put into a more general utility class instead.

System.arraycopy is more performant than manually copying all elements into another array.


Writing data of a certain integer type to a buffer I have function which writes data of a specified type to a buffer. Here is the part of this which writes Uint8, Uint16, Uint32 and Uint64, in big and little endian. As you can see that the same code is repeated several times, so I want to make this code more elegant.

...
            case BW_DATA_TYPE_UINT8:
            {
                Uint8_T val = *(static_cast(Uint8_T*, src));
                Uint8ToLittleEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] );
                bw->curPos += length;
                break;
            }

            case BW_DATA_TYPE_UINT16_LE:
            {
                Uint16_T val = *(static_cast(Uint16_T*, src));
                Uint16ToLittleEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 1 );
                bw->curPos += length;
                break;
            }

            case BW_DATA_TYPE_UINT32_LE:
            {
                Uint32_T val = *(static_cast(Uint32_T*, src));
                Uint32ToLittleEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 3 );
                bw->curPos += length;
                break;
            }

            case BW_DATA_TYPE_UINT64_LE:
            {
                Uint64_T val = *(static_cast(Uint64_T*, src));
                Uint64ToLittleEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 7 );
                bw->curPos += length;
                break;
            }

            case BW_DATA_TYPE_UINT16_BE:
            {
                Uint16_T val = *(static_cast(Uint16_T*, src));
                Uint16ToBigEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 1 );
                bw->curPos += length;
                break;
            }

            case BW_DATA_TYPE_UINT32_BE:
            {
                Uint32_T val = *(static_cast(Uint32_T*, src));
                Uint32ToBigEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 3 );
                bw->curPos += length;
                break;
            }

            case BW_DATA_TYPE_UINT64_BE:
            {
                Uint64_T val = *(static_cast(Uint64_T*, src));
                Uint64ToBigEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 7 );
                bw->curPos += length;
                break;
            }
    ...

#define CONVERT(T, F, v) T val = *(static_cast(T*, src)); \
    F( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + v ); \
    bw->curPos += length;

    [...]

    case BW_DATA_TYPE_UINT8:
    {
        CONVERT(Uint8_T, Uint8ToLittleEndianAr, 0)
        break;
    }

    case BW_DATA_TYPE_UINT16_LE:
    {
        CONVERT(Uint16_T, Uint16ToLittleEndianAr, 1)
        break;
    }

    case BW_DATA_TYPE_UINT32_LE:
    {
        CONVERT(Uint32_T, Uint32ToLittleEndianAr, 3)
        break;
    }

    case BW_DATA_TYPE_UINT64_LE:
    {
        CONVERT(Uint64_T, Uint64ToLittleEndianAr, 7)
        break;
    }

    case BW_DATA_TYPE_UINT16_BE:
    {
        CONVERT(Uint16_T, Uint16ToBigEndianAr, 1)
        break;
    }

(untested!)
You can go beyond that and even generate the whole case, but this version is a compromise to keep readability.
[EDIT] I missed the bottom of your code, but I suppose you can complete yourself... :-)


Calculate amount of necessary ingredients I have started to learn C and decided to recreate my bakery task in C.
As I am new to the language, I am unsure if I have approached the task in the right way using structs. Feedback on the style of the code would also be appreciated.

#include <stdio.h>

double cup_ingredients[4] = {4.0,0.1,12.0,14.0}; // Amount of each ingredient for 1 cupcake = {Butter, eggs, flour, sugar}
double lemon_ingredients[4] = {80.0,4.5,240.0,300.0}; // Amount of each ingredient for 1 lemon cake = {Butter, eggs, flour, sugar}
double total[4];
double cup_req;
double lemon_req;

struct Bags {
    int big_bag;
    int med_bag;
    int small_bag;
};

void calc_bag(double total_ingredient, struct Bags* bag_sizes, struct Bags* type);

int main() {
    printf("How many cupcakes would you like? ");
    scanf("%lf", &cup_req);

    for (int x = 0; x<cup_req; x++){ // For the number of cupcakes required:
        for (int y = 0; y<4; y++){ // For each ingredient
            total[y] += cup_ingredients[y]; // Add the amount of each ingredient to the total amount of that ingredient
        }
    }

    printf("How many lemon cakes would you like? ");
    scanf("%lf", &lemon_req);

    for (int x = 0; x<lemon_req; x++){ // For the number of lemon cakes:
        for (int y = 0; y<4; y++){ // For each ingredient
            total[y] += lemon_ingredients[y]; // Add the amount of each ingredient to the total amount of that ingredient
        }
    }

    //Structs for the amount of each ingredient a bag can hold
    struct Bags Butter_size = {.big_bag = 500, .med_bag = 250, .small_bag = 125};
    struct Bags Egg_size = {.big_bag = 12, .med_bag = 10, .small_bag = 6};
    struct Bags Flour_size = {.big_bag = 750, .med_bag = 500, .small_bag = 250};
    struct Bags Sugar_size = {.big_bag = 600, .med_bag = 400, .small_bag = 200};

    //Set the bags required to 0
    struct Bags Butter_req = {0,0,0};
    struct Bags Egg_req = {0,0,0};
    struct Bags Flour_req = {0,0,0};
    struct Bags Sugar_req = {0,0,0};

    //Calculate the amount of each ingredient bag required
    calc_bag(total[0], &Butter_size, &Butter_req);
    calc_bag(total[1], &Egg_size, &Egg_req);
    calc_bag(total[2], &Flour_size, &Flour_req);
    calc_bag(total[3], &Sugar_size, &Sugar_req);

    printf("\nButter: %d large bags, %d medium bags, %d small bags.", Butter_req.big_bag, Butter_req.med_bag, Butter_req.small_bag);
    printf("\nEgg: %d large bags, %d medium bags, %d small bags.", Egg_req.big_bag, Egg_req.med_bag, Egg_req.small_bag);
    printf("\nFlour: %d large bags, %d medium bags, %d small bags.", Flour_req.big_bag, Flour_req.med_bag, Flour_req.small_bag);
    printf("\nSugar: %d large bags, %d medium bags, %d small bags.", Sugar_req.big_bag, Sugar_req.med_bag, Sugar_req.small_bag);
}

void calc_bag(double total_ingredient, struct Bags* bag_sizes, struct Bags* type){
    while (total_ingredient > 0){
        if (total_ingredient > bag_sizes->big_bag) {
            type->big_bag++;
            total_ingredient -= bag_sizes->big_bag;
        }
        else if (total_ingredient > bag_sizes->med_bag) {
            type->med_bag++;
            total_ingredient -= bag_sizes->med_bag;
        }
        else if (total_ingredient > bag_sizes->small_bag) {
            type->small_bag++;
            total_ingredient -= bag_sizes->small_bag;
        }
        else {
            type->small_bag++;
            total_ingredient = 0;
        }
    }
}

I see some things that may help you improve your code.

Eliminate function prototypes by ordering

If you put the calc_bag implementations above main in the source code, you don't need the function prototype.

Avoid the use of global variables

I see that cup_ingredients and lemon_ingredients, etc. are declared as global variables rather than as local variables. It's generally better to explicitly pass variables your function will need rather than using the vague implicit linkage of a global variable.  In this case, these should all be in main rather than global.

Initialize variables

Global variables are initialized for you (to 0 for numeric variables), but local variables are not.  For that reason, you should also get into the habit of initializing variables, ideally when they're  declared.  For example:

double total[4] = {0.0, 0.0, 0.0, 0.0};
double cup_req = 0.0;
double lemon_req = 0.0;

Use const where practical

The ingredients lists cup_ingredients and lemon_ingredients, as well as the bag capacities Butter_size, etc. should all be constant.  For that reason, they should all be declared static const as in:

static const struct Bags Butter_size = {.big_bag = 500, .med_bag = 250, .small_bag = 125};

Then the calc_bag function should be this:

void calc_bag(double total_ingredient, const struct Bags* bag_sizes, struct Bags* type);

Simplify by using a typedef

The code you have isn't wrong, but it's often convenient to use a typedef for structures that are used frequently.  In this case, I'd suggest that your Bags structure could be this:

typedef struct bags_s {
    int big_bag;
    int med_bag;
    int small_bag;
} Bags;

Then instead of writing struct Bags everywhere, you can simply write Bags.

Prefer multiplication to iteration

Especially when using floating point numbers, it's most often better to multiply than to use iteration.  For example, the code currently has this:

printf("How many cupcakes would you like? ");
scanf("%lf", &cup_req);

for (int x = 0; x<cup_req; x++){
    for (int y = 0; y<4; y++){
        total[y] += cup_ingredients[y];
    }
}

That could be replaced by this:

for (int i = 0; i < 4; ++i) {
    total[i] += cup_req * cup_ingredients[i];
}

Similarly, your calc_bag function could use division rather than iteration.

Break up the code into smaller functions

The main function is quite long and does a series of identifiable steps.  Rather than having everything in one long function, it would be easier to read and maintain if each discrete step were its own function.  I'd be inclined to divide it into separate input, calculation, and output stages, each with the appropriate function.

Eliminate "magic values"

The value 4 is sprinkled through the code, but it really ought to be a named constant instead.  I'd give it a meaningful name like this:

#define INGREDIENT_COUNT 4

Rethink your data structures

The only difference between the cup_ingredients and lemon_ingredients is the name.  They're parallel structures.  This relationship could be made more clear by defining another structure which includes the name.  One might write it like this:

typedef struct recipe_s {
    char *name;
    double ingredients[INGREDIENT_COUNT];
} Recipe;

With that structure in place, one might rewrite main like this:

int main() {
    static const char *ingredient_name[INGREDIENT_COUNT] = {
        "butter", "eggs", "flour", "sugar",
    };
    static const Recipe recipes[] = {
        { "cupcakes", {4.0,0.1,12.0,14.0} },
        { "lemon cakes", {80.0,4.5,240.0,300.0} },
    };
    static const int recipe_count = sizeof(recipes) / sizeof(recipes[0]);
    Recipe total = { "total", {0.0, 0.0, 0.0, 0.0} };

    for (int i = 0; i < recipe_count; ++i) {
        double qty;
        printf("How many %s would you like? ", recipes[0].name);
        scanf("%lf", &qty);
        for (int j = 0; j < INGREDIENT_COUNT; ++j) {
            total.ingredients[j] += qty * recipes[i].ingredients[j];
        }
    }

    //Structs for the amount of each ingredient a bag can hold
    static const Bags capacity[INGREDIENT_COUNT] = {
        { 500, 250, 125}, // butter
        {  12,  10,   6}, // eggs
        { 750, 500, 250}, // flour
        { 600, 400, 200}, // sugar
    };

    // make a shopping list
    Bags shopping_list[INGREDIENT_COUNT] = {
        {0, 0, 0},  // butter
        {0, 0, 0},  // eggs
        {0, 0, 0},  // flour
        {0, 0, 0},  // sugar
    };

    //Calculate the amount of each ingredient bag required
    for (int i = 0; i < INGREDIENT_COUNT; ++i) {
        calc_bag(total.ingredients[i], &capacity[i], &shopping_list[i]);
        printf("%s: %d large bags, %d medium bags, %d small bags.\n",
            ingredient_name[i],
            shopping_list[i].big_bag,
            shopping_list[i].med_bag,
            shopping_list[i].small_bag
        );
    }
}

I'll leave it to you to divide that into smaller functions, but it should help you get an idea of how to write better C.

Other enhancements

I would be very disappointed if my grocer actually handed me a "bag of eggs."  Instead, the common quantities for different things have different units of measure such as a "dozen" or a "kilogram" or a "pound".  Using a similar idea of associating a name with constants (as with the recipes shown above), you might want to associate units of measure with each kind of ingredient.

Also, our lemon cake does not appear to have lemon as an ingredient, which makes it a somewhat less appealing confection.  Consider adding the ability to create arbitrary lists of named ingredients, consolidating them into a shopping list as above.


Automatic exception wrapper in java There's a lot of place in my code where I need to do something like this

public static void read(DataInputStream stream) throws IOException
{
  try
  {
      Object thing = getById(stream.readInt());
  }
  catch(IllegalArgumentException ex)
  {
      throw new IOException(ex); // do not throw IllegalArgumentException if the corrupted data points to invalid things
  }
}

public static Object getById(int id)
{
    for(Object thing : things)
        if(thing.getId() == id)
            return thing;

  throw new IllegalArgumentException("Thing with id " + id + " does not exist!");
}

I like the fact that read() only throws IOExceptions and wrap any detail of internal failure with IOException as having those failures are caused by having corrupted data from the stream. (And corrupted data should return IOException, could return something else but the idea is the same).

What I really don't like is the tedious process of wrapping all my exceptions everytime and incrementing the number of indents I have just to indicate that IllegalArgumentException becomes IOException. An annotation like this

@WrapExcept(IllegalArgumentException.class, ex -> new IOException(ex))
public static void read(DataInputStream stream) throws IOException
{
  Object thing = getById(stream.readInt());
}

would be perfect but I have no idea how it could be possible. The best I could come up with is something along the lines of

@FunctionalInterface
public interface ThrowingCall<R, E extends Throwable>
{
    R call() throws E;
}

public static <R, W extends Throwable, E extends Throwable, T extends Throwable>
R wrapExcept(ThrowingCall<R, T> call, Class<W> exType, Function<W, E> wrapper) throws E, T
{
    try
    {
        return call.call();
    }
    catch(Throwable t)
    {
        if(exType.isInstance(t))
            throw wrapper.apply(exType.cast(t));
        throw t;
    }
}

public static void read() throws IOException
{
    Object thing = wrapExcept(() -> getById(3), IllegalArgumentException.class, ex -> new IOException(ex));
}

Any idea to make this code look likes less verbose? Obviously what I want to improve is the read() call's readability. Also I'm not sure how ThrowingCall could be replaced by something more standard.

This looks like something Optionals can handle, so:

public static Optional<Object> getById(int id) {
    for(Object thing : things) {
        if(thing.getId() == id) {
            return Optional.of(thing);
        }
    }

    return Optional.empty();
}

Now in your read code, you can simply do this:

public static void read(DataInputStream stream) throws IOException {
    Object thing = getById(stream.readInt())
                       .orElseThrow(() -> new IOException("Could not find Id"));

    // Do something with thing
}


Merge Sort an integer array I've implemented merge sort an integer array. Is it okay to use i,j,k as variable name for looping? Should I change them to more meaningful names? Overall, any further suggestions on this code?

MergeSort.java

import java.util.Arrays;

public class MergeSort {

    public static void main(String[] args) {

    }

    public static void mergeSort(int[] inputArray) {
        int size = inputArray.length;
        if (size < 2)
            return;
        int mid = size / 2;
        int leftSize = mid;
        int rightSize = size - mid;
        int[] left = new int[leftSize];
        int[] right = new int[rightSize];
        for (int i = 0; i < mid; i++) {
            left[i] = inputArray[i];

        }
        for (int i = mid; i < size; i++) {
            right[i - mid] = inputArray[i];
        }
        mergeSort(left);
        mergeSort(right);
        merge(left, right, inputArray);
    }

    public static void merge(int[] left, int[] right, int[] arr) {
        int leftSize = left.length;
        int rightSize = right.length;
        int i = 0, j = 0, k = 0;
        while (i < leftSize && j < rightSize) {
            if (left[i] <= right[j]) {
                arr[k] = left[i];
                i++;
                k++;
            } else {
                arr[k] = right[j];
                k++;
                j++;
            }
        }
        while (i < leftSize) {
            arr[k] = left[i];
            k++;
            i++;
        }
        while (j < leftSize) {
            arr[k] = right[j];
            k++;
            j++;
        }
    }
}

MergeSortTest.java

import static org.junit.Assert.*;

import java.util.Arrays;

import org.junit.Test;

public class MergeSortTest {

    @Test
    public void reverseInput(){
        int[] arr={22,21,19,18,15,14,9,7,5};
        MergeSort.mergeSort(arr);
        assertEquals("[5, 7, 9, 14, 15, 18, 19, 21, 22]", Arrays.toString(arr));

    }
    @Test
    public void emptyInput(){
        int[] arr={};
        MergeSort.mergeSort(arr);
        assertEquals("[]", Arrays.toString(arr));
    }

    @Test
    public void alreadySorted(){
        int[] arr={1,2,3,4,5,6,7,8,9,10};
        MergeSort.mergeSort(arr);
        assertEquals("[1, 2, 3, 4, 5, 6, 7, 8, 9, 10]", Arrays.toString(arr));
    }

}

Overall I would have to say that this is the neatest and most clear implementation of a Merge Sort that I have seen. There is nothing wrong with variables i, j and k. They are standard names for looped array indexes. There is a single empty line in your code which is inconsistent with the rest of the method. That is the only nitpick I can find in terms of the style and neatness. It is a tiny, inconsequential thing.

The only recommendations I can make, are things that will improve performance, or will introduce some other common practices that may impact readability, though they are still more standard than what you have.

First, some common practice things

Consider doing post-increments inside the array-reference. This code:

while (i < leftSize && j < rightSize) {
    if (left[i] <= right[j]) {
        arr[k] = left[i];
        i++;
        k++;
    } else {
        arr[k] = right[j];
        k++;
        j++;
    }
}
while (i < leftSize) {
    arr[k] = left[i];
    k++;
    i++;
}
while (j < leftSize) {
    arr[k] = right[j];
    k++;
    j++;
}

would commonly be written as:

while (i < leftSize && j < rightSize) {
    if (left[i] <= right[j]) {
        arr[k++] = left[i++];
    } else {
        arr[k++] = right[j++];
    }
}
while (i < leftSize) {
    arr[k++] = left[i++];
}
while (j < leftSize) {
    arr[k++] = right[j++];
}

There is no change to the logic, the code is just a bit more compressed. The impact to readability is debatable, you get more code on a screen, but you have to look for the increments.
You copy the array contents using a regular loop:

int[] left = new int[leftSize];
int[] right = new int[rightSize];
for (int i = 0; i < mid; i++) {
    left[i] = inputArray[i];

}
for (int i = mid; i < size; i++) {
    right[i - mid] = inputArray[i];
}

The above would typically be done (for performance reasons), as one of two ways, either:

int[] left = new int[leftSize];
int[] right = new int[rightSize];
System.arraycopy(inputArray, 0, left, 0, leftSize);
System.arraycopy(inputArray, leftSize, right, 0, rightSize);

In Java6 and above, the Arrays utility class can be used too:

int[] left = Arrays.copyOfRange(inputArray, 0, leftSize);
int[] right = Arrays.copyOfRange(inputArray, leftSize, inputArray.length);

I would go with the Arrays.copyOfRange(...) call.

The above changes do not change the logic of your program at all, just the techniques used at the various points.

Your implementation would still be 'textbook'.

In terms of performance, though, your limit is the number of times you create, copy, and discard arrays of data.

There is a variant of the Merge Sort that uses just two arrays, the input array, and a 'temp' array that is the same size. The algorithm repeatedly merges small chunks of data from one array, to the other, then swaps them, and merges the now larger chunks back to the first, and keeps doing that until the data is sorted.

Using that algorithm means there is no additional array copying, etc. It is much faster, but the implementation would look very different to yours. Still I would recommend you try it. There are examples on Wikipedia


Summarize raw data in new sheet Note: This question has a follow up question here.

I have an Excel workbook with two sheets: "Raw data" and "Summary". In the raw data sheet there are several blocks with data structured like the image below:

I want to create a summary of the data below, in the sheet "Summary", looking like this:

The way I've done this is:

Sub TransferAndSumElements()
    Dim wb As Workbook
    Dim ws1 As Worksheet
    Dim ws2 As Worksheet

    Set wb = ActiveWorkbook
    Set ws1 = Sheets("Raw data")
    Set ws2 = Sheets("Summary")

    '##############
    ' Copy / Paste first row
    '##############
    ws1.Select
    Rows("1:1").Activate
    Selection.Copy
    ws2.Select
    Range("A1").Select
    ws2.Paste

    '##############
    ' Set Item names in summary
    '##############

    ws2.Select
    Range("A2").Select
    ActiveCell.FormulaR1C1 = "Knife"
    Range("A3").Select
    ActiveCell.FormulaR1C1 = "Fork"
    Range("A4").Select
    ActiveCell.FormulaR1C1 = "Spoon"
    Range("A5").Select
    ActiveCell.FormulaR1C1 = "Spork"
    Range("A6").Select
    ActiveCell.FormulaR1C1 = "Bowl"
    Range("A7").Select

    '##############
    ' Find Elements and Sum
    '##############

    Dim s As Long           ' Sum of elements for each Item
    Dim str As String       ' String in first column in "Summary" (Knife, Fork ...)
    Dim cellstr As String   ' String in first column in "Raw data" (To be searched through)
    Dim DataRange As Range  ' All rows in sheet 1 ("Raw data")

    Dim cellA As Range      ' Used to find cellstr = cellA.Value
    Dim cellB As Range     ' Used to increment s = s + cellB.Value

    ws1.Select              ' Select first worksheet ("This worksheet")
    LastRow = ws1.Cells.Find("*", searchorder:=xlByRows, searchdirection:=xlPrevious).Row
    Set DataRange = Range("A1:A" & LastRow)

    For i = 1 To 5          ' Iterate through Knife, Fork .. in "Summary"...

        str = ws2.Cells(i + 1, 1).Value     ' Extract names
        s = 0                               ' Initialize sum
        ws1.Activate                        ' Activate Worksheet 1 ("Raw data")

        For r = 1 To LastRow
            Set cellA = Range("A" & r, "A" & r)
            cellstr = cellA.Value
            If InStr(cellstr, str) > 0 Then
                Set cellB = Range("B" & r, "B" & r)
                s = s + cellB.Value
            End If
        Next r

        ws2.Cells(i + 1, 2).FormulaR1C1 = s

    Next i

ws2.Activate

End Sub

Some stuff that I want to improve, but haven't been able to:

Automatically fetch the item names from "Raw data". I've tried creating a Collection, but I end up with an empty row in between the others (Knife, Fork, Spoon, Spork, Blank, Bowl).

I can loop through this list after creating it, and delete the blank row, but I think there should be a way to do this in one go.

I want to create a list of words ("Knife", "Fork" .. ) and write it to Excel in one command. The way I'm doing it now is to manually write each item name.
Do the loops make sense, or should I do it in some other way?

And last but not least:

What about the big picture? Does it make sense to do it like this, or should I do it completely different?

Note: The summary can be sorted alphabetically, by number of elements, or not sorted at all. That doesn't matter.

Tab delimited data dump:

Item    Number
Knife   2
Fork    2
Spoon   1
Spork   2

Item    Number
Spork   2
Fork    3

Item    Number
Bowl    3
Knife   5

You should never, ever, be using Select or Active... in your code.

Unless you explicitly want your sub to click on a cell, or you explicitly want to work off of a user-selection, this is an incredibly slow, inefficient,  fragile way of doing things.

This:

Range("A1").Select
Selection.FormulaR1C1 = "Knife"

Or

Range("A1").Select
ActiveCell.FormulaR1C1 = "Knife"

Is much better expressed like so:

Range("A1").FormulaR1C1 = "Knife"

Furthermore, Range("A1") is implicitly calling [ActiveSheet].Range("A1"). You should explicitly qualify your references to avoid bugs down the road. Like so:

ws2.Range("A1").FormulaR1C1 = "Knife"
ws2.Range("A2").FormulaR1C1 = "Fork"
ws2.Range("A3").FormulaR1C1 = "Spoon"

And then you can use a With statement to make it even better:

With ws2
    .Range("A1").FormulaR1C1 = "Knife"
    .Range("A2").FormulaR1C1 = "Fork"
    .Range("A3").FormulaR1C1 = "Spoon"
End With


Group a list of strings by length Is it possible to write this using the stream element? I can get the most of it with stream but it is the add adding string to list which is in the map that I am caught on.

/**
 * Given a list of strings, returns a map where the keys are the unique strings lengths found in the list and the values are the strings matching each length
 * [hello,world,cat,ambulance] would return {5 -> [hello, world], 3 -> [cat] -> 3, 9 -> [ambulance]}
 * @param strings
 * @return
 */
public Map<Integer,List<String>> mapLengthToMatchingStrings(List<String> strings){

    Map<Integer,List<String>> mapILS = new HashMap<Integer, List<String>>();

    for (String str : strings) {
        int length = str.length();

        if (mapILS.containsKey(length)) {
            // Add str to list at key
            //Get list at key
            List<String> rs = mapILS.get(length);
            //Add str to list
            rs.add(str);
            //Update key in map
            mapILS.put(length, rs);
        }else {
            // Add new key list to map
            //Create list
            List<String> rs = new ArrayList<String>();
            //Add string
            rs.add(str);
            //Add key and list to map
            mapILS.put(length, rs);
        }
    }
    return mapILS;
}

It's not only possible but also a great example of how functional programming can make code much simpler and readable:

  public Map<Integer,List<String>> mapLengthToMatchingStrings(List<String> strings){
        return strings.stream().distinct().collect(Collectors.groupingBy(String::length));
    }

(The distinct part is optional, regarding whether you want duplicates in the lists)


Make a dict class from scratch in Python Here's the API my teacher assigned:

and here is what I came up with:

class MyMap:
    def __init__(self,size=10):
        self.size = size
        self.internal_list = [None] * size
    def _getHash(string,size):
        total = 0
        for ch in string:
            total += ord(ch)
        total = total % size
        return total
    def set(self,key,value):
        listnum = MyMap._getHash(key,self.size)
        if key in self.internal_list:
            print("already there")
            return None
        if self.internal_list[listnum] == None or self.internal_list[listnum] is type(tuple):
            self.internal_list[listnum] = (key, value)
        else:
            pass
    def get(self,key):
        listnum = MyMap._getHash(key,self.size)
        if self.internal_list[listnum] == None:
            return None
        else:
            return self.internal_list[listnum][2]
    def size(self):
        sizecount = 0
        count = 0
        while len(self.internal_list)-count != 0:
            if self.internal_list[count] != None:
                sizecount += 1
            count += 1
        return sizecount
    def getKeys(self):
        keylist = []
        count = 0
        while len(self.internal_list) - count != 0:
            if self.internal_list[count] != None:
                keylist.append(self.internal_list[count][0])
            count += 1
        return keylist

Just wondering

if it will fulfill the requirements of the assignment
what I can do to clean up the code

Here's what I've tested:

x = MyMap()
x.set('test',2)
x.set('crest',3)
x.set('best',5)
print(MyMap.size(x))
print(x.get('test'))
print(MyMap.getKeys(x))
print(x.internal_list)

and the results...

The first thing you need to do is learn how to unittest your code. This will assume Python3.

# filename: test_MyMap.py
import unittest
import unittest.mock as mock
from mymap import MyMap # the filename for your code

class Test(unittest.TestCase):

    # here's where you write the tests.
    def test_default_size(self):
        m = MyMap()
        self.assertEqual(m.size, 10)

    def test_set_method(self):
        """we use unittest.mock.patch to swap in an easy version of
        _getHash that we know cannot fail. This way we don't introduce
        any other reason of failure into our test for MyMap.set"""

        def replace_getHash(self,key):
            return {'string':0, 1:1, tuple(1,2):2}[key]
        with mock.patch("__main__.MyMap._getHash", new=replace_getHash):
            for key,value in [('string','value'),
                              (1,2),
                              (tuple(1,2), tuple(3,4))]:
                m.set(key, value)
                self.assertEqual(m.internal_list[m._getHash(key)], value)

    def test_get_method(self):
        # etc for all the specs you need to fulfill

if __name__ == "__main__":
    """Will run all the TestCase classes, which each run all their methods
    that begin with test_"""
    unittest.main()

The first thing you should do when you get an assignment or any other spec to write from is design a set of unit tests (as above) to write your code to meet. It's much harder to create unit tests from a program than from a spec so do it this way first! Then every time you change the code, check it against the unit tests to make sure you haven't broken anything.

As far as the actual review goes, it's going to be hard to look at until you can design a full suite of tests against it. A quick glance looks like this will fail if you give it a non-iterable key (e.g. an int, which should be doable) and will also lead to hash collision if you give it an anagram (e.g. foo.set("test",3) == foo.set("etst", 3)). These are edge cases that your unit tests should catch.

Beyond that I'd implement item access (so you can do my_map_object[some_key] instead of having to rely on my_map_object.get(some_key)) and it really seems like you could clean up your MyMap.getkeys function. Something like:

def getkeys(self):
    return [keyvalue[0] for keyvalue in self.internal_list if keyvalue is not None]

should work perfectly. It also stands to reason that

def size(self):
    return sum([1 for key in self.getkeys()])

would work too!

Oh and your MyMap._getHash function will break completely, since string will never be iterable. It should be def _getHash(self, string) (and reference self.size()) rather than def _getHash(string, size) unless you make it a @staticmethod (which WOULD make sense except that your size modulo is dependent on your particular mapping!)


Test if arithmetic operation will cause undefined behavior I've written a command line calculator as an exercise to get ready for my upcoming first programming job. The whole program source is around 450 lines of code, which I think is too long for a single question. I'll therefore post the code in parts starting with this one.

Any software needs to validate it's data. The functions below are used to test whether a basic arithmetic operation on two signed integers a and b will cause undefined behavior or not. They are checking if the result would be out of bounds and for division by zero. If they return zero the operation is then carried out and the result returned. If they return non-zero, an error message is printed out to the user. The logic of these functions is taken from this page of the SEI CERT C Coding Standard.

My main concern with this code is the is_undefined_mult function, which I don't think is very readable.

int is_undefined_add(int a, int b)
{
    return (a > 0 && b > INT_MAX - a) ||
           (a < 0 && b < INT_MAX - a);
}

int is_undefined_sub(int a, int b)
{
    return (b > 0 && a < INT_MAX + b) ||
           (b < 0 && a > INT_MAX + b);
}

int is_undefined_mult(int a, int b)
{
    if (a > 0) {
        if (b > 0) {
            if (a > INT_MAX / b) {
                return 1;
            }
        }
        else {
            if (b < INT_MIN / a) {
                return 1;
            }
        }
    }
    else {
        if (b > 0) {
            if (a < INT_MIN / b) {
                return 1;
            }
        }
        else {
            if (a != 0 && b < INT_MAX / a) {
                return 1;
            }
        }
    }

    return 0;
}

int is_undefined_div(int a, int b)
{
    return b == 0 || (a == INT_MIN && b == -1);
}

OP edited post.
Is the description backwards?

If they return non-zero the operation is then carried out and the result returned. If they return zero, an error message is printed out to the user.

int is_undefined_mult(int a, int b) {
    if (a > 0) {
        if (b > 0) {
            if (a >= INT_MAX / b) {
                return 1;  // 1 is the overflow condition here.

This hints that using a macro or enumerated result would be less error prone than 0 or 1.

a == INT_MIN && b == -1 is only a problem with 2's complement:
int is_undefined_div(int a, int b) {
  #if INT_MIN < -INT_MAX
    if (a == INT_MIN && b == -1) return 1;
  #endif
  return b == 0;
}

As @JS1 noted, replace INT_MAX with INT_MIN in 2 places.  Otherwise, per my tests, the  functions are correct,  expecting is_undefined_div().

[Edit]
Candidate is_undefined_mult() simplification - really just a collapsing of the if structure.
int is_undefined_mult1(int a, int b) {
  if (a > 0) {
    if (b > 0) {
      return a > INT_MAX / b;       // a positive, b positive
    }
    return b < INT_MIN / a;         // a positive, b not positive
  }
  if (b > 0) {
    return a < INT_MIN / b;         // a not positive, b positive
  }
  return a != 0 && b < INT_MAX / a; // a not positive, b not positive
}

[Edit2]
A potential simplification with is_undefined_add()/is_undefined_sub().
int is_undefined_add1(int a, int b) {
  return (a < 0) ? (b < INT_MIN - a) : (b > INT_MAX - a);
}

int is_undefined_sub1(int a, int b) {
  return (b < 0) ? (a > INT_MAX + b) : (a < INT_MIN + b);
}


Beginner C++ calculator I'm relatively new to programming, but am really enjoying it. I have taken a couple of classes but mostly code for fun (for now). I decided to make a program implementing some basic functions, switch statements, and user input. Please leave any feedback on how to make this program better.

I understand that I am not actually using the defined functions (add, subtract, etc.), but I put them in there with the intent of using them, I just don't know how.

#include <iostream>
using namespace std;

int add (int x, int y){
return x+y;
};

int divide (int x, int y){
return x/y;
}

int multiply (int x, int y){
return x*y;
}

int subtract (int x, int y){
return x-y;
}

int main(){

int n1;
int n2;
int user14 = 0;

SomeLine:
cout << "Enter your 2 numbers: "<< endl;
cin >> n1;
cin >> n2;

cout << "Ok, now what do you want to do with those numbers? "<< endl;

cout << "1) Add: " << endl;
cout << "2) Divide: "<<endl;
cout << "3) Multiply: "<< endl;
cout << "4) Subtration: "<< endl;

cin >> user14;

switch (user14)
{
    case 1:
    cout << n1+n2 << endl;
        break;
    case 2:
    cout << n1/n2 << endl;
        break;
    case 3:
    cout << n1*n2<< endl;
        break;
    case 4:
    cout << n1-n2 << endl;
        break;

}

char userchoice;
cout << "Would you like to perform any other operations? y/n "<< endl;

cin >> userchoice;

if (userchoice=='y'){
    goto SomeLine;
}
else if(userchoice=='n'){
    goto Exit;
}

Exit:
return 0;

};

Welcome! Here is some feedback, in no particular order.

Avoid using namespace std

This advice is often repeated for good reason. See also: code guidelines SF.6. It's useful when you're learning the C++ standard library to know what belongs in the standard library and what doesn't.

cout << "Enter your 2 numbers: "<< endl;

Becomes

std::cout << "Enter your 2 numbers: "<< std::endl;

Consistent spacing

Maybe this is caused by the formatting on Stack Exchange. There are many online style guides, I would choose one that is most readable to you and your peers and stick with it throughout the source code.

int add (int x, int y){
return x+y;
};

Becomes

int add(int x, int y) {
    return x + y;
};

Semicolons

Functions don't need semicolons at the end (main & add). This is only used for function prototypes.

int add(int x, int y) {
    return x + y;
};

Becomes

int add(int x, int y) {
    return x + y;
}

Avoid std::endl

Unless you need to flush the stream, using std::endl can be replaced with a newline character.

std::cout << "Enter your 2 numbers: "<< std::endl;

Becomes

std::cout << "Enter your 2 numbers:\n";

Combine multiple std::cout and std::cin calls.

This is just my preference. It reduces the amount of code I have to write.

std::cin >> n1;
std::cin >> n2;

Becomes

std::cin >> n1 >> n2;

And

std::cout << "1) Add: " << std::endl;
std::cout << "2) Divide: "<< std::endl;
std::cout << "3) Multiply: "<< std::endl;
std::cout << "4) Subtration: "<< std::endl;

Becomes

std::cout << "1) Add:\n2) Divide:\n3) Multiply:\n4) Subtration:\n";

In the above example, there are many other ways to do it. For example, you can use multi-line strings, adding the strings, and so on.

Make use of your functions

You define the add, divide, multiply and subtract functions, but you don't call them. Here is how you would do that:

std::cout << n1 + n2 << '\n';

Becomes

std::cout << add(n1, n2) << '\n';

It's as simple as that!

In complex applications, splitting things into functions makes the code easier to work with. For example, say you wanted the "add" operation to have saturating arithmetic (doesn't overflow). It's easier to add that logic to a separate function rather than inserting it into a long block of code.

Or, if you want to handle a division-by-zero case.

Don't use goto

While goto is useful in a few cases, it's not particularly useful here. Instead you can use a do-while loop.

SomeLine:

// code

if (userchoice == 'y'){
    goto SomeLine;
}
else if(userchoice == 'n'){
    goto Exit;
}

Exit:
return 0;

Becomes

do {
     // code
} while (userchoice == 'y');

return 0;

Variable naming

The variables x and y are used in the function signatures. You can also use those names in main. I'd prefer that over n1 and n2. Likewise, user14 is vague (do we change it to user15 if we include another operation?). Something like operation or op isn't vague. userchoice is a bit long, exit is shorter and clearly shows its purpose.

const

Make use of the const keyword where it makes sense. For example, in add, we intend for the x and y variables to never themselves be modified. So we can declare them as const, and the compiler will give us an error if we accidentally change their values.

This isn't particularly useful anything other than readability here, but can be useful if you get into more advanced topics, such as const references.

The functions add, divide, multiply, and subtract can be constexpr. The compiler may already evaluate these at compile time, but using constexpr marks them as such (broadly speaking).

Move variables to the lowest scope

Now that we've gotten rid of the gotos, we can move x, y, and op to a lower scope. They can also all be declared on the same line.

Consider edge cases

The following are a few example cases where you may wish to add extra handling -- or leave as it currently is.

What if the user enters a letter where you're expecting an integer?
What if the user enters an operation number outside of the range?
What about possible signed integer overflow (undefined behavior)?
What about division-by-zero?
What if the input stream is no longer valid after user input?
...

Conclusion

Here is the code I ended up with:

#include <iostream>

constexpr int add(const int x, const int y) {
    return x + y;
}

constexpr int divide(const int x, const int y) {
    return x / y;
}

constexpr int multiply(const int x, const int y) {
    return x * y;
}

constexpr int subtract(const int x, const int y) {
    return x - y;
}

int main() {
    char exit;

    do {
        int x, y, op;

        std::cout << "Enter your 2 numbers:\n";

        std::cin >> x >> y;

        std::cout << "Ok, now what do you want to do with those numbers?\n";
        std::cout << "1) Add:\n2) Divide:\n3) Multiply:\n4) Subtration:\n";

        std::cin >> op;

        switch (op) {
            case 1:
                std::cout << add(x, y) << '\n';
                break;
            case 2:
                std::cout << divide(x, y) << '\n';
                break;
            case 3:
                std::cout << multiply(x, y) << '\n';
                break;
            case 4:
                std::cout << subtract(x, y) << '\n';
                break;
        }

        std::cout << "Would you like to perform any other operations? y/n\n";

        std::cin >> exit;
    } while (exit == 'y');

    return 0;
}

Hopefully this has been helpful. I hope you continue to enjoy programming as much as we all do!


Slideshow by array using slice method I made a slide show using array values, it works, but I strongly believe that the way I did is too long.

There should be an easier or more compact way to achieve this.

$(document).ready(function () {

  var num = 0;
  var myArray = ["1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13"];
  var batch = 1;
  var value = 0;
  var tdLength = 5;
  var flag = true;
  var cal = 0;

  var showArray = function (num) {

    var required = myArray.slice(num, (num+5));
    $('.info').empty();

    $.each(required, function (item, n) {

      $('.info').append(
         '
'+ n +'
'
        )

    })

    //value += 5;

  }

  function add(amount) {
    num = (num + tdLength - 1 + amount) % tdLength + 1

    cal = (num-1) * 5;
    //console.log(myArray.slice(cal, cal+5));
    showArray(cal);

  }

 $('#next-arrow').click(function(e){

   flag = true;

   if(cal >= (tdLength+5)) {
     $(e.target).css({ opacity:0.5 });
     return
   }
   $(e.target).css({ opacity:1});
   add(batch)

 });
 $('#prev-arrow').click(function(e){

   flag = false;

   if(cal <= 1) {
     $(e.target).css({ opacity:0.5 });
     return
   }

   $(e.target).css({ opacity:1});
   add(-batch)

 });

 $('#next-arrow').click();

})

HTML :

      Previous
      Next

Live Demo

JavaScript does type conversion automatically, there is no need to create the myArray variable. You can do that entire bit using a for loop similar to this:

var showArray = function (num) {
  $('.info').empty();
  for (var i = num; i < Math.max(num + 5, 13); ++i) {
    $('.info').append(`<h1>${n}</h1>`);
  }
};

(Maybe instead of hardcoding 13, you have a const for the length)
Keep your use of semicolons consistent. Either have them or don't. Pick a style convention and stick to it.


Calculation of mortgage repayments I've just started learning R, but I feel that I'm using it only as a normal procedural language without taking advantage of some built-in functions and data structures.

I wrote a function that calculates repayments for mortgage with an option of specifying list of overpayments to reduce monthly payments.  Please suggest any improvements on making it more native to R.

source("http://pastebin.com/raw.php?i=q7tyiEmM") ## for pmt()
mortgage <- function(price=1000000, deposit=0.10, rate=0.04, years=25, over=c()) {
    loan <- price - price * deposit
    months <- years * 12
    capital <- c()
    interest <- c()
    payments <- c()
    currentCapital <- loan
    monthlyRate <- rate / 12
    payment <- -pmt(monthlyRate, nper = months + 1, pv = loan)

    cat("Initial capital: ", loan, "\n")
    cat("Months: ", months, "\n")
    cat("Payment: ", payment, "\n")

    totalShouldPay <- payment * months
    totalPaid <- 0

    for(currentMonth in 1:months) {
        interest <- currentCapital * monthlyRate
        repaid <- payment - interest
        currentCapital <- currentCapital - repaid

        overPayments <- subset(over, over$month == currentMonth)
        if (length(overPayments) > 0 && nrow(overPayments) > 0) {
            overPayments <- sum(overPayments$payment)
            currentCapital <- currentCapital - overPayments
            payment <- -pmt(monthlyRate, nper = months + 1 - currentMonth, pv = currentCapital)
        }

        capital <- append(capital, currentCapital)

        totalPaid <- totalPaid + payment
        payments <- append(payments, payment)
    }
    cat("Would have paid: ", round(totalShouldPay, 2), "\n")
    cat("Will pay: ", round(totalPaid, 2), "\n")
    cat("Saved: ", round(totalShouldPay - totalPaid, 2), "\n")
    return(data.frame(capital=capital, payments=payments))
}

overPayments <- data.frame(
    month   = c(20,    25,    30,    35,    40),
    payment = c(20000, 20000, 20000, 20000, 20000))

cap <- mortgage(
    over = overPayments
)

#print(cap)
par(mfcol=c(1, 2))
plot(cap$payments, type="s", main = "Payments", xlab = "Month", ylab = "Payment")
plot(cap$capital, type="l", main = "Capital", xlab = "Month", ylab = "Remaining loan")

I spent clearly too much time on this... Main ideas:

your understanding of how the payment on a fixed mortgage is computed was wrong. The payment is computed once at the beginning and remains the same regardless of prepayments, which is why people who choose to prepay end up repaying their whole mortgage before the scheduled term, as the example below should show.
I added pmt as an argument to the function as it is best practice for a function to not use any variable defined outside its scope (the dreaded global variables)
A big no-no in your code was the use of constructs like capital <- append(capital, currentCapital). At each iteration, a new and longer vector is created. This is slow and uses a lot of memory. Granted, you would need a very long mortgage to see the negative effects but it's good to learn to implement this the right way. You could a) pre-allocate a vector of length term.in.months and replace its value like this: capital[month.idx] <- currentCapital. Or b) use a list: lists use pointers in memory so they don't suffer the issue I was describing as they grow in size. My implementation uses this b) approach where I grow a list named db. At the end, I am turning this list of monthly data into a data.frame.
You'll see I use things like 12L instead of 12. That's the difference between integers and numerics. It is recommended to use integers when possible as it can prevent (rare) issues regarding floating point errors. It uses less memory and can sometimes make your code faster. Again, you won't notice it here, but it is a good practice.
Regarding @janos' comment about trying to avoid for loops. I agree, R works best and faster when vectorization can replace loops. However, your particular algorithm is a case of an iterative (state i+1 depends on state i) process so it cannot be vectorized. Whereas vectorization can replace loops only in processes where each iteration can be computed independently. So you will find that my answer still has a loop, in the form of a while().
I modified the variables' names to be closer to the official language. (I work in finance.)

mortgage <- function(property.value = 1e6,
                     downpayment = 0.10,
                     interest.rate = 0.04,
                     term = 25L,
                     prepayments = NULL,
                     pmt.fun = pmt) {

   original.balance <- property.value * (1 - downpayment)
   monthly.rate     <- interest.rate / 12L
   term.in.months   <- term * 12L
   monthly.payment  <- -pmt.fun(monthly.rate, nper = term.in.months,
                                              pv = original.balance)
   db <- list()
   month.idx <- 1L
   start.balance <- original.balance

   while(start.balance > 0) {

      balance.due       <- start.balance * (1 + monthly.rate)
      payment.due       <- min(monthly.payment, balance.due)
      prepayment        <- if (is.null(prepayments)) 0 else
                           min(sum(subset(prepayments, month == month.idx)$payment),
                               balance.due - payment.due)
      total.payment     <- payment.due + prepayment
      interest.payment  <- start.balance * monthly.rate
      end.balance       <- balance.due - total.payment
      principal.payment <- start.balance - end.balance

      db[[month.idx]] <- c(month             = month.idx,
                           start.balance     = start.balance,
                           balance.due       = balance.due,
                           payment.due       = payment.due,
                           total.payment     = total.payment,
                           prepayment        = prepayment,
                           interest.payment  = interest.payment,
                           principal.payment = principal.payment,
                           end.balance       = end.balance)
      month.idx     <- month.idx + 1L
      start.balance <- end.balance
   }
   as.data.frame(do.call(rbind, db))
}

source("http://pastebin.com/raw.php?i=q7tyiEmM")  # for pmt()

overPayments <- data.frame(
   month   = c(20,    25,    30,    35,    40),
   payment = c(20000, 20000, 20000, 20000, 20000))

cap1 <- mortgage()
cap2 <- mortgage(prepayments = overPayments)

par(mfcol=c(1, 2))
plot(c(cap1$start.balance, 0), type="l",
     main = "Without Prepayments", xlab = "Month", ylab = "Balance")
plot(c(cap2$start.balance, 0), type="l",
     main = "With Prepayments",    xlab = "Month", ylab = "Balance")


Adding a zoom attr to a container I have two buttons, + and -, to add a zoom attr to a container. I'm not sure how I can rewrite this in fewer line of code.

jsFiddle

var ZOOM_OFFSET = 20;
var MAX_ZOOM = 60;
var MIN_ZOOM = 0;

$('.schedule-zoom .zoom-btn:not(.disable)').click(function(){
  var action_button = $(this);
  var zoom_out_button = $('.schedule-zoom .zoom-out');
  var zoom_in_button = $('.schedule-zoom .zoom-in');

  var container = $('.fixed-container');
  if (container.attr("zoom") === undefined) {
    container.attr("zoom", 0);
  }
  var zoom_value = parseInt(container.attr("zoom"));

  if (action_button.hasClass("zoom-in")) {
    zoom_out_button.removeClass("disable");

    if(zoom_value < MAX_ZOOM) {
      container.attr("zoom", zoom_value + ZOOM_OFFSET);
    } else {
      action_button.addClass("disable");
    }
  }

  else if (action_button.hasClass("zoom-out")) {

    zoom_in_button.removeClass("disable");
    if(zoom_value > MIN_ZOOM) {
      container.attr("zoom", zoom_value - ZOOM_OFFSET);
    } else {
      action_button.addClass("disable");
    }
  }

  container.html(container.attr("zoom"));

});

    0

    <button class="zoom-btn zoom-in">+</button>
    <button class="zoom-btn zoom-out">-</button>

Let's remove all if/else checks:

Set direction to 1 or -1 based on the button class and multiply it by ZOOM_OFFSET
Use Math.max, Math.min to constrain zoom value in the range
Use .toggleClass('.foo', condition) to disable the buttons conditionally
Set the attribute just once, in the end: no need to treat the absence of the attribute specially, just use a fallback parseInt(foo) || MIN_ZOOM

And fix some inconsistencies:

Use camelCase names for normal variables, which is a standard JavaScript convention
Use single or double quotes for strings consistently
Prefer .text(foo) over .html(foo) in case of a simple text without HTML tags

$('.schedule-zoom .zoom-btn:not(.disable)').click(function() {
  var direction = $(this).hasClass('zoom-in') ? 1 : -1;
  var container = $('.fixed-container');

  var zoom = parseInt(container.attr('zoom')) || MIN_ZOOM;
  zoom = Math.max(MIN_ZOOM, Math.min(MAX_ZOOM, zoom + direction * ZOOM_OFFSET));

  container.attr('zoom', zoom).text(zoom);
  $('.schedule-zoom .zoom-in').toggleClass('disable', zoom == MAX_ZOOM);
  $('.schedule-zoom .zoom-out').toggleClass('disable', zoom == MIN_ZOOM);
});


RXJS queue up multiple http requests and send when x conditions happens Just started to use rxjs few weeks ago, I think I'm getting most of it right, but my code seems to be not that efficient and prone to bugs I haven't run into yet!

I need to be able to send http requests my app is online(1) and when the user is authenticated(2) and when there's anything to send(3). I'm pretty sure there's a smart way to combine those 3 conditions the "rxjs" way :)

SuperHttpRequest

class SuperHttpRequest extends HttpRequest<any> {

  _id: string; // Used for localStorage

  constructor(method: string, url: string, body: any, _id?: string) {
    super(method, url, body);
    this._id = _id || UUID.UUID();
  }
}

HttpRequestHandler

@Injectable()
class HttpRequestHandler extends ReplaySubject<SuperHttpRequest> {

  nbItemsInQueue$: ReplaySubject<Number>; // Needed by a component (sync progress), would be better to use the combined http request time
  private requests: SuperHttpRequest[];
  private requestsSubcription: Subscription;

  constructor(
    private network: Network, // Ionic service
    private http: HttpClient,
    private localStore: LocalStore<SuperHttpRequest[]>,
    private authService: AuthenticationService) {

    super();

    this.requests = [];
    this.nbItemsInQueue$ = new ReplaySubject(this.requests.length);

    this.localStore.get('http-request')
      .then(requests => {
        if (requests) {
          requests.forEach(request => {
            this.next(new SuperHttpRequest(request.method, request.url, request.body, request._id));
          });
        }
      });

    this.nbItemsInQueue$
      .subscribe(() => this.localStore.set('http-request', this.requests));

    this.authService.currentUser$
      .subscribe(user => {
        if (!user) { if (this.requestsSubcription) { this.requestsSubcription.unsubscribe(); } }
        else {
          this.requestsSubcription = this
            .takeWhile(() => {
              if (navigator.onLine) { return true; }
              else { throw Error(); }
            })
            .retryWhen(() => this.network.onConnect())
            .subscribe(request => this.handleHttpRequest(user, request));
        }
      });
  }

  private handleHttpRequest(user: Credentials, request: SuperHttpRequest) {
    this.http.request(request.clone({ setHeaders: { Authorization: `Bearer ${user.token}` } }))
      .retryWhen(error => {
        return error
          .mergeMap((error: any) => {
            if (error.status === 503) {
              return of(error.status).delay(2000)
            }
            return Observable.throw({ error: 'No retry' });
          })
          .take(5)
          .concat(Observable.throw({ error: 'Sorry, there was an error (after 5 retries)' }));
      })
      .subscribe(res => {
        if (res instanceof HttpResponse) {
          remove(this.requests, (req) => req._id === request._id);
          this.nbItemsInQueue$.next(this.requests.length);
        }
      });
  }

  next(request: SuperHttpRequest) {
    this.requests.push(request);
    this.nbItemsInQueue$.next(this.requests.length);
    super.next(request);
  }

}

//Maintain Queue of http Observable
const quequeHTTP = [];

//Dummy Http Request
getData(){
   return this.http.get('./api');
}

addInQueuqe(observable:Observable<any>){
     quequeHTTP.push(observable);
}

//Dummy addition 4 request
addInQueuqe(this.getData());
addInQueuqe(this.getData());
addInQueuqe(this.getData());
addInQueuqe(this.getData());

eventWhenXHappen(){
  Observable.forkJoin(quequeHTTP).subscribe((resArr)=>{
     console.log('When all request succesfully executed ');
},(err)=>{
  console.log('When error occur');
})

}